[libvirt] [PATCHv2 0/3] Split out disk source parsing code
Version 2 incorporates review feedback. Peter Krempa (3): conf: Split out code to parse the source of a disk definition conf: Rename virDomainDiskHostDefFree to virDomainDiskHostDefClear conf: Refactor virDomainDiskSourceDefParse src/conf/domain_conf.c | 269 ++++++++++++++++++++++++++--------------------- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 4 +- 4 files changed, 154 insertions(+), 123 deletions(-) -- 1.8.4.2
To avoid code duplication between snapshot configuration code that parses the disk source too we need to split out this code that will be reused later on. This patch tries to be code movement, some aspects of this function will be refactored later. --- Notes: Version 2: - move operations on "protocol" and "protocol_transport" to the new function - set expected_secret_usage only if disk is of type NETWORK src/conf/domain_conf.c | 266 +++++++++++++++++++++++++++---------------------- 1 file changed, 149 insertions(+), 117 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 50ce44d..3efdb9d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4729,6 +4729,142 @@ cleanup: return ret; } + +static int +virDomainDiskSourceDefParse(xmlNodePtr node, + int type, + char **source, + int *proto, + size_t *ndefhosts, + virDomainDiskHostDefPtr *defhosts, + virDomainDiskSourcePoolDefPtr *srcpool) +{ + char *protocol = NULL; + char *transport = NULL; + virDomainDiskHostDefPtr hosts = NULL; + int nhosts = 0; + xmlNodePtr child; + int ret = -1; + + switch (type) { + case VIR_DOMAIN_DISK_TYPE_FILE: + *source = virXMLPropString(node, "file"); + break; + case VIR_DOMAIN_DISK_TYPE_BLOCK: + *source = virXMLPropString(node, "dev"); + break; + case VIR_DOMAIN_DISK_TYPE_DIR: + *source = virXMLPropString(node, "dir"); + break; + case VIR_DOMAIN_DISK_TYPE_NETWORK: + protocol = virXMLPropString(node, "protocol"); + if (protocol == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing protocol type")); + goto error; + } + *proto = virDomainDiskProtocolTypeFromString(protocol); + if (*proto < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown protocol type '%s'"), + protocol); + goto error; + } + + if (!(*source = virXMLPropString(node, "name")) && + *proto != VIR_DOMAIN_DISK_PROTOCOL_NBD) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name for disk source")); + goto error; + } + child = node->children; + while (child != NULL) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, BAD_CAST "host")) { + if (VIR_REALLOC_N(hosts, nhosts + 1) < 0) + goto error; + hosts[nhosts].name = NULL; + hosts[nhosts].port = NULL; + hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; + hosts[nhosts].socket = NULL; + nhosts++; + + /* transport can be tcp (default), unix or rdma. */ + transport = virXMLPropString(child, "transport"); + if (transport != NULL) { + hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(transport); + if (hosts[nhosts - 1].transport < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown protocol transport type '%s'"), + transport); + goto error; + } + } + hosts[nhosts - 1].socket = virXMLPropString(child, "socket"); + if (hosts[nhosts - 1].transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && + hosts[nhosts - 1].socket == NULL) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing socket for unix transport")); + goto error; + } + if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && + hosts[nhosts - 1].socket != NULL) { + virReportError(VIR_ERR_XML_ERROR, + _("transport %s does not support socket attribute"), + transport); + goto error; + } + VIR_FREE(transport); + if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + hosts[nhosts - 1].name = virXMLPropString(child, "name"); + if (!hosts[nhosts - 1].name) { + virReportError(VIR_ERR_XML_ERROR, + "%s", _("missing name for host")); + goto error; + } + hosts[nhosts - 1].port = virXMLPropString(child, "port"); + } + } + child = child->next; + } + break; + case VIR_DOMAIN_DISK_TYPE_VOLUME: + if (virDomainDiskSourcePoolDefParse(node, srcpool) < 0) + goto error; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk type %s"), + virDomainDiskTypeToString(type)); + goto error; + } + + /* People sometimes pass a bogus '' source path + when they mean to omit the source element + completely (e.g. CDROM without media). This is + just a little compatibility check to help + those broken apps */ + if (*source && STREQ(*source, "")) + VIR_FREE(*source); + + *ndefhosts = nhosts; + *defhosts = hosts; + nhosts = 0; + + ret = 0; + +error: + VIR_FREE(protocol); + VIR_FREE(transport); + while (nhosts > 0) { + virDomainDiskHostDefFree(&hosts[nhosts - 1]); + nhosts--; + } + + return ret; +} + + #define VENDOR_LEN 8 #define PRODUCT_LEN 16 @@ -4757,11 +4893,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *driverType = NULL; char *source = NULL; char *target = NULL; - char *protocol = NULL; - char *protocol_transport = NULL; char *trans = NULL; - virDomainDiskHostDefPtr hosts = NULL; - int nhosts = 0; char *bus = NULL; char *cachetag = NULL; char *error_policy = NULL; @@ -4824,116 +4956,27 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { - if (!source && !hosts && !def->srcpool && + if (!source && !def->hosts && !def->srcpool && xmlStrEqual(cur->name, BAD_CAST "source")) { sourceNode = cur; - switch (def->type) { - case VIR_DOMAIN_DISK_TYPE_FILE: - source = virXMLPropString(cur, "file"); - break; - case VIR_DOMAIN_DISK_TYPE_BLOCK: - source = virXMLPropString(cur, "dev"); - break; - case VIR_DOMAIN_DISK_TYPE_DIR: - source = virXMLPropString(cur, "dir"); - break; - case VIR_DOMAIN_DISK_TYPE_NETWORK: - protocol = virXMLPropString(cur, "protocol"); - if (protocol == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing protocol type")); - goto error; - } - def->protocol = virDomainDiskProtocolTypeFromString(protocol); - if (def->protocol < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown protocol type '%s'"), - protocol); - goto error; - } - if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI) { + if (virDomainDiskSourceDefParse(cur, def->type, + &source, + &def->protocol, + &def->nhosts, + &def->hosts, + &def->srcpool) < 0) + goto error; + + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { + if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_ISCSI) expected_secret_usage = VIR_SECRET_USAGE_TYPE_ISCSI; - } else if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) { + else if (def->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD) expected_secret_usage = VIR_SECRET_USAGE_TYPE_CEPH; - } - if (!(source = virXMLPropString(cur, "name")) && - def->protocol != VIR_DOMAIN_DISK_PROTOCOL_NBD) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing name for disk source")); - goto error; - } - child = cur->children; - while (child != NULL) { - if (child->type == XML_ELEMENT_NODE && - xmlStrEqual(child->name, BAD_CAST "host")) { - if (VIR_REALLOC_N(hosts, nhosts + 1) < 0) - goto error; - hosts[nhosts].name = NULL; - hosts[nhosts].port = NULL; - hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - hosts[nhosts].socket = NULL; - nhosts++; - - /* transport can be tcp (default), unix or rdma. */ - protocol_transport = virXMLPropString(child, "transport"); - if (protocol_transport != NULL) { - hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(protocol_transport); - if (hosts[nhosts - 1].transport < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown protocol transport type '%s'"), - protocol_transport); - goto error; - } - } - hosts[nhosts - 1].socket = virXMLPropString(child, "socket"); - if (hosts[nhosts - 1].transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && - hosts[nhosts - 1].socket == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing socket for unix transport")); - goto error; - } - if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && - hosts[nhosts - 1].socket != NULL) { - virReportError(VIR_ERR_XML_ERROR, - _("transport %s does not support socket attribute"), - protocol_transport); - goto error; - } - VIR_FREE(protocol_transport); - if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { - hosts[nhosts - 1].name = virXMLPropString(child, "name"); - if (!hosts[nhosts - 1].name) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing name for host")); - goto error; - } - hosts[nhosts - 1].port = virXMLPropString(child, "port"); - } - } - child = child->next; - } - break; - case VIR_DOMAIN_DISK_TYPE_VOLUME: - if (virDomainDiskSourcePoolDefParse(cur, &def->srcpool) < 0) - goto error; - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %s"), - virDomainDiskTypeToString(def->type)); - goto error; } startupPolicy = virXMLPropString(cur, "startupPolicy"); - /* People sometimes pass a bogus '' source path - when they mean to omit the source element - completely (e.g. CDROM without media). This is - just a little compatibility check to help - those broken apps */ - if (source && STREQ(source, "")) - VIR_FREE(source); } else if (!target && xmlStrEqual(cur->name, BAD_CAST "target")) { target = virXMLPropString(cur, "dev"); @@ -5229,7 +5272,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, /* Only CDROM and Floppy devices are allowed missing source path * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ - if (source == NULL && hosts == NULL && !def->srcpool && + if (source == NULL && def->hosts == NULL && !def->srcpool && def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && def->device != VIR_DOMAIN_DISK_DEVICE_LUN && def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { @@ -5544,10 +5587,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, source = NULL; def->dst = target; target = NULL; - def->hosts = hosts; - hosts = NULL; - def->nhosts = nhosts; - nhosts = 0; def->auth.username = authUsername; authUsername = NULL; def->driverName = driverName; @@ -5601,13 +5640,6 @@ cleanup: VIR_FREE(tray); VIR_FREE(removable); VIR_FREE(trans); - while (nhosts > 0) { - virDomainDiskHostDefFree(&hosts[nhosts - 1]); - nhosts--; - } - VIR_FREE(hosts); - VIR_FREE(protocol); - VIR_FREE(protocol_transport); VIR_FREE(device); VIR_FREE(authUsername); VIR_FREE(usageType); -- 1.8.4.2
On 11/07/2013 11:16 AM, Peter Krempa wrote:
To avoid code duplication between snapshot configuration code that parses the disk source too we need to split out this code that will be reused later on.
This patch tries to be code movement, some aspects of this function will be refactored later. ---
Notes: Version 2: - move operations on "protocol" and "protocol_transport" to the new function - set expected_secret_usage only if disk is of type NETWORK
src/conf/domain_conf.c | 266 +++++++++++++++++++++++++++---------------------- 1 file changed, 149 insertions(+), 117 deletions(-)
ACK
The function destroys only the contents not the object itself thus it should be called Clear. --- Notes: Version 2: - already ACKed, no change (couldn't be pushed due to conflicts) src/conf/domain_conf.c | 6 +++--- src/conf/domain_conf.h | 2 +- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3efdb9d..4d3812a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1216,13 +1216,13 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) } for (i = 0; i < def->nhosts; i++) - virDomainDiskHostDefFree(&def->hosts[i]); + virDomainDiskHostDefClear(&def->hosts[i]); VIR_FREE(def->hosts); VIR_FREE(def); } -void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def) +void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def) { if (!def) return; @@ -4857,7 +4857,7 @@ error: VIR_FREE(protocol); VIR_FREE(transport); while (nhosts > 0) { - virDomainDiskHostDefFree(&hosts[nhosts - 1]); + virDomainDiskHostDefClear(&hosts[nhosts - 1]); nhosts--; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4520ae4..458be97 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2208,7 +2208,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def); void virDomainInputDefFree(virDomainInputDefPtr def); void virDomainDiskDefFree(virDomainDiskDefPtr def); void virDomainLeaseDefFree(virDomainLeaseDefPtr def); -void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def); +void virDomainDiskHostDefClear(virDomainDiskHostDefPtr def); int virDomainDeviceFindControllerModel(virDomainDefPtr def, virDomainDeviceInfoPtr info, int controllerType); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 76016ca..a705c56 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -197,7 +197,7 @@ virDomainDiskErrorPolicyTypeToString; virDomainDiskFindByBusAndDst; virDomainDiskGeometryTransTypeFromString; virDomainDiskGeometryTransTypeToString; -virDomainDiskHostDefFree; +virDomainDiskHostDefClear; virDomainDiskIndexByName; virDomainDiskInsert; virDomainDiskInsertPreAlloced; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e48c9c2..4922845 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3509,7 +3509,7 @@ cleanup: return ret; error: - virDomainDiskHostDefFree(def->hosts); + virDomainDiskHostDefClear(def->hosts); VIR_FREE(def->hosts); goto cleanup; } @@ -3611,7 +3611,7 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return 0; error: - virDomainDiskHostDefFree(h); + virDomainDiskHostDefClear(h); VIR_FREE(h); return -1; } -- 1.8.4.2
Now that the function is separate clean out a few ugly places and fix up error messages. --- Notes: Version 2: - rebased to changes in 1/3 of this series src/conf/domain_conf.c | 119 ++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 60 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d3812a..a1c39c2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4735,17 +4735,18 @@ virDomainDiskSourceDefParse(xmlNodePtr node, int type, char **source, int *proto, - size_t *ndefhosts, - virDomainDiskHostDefPtr *defhosts, + size_t *nhosts, + virDomainDiskHostDefPtr *hosts, virDomainDiskSourcePoolDefPtr *srcpool) { char *protocol = NULL; char *transport = NULL; - virDomainDiskHostDefPtr hosts = NULL; - int nhosts = 0; + virDomainDiskHostDef host; xmlNodePtr child; int ret = -1; + memset(&host, 0, sizeof(host)); + switch (type) { case VIR_DOMAIN_DISK_TYPE_FILE: *source = virXMLPropString(node, "file"); @@ -4757,110 +4758,108 @@ virDomainDiskSourceDefParse(xmlNodePtr node, *source = virXMLPropString(node, "dir"); break; case VIR_DOMAIN_DISK_TYPE_NETWORK: - protocol = virXMLPropString(node, "protocol"); - if (protocol == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing protocol type")); - goto error; + if (!(protocol = virXMLPropString(node, "protocol"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing network source protocol type")); + goto cleanup; } - *proto = virDomainDiskProtocolTypeFromString(protocol); - if (*proto < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown protocol type '%s'"), - protocol); - goto error; + + if ((*proto = virDomainDiskProtocolTypeFromString(protocol)) < 0){ + virReportError(VIR_ERR_XML_ERROR, + _("unknown protocol type '%s'"), protocol); + goto cleanup; } if (!(*source = virXMLPropString(node, "name")) && *proto != VIR_DOMAIN_DISK_PROTOCOL_NBD) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing name for disk source")); - goto error; + goto cleanup; } + child = node->children; while (child != NULL) { if (child->type == XML_ELEMENT_NODE && xmlStrEqual(child->name, BAD_CAST "host")) { - if (VIR_REALLOC_N(hosts, nhosts + 1) < 0) - goto error; - hosts[nhosts].name = NULL; - hosts[nhosts].port = NULL; - hosts[nhosts].transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; - hosts[nhosts].socket = NULL; - nhosts++; + + host.transport = VIR_DOMAIN_DISK_PROTO_TRANS_TCP; /* transport can be tcp (default), unix or rdma. */ - transport = virXMLPropString(child, "transport"); - if (transport != NULL) { - hosts[nhosts - 1].transport = virDomainDiskProtocolTransportTypeFromString(transport); - if (hosts[nhosts - 1].transport < 0) { + if ((transport = virXMLPropString(child, "transport"))) { + host.transport = virDomainDiskProtocolTransportTypeFromString(transport); + if (host.transport < 0) { virReportError(VIR_ERR_XML_ERROR, _("unknown protocol transport type '%s'"), transport); - goto error; + goto cleanup; } } - hosts[nhosts - 1].socket = virXMLPropString(child, "socket"); - if (hosts[nhosts - 1].transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && - hosts[nhosts - 1].socket == NULL) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing socket for unix transport")); - goto error; + + host.socket = virXMLPropString(child, "socket"); + + if (host.transport == VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && + host.socket == NULL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing socket for unix transport")); + goto cleanup; } - if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && - hosts[nhosts - 1].socket != NULL) { + + if (host.transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX && + host.socket != NULL) { virReportError(VIR_ERR_XML_ERROR, - _("transport %s does not support socket attribute"), + _("transport '%s' does not support " + "socket attribute"), transport); - goto error; + goto cleanup; } + VIR_FREE(transport); - if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { - hosts[nhosts - 1].name = virXMLPropString(child, "name"); - if (!hosts[nhosts - 1].name) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing name for host")); - goto error; + + if (host.transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (!(host.name = virXMLPropString(child, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing name for host")); + goto cleanup; } - hosts[nhosts - 1].port = virXMLPropString(child, "port"); + + host.port = virXMLPropString(child, "port"); } + + if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host)) + goto cleanup; } child = child->next; } break; case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(node, srcpool) < 0) - goto error; + goto cleanup; break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), virDomainDiskTypeToString(type)); - goto error; + goto cleanup; } - /* People sometimes pass a bogus '' source path - when they mean to omit the source element - completely (e.g. CDROM without media). This is - just a little compatibility check to help - those broken apps */ + /* People sometimes pass a bogus '' source path when they mean to omit the + * source element completely (e.g. CDROM without media). This is just a + * little compatibility check to help those broken apps */ if (*source && STREQ(*source, "")) VIR_FREE(*source); - *ndefhosts = nhosts; - *defhosts = hosts; - nhosts = 0; - ret = 0; error: - VIR_FREE(protocol); - VIR_FREE(transport); - while (nhosts > 0) { + while (nhosts > 0) { virDomainDiskHostDefClear(&hosts[nhosts - 1]); nhosts--; } +cleanup: + virDomainDiskHostDefClear(&host); + VIR_FREE(protocol); + VIR_FREE(transport); return ret; } -- 1.8.4.2
On 11/07/13 11:16, Peter Krempa wrote:
Now that the function is separate clean out a few ugly places and fix up error messages. ---
Notes: Version 2: - rebased to changes in 1/3 of this series
src/conf/domain_conf.c | 119 ++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 60 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4d3812a..a1c39c2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
...
- when they mean to omit the source element - completely (e.g. CDROM without media). This is - just a little compatibility check to help - those broken apps */ + /* People sometimes pass a bogus '' source path when they mean to omit the + * source element completely (e.g. CDROM without media). This is just a + * little compatibility check to help those broken apps */ if (*source && STREQ(*source, "")) VIR_FREE(*source);
- *ndefhosts = nhosts; - *defhosts = hosts; - nhosts = 0; - ret = 0;
error: - VIR_FREE(protocol); - VIR_FREE(transport); - while (nhosts > 0) { + while (nhosts > 0) { virDomainDiskHostDefClear(&hosts[nhosts - 1]); nhosts--; }
Yuck; I forgot to nuke the whole error section when solving the conflict :/. Everything between the error: label and the cleanup label is now removed in my private branch. I can re-send this patch if required
+cleanup: + virDomainDiskHostDefClear(&host); + VIR_FREE(protocol); + VIR_FREE(transport); return ret; }
Sorry for the mess. PEter
On 11/07/2013 11:16 AM, Peter Krempa wrote:
Now that the function is separate clean out a few ugly places and fix up error messages. ---
Notes: Version 2: - rebased to changes in 1/3 of this series
src/conf/domain_conf.c | 119 ++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 60 deletions(-)
VIR_FREE(transport); - if (hosts[nhosts - 1].transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { - hosts[nhosts - 1].name = virXMLPropString(child, "name"); - if (!hosts[nhosts - 1].name) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing name for host")); - goto error; + + if (host.transport != VIR_DOMAIN_DISK_PROTO_TRANS_UNIX) { + if (!(host.name = virXMLPropString(child, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing name for host")); + goto cleanup; } - hosts[nhosts - 1].port = virXMLPropString(child, "port"); + + host.port = virXMLPropString(child, "port"); } + + if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host))
if (VIR_APPEND_ELEMENT(..) < 0) would look clearer.
+ goto cleanup; } child = child->next; } break;
ACK with the following code dropped:
error: - VIR_FREE(protocol); - VIR_FREE(transport); - while (nhosts > 0) { + while (nhosts > 0) { virDomainDiskHostDefClear(&hosts[nhosts - 1]); nhosts--; }
On 11/07/13 16:57, Ján Tomko wrote:
On 11/07/2013 11:16 AM, Peter Krempa wrote:
Now that the function is separate clean out a few ugly places and fix up error messages. ---
Notes: Version 2: - rebased to changes in 1/3 of this series
src/conf/domain_conf.c | 119 ++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 60 deletions(-)
...
+ + if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host))
if (VIR_APPEND_ELEMENT(..) < 0) would look clearer.
+ goto cleanup; } child = child->next; } break;
ACK with the following code dropped:
error: - VIR_FREE(protocol); - VIR_FREE(transport); - while (nhosts > 0) { + while (nhosts > 0) { virDomainDiskHostDefClear(&hosts[nhosts - 1]); nhosts--; }
I fixed those two places and pushed the series. Thanks for the review. Peter
participants (2)
-
Ján Tomko -
Peter Krempa