[libvirt PATCH 00/14] Random fixes and improvements
A collection of drive-by fixes and improvements that quickly derailed into the g_auto-ification of src/conf/interface_conf.c. Tim Wiederhake (14): docs: coding-style: Clarify on virXXXPtr types virQEMUCapsSEVInfoCopy: Remove superfluous g_auto usage virInterfaceDefDevFormat: Add missing error handling conf: interface: Preparation for g_auto conf: interface: Use g_auto conf: interface: Remove ret and goto virInterfaceDefParseXML: Inline trivial virInterfaceDefParseName virInterfaceDefParseIP: Simplify and cleanup virInterfaceDefParseDhcp: Simplify and cleanup virInterfaceDefParseProtoIPv4: Simplify and cleanup virInterfaceDefParseProtoIPv6: Simplify and cleanup virInterfaceDefParseIfAdressing: Simplify and cleanup virInterfaceDefParseXML: Simplify and cleanup virInterfaceDefParse: Simplify and cleanup docs/coding-style.rst | 5 + src/conf/interface_conf.c | 357 ++++++++++++----------------------- src/qemu/qemu_capabilities.c | 4 +- 3 files changed, 128 insertions(+), 238 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..ab7634dc14 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + :: typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... }; + Historically, libvirt also declared pointer types 'virXXXPtr', + which have been dropped where possible. Do not introduce new + such types. + Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word -- 2.31.1
On 1/12/22 14:10, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- docs/coding-style.rst | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 470c61860f..ab7634dc14 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -54,6 +54,7 @@ Struct type names and each following word should have its first letter in uppercase. The struct name should be the same as the typedef name with a leading underscore. + ::
typedef struct _virHashTable virHashTable; @@ -61,6 +62,10 @@ Struct type names ... };
+ Historically, libvirt also declared pointer types 'virXXXPtr', + which have been dropped where possible. Do not introduce new + such types. +
Maybe: Do not introduce new such types for internal types. We do want those types for public APIs to stay consistent.
Function names All functions should have a 'vir' prefix in their name, followed by one or more words with first letter of each word
Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5f1eb5014c..f8db6fd370 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1891,7 +1891,7 @@ static int virQEMUCapsSEVInfoCopy(virSEVCapability **dst, virSEVCapability *src) { - g_autoptr(virSEVCapability) tmp = NULL; + virSEVCapability *tmp; if (!src) { *dst = NULL; @@ -1908,7 +1908,7 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst, tmp->max_guests = src->max_guests; tmp->max_es_guests = src->max_es_guests; - *dst = g_steal_pointer(&tmp); + *dst = tmp; return 0; } -- 2.31.1
On 1/12/22 14:10, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5f1eb5014c..f8db6fd370 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1891,7 +1891,7 @@ static int virQEMUCapsSEVInfoCopy(virSEVCapability **dst, virSEVCapability *src) { - g_autoptr(virSEVCapability) tmp = NULL; + virSEVCapability *tmp;
if (!src) { *dst = NULL; @@ -1908,7 +1908,7 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst, tmp->max_guests = src->max_guests; tmp->max_es_guests = src->max_es_guests;
- *dst = g_steal_pointer(&tmp); + *dst = tmp; return 0; }
I have to admit that I find code as-is more future proof. If we ever want to add a check there, it can simply return NULL, -1 or whatever. But at the same time it's matter of preference. Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index f2b3804bec..150616bda9 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -1089,13 +1089,16 @@ virInterfaceDefDevFormat(virBuffer *buf, virBufferAsprintf(buf, "<mac address='%s'/>\n", def->mac); break; case VIR_INTERFACE_TYPE_BRIDGE: - virInterfaceBridgeDefFormat(buf, def); + if (virInterfaceBridgeDefFormat(buf, def) < 0) + return -1; break; case VIR_INTERFACE_TYPE_BOND: - virInterfaceBondDefFormat(buf, def); + if (virInterfaceBondDefFormat(buf, def) < 0) + return -1; break; case VIR_INTERFACE_TYPE_VLAN: - virInterfaceVlanDefFormat(buf, def); + if (virInterfaceVlanDefFormat(buf, def) < 0) + return -1; break; } -- 2.31.1
These changes make the g_auto-ification in the next commit clearer. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 150616bda9..cbe6aad957 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -453,14 +453,12 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, } proto->family = tmp; if (STREQ(tmp, "ipv4")) { - ret = virInterfaceDefParseProtoIPv4(proto, ctxt); - if (ret != 0) { + if (virInterfaceDefParseProtoIPv4(proto, ctxt) != 0) { virInterfaceProtocolDefFree(proto); goto error; } } else if (STREQ(tmp, "ipv6")) { - ret = virInterfaceDefParseProtoIPv6(proto, ctxt); - if (ret != 0) { + if (virInterfaceDefParseProtoIPv6(proto, ctxt) != 0) { virInterfaceProtocolDefFree(proto); goto error; } @@ -741,10 +739,12 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, } switch (type) { - case VIR_INTERFACE_TYPE_ETHERNET: - if ((tmp = virXPathString("string(./mac/@address)", ctxt))) - def->mac = tmp; + case VIR_INTERFACE_TYPE_ETHERNET: { + char *mac = virXPathString("string(./mac/@address)", ctxt); + if (mac != NULL) + def->mac = mac; break; + } case VIR_INTERFACE_TYPE_BRIDGE: { xmlNodePtr bridge; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 67 ++++++++++++--------------------------- 1 file changed, 20 insertions(+), 47 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index cbe6aad957..4b464bdf4f 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -68,6 +68,7 @@ virInterfaceProtocolDefFree(virInterfaceProtocolDef *def) g_free(def->gateway); g_free(def); } +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virInterfaceProtocolDef, virInterfaceProtocolDefFree); void @@ -155,9 +156,8 @@ static int virInterfaceDefParseStartMode(virInterfaceDef *def, xmlXPathContextPtr ctxt) { - char *tmp; + g_autofree char *tmp = virXPathString("string(./start/@mode)", ctxt); - tmp = virXPathString("string(./start/@mode)", ctxt); if (tmp == NULL) { def->startmode = VIR_INTERFACE_START_UNSPECIFIED; } else if (STREQ(tmp, "onboot")) { @@ -169,10 +169,8 @@ virInterfaceDefParseStartMode(virInterfaceDef *def, } else { virReportError(VIR_ERR_XML_ERROR, _("unknown interface startmode %s"), tmp); - VIR_FREE(tmp); return -1; } - VIR_FREE(tmp); return 0; } @@ -180,10 +178,9 @@ virInterfaceDefParseStartMode(virInterfaceDef *def, static int virInterfaceDefParseBondMode(xmlXPathContextPtr ctxt) { - char *tmp; + g_autofree char *tmp = virXPathString("string(./@mode)", ctxt); int ret = 0; - tmp = virXPathString("string(./@mode)", ctxt); if (tmp == NULL) return VIR_INTERFACE_BOND_NONE; if (STREQ(tmp, "balance-rr")) { @@ -205,7 +202,6 @@ virInterfaceDefParseBondMode(xmlXPathContextPtr ctxt) _("unknown bonding mode %s"), tmp); ret = -1; } - VIR_FREE(tmp); return ret; } @@ -213,10 +209,9 @@ virInterfaceDefParseBondMode(xmlXPathContextPtr ctxt) static int virInterfaceDefParseBondMiiCarrier(xmlXPathContextPtr ctxt) { - char *tmp; + g_autofree char *tmp = virXPathString("string(./miimon/@carrier)", ctxt); int ret = 0; - tmp = virXPathString("string(./miimon/@carrier)", ctxt); if (tmp == NULL) return VIR_INTERFACE_BOND_MII_NONE; if (STREQ(tmp, "ioctl")) { @@ -228,7 +223,6 @@ virInterfaceDefParseBondMiiCarrier(xmlXPathContextPtr ctxt) _("unknown mii bonding carrier %s"), tmp); ret = -1; } - VIR_FREE(tmp); return ret; } @@ -236,10 +230,9 @@ virInterfaceDefParseBondMiiCarrier(xmlXPathContextPtr ctxt) static int virInterfaceDefParseBondArpValid(xmlXPathContextPtr ctxt) { - char *tmp; + g_autofree char *tmp = virXPathString("string(./arpmon/@validate)", ctxt); int ret = 0; - tmp = virXPathString("string(./arpmon/@validate)", ctxt); if (tmp == NULL) return VIR_INTERFACE_BOND_ARP_NONE; if (STREQ(tmp, "active")) { @@ -253,7 +246,6 @@ virInterfaceDefParseBondArpValid(xmlXPathContextPtr ctxt) _("unknown arp bonding validate %s"), tmp); ret = -1; } - VIR_FREE(tmp); return ret; } @@ -263,7 +255,7 @@ virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, xmlNodePtr dhcp, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - char *tmp; + g_autofree char *tmp = NULL; int ret = 0; def->dhcp = 1; @@ -280,7 +272,6 @@ virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, } else { def->peerdns = state ? 1 : 0; } - VIR_FREE(tmp); } return ret; @@ -317,7 +308,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, xmlXPathContextPtr ctxt) { xmlNodePtr dhcp; - xmlNodePtr *ipNodes = NULL; + g_autofree xmlNodePtr *ipNodes = NULL; int nipNodes, ret = -1; size_t i; char *tmp; @@ -357,7 +348,6 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, ret = 0; error: - VIR_FREE(ipNodes); return ret; } @@ -367,7 +357,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, xmlXPathContextPtr ctxt) { xmlNodePtr dhcp, autoconf; - xmlNodePtr *ipNodes = NULL; + g_autofree xmlNodePtr *ipNodes = NULL; int nipNodes, ret = -1; size_t i; char *tmp; @@ -411,7 +401,6 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, ret = 0; error: - VIR_FREE(ipNodes); return ret; } @@ -421,7 +410,7 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - xmlNodePtr *protoNodes = NULL; + g_autofree xmlNodePtr *protoNodes = NULL; int nProtoNodes, pp, ret = -1; char *tmp; @@ -439,42 +428,33 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, def->nprotos = 0; for (pp = 0; pp < nProtoNodes; pp++) { - virInterfaceProtocolDef *proto; - - proto = g_new0(virInterfaceProtocolDef, 1); + g_autoptr(virInterfaceProtocolDef) proto = g_new0(virInterfaceProtocolDef, 1); ctxt->node = protoNodes[pp]; tmp = virXPathString("string(./@family)", ctxt); if (tmp == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("protocol misses the family attribute")); - virInterfaceProtocolDefFree(proto); goto error; } proto->family = tmp; if (STREQ(tmp, "ipv4")) { - if (virInterfaceDefParseProtoIPv4(proto, ctxt) != 0) { - virInterfaceProtocolDefFree(proto); + if (virInterfaceDefParseProtoIPv4(proto, ctxt) != 0) goto error; - } } else if (STREQ(tmp, "ipv6")) { - if (virInterfaceDefParseProtoIPv6(proto, ctxt) != 0) { - virInterfaceProtocolDefFree(proto); + if (virInterfaceDefParseProtoIPv6(proto, ctxt) != 0) goto error; - } } else { virReportError(VIR_ERR_XML_ERROR, _("unsupported protocol family '%s'"), tmp); - virInterfaceProtocolDefFree(proto); goto error; } - def->protos[def->nprotos++] = proto; + def->protos[def->nprotos++] = g_steal_pointer(&proto); } ret = 0; error: - VIR_FREE(protoNodes); return ret; } @@ -484,17 +464,16 @@ static int virInterfaceDefParseBridge(virInterfaceDef *def, xmlXPathContextPtr ctxt) { - xmlNodePtr *interfaces = NULL; - xmlNodePtr bridge; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autofree xmlNodePtr *interfaces = NULL; virInterfaceDef *itf; - char *tmp = NULL; + g_autofree char *tmp = NULL; int nbItf; size_t i; int ret = 0; - bridge = ctxt->node; def->data.bridge.stp = -1; - if ((tmp = virXMLPropString(bridge, "stp"))) { + if ((tmp = virXMLPropString(ctxt->node, "stp"))) { if (STREQ(tmp, "on")) { def->data.bridge.stp = 1; } else if (STREQ(tmp, "off")) { @@ -506,7 +485,7 @@ virInterfaceDefParseBridge(virInterfaceDef *def, goto error; } } - def->data.bridge.delay = virXMLPropString(bridge, "delay"); + def->data.bridge.delay = virXMLPropString(ctxt->node, "delay"); nbItf = virXPathNodeSet("./interface", ctxt, &interfaces); if (nbItf < 0) { @@ -530,9 +509,6 @@ virInterfaceDefParseBridge(virInterfaceDef *def, } error: - VIR_FREE(tmp); - VIR_FREE(interfaces); - ctxt->node = bridge; return ret; } @@ -541,8 +517,8 @@ static int virInterfaceDefParseBondItfs(virInterfaceDef *def, xmlXPathContextPtr ctxt) { - xmlNodePtr *interfaces = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autofree xmlNodePtr *interfaces = NULL; virInterfaceDef *itf; int nbItf; size_t i; @@ -573,7 +549,6 @@ virInterfaceDefParseBondItfs(virInterfaceDef *def, ret = 0; cleanup: - VIR_FREE(interfaces); return ret; } @@ -679,7 +654,7 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, { g_autoptr(virInterfaceDef) def = NULL; int type; - char *tmp; + g_autofree char *tmp = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr lnk; @@ -695,10 +670,8 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, if (type == -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown interface type %s"), tmp); - VIR_FREE(tmp); return NULL; } - VIR_FREE(tmp); def = g_new0(virInterfaceDef, 1); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 129 +++++++++++++++----------------------- 1 file changed, 49 insertions(+), 80 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 4b464bdf4f..47f9da797d 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -179,30 +179,27 @@ static int virInterfaceDefParseBondMode(xmlXPathContextPtr ctxt) { g_autofree char *tmp = virXPathString("string(./@mode)", ctxt); - int ret = 0; if (tmp == NULL) return VIR_INTERFACE_BOND_NONE; if (STREQ(tmp, "balance-rr")) { - ret = VIR_INTERFACE_BOND_BALRR; + return VIR_INTERFACE_BOND_BALRR; } else if (STREQ(tmp, "active-backup")) { - ret = VIR_INTERFACE_BOND_ABACKUP; + return VIR_INTERFACE_BOND_ABACKUP; } else if (STREQ(tmp, "balance-xor")) { - ret = VIR_INTERFACE_BOND_BALXOR; + return VIR_INTERFACE_BOND_BALXOR; } else if (STREQ(tmp, "broadcast")) { - ret = VIR_INTERFACE_BOND_BCAST; + return VIR_INTERFACE_BOND_BCAST; } else if (STREQ(tmp, "802.3ad")) { - ret = VIR_INTERFACE_BOND_8023AD; + return VIR_INTERFACE_BOND_8023AD; } else if (STREQ(tmp, "balance-tlb")) { - ret = VIR_INTERFACE_BOND_BALTLB; + return VIR_INTERFACE_BOND_BALTLB; } else if (STREQ(tmp, "balance-alb")) { - ret = VIR_INTERFACE_BOND_BALALB; - } else { - virReportError(VIR_ERR_XML_ERROR, - _("unknown bonding mode %s"), tmp); - ret = -1; + return VIR_INTERFACE_BOND_BALALB; } - return ret; + + virReportError(VIR_ERR_XML_ERROR, _("unknown bonding mode %s"), tmp); + return -1; } @@ -210,20 +207,17 @@ static int virInterfaceDefParseBondMiiCarrier(xmlXPathContextPtr ctxt) { g_autofree char *tmp = virXPathString("string(./miimon/@carrier)", ctxt); - int ret = 0; if (tmp == NULL) return VIR_INTERFACE_BOND_MII_NONE; if (STREQ(tmp, "ioctl")) { - ret = VIR_INTERFACE_BOND_MII_IOCTL; + return VIR_INTERFACE_BOND_MII_IOCTL; } else if (STREQ(tmp, "netif")) { - ret = VIR_INTERFACE_BOND_MII_NETIF; - } else { - virReportError(VIR_ERR_XML_ERROR, - _("unknown mii bonding carrier %s"), tmp); - ret = -1; + return VIR_INTERFACE_BOND_MII_NETIF; } - return ret; + + virReportError(VIR_ERR_XML_ERROR, _("unknown mii bonding carrier %s"), tmp); + return -1; } @@ -231,22 +225,19 @@ static int virInterfaceDefParseBondArpValid(xmlXPathContextPtr ctxt) { g_autofree char *tmp = virXPathString("string(./arpmon/@validate)", ctxt); - int ret = 0; if (tmp == NULL) return VIR_INTERFACE_BOND_ARP_NONE; if (STREQ(tmp, "active")) { - ret = VIR_INTERFACE_BOND_ARP_ACTIVE; + return VIR_INTERFACE_BOND_ARP_ACTIVE; } else if (STREQ(tmp, "backup")) { - ret = VIR_INTERFACE_BOND_ARP_BACKUP; + return VIR_INTERFACE_BOND_ARP_BACKUP; } else if (STREQ(tmp, "all")) { - ret = VIR_INTERFACE_BOND_ARP_ALL; - } else { - virReportError(VIR_ERR_XML_ERROR, - _("unknown arp bonding validate %s"), tmp); - ret = -1; + return VIR_INTERFACE_BOND_ARP_ALL; } - return ret; + + virReportError(VIR_ERR_XML_ERROR, _("unknown arp bonding validate %s"), tmp); + return -1; } @@ -256,7 +247,6 @@ virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, { VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree char *tmp = NULL; - int ret = 0; def->dhcp = 1; ctxt->node = dhcp; @@ -268,13 +258,12 @@ virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, if (virStringParseYesNo(tmp, &state) < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown dhcp peerdns value %s"), tmp); - ret = -1; - } else { - def->peerdns = state ? 1 : 0; + return -1; } + def->peerdns = state ? 1 : 0; } - return ret; + return 0; } @@ -309,7 +298,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, { xmlNodePtr dhcp; g_autofree xmlNodePtr *ipNodes = NULL; - int nipNodes, ret = -1; + int nipNodes; size_t i; char *tmp; @@ -340,15 +329,12 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, ctxt->node = ipNodes[i]; if (virInterfaceDefParseIP(ip, ctxt) < 0) { virInterfaceIPDefFree(ip); - goto error; + return -1; } def->ips[def->nips++] = ip; } - ret = 0; - - error: - return ret; + return 0; } @@ -358,7 +344,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, { xmlNodePtr dhcp, autoconf; g_autofree xmlNodePtr *ipNodes = NULL; - int nipNodes, ret = -1; + int nipNodes; size_t i; char *tmp; @@ -393,15 +379,12 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, ctxt->node = ipNodes[i]; if (virInterfaceDefParseIP(ip, ctxt) < 0) { virInterfaceIPDefFree(ip); - goto error; + return -1; } def->ips[def->nips++] = ip; } - ret = 0; - - error: - return ret; + return 0; } @@ -411,12 +394,12 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, { VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree xmlNodePtr *protoNodes = NULL; - int nProtoNodes, pp, ret = -1; + int nProtoNodes, pp; char *tmp; nProtoNodes = virXPathNodeSet("./protocol", ctxt, &protoNodes); if (nProtoNodes < 0) - goto error; + return -1; if (nProtoNodes == 0) { /* no protocols is an acceptable outcome */ @@ -435,28 +418,24 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, if (tmp == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("protocol misses the family attribute")); - goto error; + return -1; } proto->family = tmp; if (STREQ(tmp, "ipv4")) { if (virInterfaceDefParseProtoIPv4(proto, ctxt) != 0) - goto error; + return -1; } else if (STREQ(tmp, "ipv6")) { if (virInterfaceDefParseProtoIPv6(proto, ctxt) != 0) - goto error; + return -1; } else { virReportError(VIR_ERR_XML_ERROR, _("unsupported protocol family '%s'"), tmp); - goto error; + return -1; } def->protos[def->nprotos++] = g_steal_pointer(&proto); } - ret = 0; - - error: - return ret; - + return 0; } @@ -470,7 +449,6 @@ virInterfaceDefParseBridge(virInterfaceDef *def, g_autofree char *tmp = NULL; int nbItf; size_t i; - int ret = 0; def->data.bridge.stp = -1; if ((tmp = virXMLPropString(ctxt->node, "stp"))) { @@ -482,15 +460,14 @@ virInterfaceDefParseBridge(virInterfaceDef *def, virReportError(VIR_ERR_XML_ERROR, _("bridge interface stp should be on or off got %s"), tmp); - goto error; + return 0; } } def->data.bridge.delay = virXMLPropString(ctxt->node, "delay"); nbItf = virXPathNodeSet("./interface", ctxt, &interfaces); if (nbItf < 0) { - ret = -1; - goto error; + return -1; } if (nbItf > 0) { def->data.bridge.itf = g_new0(struct _virInterfaceDef *, nbItf); @@ -500,16 +477,14 @@ virInterfaceDefParseBridge(virInterfaceDef *def, ctxt->node = interfaces[i]; itf = virInterfaceDefParseXML(ctxt, VIR_INTERFACE_TYPE_BRIDGE); if (itf == NULL) { - ret = -1; def->data.bridge.nbItf = i; - goto error; + return -1; } def->data.bridge.itf[i] = itf; } } - error: - return ret; + return 0; } @@ -522,15 +497,13 @@ virInterfaceDefParseBondItfs(virInterfaceDef *def, virInterfaceDef *itf; int nbItf; size_t i; - int ret = -1; nbItf = virXPathNodeSet("./interface", ctxt, &interfaces); if (nbItf < 0) - goto cleanup; + return -1; if (nbItf == 0) { - ret = 0; - goto cleanup; + return 0; } def->data.bond.itf = g_new0(struct _virInterfaceDef *, nbItf); @@ -542,14 +515,12 @@ virInterfaceDefParseBondItfs(virInterfaceDef *def, itf = virInterfaceDefParseXML(ctxt, VIR_INTERFACE_TYPE_BOND); if (itf == NULL) { def->data.bond.nbItf = i; - goto cleanup; + return -1; } def->data.bond.itf[i] = itf; } - ret = 0; - cleanup: - return ret; + return 0; } @@ -823,7 +794,6 @@ virInterfaceBridgeDefFormat(virBuffer *buf, const virInterfaceDef *def) { size_t i; - int ret = 0; virBufferAddLit(buf, "<bridge"); if (def->data.bridge.stp == 1) @@ -838,12 +808,12 @@ virInterfaceBridgeDefFormat(virBuffer *buf, for (i = 0; i < def->data.bridge.nbItf; i++) { if (virInterfaceDefDevFormat(buf, def->data.bridge.itf[i], VIR_INTERFACE_TYPE_BRIDGE) < 0) - ret = -1; + return -1; } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</bridge>\n"); - return ret; + return 0; } @@ -852,7 +822,6 @@ virInterfaceBondDefFormat(virBuffer *buf, const virInterfaceDef *def) { size_t i; - int ret = 0; virBufferAddLit(buf, "<bond"); if (def->data.bond.mode == VIR_INTERFACE_BOND_BALRR) @@ -903,12 +872,12 @@ virInterfaceBondDefFormat(virBuffer *buf, for (i = 0; i < def->data.bond.nbItf; i++) { if (virInterfaceDefDevFormat(buf, def->data.bond.itf[i], VIR_INTERFACE_TYPE_BOND) < 0) - ret = -1; + return -1; } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</bond>\n"); - return ret; + return 0; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 47f9da797d..f3fc47bd69 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -116,23 +116,6 @@ virInterfaceDefFree(virInterfaceDef *def) } -static int -virInterfaceDefParseName(virInterfaceDef *def, - xmlXPathContextPtr ctxt) -{ - char *tmp; - - tmp = virXPathString("string(./@name)", ctxt); - if (tmp == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("interface has no name")); - return -1; - } - def->name = tmp; - return 0; -} - - static int virInterfaceDefParseMtu(virInterfaceDef *def, xmlXPathContextPtr ctxt) @@ -662,8 +645,10 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, } def->type = type; - if (virInterfaceDefParseName(def, ctxt) < 0) - return NULL; + if ((def->name = virXMLPropString(ctxt->node, "name")) == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("interface has no name")); + return NULL; + } if (parentIfType == VIR_INTERFACE_TYPE_LAST) { /* only recognize these in toplevel bond interfaces */ -- 2.31.1
On 1/12/22 14:10, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 47f9da797d..f3fc47bd69 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -116,23 +116,6 @@ virInterfaceDefFree(virInterfaceDef *def) }
-static int -virInterfaceDefParseName(virInterfaceDef *def, - xmlXPathContextPtr ctxt) -{ - char *tmp; - - tmp = virXPathString("string(./@name)", ctxt); - if (tmp == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("interface has no name")); - return -1; - } - def->name = tmp; - return 0; -} - - static int virInterfaceDefParseMtu(virInterfaceDef *def, xmlXPathContextPtr ctxt) @@ -662,8 +645,10 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, } def->type = type;
- if (virInterfaceDefParseName(def, ctxt) < 0) - return NULL; + if ((def->name = virXMLPropString(ctxt->node, "name")) == NULL) {
At your discretion (here and in the rest of patches): if (!(def->name = ...))) {
+ virReportError(VIR_ERR_XML_ERROR, "%s", _("interface has no name")); + return NULL; + }
if (parentIfType == VIR_INTERFACE_TYPE_LAST) { /* only recognize these in toplevel bond interfaces */
Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index f3fc47bd69..0b3e5716f3 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -252,24 +252,13 @@ virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, static int virInterfaceDefParseIP(virInterfaceIPDef *def, - xmlXPathContextPtr ctxt) + xmlNodePtr node) { - int ret = 0; - char *tmp; - long l; - - tmp = virXPathString("string(./@address)", ctxt); - def->address = tmp; - if (tmp != NULL) { - ret = virXPathLong("string(./@prefix)", ctxt, &l); - if (ret == 0) { - def->prefix = (int) l; - } else if (ret == -2) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("Invalid ip address prefix value")); - return -1; - } - } + if ((def->address = virXMLPropString(node, "address")) == NULL) + return 0; + + if (virXMLPropInt(node, "prefix", 0, VIR_XML_PROP_NONE, &def->prefix, 0) < 0) + return -1; return 0; } @@ -309,8 +298,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, ip = g_new0(virInterfaceIPDef, 1); - ctxt->node = ipNodes[i]; - if (virInterfaceDefParseIP(ip, ctxt) < 0) { + if (virInterfaceDefParseIP(ip, ipNodes[i]) < 0) { virInterfaceIPDefFree(ip); return -1; } @@ -359,8 +347,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, ip = g_new0(virInterfaceIPDef, 1); - ctxt->node = ipNodes[i]; - if (virInterfaceDefParseIP(ip, ctxt) < 0) { + if (virInterfaceDefParseIP(ip, ipNodes[i]) < 0) { virInterfaceIPDefFree(ip); return -1; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 0b3e5716f3..bfeb69e664 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -226,24 +226,18 @@ virInterfaceDefParseBondArpValid(xmlXPathContextPtr ctxt) static int virInterfaceDefParseDhcp(virInterfaceProtocolDef *def, - xmlNodePtr dhcp, xmlXPathContextPtr ctxt) + xmlNodePtr dhcp) { - VIR_XPATH_NODE_AUTORESTORE(ctxt) - g_autofree char *tmp = NULL; + virTristateBool peerdns; def->dhcp = 1; - ctxt->node = dhcp; def->peerdns = -1; - /* Not much to do in the current version */ - tmp = virXPathString("string(./@peerdns)", ctxt); - if (tmp) { - bool state = false; - if (virStringParseYesNo(tmp, &state) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown dhcp peerdns value %s"), tmp); - return -1; - } - def->peerdns = state ? 1 : 0; + + if (virXMLPropTristateBool(dhcp, "peerdns", VIR_XML_PROP_NONZERO, &peerdns) < 0) + return -1; + + if (peerdns != VIR_TRISTATE_BOOL_ABSENT) { + def->peerdns = peerdns == VIR_TRISTATE_BOOL_YES ? 1 : 0; } return 0; @@ -279,7 +273,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, dhcp = virXPathNode("./dhcp", ctxt); if (dhcp != NULL) { - if (virInterfaceDefParseDhcp(def, dhcp, ctxt) < 0) + if (virInterfaceDefParseDhcp(def, dhcp) < 0) return -1; } @@ -328,7 +322,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, dhcp = virXPathNode("./dhcp", ctxt); if (dhcp != NULL) { - if (virInterfaceDefParseDhcp(def, dhcp, ctxt) < 0) + if (virInterfaceDefParseDhcp(def, dhcp) < 0) return -1; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index bfeb69e664..c41eecac29 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -266,10 +266,8 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, g_autofree xmlNodePtr *ipNodes = NULL; int nipNodes; size_t i; - char *tmp; - tmp = virXPathString("string(./route[1]/@gateway)", ctxt); - def->gateway = tmp; + def->gateway = virXPathString("string(./route[1]/@gateway)", ctxt); dhcp = virXPathNode("./dhcp", ctxt); if (dhcp != NULL) { @@ -287,10 +285,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDef *def, def->nips = 0; for (i = 0; i < nipNodes; i++) { - - virInterfaceIPDef *ip; - - ip = g_new0(virInterfaceIPDef, 1); + virInterfaceIPDef *ip = g_new0(virInterfaceIPDef, 1); if (virInterfaceDefParseIP(ip, ipNodes[i]) < 0) { virInterfaceIPDefFree(ip); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index c41eecac29..8be439a2a1 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -302,17 +302,14 @@ static int virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, xmlXPathContextPtr ctxt) { - xmlNodePtr dhcp, autoconf; + xmlNodePtr dhcp; g_autofree xmlNodePtr *ipNodes = NULL; int nipNodes; size_t i; - char *tmp; - tmp = virXPathString("string(./route[1]/@gateway)", ctxt); - def->gateway = tmp; + def->gateway = virXPathString("string(./route[1]/@gateway)", ctxt); - autoconf = virXPathNode("./autoconf", ctxt); - if (autoconf != NULL) + if (virXPathNode("./autoconf", ctxt) != NULL) def->autoconf = 1; dhcp = virXPathNode("./dhcp", ctxt); @@ -331,10 +328,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDef *def, def->nips = 0; for (i = 0; i < nipNodes; i++) { - - virInterfaceIPDef *ip; - - ip = g_new0(virInterfaceIPDef, 1); + virInterfaceIPDef *ip = g_new0(virInterfaceIPDef, 1); if (virInterfaceDefParseIP(ip, ipNodes[i]) < 0) { virInterfaceIPDefFree(ip); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 8be439a2a1..8f8e8871cb 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -348,7 +348,6 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autofree xmlNodePtr *protoNodes = NULL; int nProtoNodes, pp; - char *tmp; nProtoNodes = virXPathNodeSet("./protocol", ctxt, &protoNodes); if (nProtoNodes < 0) @@ -363,26 +362,24 @@ virInterfaceDefParseIfAdressing(virInterfaceDef *def, def->nprotos = 0; for (pp = 0; pp < nProtoNodes; pp++) { - g_autoptr(virInterfaceProtocolDef) proto = g_new0(virInterfaceProtocolDef, 1); - ctxt->node = protoNodes[pp]; - tmp = virXPathString("string(./@family)", ctxt); - if (tmp == NULL) { + if ((proto->family = virXMLPropString(protoNodes[pp], "family")) == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("protocol misses the family attribute")); return -1; } - proto->family = tmp; - if (STREQ(tmp, "ipv4")) { + + ctxt->node = protoNodes[pp]; + if (STREQ(proto->family, "ipv4")) { if (virInterfaceDefParseProtoIPv4(proto, ctxt) != 0) return -1; - } else if (STREQ(tmp, "ipv6")) { + } else if (STREQ(proto->family, "ipv6")) { if (virInterfaceDefParseProtoIPv6(proto, ctxt) != 0) return -1; } else { virReportError(VIR_ERR_XML_ERROR, - _("unsupported protocol family '%s'"), tmp); + _("unsupported protocol family '%s'"), proto->family); return -1; } def->protos[def->nprotos++] = g_steal_pointer(&proto); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 8f8e8871cb..7c2a0f162c 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -573,26 +573,14 @@ static virInterfaceDef * virInterfaceDefParseXML(xmlXPathContextPtr ctxt, int parentIfType) { - g_autoptr(virInterfaceDef) def = NULL; - int type; - g_autofree char *tmp = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autoptr(virInterfaceDef) def = NULL; + virInterfaceType type; xmlNodePtr lnk; - - /* check @type */ - tmp = virXPathString("string(./@type)", ctxt); - if (tmp == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("interface misses the type attribute")); + if (virXMLPropEnum(ctxt->node, "type", virInterfaceTypeFromString, + VIR_XML_PROP_REQUIRED, &type) < 0) return NULL; - } - type = virInterfaceTypeFromString(tmp); - if (type == -1) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown interface type %s"), tmp); - return NULL; - } def = g_new0(virInterfaceDef, 1); @@ -680,7 +668,8 @@ virInterfaceDefParseXML(xmlXPathContextPtr ctxt, return NULL; break; } - + case VIR_INTERFACE_TYPE_LAST: + return NULL; } return g_steal_pointer(&def); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/interface_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 7c2a0f162c..305a312327 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -704,14 +704,13 @@ virInterfaceDefParse(const char *xmlStr, unsigned int flags) { g_autoptr(xmlDoc) xml = NULL; - virInterfaceDef *def = NULL; - if ((xml = virXMLParse(filename, xmlStr, _("(interface_definition)"), "interface.rng", - flags & VIR_INTERFACE_DEFINE_VALIDATE))) { - def = virInterfaceDefParseNode(xml, xmlDocGetRootElement(xml)); - } + xml = virXMLParse(filename, xmlStr, _("(interface_definition)"), + "interface.rng", flags & VIR_INTERFACE_DEFINE_VALIDATE); + if (!xml) + return NULL; - return def; + return virInterfaceDefParseNode(xml, xmlDocGetRootElement(xml)); } -- 2.31.1
On 1/12/22 14:10, Tim Wiederhake wrote:
A collection of drive-by fixes and improvements that quickly derailed into the g_auto-ification of src/conf/interface_conf.c.
Tim Wiederhake (14): docs: coding-style: Clarify on virXXXPtr types virQEMUCapsSEVInfoCopy: Remove superfluous g_auto usage virInterfaceDefDevFormat: Add missing error handling conf: interface: Preparation for g_auto conf: interface: Use g_auto conf: interface: Remove ret and goto virInterfaceDefParseXML: Inline trivial virInterfaceDefParseName virInterfaceDefParseIP: Simplify and cleanup virInterfaceDefParseDhcp: Simplify and cleanup virInterfaceDefParseProtoIPv4: Simplify and cleanup virInterfaceDefParseProtoIPv6: Simplify and cleanup virInterfaceDefParseIfAdressing: Simplify and cleanup virInterfaceDefParseXML: Simplify and cleanup virInterfaceDefParse: Simplify and cleanup
docs/coding-style.rst | 5 + src/conf/interface_conf.c | 357 ++++++++++++----------------------- src/qemu/qemu_capabilities.c | 4 +- 3 files changed, 128 insertions(+), 238 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Wed, 2022-01-12 at 15:36 +0100, Michal Prívozník wrote:
On 1/12/22 14:10, Tim Wiederhake wrote:
A collection of drive-by fixes and improvements that quickly derailed into the g_auto-ification of src/conf/interface_conf.c.
Tim Wiederhake (14): docs: coding-style: Clarify on virXXXPtr types virQEMUCapsSEVInfoCopy: Remove superfluous g_auto usage virInterfaceDefDevFormat: Add missing error handling conf: interface: Preparation for g_auto conf: interface: Use g_auto conf: interface: Remove ret and goto virInterfaceDefParseXML: Inline trivial virInterfaceDefParseName virInterfaceDefParseIP: Simplify and cleanup virInterfaceDefParseDhcp: Simplify and cleanup virInterfaceDefParseProtoIPv4: Simplify and cleanup virInterfaceDefParseProtoIPv6: Simplify and cleanup virInterfaceDefParseIfAdressing: Simplify and cleanup virInterfaceDefParseXML: Simplify and cleanup virInterfaceDefParse: Simplify and cleanup
docs/coding-style.rst | 5 + src/conf/interface_conf.c | 357 ++++++++++++------------------- ---- src/qemu/qemu_capabilities.c | 4 +- 3 files changed, 128 insertions(+), 238 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal
Thanks! I will drop patches 1 and 2 from the series for now: I want to rewrite #2 entirely and have additional changes for #1. Tim
participants (2)
-
Michal Prívozník -
Tim Wiederhake