[libvirt] [PATCH 0/4] cleanup: Use bool type instead of bitfield - More
Which are missed with pattern ": 1" in the previous cleanup series. After these, all the bitfield use under src/conf/ are destroyed, except the ones which are required by the existing APIs. Osier Yang (4): cleanup: Change datatype of accel's members to boolean cleanup: Change datatype of graphic's members to boolean cleanup: Change datatype of usbdev->allow to boolean cleanup: Change datatype of net->stp to boolean src/conf/domain_conf.c | 44 ++++++++++++++++++++-------------------- src/conf/domain_conf.h | 20 +++++++++--------- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- src/parallels/parallels_driver.c | 4 ++-- src/qemu/qemu_command.c | 6 +++--- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 4 ++-- src/xenxs/xen_sxpr.c | 4 ++-- src/xenxs/xen_xm.c | 2 +- 10 files changed, 47 insertions(+), 47 deletions(-) -- 1.8.1.4
--- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 68f024f..62f448d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8150,17 +8150,17 @@ virDomainVideoAccelDefParseXML(const xmlNodePtr node) { if (support3d) { if (STREQ(support3d, "yes")) - def->support3d = 1; + def->support3d = true; else - def->support3d = 0; + def->support3d = false; VIR_FREE(support3d); } if (support2d) { if (STREQ(support2d, "yes")) - def->support2d = 1; + def->support2d = true; else - def->support2d = 0; + def->support2d = false; VIR_FREE(support2d); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3ab0f15..cae6547 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1188,8 +1188,8 @@ enum virDomainVideoType { typedef struct _virDomainVideoAccelDef virDomainVideoAccelDef; typedef virDomainVideoAccelDef *virDomainVideoAccelDefPtr; struct _virDomainVideoAccelDef { - unsigned int support3d :1; - unsigned int support2d :1; + bool support3d; + bool support2d; }; -- 1.8.1.4
--- src/conf/domain_conf.c | 32 ++++++++++++++++---------------- src/conf/domain_conf.h | 14 +++++++------- src/parallels/parallels_driver.c | 4 ++-- src/qemu/qemu_command.c | 6 +++--- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 4 ++-- src/xenxs/xen_sxpr.c | 4 ++-- src/xenxs/xen_xm.c | 2 +- 8 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 62f448d..70df165 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7297,18 +7297,18 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if (def->data.vnc.port == -1) { if (flags & VIR_DOMAIN_XML_INACTIVE) def->data.vnc.port = 0; - def->data.vnc.autoport = 1; + def->data.vnc.autoport = true; } } else { def->data.vnc.port = 0; - def->data.vnc.autoport = 1; + def->data.vnc.autoport = true; } if ((autoport = virXMLPropString(node, "autoport")) != NULL) { if (STREQ(autoport, "yes")) { if (flags & VIR_DOMAIN_XML_INACTIVE) def->data.vnc.port = 0; - def->data.vnc.autoport = 1; + def->data.vnc.autoport = true; } VIR_FREE(autoport); } @@ -7324,9 +7324,9 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if (fullscreen != NULL) { if (STREQ(fullscreen, "yes")) { - def->data.sdl.fullscreen = 1; + def->data.sdl.fullscreen = true; } else if (STREQ(fullscreen, "no")) { - def->data.sdl.fullscreen = 0; + def->data.sdl.fullscreen = false; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown fullscreen value '%s'"), fullscreen); @@ -7335,7 +7335,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, } VIR_FREE(fullscreen); } else { - def->data.sdl.fullscreen = 0; + def->data.sdl.fullscreen = false; } def->data.sdl.xauth = virXMLPropString(node, "xauth"); def->data.sdl.display = virXMLPropString(node, "display"); @@ -7354,17 +7354,17 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, } /* Legacy compat syntax, used -1 for auto-port */ if (def->data.rdp.port == -1) - def->data.rdp.autoport = 1; + def->data.rdp.autoport = true; VIR_FREE(port); } else { def->data.rdp.port = 0; - def->data.rdp.autoport = 1; + def->data.rdp.autoport = true; } if ((autoport = virXMLPropString(node, "autoport")) != NULL) { if (STREQ(autoport, "yes")) - def->data.rdp.autoport = 1; + def->data.rdp.autoport = true; VIR_FREE(autoport); } @@ -7374,14 +7374,14 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if ((replaceUser = virXMLPropString(node, "replaceUser")) != NULL) { if (STREQ(replaceUser, "yes")) { - def->data.rdp.replaceUser = 1; + def->data.rdp.replaceUser = true; } VIR_FREE(replaceUser); } if ((multiUser = virXMLPropString(node, "multiUser")) != NULL) { if (STREQ(multiUser, "yes")) { - def->data.rdp.multiUser = 1; + def->data.rdp.multiUser = true; } VIR_FREE(multiUser); } @@ -7391,9 +7391,9 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if (fullscreen != NULL) { if (STREQ(fullscreen, "yes")) { - def->data.desktop.fullscreen = 1; + def->data.desktop.fullscreen = true; } else if (STREQ(fullscreen, "no")) { - def->data.desktop.fullscreen = 0; + def->data.desktop.fullscreen = false; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown fullscreen value '%s'"), fullscreen); @@ -7402,7 +7402,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, } VIR_FREE(fullscreen); } else { - def->data.desktop.fullscreen = 0; + def->data.desktop.fullscreen = false; } def->data.desktop.display = virXMLPropString(node, "display"); @@ -7441,7 +7441,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if ((autoport = virXMLPropString(node, "autoport")) != NULL) { if (STREQ(autoport, "yes")) - def->data.spice.autoport = 1; + def->data.spice.autoport = true; VIR_FREE(autoport); } @@ -7461,7 +7461,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { /* Legacy compat syntax, used -1 for auto-port */ - def->data.spice.autoport = 1; + def->data.spice.autoport = true; } if (def->data.spice.autoport && (flags & VIR_DOMAIN_XML_INACTIVE)) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cae6547..7088bee 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1348,7 +1348,7 @@ struct _virDomainGraphicsDef { union { struct { int port; - unsigned int autoport :1; + bool autoport; char *keymap; char *socket; virDomainGraphicsAuthDef auth; @@ -1356,17 +1356,17 @@ struct _virDomainGraphicsDef { struct { char *display; char *xauth; - int fullscreen; + bool fullscreen; } sdl; struct { int port; - unsigned int autoport :1; - unsigned int replaceUser :1; - unsigned int multiUser :1; + bool autoport; + bool replaceUser; + bool multiUser; } rdp; struct { char *display; - unsigned int fullscreen :1; + bool fullscreen; } desktop; struct { int port; @@ -1374,7 +1374,7 @@ struct _virDomainGraphicsDef { int mousemode; char *keymap; virDomainGraphicsAuthDef auth; - unsigned int autoport :1; + bool autoport; int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST]; int defaultMode; /* enum virDomainGraphicsSpiceChannelMode */ int image; diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 451e948..5b8d85f 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -615,13 +615,13 @@ parallelsAddVNCInfo(virDomainDefPtr def, virJSONValuePtr jobj_root) if (STREQ(tmp, "auto")) { if (virJSONValueObjectGetNumberUint(jobj, "port", &port) < 0) port = 0; - gr->data.vnc.autoport = 1; + gr->data.vnc.autoport = true; } else { if (virJSONValueObjectGetNumberUint(jobj, "port", &port) < 0) { parallelsParseError(); goto cleanup; } - gr->data.vnc.autoport = 0; + gr->data.vnc.autoport = false; } gr->type = VIR_DOMAIN_GRAPHICS_TYPE_VNC; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d899239..ea31952 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9079,7 +9079,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, virDomainDefPtr def; int i; int nographics = 0; - int fullscreen = 0; + bool fullscreen = false; char *path; int nnics = 0; const char **nics = NULL; @@ -9244,7 +9244,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, goto no_memory; } vnc->data.vnc.port += 5900; - vnc->data.vnc.autoport = 0; + vnc->data.vnc.autoport = false; } if (VIR_REALLOC_N(def->graphics, def->ngraphics+1) < 0) { @@ -9401,7 +9401,7 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, } else if (STREQ(arg, "-nographic")) { nographics = 1; } else if (STREQ(arg, "-full-screen")) { - fullscreen = 1; + fullscreen = true; } else if (STREQ(arg, "-localtime")) { def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME; } else if (STREQ(arg, "-kernel")) { diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index ff8f9d1..0c82675 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2566,7 +2566,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { VBOX_UTF16_FREE(VRDEPortsValue); #endif /* VBOX_API_VERSION >= 4000 */ } else { - def->graphics[def->ngraphics]->data.rdp.autoport = 1; + def->graphics[def->ngraphics]->data.rdp.autoport = true; } def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_RDP; @@ -2590,12 +2590,12 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { VRDxServer->vtbl->GetAllowMultiConnection(VRDxServer, &allowMultiConnection); if (allowMultiConnection) { - def->graphics[def->ngraphics]->data.rdp.multiUser = 1; + def->graphics[def->ngraphics]->data.rdp.multiUser = true; } VRDxServer->vtbl->GetReuseSingleConnection(VRDxServer, &reuseSingleConnection); if (reuseSingleConnection) { - def->graphics[def->ngraphics]->data.rdp.replaceUser = 1; + def->graphics[def->ngraphics]->data.rdp.replaceUser = true; } def->ngraphics++; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 5dc5046..f520a85 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1870,14 +1870,14 @@ virVMXParseVNC(virConfPtr conf, virDomainGraphicsDefPtr *def) "is missing, the VNC port is unknown"); (*def)->data.vnc.port = 0; - (*def)->data.vnc.autoport = 1; + (*def)->data.vnc.autoport = true; } else { if (port < 5900 || port > 5964) { VIR_WARN("VNC port %lld it out of [5900..5964] range", port); } (*def)->data.vnc.port = port; - (*def)->data.vnc.autoport = 0; + (*def)->data.vnc.autoport = false; } return 0; diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index a179612..f2c345b 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -825,7 +825,7 @@ xenParseSxprGraphicsOld(virDomainDefPtr def, port = 5900 + def->id; if ((unused && STREQ(unused, "1")) || port == -1) - graphics->data.vnc.autoport = 1; + graphics->data.vnc.autoport = true; graphics->data.vnc.port = port; if (listenAddr && @@ -949,7 +949,7 @@ xenParseSxprGraphicsNew(virDomainDefPtr def, } if ((unused && STREQ(unused, "1")) || port == -1) - graphics->data.vnc.autoport = 1; + graphics->data.vnc.autoport = true; if (port >= 0 && port < 5900) port += 5900; diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index b2e7645..0e9f8d2 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1012,7 +1012,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { if (STRPREFIX(key, "vncunused=")) { if (STREQ(key + 10, "1")) - graphics->data.vnc.autoport = 1; + graphics->data.vnc.autoport = true; } else if (STRPREFIX(key, "vnclisten=")) { if (virDomainGraphicsListenSetAddress(graphics, 0, key+10, -1, true) < 0) -- 1.8.1.4
--- src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70df165..79a116f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8557,9 +8557,9 @@ virDomainRedirFilterUsbDevDefParseXML(const xmlNodePtr node) allow = virXMLPropString(node, "allow"); if (allow) { if (STREQ(allow, "yes")) - def->allow = 1; + def->allow = true; else if (STREQ(allow, "no")) - def->allow = 0; + def->allow = false; else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid allow value, either 'yes' or 'no'")); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7088bee..4033e4c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1413,7 +1413,7 @@ struct _virDomainRedirFilterUsbDevDef { int vendor; int product; int version; - unsigned int allow :1; + bool allow; }; struct _virDomainRedirFilterDef { -- 1.8.1.4
--- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 968cf11..75584a0 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1748,7 +1748,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); stp = virXPathString("string(./bridge[1]/@stp)", ctxt); - def->stp = (stp && STREQ(stp, "off")) ? 0 : 1; + def->stp = (stp && STREQ(stp, "off")) ? false : true; if (virXPathULong("string(./bridge[1]/@delay)", ctxt, &def->delay) < 0) def->delay = 0; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 6ce9a00..1a86e3d 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -195,7 +195,7 @@ struct _virNetworkDef { char *bridge; /* Name of bridge device */ char *domain; unsigned long delay; /* Bridge forward delay (ms) */ - unsigned int stp :1; /* Spanning tree protocol */ + bool stp; /* Spanning tree protocol */ virMacAddr mac; /* mac address of bridge device */ bool mac_specified; -- 1.8.1.4
On 04/12/2013 05:21 AM, Osier Yang wrote:
--- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 968cf11..75584a0 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1748,7 +1748,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); stp = virXPathString("string(./bridge[1]/@stp)", ctxt); - def->stp = (stp && STREQ(stp, "off")) ? 0 : 1; + def->stp = (stp && STREQ(stp, "off")) ? false : true;
Your conversion is fine, but this does highlight that the code here interprets *everything* except "off" as "on" (including "OFF", "no", "Off", "0"). There are actually many similar instances in the parser code. Should we continue to be so silently strict? Or complain any time the string isn't *exactly* "on" or "off"? Or make it extremely tolerant about what is entered (0, off, Off, no, false all mean the same thing)? Is it worth making a helper function that takes a const char * and returns true/false according to whatever rules we decide, then use that everywhere there is an on/off (or yes/no) attribute?
On 04/12/2013 08:38 AM, Laine Stump wrote:
On 04/12/2013 05:21 AM, Osier Yang wrote:
---
Your conversion is fine, but this does highlight that the code here interprets *everything* except "off" as "on" (including "OFF", "no", "Off", "0").
There are actually many similar instances in the parser code. Should we continue to be so silently strict? Or complain any time the string isn't *exactly* "on" or "off"? Or make it extremely tolerant about what is entered (0, off, Off, no, false all mean the same thing)?
It also begs the question - is our .rng schema capable of parsing the same set of options as our C code accepts, and flagging the typos?
Is it worth making a helper function that takes a const char * and returns true/false according to whatever rules we decide, then use that everywhere there is an on/off (or yes/no) attribute?
Yes, I think having a common parse helper function, as well as using a common <define> throughout the RNG schema that accepts the same things as the C code, would go a long way to making this friendlier to users. I also think that adding a way to canonicalize an XML snippet, including the option to reject it if it fails the RNG schema, would be a helpful addition to the API (we've mentioned the idea in the past, but it has not been implemented yet). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/04/13 23:17, Eric Blake wrote:
On 04/12/2013 08:38 AM, Laine Stump wrote:
On 04/12/2013 05:21 AM, Osier Yang wrote:
--- Your conversion is fine, but this does highlight that the code here interprets *everything* except "off" as "on" (including "OFF", "no", "Off", "0").
There are actually many similar instances in the parser code. Should we continue to be so silently strict? Or complain any time the string isn't *exactly* "on" or "off"? Or make it extremely tolerant about what is entered (0, off, Off, no, false all mean the same thing)? It also begs the question - is our .rng schema capable of parsing the same set of options as our C code accepts, and flagging the typos?
As far as I saw from the parsing code, the answer is "no", applies for most of the parser.
Is it worth making a helper function that takes a const char * and returns true/false according to whatever rules we decide, then use that everywhere there is an on/off (or yes/no) attribute? Yes, I think having a common parse helper function, as well as using a common <define> throughout the RNG schema that accepts the same things as the C code, would go a long way to making this friendlier to users.
Agreed, this can reduce much redundant code.
I also think that adding a way to canonicalize an XML snippet, including the option to reject it if it fails the RNG schema, would be a helpful addition to the API (we've mentioned the idea in the past, but it has not been implemented yet).
It was on my plate, feel sorry for I didn't finish it, though once raised up the proposal. Anyway, I was planning to pick it up again recently. Except the API, validating the XML when define the objects is nice to have too (though I'm a bit worried about the regressions, as the RNG schema could also have bugs). Osier
On 04/12/2013 05:21 AM, Osier Yang wrote:
Which are missed with pattern ": 1" in the previous cleanup series. After these, all the bitfield use under src/conf/ are destroyed, except the ones which are required by the existing APIs.
Osier Yang (4): cleanup: Change datatype of accel's members to boolean cleanup: Change datatype of graphic's members to boolean cleanup: Change datatype of usbdev->allow to boolean cleanup: Change datatype of net->stp to boolean
src/conf/domain_conf.c | 44 ++++++++++++++++++++-------------------- src/conf/domain_conf.h | 20 +++++++++--------- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- src/parallels/parallels_driver.c | 4 ++-- src/qemu/qemu_command.c | 6 +++--- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 4 ++-- src/xenxs/xen_sxpr.c | 4 ++-- src/xenxs/xen_xm.c | 2 +- 10 files changed, 47 insertions(+), 47 deletions(-)
Again mechanical useful cleanup. ACK to the series.
On 12/04/13 22:38, Laine Stump wrote:
On 04/12/2013 05:21 AM, Osier Yang wrote:
Which are missed with pattern ": 1" in the previous cleanup series. After these, all the bitfield use under src/conf/ are destroyed, except the ones which are required by the existing APIs.
Osier Yang (4): cleanup: Change datatype of accel's members to boolean cleanup: Change datatype of graphic's members to boolean cleanup: Change datatype of usbdev->allow to boolean cleanup: Change datatype of net->stp to boolean
src/conf/domain_conf.c | 44 ++++++++++++++++++++-------------------- src/conf/domain_conf.h | 20 +++++++++--------- src/conf/network_conf.c | 2 +- src/conf/network_conf.h | 2 +- src/parallels/parallels_driver.c | 4 ++-- src/qemu/qemu_command.c | 6 +++--- src/vbox/vbox_tmpl.c | 6 +++--- src/vmx/vmx.c | 4 ++-- src/xenxs/xen_sxpr.c | 4 ++-- src/xenxs/xen_xm.c | 2 +- 10 files changed, 47 insertions(+), 47 deletions(-)
Again mechanical useful cleanup. ACK to the series.
Thanks, I pushed it. Osier
participants (3)
-
Eric Blake -
Laine Stump -
Osier Yang