Re: [PREPOST 10/17] src/xenxs:Refactor xenParseXM( )

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

 



David Kiarie wrote:
> From: Kiarie Kahurani <davidkiarie4@xxxxxxxxx>
>
> A couple of miscellaneous fixed and wrap code common code into
> xenParseConfigCommon(...).I will drop some of the functions later
> though
>
> signed-off-by: David Kiarie<davidkiarie4@xxxxxxxxx>
> ---
>  src/xenxs/xen_xm.c | 134 ++++++++++++++++++++++++++++-------------------------
>  src/xenxs/xen_xm.h |   3 +-
>  2 files changed, 73 insertions(+), 64 deletions(-)
>
> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index 312d890..657dd1c 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
> @@ -458,7 +458,7 @@ static int xenParseXMVif(virConfPtr conf, virDomainDefPtr def)
>      }
>  
>      return 0;
> -cleanup:
> + cleanup:
>   

This should be in 3/17 to fix 'make syntax-check'.

>      VIR_FREE(script);
>      return -1;
>  }
> @@ -1026,7 +1026,8 @@ static int xenParseXMOS(virConfPtr conf, virDomainDefPtr def)
>  
>      if (STREQ(def->os.type, "hvm")) {
>          const char *boot;
> -
> +        if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
> +            return -1;
>   

This replicates the next lines of code.  Not needed IMO.

>          if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
>              return -1;
>  
> @@ -1122,54 +1123,13 @@ static int xenParseXMMisc(virConfPtr conf, virDomainDefPtr def)
>  
>      return 0;
>  }
> -virDomainDefPtr
> -xenParseXM(virConfPtr conf, int xendConfigVersion,
> -                       virCapsPtr caps)
> +static int xenParseXMCharDev(virConfPtr conf, virDomainDefPtr def)
>  {
>      const char *str;
> -    int hvm = 0;
> -    virConfValuePtr list;
> -    virDomainDefPtr def = NULL;
> -    virDomainDiskDefPtr disk = NULL;
> -    virDomainNetDefPtr net = NULL;
> -    virDomainGraphicsDefPtr graphics = NULL;
> -    char *script = NULL;
> -    char *listenAddr = NULL;
> -
> -    if (VIR_ALLOC(def) < 0)
> -        return NULL;
> -
> -    def->virtType = VIR_DOMAIN_VIRT_XEN;
> -    def->id = -1;
> -    if (xenParseXMGeneral(conf, def, caps) < 0)
> -        goto cleanup;
> -
> -    if (xenParseXMOS(conf, def) < 0)
> -        goto cleanup;
> +    virConfValuePtr value = NULL;
> +    virDomainChrDefPtr chr = NULL;
>  
> -    hvm = (STREQ(def->os.type, "hvm"));
> -    if (xenParseXMMem(conf, def) < 0)
> -        goto cleanup;
> -    if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
> -        goto cleanup;
> -    if (xenParseXMVif(conf, def) < 0)
> -        goto cleanup;
> -    if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0)
> -        goto cleanup;
> -    if (xenParseXMEventsActions(conf, def) < 0)
> -        goto cleanup;
> -    if (xenParseXMPCI(conf, def) < 0)
> -        goto cleanup;
> -    if (xenParseXMCPUFeatures(conf, def) < 0)
> -        goto cleanup;
> -    if (xenParseXMDisk(conf, def, xendConfigVersion) < 0)
> -        goto cleanup;
> -    if (xenParseXMVfb(conf, def, xendConfigVersion) < 0)
> -        goto cleanup;
> -    if (xenParseXMMisc(conf, def) < 0)
> -        goto cleanup;
> -    if (hvm) {
> -        virDomainChrDefPtr chr = NULL;
> +    if (STREQ(def->os.type, "hvm")) {
>  
>          if (xenXMConfigGetString(conf, "parallel", &str, NULL) < 0)
>              goto cleanup;
> @@ -1179,7 +1139,6 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>  
>          if (chr) {
>              if (VIR_ALLOC_N(def->parallels, 1) < 0) {
> -                virDomainChrDefFree(chr);
>                  goto cleanup;
>              }
>              chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL;
> @@ -1190,21 +1149,21 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>          }
>  
>          /* Try to get the list of values to support multiple serial ports */
> -        list = virConfGetValue(conf, "serial");
> -        if (list && list->type == VIR_CONF_LIST) {
> +        value = virConfGetValue(conf, "serial");
> +        if (value && value->type == VIR_CONF_LIST) {
>              int portnum = -1;
>  
> -            list = list->list;
> -            while (list) {
> +            value = value->list;
> +            while (value) {
>                  char *port = NULL;
>  
> -                if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
> +                if ((value->type != VIR_CONF_STRING) || (value->str == NULL))
>                      goto cleanup;
>  
> -                port = list->str;
> +                port = value->str;
>                  portnum++;
>                  if (STREQ(port, "none")) {
> -                    list = list->next;
> +                    value = value->next;
>                      continue;
>                  }
>  
> @@ -1215,11 +1174,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>                  chr->target.port = portnum;
>  
>                  if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) {
> -                    virDomainChrDefFree(chr);
>                      goto cleanup;
>                  }
>  
> -                list = list->next;
> +                value = value->next;
>              }
>          } else {
>              /* If domain is not using multiple serial ports we parse data old way */
> @@ -1249,16 +1207,66 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>          def->consoles[0]->target.port = 0;
>          def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN;
>      }
> -    VIR_FREE(script);
> +
> +    return 0;
> +
> + cleanup:
> +    virDomainChrDefFree(chr);
> +    return -1;
> +}
>   

I think introducing xenParseXMCharDev and xenParseConfigCommon should be
done in separate patches.

> +int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def,
> +                                virCapsPtr caps, int xendConfigVersion )
> +{
> +
> +    if (xenParseXMGeneral(conf, def, caps) < 0)
> +        return -1;
> +
> +    if (xenParseXMOS(conf, def) < 0)
> +        return -1;
> +
> +    if (xenParseXMMem(conf, def) < 0)
> +        return -1;
> +    if (xenParseXMTimeOffset(conf, def, xendConfigVersion) < 0)
> +        return -1;
> +    if (xenParseXMEventsActions(conf, def) < 0)
> +        return -1;
> +    if (xenParseXMPCI(conf, def) < 0)
> +        return -1;
> +    if (xenParseXMCPUFeatures(conf, def) < 0)
> +        return -1;
> +
> +    if (xenParseXMMisc(conf, def) < 0)
> +        return -1;
> +    if (xenParseXMCharDev(conf, def) < 0)
> +        return -1;
> +
> +    if (xenParseXMVfb(conf, def, xendConfigVersion) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +virDomainDefPtr
> +xenParseXM(virConfPtr conf, int xendConfigVersion,
> +                       virCapsPtr caps)
> +{
> +    virDomainDefPtr def = NULL;
> +
> +    if (VIR_ALLOC(def) < 0)
> +        return NULL;
> +
> +    def->virtType = VIR_DOMAIN_VIRT_XEN;
> +    def->id = -1;
> +    if (xenParseConfigCommon(conf, def, caps, xendConfigVersion) < 0)
> +        goto cleanup;
> +    if (xenParseXMVif(conf, def) < 0)
> +        goto cleanup;
> +    if (xenParseXMDisk(conf, def, xendConfigVersion) < 0)
> +        goto cleanup;
> +
>      return def;
>  
>   cleanup:
> -    virDomainGraphicsDefFree(graphics);
> -    virDomainNetDefFree(net);
> -    virDomainDiskDefFree(disk);
>      virDomainDefFree(def);
> -    VIR_FREE(script);
> -    VIR_FREE(listenAddr);
>   

All of these *Free calls should have been removed in the respective
patches that removed the code from xenParseXM.

>      return NULL;
>  }
>  
> diff --git a/src/xenxs/xen_xm.h b/src/xenxs/xen_xm.h
> index 629a4b3..621cbe1 100644
> --- a/src/xenxs/xen_xm.h
> +++ b/src/xenxs/xen_xm.h
> @@ -29,7 +29,8 @@
>  # include "internal.h"
>  # include "virconf.h"
>  # include "domain_conf.h"
> -
> +int xenParseConfigCommon(virConfPtr conf, virDomainDefPtr def,
> +                         virCapsPtr caps, int xendConfigVersion);
>   

Blank line between these function declarations.

Regards,
Jim

>  virConfPtr xenFormatXM(virConnectPtr conn, virDomainDefPtr def,
>                         int xendConfigVersion);
>  
>   

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]