[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/6] Convert capabilities / domain_conf to use virArch



On Tue, Dec 11, 2012 at 14:53:39 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Convert the host capabilities and domain config structs to
> use the virArch datatype. Update the parsers and all drivers
> to take account of datatype change
...
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index a8ee2cf..97d1b80 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
...
> @@ -556,12 +546,12 @@ virCapabilitiesSupportsGuestOSType(virCapsPtr caps,
>  extern int
>  virCapabilitiesSupportsGuestOSTypeArch(virCapsPtr caps,
>                                         const char *ostype,
> -                                       const char *arch)
> +                                       virArch arch)
>  {
>      int i;
>      for (i = 0 ; i < caps->nguests ; i++) {
>          if (STREQ(caps->guests[i]->ostype, ostype) &&
> -            STREQ(caps->guests[i]->arch.name, arch))
> +            (caps->guests[i]->arch.id == arch))

Unneeded ().

>              return 1;
>      }
>      return 0;
> @@ -576,28 +566,35 @@ virCapabilitiesSupportsGuestOSTypeArch(virCapsPtr caps,
>   * Returns the first architecture able to run the
>   * requested operating system type
>   */
> -extern const char *
> +extern virArch
>  virCapabilitiesDefaultGuestArch(virCapsPtr caps,
>                                  const char *ostype,
>                                  const char *domain)
>  {
>      int i, j;
> -    const char *arch = NULL;
> +
> +    /* First try to find one matching host arch */
>      for (i = 0 ; i < caps->nguests ; i++) {
>          if (STREQ(caps->guests[i]->ostype, ostype)) {
>              for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
> -                if (STREQ(caps->guests[i]->arch.domains[j]->type, domain)) {
> -                    /* Use the first match... */
> -                    if (!arch)
> -                        arch = caps->guests[i]->arch.name;
> -                    /* ...unless we can match the host's architecture. */
> -                    if (STREQ(caps->guests[i]->arch.name, caps->host.arch))
> -                        return caps->guests[i]->arch.name;
> -                }
> +                if (STREQ(caps->guests[i]->arch.domains[j]->type, domain) &&
> +                    (caps->guests[i]->arch.id == caps->host.arch))

You've started to love these extra () recently :-)

> +                    return caps->guests[i]->arch.id;
>              }
>          }
>      }
> -    return arch;
> +
> +    /* Otherwise find the first match */
> +    for (i = 0 ; i < caps->nguests ; i++) {
> +        if (STREQ(caps->guests[i]->ostype, ostype)) {
> +            for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
> +                if (STREQ(caps->guests[i]->arch.domains[j]->type, domain))
> +                    return caps->guests[i]->arch.id;
> +            }
> +        }
> +    }
> +

I think both the original and the new version are equally readable, but
if you think this new version is better, I'm fine with it.

> +    return VIR_ARCH_I686;

However, this should definitely return VIR_ARCH_NONE. The original
version would return NULL in case of no match.

>  }
>  
>  /**
...
> @@ -623,11 +620,12 @@ virCapabilitiesDefaultGuestMachine(virCapsPtr caps,
>          virCapsGuestPtr guest = caps->guests[i];
>          int j;
>  
> -        if (!STREQ(guest->ostype, ostype) || !STREQ(guest->arch.name, arch))
> +        if (!STREQ(guest->ostype, ostype) ||
> +            (guest->arch.id != arch))

()

>              continue;
>  
>          for (j = 0; j < guest->arch.ndomains; j++) {
> -            virCapsGuestDomainPtr dom= guest->arch.domains[j];
> +            virCapsGuestDomainPtr dom = guest->arch.domains[j];
>  
>              if (!STREQ(dom->type, domain))
>                  continue;
> @@ -659,14 +657,14 @@ virCapabilitiesDefaultGuestMachine(virCapsPtr caps,
>  extern const char *
>  virCapabilitiesDefaultGuestEmulator(virCapsPtr caps,
>                                      const char *ostype,
> -                                    const char *arch,
> +                                    virArch arch,
>                                      const char *domain)
>  {
>      int i, j;
>      for (i = 0 ; i < caps->nguests ; i++) {
>          char *emulator;
>          if (STREQ(caps->guests[i]->ostype, ostype) &&
> -            STREQ(caps->guests[i]->arch.name, arch)) {
> +            (caps->guests[i]->arch.id == arch)) {

()

>              emulator = caps->guests[i]->arch.defaultInfo.emulator;
>              for (j = 0 ; j < caps->guests[i]->arch.ndomains ; j++) {
>                  if (STREQ(caps->guests[i]->arch.domains[j]->type, domain)) {
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6aa5f79..1f978db 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -9412,12 +9411,20 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>          goto error;
>      }
>  
> -    def->os.arch = virXPathString("string(./os/type[1]/@arch)", ctxt);
> -    if (def->os.arch) {
> +    tmp = virXPathString("string(./os/type[1]/@arch)", ctxt);
> +    if (tmp) {
> +        virArch arch = virArchFromString(tmp);
> +        if (!arch) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unknown architecture %s"),
> +                           tmp);
> +            goto error;
> +        }
> +

Memory leak, you need to call VIR_FREE(tmp) here.

>          if (!virCapabilitiesSupportsGuestArch(caps, def->os.arch)) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("No guest options available for arch '%s'"),
> -                           def->os.arch);
> +                           virArchToString(def->os.arch));
>              goto error;
>          }
>  
> @@ -9426,20 +9433,17 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>                                                      def->os.arch)) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("No os type '%s' available for arch '%s'"),
> -                           def->os.type, def->os.arch);
> +                           def->os.type, virArchToString(def->os.arch));
>              goto error;
>          }
>      } else {
> -        const char *defaultArch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
> -        if (defaultArch == NULL) {
> +        def->os.arch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));

Make this line shorter while changing it.

> +        if (!def->os.arch) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("no supported architecture for os type '%s'"),
>                             def->os.type);
>              goto error;
>          }
> -        if (!(def->os.arch = strdup(defaultArch))) {
> -            goto no_memory;
> -        }
>      }
>  
>      def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt);
> @@ -10050,7 +10054,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>  
>          def->memballoon = memballoon;
>          VIR_FREE(nodes);
> -    } else if (!STREQ(def->os.arch,"s390x")) {
> +    } else if ((def->os.arch != VIR_ARCH_S390) &&
> +               (def->os.arch != VIR_ARCH_S390X)) {

Too many extra (), I won't report them anymore :-)

>          /* TODO: currently no balloon support on s390 -> no default balloon */
>          if (def->virtType == VIR_DOMAIN_VIRT_XEN ||
>              def->virtType == VIR_DOMAIN_VIRT_QEMU ||
...
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 302f81c..c02cb19 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
...
> @@ -334,14 +333,10 @@ libxlDoNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info)
>          return -1;
>      }
>  
> -    uname(&utsname);
> -    if (virStrncpy(info->model,
> -                   utsname.machine,
> -                   strlen(utsname.machine),
> -                   sizeof(info->model)) == NULL) {
> +    if (virStrcpyStatic(nodeinfo->model, virArchToString(hostarch)) == NULL) {

This wouldn't even compile; s/nodeinfo/info/

>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("machine type %s too big for destination"),
> -                       utsname.machine);
> +                       virArchToString(hostarch));
>          return -1;
>      }
>  
...
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 01a1b98..c9146c4 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -899,11 +882,14 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache)
>      virCapabilitiesAddHostMigrateTransport(caps,
>                                             "tcp");
>  
> -    /* First the pure HVM guests */
> -    for (i = 0 ; i < ARRAY_CARDINALITY(arches) ; i++)
> +    /* QEMU can support pretty much every arch that exists,
> +     * so just probe for them all - we gracefully fail
> +     * if a qemu-system-$ARCH bvinary can't be found

s/bvinary/binary/

> +     */
> +    for (i = 0 ; i < VIR_ARCH_LAST ; i++)
>          if (qemuCapsInitGuest(caps, cache,
> -                              utsname.machine,
> -                              arches[i]) < 0)
> +                              virArchFromHost(),
> +                              i) < 0)
>              goto error;
>  
>      /* QEMU Requires an emulator in the XML */
...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6968f74..267dce7 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
...
> @@ -8338,16 +8337,17 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
>      else
>          path = strstr(def->emulator, "qemu");
>      if (def->virtType == VIR_DOMAIN_VIRT_KVM)
> -        def->os.arch = strdup(caps->host.cpu->arch);
> +        def->os.arch = caps->host.arch;
>      else if (path &&
> -             STRPREFIX(path, "qemu-system-"))
> -        def->os.arch = strdup(path + strlen("qemu-system-"));
> -    else
> -        def->os.arch = strdup("i686");
> +             STRPREFIX(path, "qemu-system-")) {
> +        def->os.arch = virArchFromString(path + strlen("qemu-system-"));
> +    } else
> +        def->os.arch = VIR_ARCH_I686;
>      if (!def->os.arch)
>          goto no_memory;

No need for the extra {}, however, if you want to add them, add them to
all branches of this if statement.

>  
> -    if (STREQ(def->os.arch, "i686")||STREQ(def->os.arch, "x86_64"))
> +    if ((def->os.arch == VIR_ARCH_I686) ||
> +        (def->os.arch == VIR_ARCH_X86_64))
>          def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI)
>          /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/;
>  #define WANT_VALUE()                                                   \
...
> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index 23b3037..1b04b4c 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
...
> @@ -290,15 +290,13 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>      if (!(def->os.type = strdup(hvm ? "hvm" : "xen")))
>          goto no_memory;
>  
> -    defaultArch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));
> -    if (defaultArch == NULL) {
> +    def->os.arch = virCapabilitiesDefaultGuestArch(caps, def->os.type, virDomainVirtTypeToString(def->virtType));

Line too long.

> +    if (!def->os.arch) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("no supported architecture for os type '%s'"),
>                         def->os.type);
>          goto cleanup;
>      }
> -    if (!(def->os.arch = strdup(defaultArch)))
> -        goto no_memory;
>  
>      defaultMachine = virCapabilitiesDefaultGuestMachine(caps,
>                                                          def->os.type,

ACK with the small issues fixed.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]