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

Re: [libvirt] [PATCH v3 07/34] Adapt to VIR_STRDUP and VIR_STRNDUP in src/libxl/*



On 05/03/2013 08:53 AM, Michal Privoznik wrote:
> ---
>  src/libxl/libxl_conf.c   | 86 ++++++++++++++----------------------------------
>  src/libxl/libxl_driver.c | 14 +++-----
>  2 files changed, 29 insertions(+), 71 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> @@ -418,37 +414,26 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          b_info->shadow_memkb = 4 * (256 * libxl_bitmap_count_set(&b_info->avail_vcpus) +
>                                      2 * (b_info->max_memkb / 1024));
>      } else {
> -        if (def->os.bootloader) {
> -            if ((b_info->u.pv.bootloader = strdup(def->os.bootloader)) == NULL) {
> -                virReportOOMError();
> -                goto error;
> -            }
> -        }
> +        if (def->os.bootloader &&
> +            VIR_STRDUP(b_info->u.pv.bootloader, def->os.bootloader) < 0)

You can pass NULL source arg now :)

> +            goto error;
>          if (def->os.bootloaderArgs) {
>              if (!(b_info->u.pv.bootloader_args =
>                    virStringSplit(def->os.bootloaderArgs, " \t\n", 0)))
>                  goto error;
>          }
> -        if (def->os.cmdline) {
> -            if ((b_info->u.pv.cmdline = strdup(def->os.cmdline)) == NULL) {
> -                virReportOOMError();
> -                goto error;
> -            }
> -        }
> +        if (def->os.cmdline &&
> +            VIR_STRDUP(b_info->u.pv.cmdline, def->os.cmdline) < 0)
> +            goto error;

and again...

>          if (def->os.kernel) {
> -            /* libxl_init_build_info() sets kernel.path = strdup("hvmloader") */
> +            /* libxl_init_build_info() sets VIR_STRDUP(kernel.path, "hvmloader") */
>              VIR_FREE(b_info->u.pv.kernel);
> -            if ((b_info->u.pv.kernel = strdup(def->os.kernel)) == NULL) {
> -                virReportOOMError();
> +            if (VIR_STRDUP(b_info->u.pv.kernel, def->os.kernel) < 0)
>                  goto error;

[Not here, of course]

> -            }
> -        }
> -        if (def->os.initrd) {
> -            if ((b_info->u.pv.ramdisk = strdup(def->os.initrd)) == NULL) {
> -                virReportOOMError();
> -                goto error;
> -            }
>          }
> +        if (def->os.initrd &&
> +            VIR_STRDUP(b_info->u.pv.ramdisk, def->os.initrd) < 0)
> +            goto error;
>      }

...and a third time.

> @@ -461,15 +446,11 @@ error:
>  int
>  libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk)
>  {
> -    if (l_disk->src && (x_disk->pdev_path = strdup(l_disk->src)) == NULL) {
> -        virReportOOMError();
> +    if (l_disk->src && VIR_STRDUP(x_disk->pdev_path, l_disk->src) < 0)
>          return -1;
> -    }
>  
> -    if (l_disk->dst && (x_disk->vdev = strdup(l_disk->dst)) == NULL) {
> -        virReportOOMError();
> +    if (l_disk->dst && VIR_STRDUP(x_disk->vdev, l_disk->dst) < 0)
>          return -1;
> -    }

These can be simplified as well.

> @@ -575,31 +556,22 @@ libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic)
>      virMacAddrGetRaw(&l_nic->mac, x_nic->mac);
>  
>      if (l_nic->model && !STREQ(l_nic->model, "netfront")) {
> -        if ((x_nic->model = strdup(l_nic->model)) == NULL) {
> -            virReportOOMError();
> +        if (VIR_STRDUP(x_nic->model, l_nic->model) < 0)
>              return -1;
> -        }
>          x_nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
>      } else {
>          x_nic->nictype = LIBXL_NIC_TYPE_VIF;
>      }
>  
> -    if (l_nic->ifname && (x_nic->ifname = strdup(l_nic->ifname)) == NULL) {
> -        virReportOOMError();
> +    if (l_nic->ifname && VIR_STRDUP(x_nic->ifname, l_nic->ifname) < 0)
>          return -1;
> -    }

And this one.

>  
>      if (l_nic->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>          if (l_nic->data.bridge.brname &&
> -            (x_nic->bridge = strdup(l_nic->data.bridge.brname)) == NULL) {
> -            virReportOOMError();
> +            VIR_STRDUP(x_nic->bridge, l_nic->data.bridge.brname) < 0)
>              return -1;
> -        }
> -        if (l_nic->script &&
> -            (x_nic->script = strdup(l_nic->script)) == NULL) {
> -            virReportOOMError();
> +        if (l_nic->script && VIR_STRDUP(x_nic->script, l_nic->script) < 0)
>              return -1;

And these two.

> @@ -656,16 +628,11 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>          case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
>              libxl_defbool_set(&x_vfb->sdl.enable, 1);
>              if (l_vfb->data.sdl.display &&
> -                (x_vfb->sdl.display = strdup(l_vfb->data.sdl.display)) == NULL) {
> -                virReportOOMError();
> +                VIR_STRDUP(x_vfb->sdl.display, l_vfb->data.sdl.display) < 0)
>                  return -1;
> -            }
>              if (l_vfb->data.sdl.xauth &&
> -                (x_vfb->sdl.xauthority =
> -                    strdup(l_vfb->data.sdl.xauth)) == NULL) {
> -                virReportOOMError();
> +                VIR_STRDUP(x_vfb->sdl.xauthority, l_vfb->data.sdl.xauth) < 0)
>                  return -1;
> -            }

And some more.

> @@ -686,19 +653,14 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>  
>              listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0);
>              if (listenAddr) {
> -                /* libxl_device_vfb_init() does strdup("127.0.0.1") */
> +                /* libxl_device_vfb_init() does VIR_STRDUP("127.0.0.1") */
>                  VIR_FREE(x_vfb->vnc.listen);
> -                if ((x_vfb->vnc.listen = strdup(listenAddr)) == NULL) {
> -                    virReportOOMError();
> +                if (VIR_STRDUP(x_vfb->vnc.listen, listenAddr) < 0)
>                      return -1;
> -                }

[this one can't be simplified]

>              }
>              if (l_vfb->data.vnc.keymap &&
> -                (x_vfb->keymap =
> -                    strdup(l_vfb->data.vnc.keymap)) == NULL) {
> -                virReportOOMError();
> +                VIR_STRDUP(x_vfb->keymap, l_vfb->data.vnc.keymap) < 0)
>                  return -1;
> -            }

Another case for simplification.

> +++ b/src/libxl/libxl_driver.c
> @@ -3879,18 +3878,18 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams)
>          *nparams = 0;
>      switch (sched_id) {
>      case LIBXL_SCHEDULER_SEDF:
> -        ret = strdup("sedf");
> +        ignore_value(VIR_STRDUP(ret, "sedf"));
>          break;
>      case LIBXL_SCHEDULER_CREDIT:
> -        ret = strdup("credit");
> +        ignore_value(VIR_STRDUP(ret, "credit"));
>          if (nparams)
>              *nparams = XEN_SCHED_CREDIT_NPARAM;
>          break;
>      case LIBXL_SCHEDULER_CREDIT2:
> -        ret = strdup("credit2");
> +        ignore_value(VIR_STRDUP(ret, "credit2"));
>          break;
>      case LIBXL_SCHEDULER_ARINC653:
> -        ret = strdup("arinc653");
> +        ignore_value(VIR_STRDUP(ret, "arinc653"));
>          break;

This is correct, but I wonder if looks any better with fewer
ignore_value calls as follows:

const char *name;
switch (sched_id) {
case LIBXL_SCHEDULER_SEDF:
    name = "sedf";
    break;
case LIBXL_SCHEDULER_CREDIT:
    name = "credit";
    if (nparams)
        *nparams = XEN_SCHED_CREDIT_NPARAM;
    break;
...
}
ignore_value(VIR_STRDUP(ret, name));

ACK.  I'm okay whether you make the simplifications possible for a NULL
source argument now or as a followup patch, and didn't spot anything
wrong with this patch as-is.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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