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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list