[libvirt] [PATCH] qemu: minor cleanups
Some cleanups around serial chardevs and miscellaneous things I've found inconsistent or not very clean. 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) { 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 */ + case VIR_DOMAIN_CHR_TYPE_LAST: + /* coverity[dead_error_begin] */ + break; } if (virBufferError(&buf)) { -- 1.8.5.3
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
On 02/03/2014 10:52 AM, Christophe Fergeau wrote:
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
I agree - please break up into smaller pieces, since it makes backporting easier (not all the problems being cleaned here were introduced in the same release).
- 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
Adding the explicit cast DOES help - it makes the compiler flag a warning (which with -Werror is fatal) if the user adds an enum value but forgets to update this switch statement.
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;
Rather than a big long hairy comment here, I'd go with the simpler: ... case VIR_DOMAIN_CHR_TYPE_LAST: /* nada */ break; } return true; so that at least one case falls out of the switch statement, and then the ending return is no longer dead code.
#define NET_MODEL_CHARS \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
Wonder what happened to cause the duplication in the original? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Feb 03, 2014 at 11:10:10AM -0700, Eric Blake wrote:
On 02/03/2014 10:52 AM, Christophe Fergeau wrote:
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
I agree - please break up into smaller pieces, since it makes backporting easier (not all the problems being cleaned here were introduced in the same release).
OK, will do, I just missed your mail when replying to Christophe.
- 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
Adding the explicit cast DOES help - it makes the compiler flag a warning (which with -Werror is fatal) if the user adds an enum value but forgets to update this switch statement.
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;
Rather than a big long hairy comment here, I'd go with the simpler:
... case VIR_DOMAIN_CHR_TYPE_LAST: /* nada */ break; }
return true;
so that at least one case falls out of the switch statement, and then the ending return is no longer dead code.
#define NET_MODEL_CHARS \ - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-"
Wonder what happened to cause the duplication in the original?
I can only guess that it might have been a change from 0-9, but I can't find anyone better than you to ask ;) v2 coming up Martin
On Mon, Feb 03, 2014 at 06:52:49PM +0100, Christophe Fergeau wrote:
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
If you want to, I have no problems doing that, I just thought these are all basically no-op clean-ups anyway.
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
Actually, for now it doesn't. But it might do the job of future problem catcher if new values are added to that enum and it might be desirable to add them to this switch (as compiler will find that). In case they are not, adding them to the no-op branch shows that the author of such patch probably thought it through (or at least know that there's a code taking decision based on such value). I like the idea of listing all the enums and compiler reminding about places depending on them. Also, listing them (at least for no-op cases) in the same order that's in the enum makes it faster to skim through the code someimes in case there's more of them. That motivated me to fix a least few of these switches.
case VIR_DOMAIN_CHR_TYPE_PTY: case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE:
[...]
@@ -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.
Oh, that's right. I'll just join it with the below one if omitting the comment is ok. Thanks for looking at the patch, Martin
+ 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
On Mon, Feb 03, 2014 at 09:31:11PM +0100, Martin Kletzander wrote:
On Mon, Feb 03, 2014 at 06:52:49PM +0100, Christophe Fergeau wrote:
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
I like the idea of listing all the enums and compiler reminding about places depending on them.
Yup, I did not realize the cast would let the compiler warn about missing enum values, so just disregard my comment about the casts. Thanks to Eric and you for the explanation :) Christophe
participants (3)
-
Christophe Fergeau -
Eric Blake -
Martin Kletzander