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

Re: [libvirt] [PATCH] qemu: minor cleanups



On Fri, Jan 31, 2014 at 05:08:32PM +0100, Martin Kletzander wrote:
> Some cleanups around serial chardevs and miscellaneous things I've
> found inconsistent or not very clean.

A few comments below, I'd tend to split at least the bigger bits in
separate patches to avoid a bunch of unrelated changes in a single commit

Christophe

> 
> Signed-off-by: Martin Kletzander <mkletzan redhat com>
> ---
>  src/conf/domain_conf.c       | 43 ++++++++++++++++++++++++++-----------------
>  src/qemu/qemu_capabilities.c |  4 ++--
>  src/qemu/qemu_command.c      | 10 ++++++++--
>  3 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 28e24f9..fa1ecb5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1559,7 +1559,7 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
>      if (tgt->type != src->type)
>          return false;
> 
> -    switch (src->type) {
> +    switch ((enum virDomainChrType)src->type) {

Not sure this one improves things, the type of the enum is in the
namespacing of the values anyway (VIR_DOMAIN_CHR_TYPE_*), and it's only
done for a minority of the switch() calls in domain_conf.c

>      case VIR_DOMAIN_CHR_TYPE_PTY:
>      case VIR_DOMAIN_CHR_TYPE_DEV:
>      case VIR_DOMAIN_CHR_TYPE_FILE:
> @@ -1583,16 +1583,22 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
>              STREQ_NULLABLE(src->data.nix.path, tgt->data.nix.path);
>          break;
> 
> +    case VIR_DOMAIN_CHR_TYPE_NULL:
>      case VIR_DOMAIN_CHR_TYPE_VC:
>      case VIR_DOMAIN_CHR_TYPE_STDIO:
>      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> +    case VIR_DOMAIN_CHR_TYPE_LAST:
>          /* nada */
>          return true;
>      }
> 
> -    /* This should happen only on new,
> -     * yet unhandled type */
> +    /* Even though gcc is able to detect that all possible values are
> +     * handled in the switch above, it is not capable of realizing
> +     * there isn't any possibility of escaping that switch without
> +     * return.  So for the sake of clean compilation, there has to be
> +     * a return here */
> 
> +    /* coverity[dead_error_begin] */
>      return false;
>  }
> 
> @@ -6415,7 +6421,7 @@ error:
>  }
> 
>  #define NET_MODEL_CHARS \
> -    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-"
> +    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
> 
>  /* Parse the XML definition for a network interface
>   * @param node XML nodeset to parse for net definition
> @@ -7182,11 +7188,11 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>      }
> 
>      switch ((enum virDomainChrType) def->type) {
> -    case VIR_DOMAIN_CHR_TYPE_LAST:
>      case VIR_DOMAIN_CHR_TYPE_NULL:
> +    case VIR_DOMAIN_CHR_TYPE_VC:
>      case VIR_DOMAIN_CHR_TYPE_STDIO:
>      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> -    case VIR_DOMAIN_CHR_TYPE_VC:
> +    case VIR_DOMAIN_CHR_TYPE_LAST:
>          break;
> 
>      case VIR_DOMAIN_CHR_TYPE_PTY:
> @@ -15573,11 +15579,13 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>      }
>      virBufferAddLit(buf, ">\n");
> 
> -    switch (def->type) {
> +    virBufferAdjustIndent(buf, 6);
> +    switch ((enum virDomainChrType)def->type) {
>      case VIR_DOMAIN_CHR_TYPE_NULL:
>      case VIR_DOMAIN_CHR_TYPE_VC:
>      case VIR_DOMAIN_CHR_TYPE_STDIO:
>      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> +    case VIR_DOMAIN_CHR_TYPE_LAST:
>          /* nada */
>          break;
> 
> @@ -15588,7 +15596,7 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          if (def->type != VIR_DOMAIN_CHR_TYPE_PTY ||
>              (def->data.file.path &&
>               !(flags & VIR_DOMAIN_XML_INACTIVE))) {
> -            virBufferEscapeString(buf, "      <source path='%s'/>\n",
> +            virBufferEscapeString(buf, "<source path='%s'/>\n",
>                                    def->data.file.path);
>          }
>          break;
> @@ -15597,53 +15605,54 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>          if (def->data.udp.bindService &&
>              def->data.udp.bindHost) {
>              virBufferAsprintf(buf,
> -                              "      <source mode='bind' host='%s' "
> +                              "<source mode='bind' host='%s' "
>                                "service='%s'/>\n",
>                                def->data.udp.bindHost,
>                                def->data.udp.bindService);
>          } else if (def->data.udp.bindHost) {
> -            virBufferAsprintf(buf, "      <source mode='bind' host='%s'/>\n",
> +            virBufferAsprintf(buf, "<source mode='bind' host='%s'/>\n",
>                                def->data.udp.bindHost);
>          } else if (def->data.udp.bindService) {
> -            virBufferAsprintf(buf, "      <source mode='bind' service='%s'/>\n",
> +            virBufferAsprintf(buf, "<source mode='bind' service='%s'/>\n",
>                                def->data.udp.bindService);
>          }
> 
>          if (def->data.udp.connectService &&
>              def->data.udp.connectHost) {
>              virBufferAsprintf(buf,
> -                              "      <source mode='connect' host='%s' "
> +                              "<source mode='connect' host='%s' "
>                                "service='%s'/>\n",
>                                def->data.udp.connectHost,
>                                def->data.udp.connectService);
>          } else if (def->data.udp.connectHost) {
> -            virBufferAsprintf(buf, "      <source mode='connect' host='%s'/>\n",
> +            virBufferAsprintf(buf, "<source mode='connect' host='%s'/>\n",
>                                def->data.udp.connectHost);
>          } else if (def->data.udp.connectService) {
>              virBufferAsprintf(buf,
> -                              "      <source mode='connect' service='%s'/>\n",
> +                              "<source mode='connect' service='%s'/>\n",
>                                def->data.udp.connectService);
>          }
>          break;
> 
>      case VIR_DOMAIN_CHR_TYPE_TCP:
>          virBufferAsprintf(buf,
> -                          "      <source mode='%s' host='%s' service='%s'/>\n",
> +                          "<source mode='%s' host='%s' service='%s'/>\n",
>                            def->data.tcp.listen ? "bind" : "connect",
>                            def->data.tcp.host,
>                            def->data.tcp.service);
> -        virBufferAsprintf(buf, "      <protocol type='%s'/>\n",
> +        virBufferAsprintf(buf, "<protocol type='%s'/>\n",
>                            virDomainChrTcpProtocolTypeToString(
>                                def->data.tcp.protocol));
>          break;
> 
>      case VIR_DOMAIN_CHR_TYPE_UNIX:
> -        virBufferAsprintf(buf, "      <source mode='%s'",
> +        virBufferAsprintf(buf, "<source mode='%s'",
>                            def->data.nix.listen ? "bind" : "connect");
>          virBufferEscapeString(buf, " path='%s'", def->data.nix.path);
>          virBufferAddLit(buf, "/>\n");
>          break;
>      }
> +    virBufferAdjustIndent(buf, -6);
> 
>      return 0;
>  }
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8420047..8aec293 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_capabilities.c: QEMU capabilities generation
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -247,7 +247,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "boot-strict", /* 160 */
>                "pvpanic",
>                "enable-fips",
> -              "spice-file-xfer-disable"
> +              "spice-file-xfer-disable",
>      );
> 
>  struct _virQEMUCaps {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 96b8825..2db745a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_command.c: QEMU command generation
>   *
> - * Copyright (C) 2006-2013 Red Hat, Inc.
> + * Copyright (C) 2006-2014 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -6004,7 +6004,7 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix)
>      if (prefix)
>          virBufferAdd(&buf, prefix, strlen(prefix));
> 
> -    switch (dev->type) {
> +    switch ((enum virDomainChrType)dev->type) {
>      case VIR_DOMAIN_CHR_TYPE_NULL:
>          virBufferAddLit(&buf, "null");
>          break;
> @@ -6071,6 +6071,12 @@ qemuBuildChrArgStr(virDomainChrSourceDefPtr dev, const char *prefix)
>                            dev->data.nix.path,
>                            dev->data.nix.listen ? ",server,nowait" : "");
>          break;
> +
> +    case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> +        /* spicevmc doesn't have any '-serial' compatible option */

This is misleading as this function is also called for -monitor, and
-parallel.

> +    case VIR_DOMAIN_CHR_TYPE_LAST:
> +        /* coverity[dead_error_begin] */
> +        break;
>      }
> 
>      if (virBufferError(&buf)) {
> -- 
> 1.8.5.3
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: pgpD8yitfSAqY.pgp
Description: PGP signature


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