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

Re: [libvirt] [PATCH] Use virBuffer when constructing QEMU character device command line



On Wed, Nov 04, 2009 at 05:43:51PM +0000, Matthew Booth wrote:
> This patch updates qemudBuildCommandLineChrDevStr to use a virBuffer instead of
> a char[]. This is slightly tidier, and it makes it cleaner to (ap|pre)pend the
> output in other command lines.
> 
> * src/qemu/qemu_conf.c: Update qemudBuildCommandLineChrDevStr to use a virBuffer
> ---
>  src/qemu/qemu_conf.c |   97 +++++++++++++++++++++++--------------------------
>  1 files changed, 46 insertions(+), 51 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 40613dd..052ae98 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1409,83 +1409,66 @@ qemuBuildHostNetStr(virConnectPtr conn,
>      return 0;
>  }
>  
> -static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev,
> -                                          char *buf,
> -                                          int buflen)
> +static void qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev,
> +                                           virBufferPtr buf)
>  {
>      switch (dev->type) {
>      case VIR_DOMAIN_CHR_TYPE_NULL:
> -        if (virStrcpy(buf, "null", buflen) == NULL)
> -            return -1;
> +        virBufferAddLit(buf, "null");
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_VC:
> -        if (virStrcpy(buf, "vc", buflen) == NULL)
> -            return -1;
> +        virBufferAddLit(buf, "vc");
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_PTY:
> -        if (virStrcpy(buf, "pty", buflen) == NULL)
> -            return -1;
> +        virBufferAddLit(buf, "pty");
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_DEV:
> -        if (snprintf(buf, buflen, "%s",
> -                     dev->data.file.path) >= buflen)
> -            return -1;
> +        virBufferStrcat(buf, dev->data.file.path, NULL);
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_FILE:
> -        if (snprintf(buf, buflen, "file:%s",
> -                     dev->data.file.path) >= buflen)
> -            return -1;
> +        virBufferVSprintf(buf, "file:%s", dev->data.file.path);
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_PIPE:
> -        if (snprintf(buf, buflen, "pipe:%s",
> -                     dev->data.file.path) >= buflen)
> -            return -1;
> +        virBufferVSprintf(buf, "pipe:%s", dev->data.file.path);
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_STDIO:
> -        if (virStrcpy(buf, "stdio", buflen) == NULL)
> -            return -1;
> +        virBufferAddLit(buf, "stdio");
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_UDP:
> -        if (snprintf(buf, buflen, "udp:%s:%s %s:%s",
> -                     dev->data.udp.connectHost,
> -                     dev->data.udp.connectService,
> -                     dev->data.udp.bindHost,
> -                     dev->data.udp.bindService) >= buflen)
> -            return -1;
> +        virBufferVSprintf(buf, "udp:%s:%s %s:%s",
> +                          dev->data.udp.connectHost,
> +                          dev->data.udp.connectService,
> +                          dev->data.udp.bindHost,
> +                          dev->data.udp.bindService);
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_TCP:
>          if (dev->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET) {
> -            if (snprintf(buf, buflen, "telnet:%s:%s%s",
> -                         dev->data.tcp.host,
> -                         dev->data.tcp.service,
> -                         dev->data.tcp.listen ? ",server,nowait" : "") >= buflen)
> -                return -1;
> +            virBufferVSprintf(buf, "telnet:%s:%s%s",
> +                              dev->data.tcp.host,
> +                              dev->data.tcp.service,
> +                              dev->data.tcp.listen ? ",server,nowait" : "");
>          } else {
> -            if (snprintf(buf, buflen, "tcp:%s:%s%s",
> -                         dev->data.tcp.host,
> -                         dev->data.tcp.service,
> -                         dev->data.tcp.listen ? ",server,nowait" : "") >= buflen)
> -                return -1;
> +            virBufferVSprintf(buf, "tcp:%s:%s%s",
> +                              dev->data.tcp.host,
> +                              dev->data.tcp.service,
> +                              dev->data.tcp.listen ? ",server,nowait" : "");
>          }
>          break;
>  
>      case VIR_DOMAIN_CHR_TYPE_UNIX:
> -        if (snprintf(buf, buflen, "unix:%s%s",
> -                     dev->data.nix.path,
> -                     dev->data.nix.listen ? ",server,nowait" : "") >= buflen)
> -            return -1;
> +        virBufferVSprintf(buf, "unix:%s%s",
> +                          dev->data.nix.path,
> +                          dev->data.nix.listen ? ",server,nowait" : "");
>          break;
>      }
> -
> -    return 0;
>  }
>  
>  #define QEMU_SERIAL_PARAM_ACCEPTED_CHARS \
> @@ -1773,13 +1756,17 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          ADD_ARG_LIT("-nographic");
>  
>      if (monitor_chr) {
> -        char buf[4096];
> +        virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
> -        if (qemudBuildCommandLineChrDevStr(monitor_chr, buf, sizeof(buf)) < 0)
> +        qemudBuildCommandLineChrDevStr(monitor_chr, &buf);
> +        const char *argStr = virBufferContentAndReset(&buf);
> +        if (argStr == NULL)
>              goto error;
>  
>          ADD_ARG_LIT("-monitor");
> -        ADD_ARG_LIT(buf);
> +        ADD_ARG_LIT(argStr);
> +
> +        VIR_FREE(argStr);
>      }
>  
>      if (def->localtime)
> @@ -2059,14 +2046,18 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          ADD_ARG_LIT("none");
>      } else {
>          for (i = 0 ; i < def->nserials ; i++) {
> -            char buf[4096];
> +            virBuffer buf = VIR_BUFFER_INITIALIZER;
>              virDomainChrDefPtr serial = def->serials[i];
>  
> -            if (qemudBuildCommandLineChrDevStr(serial, buf, sizeof(buf)) < 0)
> +            qemudBuildCommandLineChrDevStr(serial, &buf);
> +            const char *argStr = virBufferContentAndReset(&buf);
> +            if (argStr == NULL)
>                  goto error;
>  
>              ADD_ARG_LIT("-serial");
> -            ADD_ARG_LIT(buf);
> +            ADD_ARG_LIT(argStr);
> +
> +            VIR_FREE(argStr);
>          }
>      }
>  
> @@ -2075,14 +2066,18 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          ADD_ARG_LIT("none");
>      } else {
>          for (i = 0 ; i < def->nparallels ; i++) {
> -            char buf[4096];
> +            virBuffer buf = VIR_BUFFER_INITIALIZER;
>              virDomainChrDefPtr parallel = def->parallels[i];
>  
> -            if (qemudBuildCommandLineChrDevStr(parallel, buf, sizeof(buf)) < 0)
> +            qemudBuildCommandLineChrDevStr(parallel, &buf);
> +            const char *argStr = virBufferContentAndReset(&buf);
> +            if (argStr == NULL)
>                  goto error;
>  
>              ADD_ARG_LIT("-parallel");
> -            ADD_ARG_LIT(buf);
> +            ADD_ARG_LIT(argStr);
> +
> +            VIR_FREE(argStr);

Instead of checking for NULL here, I prefer it when code uses an
explicit check on virBufferError(), which then guarentees the  data
is non-NULL. Also you can avoid the intermediate variable, by using
ADD_ARG instead of ADD_ARG_LIT

So, eg

             qemudBuildCommandLineChrDevStr(parallel, &buf);
             if (virBufferError(&buf))
                  goto error;

             ADD_ARG_LIT("-parallel");
             ADD_ARG_LIT(virBufferContentAndReset(&buf));


Likewise for the same code further up in yurou patch

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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