Re: [PREPOST 07/17] src/xenxs:Refactor disk config parsing code

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

 



David Kiarie wrote:
> From: Kiarie Kahurani <davidkiarie4@xxxxxxxxx>
>
> Introduce the function
>  xenParseXMDisk(..........);
>
> Parses disk config
>
> signed-off-by: David Kiarie<davidkiarie4@xxxxxxxxx>
> ---
>  src/xenxs/xen_xm.c | 204 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 108 insertions(+), 96 deletions(-)
>
> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index ebeeeb5..80a7941 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
> @@ -685,87 +685,14 @@ static int xenParseXMCPUFeatures(virConfPtr conf, virDomainDefPtr def)
>  
>      return 0;
>  }
> -virDomainDefPtr
> -xenParseXM(virConfPtr conf, int xendConfigVersion,
> -                       virCapsPtr caps)
> +static int xenParseXMDisk(virConfPtr conf, virDomainDefPtr def,
> +                   int xendConfigVersion)
>  {
> -    const char *str;
> -    int hvm = 0;
> -    int val;
> -    virConfValuePtr list;
> -    virDomainDefPtr def = NULL;
> +    const char *str = NULL;
>      virDomainDiskDefPtr disk = NULL;
> -    virDomainNetDefPtr net = NULL;
> -    virDomainGraphicsDefPtr graphics = NULL;
> -    size_t i;
> -    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;
> -    hvm = (STREQ(def->os.type, "hvm"));
> -    if (hvm) {
> -        const char *boot;
> -        if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0)
> -            goto cleanup;
> -
> -        if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0)
> -            goto cleanup;
> +    int hvm = STREQ(def->os.type, "hvm");
> +    virConfValuePtr list = virConfGetValue(conf, "disk");
>  
> -        for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) {
> -            switch (*boot) {
> -            case 'a':
> -                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY;
> -                break;
> -            case 'd':
> -                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM;
> -                break;
> -            case 'n':
> -                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET;
> -                break;
> -            case 'c':
> -            default:
> -                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK;
> -                break;
> -            }
> -            def->os.nBootDevs++;
> -        }
> -    } else {
> -        const char *extra, *root;
> -
> -        if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
> -            goto cleanup;
> -        if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
> -            goto cleanup;
> -
> -        if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0)
> -            goto cleanup;
> -        if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
> -            goto cleanup;
> -        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
> -            goto cleanup;
> -        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
> -            goto cleanup;
> -
> -        if (root) {
> -            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> -                goto cleanup;
> -        } else {
> -            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> -                goto cleanup;
> -        }
> -    }
> -    if (xenParseXMMem(conf, def) < 0)
> -        goto cleanup;
> -    if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
> -        goto cleanup;
> -
> -    list = virConfGetValue(conf, "disk");
>      if (list && list->type == VIR_CONF_LIST) {
>          list = list->list;
>          while (list) {
> @@ -779,7 +706,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              head = list->str;
>  
>              if (!(disk = virDomainDiskDefNew()))
> -                goto cleanup;
> +                return -1;
>  
>              /*
>               * Disks have 3 components, SOURCE,DEST-DEVICE,MODE
> @@ -798,10 +725,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>                  ignore_value(virDomainDiskSetSource(disk, NULL));
>              } else {
>                  if (VIR_STRNDUP(tmp, head, offset - head) < 0)
> -                    goto cleanup;
> +                    return -1;
>                  if (virDomainDiskSetSource(disk, tmp) < 0) {
>                      VIR_FREE(tmp);
> -                    goto cleanup;
> +                    return -1;
>                  }
>                  VIR_FREE(tmp);
>              }
> @@ -815,12 +742,12 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              if (!(offset = strchr(head, ',')))
>                  goto skipdisk;
>              if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0)
> -                goto cleanup;
> +                return -1;
>              if (virStrncpy(disk->dst, head, offset - head,
>                             (offset - head) + 1) == NULL) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Dest file %s too big for destination"), head);
> -                goto cleanup;
> +                return -1;
>              }
>              head = offset + 1;
>  
> @@ -832,16 +759,16 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>                  if ((tmp = strchr(src, ':')) != NULL) {
>                      len = tmp - src;
>                      if (VIR_STRNDUP(tmp, src, len) < 0)
> -                        goto cleanup;
> +                        return -1;
>                      if (virDomainDiskSetDriver(disk, tmp) < 0) {
>                          VIR_FREE(tmp);
> -                        goto cleanup;
> +                        return -1;
>                      }
>                      VIR_FREE(tmp);
>  
>                      /* Strip the prefix we found off the source file name */
>                      if (virDomainDiskSetSource(disk, src + len + 1) < 0)
> -                        goto cleanup;
> +                        return -1;
>                      src = virDomainDiskGetSource(disk);
>                  }
>  
> @@ -854,7 +781,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>                      len = tmp - src;
>  
>                      if (VIR_STRNDUP(driverType, src, len) < 0)
> -                        goto cleanup;
> +                        return -1;
>                      if (STREQ(driverType, "aio"))
>                          virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
>                      else
> @@ -865,12 +792,12 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>                          virReportError(VIR_ERR_INTERNAL_ERROR,
>                                         _("Unknown driver type %s"),
>                                         src);
> -                        goto cleanup;
> +                        return -1;
>                      }
>  
>                      /* Strip the prefix we found off the source file name */
>                      if (virDomainDiskSetSource(disk, src + len + 1) < 0)
> -                        goto cleanup;
> +                        return -1;
>                      src = virDomainDiskGetSource(disk);
>                  }
>              }
> @@ -878,7 +805,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              /* No source, or driver name, so fix to phy: */
>              if (!virDomainDiskGetDriver(disk) &&
>                  virDomainDiskSetDriver(disk, "phy") < 0)
> -                goto cleanup;
> +                return -1;
>   

The problem will all these 'return -1' is that you leak 'disk'.  I think
it would be good to have cleanup label in this function.  I'd suggested
a local variable 'ret' initialized to -1, which you set to 0 when the
function succeeds.  You can then cleanup and return 'ret' in the cleanup
label.

>  
>  
>              /* phy: type indicates a block device */
> @@ -912,7 +839,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>  
>              /* Maintain list in sorted order according to target device name */
>              if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
> -                goto cleanup;
> +                return -1;
>  
>              skipdisk:
>              list = list->next;
>   

The next line here is

            virDomainDiskDefFree(disk);

which is not right in the case VIR_APPEND_ELEMENT succeeds (pre-existing
bug).  'disk' should be set to NULL if VIR_APPEND_ELEMENT succeeds, so
we don't free the disk we've just added to the disk list.


> @@ -922,26 +849,108 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>  
>      if (hvm && xendConfigVersion == XEND_CONFIG_VERSION_3_0_2) {
>          if (xenXMConfigGetString(conf, "cdrom", &str, NULL) < 0)
> -            goto cleanup;
> +            return -1;
>          if (str) {
>              if (!(disk = virDomainDiskDefNew()))
> -                goto cleanup;
> +                return -1;
>  
>              virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
>              disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
>              if (virDomainDiskSetDriver(disk, "file") < 0)
> -                goto cleanup;
> +                return -1;
>              if (virDomainDiskSetSource(disk, str) < 0)
> -                goto cleanup;
> +                return -1;
>              if (VIR_STRDUP(disk->dst, "hdc") < 0)
> -                goto cleanup;
> +                return -1;
>              disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
>              disk->src->readonly = true;
>  
>              if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0)
> +                return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +virDomainDefPtr
> +xenParseXM(virConfPtr conf, int xendConfigVersion,
> +                       virCapsPtr caps)
> +{
> +    const char *str;
> +    int hvm = 0;
> +    int val;
> +    virConfValuePtr list;
> +    virDomainDefPtr def = NULL;
> +    virDomainDiskDefPtr disk = NULL;
>   

'disk' is no longer used in this function and can be removed.

Regards,
Jim

> +    virDomainNetDefPtr net = NULL;
> +    virDomainGraphicsDefPtr graphics = NULL;
> +    size_t i;
> +    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;
> +    hvm = (STREQ(def->os.type, "hvm"));
> +    if (hvm) {
> +        const char *boot;
> +        if (xenXMConfigCopyString(conf, "kernel", &def->os.loader) < 0)
> +            goto cleanup;
> +
> +        if (xenXMConfigGetString(conf, "boot", &boot, "c") < 0)
> +            goto cleanup;
> +
> +        for (i = 0; i < VIR_DOMAIN_BOOT_LAST && boot[i]; i++) {
> +            switch (*boot) {
> +            case 'a':
> +                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_FLOPPY;
> +                break;
> +            case 'd':
> +                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_CDROM;
> +                break;
> +            case 'n':
> +                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_NET;
> +                break;
> +            case 'c':
> +            default:
> +                def->os.bootDevs[i] = VIR_DOMAIN_BOOT_DISK;
> +                break;
> +            }
> +            def->os.nBootDevs++;
> +        }
> +    } else {
> +        const char *extra, *root;
> +
> +        if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
> +            goto cleanup;
> +        if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
> +            goto cleanup;
> +
> +        if (xenXMConfigCopyStringOpt(conf, "kernel", &def->os.kernel) < 0)
> +            goto cleanup;
> +        if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
> +            goto cleanup;
> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
> +            goto cleanup;
> +        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
> +            goto cleanup;
> +
> +        if (root) {
> +            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> +                goto cleanup;
> +        } else {
> +            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
>                  goto cleanup;
>          }
>      }
> +    if (xenParseXMMem(conf, def) < 0)
> +        goto cleanup;
> +    if (xenXMConfigCopyStringOpt(conf, "device_model", &def->emulator) < 0)
> +        goto cleanup;
>     if (xenParseXMVif(conf, def) < 0)
>         goto cleanup;
>  
> @@ -953,6 +962,9 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>         goto cleanup;
>     if (xenParseXMCPUFeatures(conf, def) < 0)
>         goto cleanup;
> +   if (xenParseXMDisk(conf, def, xendConfigVersion) < 0)
> +       goto cleanup;
> +
>     if (hvm) {
>          if (xenXMConfigGetString(conf, "usbdevice", &str, NULL) < 0)
>              goto cleanup;
>   

--
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]