[libvirt] [PATCHv3 0/7] Finish the conversion to virConfGetValue* functions
This patchset finishes the conversion to virConfGetValue* functions, started by Daniel Berrange a few months ago. Please, mind that although we could make virConfGetValue* functions more generic in order to support numbers and booleans as strings, that doesn't seem the safest path to take. The side-effect of this is that we will have to live with some specific code doing that as part of vmx and xen_common. Once this patchset gets merged, https://wiki.libvirt.org/page/BiteSizedTasks#Finish_conversion_to_virConfGet... can be removed. - Changes since v1: All the "values" from virConfGetValueString() are freed - Changes since v2: All comments from Ján Tomko have been addressed; A few leaks were (possibly) found and they're addressed in the last patch of the series; Fabiano Fidêncio (7): xen_xm: Split the per-disk logic from xenParseXMDisk() xen_vm: convert to typesafe virConf accessors vmx: convert to typesafe virConf accessors xen_common: Split per-PCI logic from xenParsePCI() xen_common: Split per-Vfi logic from xenParseVif() xen_common: convert to typesafe virConf acessors xen_common: Fix a few memory leaks src/vmx/vmx.c | 194 +++++-------- src/xenconfig/xen_common.c | 701 +++++++++++++++++++++++---------------------- src/xenconfig/xen_xm.c | 306 ++++++++++---------- 3 files changed, 595 insertions(+), 606 deletions(-) -- 2.14.3
xenParseXMDisk() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParseXMDisk(): it's a new function that keeps the old name. It's responsible for the whole per-disk logic from the old xenParseXMDisk(); - xenParseXMDiskList(): it's basically the old xenParseXMDisk(), but now it just iterates over the list of disks, calling xenParseXMDisk() per each disk. This patch is basically preparing the ground for the future when typesafe virConf acessors will be used. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_xm.c | 304 +++++++++++++++++++++++++------------------------ 1 file changed, 156 insertions(+), 148 deletions(-) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 4becb40b4c..be50a13909 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -107,179 +107,187 @@ xenParseXMOS(virConfPtr conf, virDomainDefPtr def) } -static int -xenParseXMDisk(virConfPtr conf, virDomainDefPtr def) +static virDomainDiskDefPtr +xenParseXMDisk(char *entry, int hvm) { virDomainDiskDefPtr disk = NULL; - int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list = virConfGetValue(conf, "disk"); + char *head; + char *offset; + char *tmp; + const char *src; - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - char *head; - char *offset; - char *tmp; - const char *src; + if (!(disk = virDomainDiskDefNew(NULL))) + return NULL; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skipdisk; + head = entry; + /* + * Disks have 3 components, SOURCE,DEST-DEVICE,MODE + * eg, phy:/dev/HostVG/XenGuest1,xvda,w + * The SOURCE is usually prefixed with a driver type, + * and optionally driver sub-type + * The DEST-DEVICE is optionally post-fixed with disk type + */ + + /* Extract the source file path*/ + if (!(offset = strchr(head, ','))) + goto error; + + if (offset == head) { + /* No source file given, eg CDROM with no media */ + ignore_value(virDomainDiskSetSource(disk, NULL)); + } else { + if (VIR_STRNDUP(tmp, head, offset - head) < 0) + goto error; - head = list->str; - if (!(disk = virDomainDiskDefNew(NULL))) - return -1; + if (virDomainDiskSetSource(disk, tmp) < 0) { + VIR_FREE(tmp); + goto error; + } + VIR_FREE(tmp); + } - /* - * Disks have 3 components, SOURCE,DEST-DEVICE,MODE - * eg, phy:/dev/HostVG/XenGuest1,xvda,w - * The SOURCE is usually prefixed with a driver type, - * and optionally driver sub-type - * The DEST-DEVICE is optionally post-fixed with disk type - */ - - /* Extract the source file path*/ - if (!(offset = strchr(head, ','))) - goto skipdisk; - - if (offset == head) { - /* No source file given, eg CDROM with no media */ - ignore_value(virDomainDiskSetSource(disk, NULL)); - } else { - if (VIR_STRNDUP(tmp, head, offset - head) < 0) - goto cleanup; - - if (virDomainDiskSetSource(disk, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } + head = offset + 1; + /* Remove legacy ioemu: junk */ + if (STRPREFIX(head, "ioemu:")) + head = head + 6; + + /* Extract the dest device name */ + if (!(offset = strchr(head, ','))) + goto error; + + if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) + goto error; + + if (virStrncpy(disk->dst, head, offset - head, + (offset - head) + 1) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Dest file %s too big for destination"), head); + goto error; + } + + head = offset + 1; + /* Extract source driver type */ + src = virDomainDiskGetSource(disk); + if (src) { + size_t len; + /* The main type phy:, file:, tap: ... */ + if ((tmp = strchr(src, ':')) != NULL) { + len = tmp - src; + if (VIR_STRNDUP(tmp, src, len) < 0) + goto error; + + if (virDomainDiskSetDriver(disk, tmp) < 0) { VIR_FREE(tmp); + goto error; } + VIR_FREE(tmp); - head = offset + 1; - /* Remove legacy ioemu: junk */ - if (STRPREFIX(head, "ioemu:")) - head = head + 6; + /* Strip the prefix we found off the source file name */ + if (virDomainDiskSetSource(disk, src + len + 1) < 0) + goto error; - /* Extract the dest device name */ - if (!(offset = strchr(head, ','))) - goto skipdisk; + src = virDomainDiskGetSource(disk); + } - if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) - goto cleanup; + /* And the sub-type for tap:XXX: type */ + if (STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap") || + STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap2")) { + char *driverType; - if (virStrncpy(disk->dst, head, offset - head, - (offset - head) + 1) == NULL) { + if (!(tmp = strchr(src, ':'))) + goto error; + len = tmp - src; + + if (VIR_STRNDUP(driverType, src, len) < 0) + goto error; + + if (STREQ(driverType, "aio")) + virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); + else + virDomainDiskSetFormat(disk, + virStorageFileFormatTypeFromString(driverType)); + VIR_FREE(driverType); + if (virDomainDiskGetFormat(disk) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Dest file %s too big for destination"), head); - goto cleanup; + _("Unknown driver type %s"), + src); + goto error; } - head = offset + 1; - /* Extract source driver type */ + /* Strip the prefix we found off the source file name */ + if (virDomainDiskSetSource(disk, src + len + 1) < 0) + goto error; src = virDomainDiskGetSource(disk); - if (src) { - size_t len; - /* The main type phy:, file:, tap: ... */ - if ((tmp = strchr(src, ':')) != NULL) { - len = tmp - src; - if (VIR_STRNDUP(tmp, src, len) < 0) - goto cleanup; - - if (virDomainDiskSetDriver(disk, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } - VIR_FREE(tmp); - - /* Strip the prefix we found off the source file name */ - if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto cleanup; - - src = virDomainDiskGetSource(disk); - } + } + } - /* And the sub-type for tap:XXX: type */ - if (STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap") || - STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap2")) { - char *driverType; - - if (!(tmp = strchr(src, ':'))) - goto skipdisk; - len = tmp - src; - - if (VIR_STRNDUP(driverType, src, len) < 0) - goto cleanup; - - if (STREQ(driverType, "aio")) - virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW); - else - virDomainDiskSetFormat(disk, - virStorageFileFormatTypeFromString(driverType)); - VIR_FREE(driverType); - if (virDomainDiskGetFormat(disk) <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown driver type %s"), - src); - goto cleanup; - } - - /* Strip the prefix we found off the source file name */ - if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto cleanup; - src = virDomainDiskGetSource(disk); - } - } + /* No source, or driver name, so fix to phy: */ + if (!virDomainDiskGetDriver(disk) && + virDomainDiskSetDriver(disk, "phy") < 0) + goto error; + + /* phy: type indicates a block device */ + virDomainDiskSetType(disk, + STREQ(virDomainDiskGetDriver(disk), "phy") ? + VIR_STORAGE_TYPE_BLOCK : + VIR_STORAGE_TYPE_FILE); + + /* Check for a :cdrom/:disk postfix */ + disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; + if ((tmp = strchr(disk->dst, ':')) != NULL) { + if (STREQ(tmp, ":cdrom")) + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + tmp[0] = '\0'; + } - /* No source, or driver name, so fix to phy: */ - if (!virDomainDiskGetDriver(disk) && - virDomainDiskSetDriver(disk, "phy") < 0) - goto cleanup; + if (STRPREFIX(disk->dst, "xvd") || !hvm) + disk->bus = VIR_DOMAIN_DISK_BUS_XEN; + else if (STRPREFIX(disk->dst, "sd")) + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; + else + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - /* phy: type indicates a block device */ - virDomainDiskSetType(disk, - STREQ(virDomainDiskGetDriver(disk), "phy") ? - VIR_STORAGE_TYPE_BLOCK : - VIR_STORAGE_TYPE_FILE); - - /* Check for a :cdrom/:disk postfix */ - disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; - if ((tmp = strchr(disk->dst, ':')) != NULL) { - if (STREQ(tmp, ":cdrom")) - disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; - tmp[0] = '\0'; - } + if (STREQ(head, "r") || STREQ(head, "ro")) + disk->src->readonly = true; + else if (STREQ(head, "w!") || STREQ(head, "!")) + disk->src->shared = true; - if (STRPREFIX(disk->dst, "xvd") || !hvm) { - disk->bus = VIR_DOMAIN_DISK_BUS_XEN; - } else if (STRPREFIX(disk->dst, "sd")) { - disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; - } else { - disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - } + return disk; - if (STREQ(head, "r") || - STREQ(head, "ro")) - disk->src->readonly = true; - else if ((STREQ(head, "w!")) || - (STREQ(head, "!"))) - disk->src->shared = true; + error: + virDomainDiskDefFree(disk); + return NULL; +} - /* Maintain list in sorted order according to target device name */ - if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) - goto cleanup; - skipdisk: - list = list->next; - virDomainDiskDefFree(disk); - disk = NULL; - } +static int +xenParseXMDiskList(virConfPtr conf, virDomainDefPtr def) +{ + int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; + virConfValuePtr list = virConfGetValue(conf, "disk"); + + if (!list || list->type != VIR_CONF_LIST) + return 0; + + for (list = list->list; list; list = list->next) { + virDomainDiskDefPtr disk; + int rc; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + continue; + + if (!(disk = xenParseXMDisk(list->str, hvm))) + continue; + + /* Maintain list in sorted order according to target device name */ + rc = VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk); + virDomainDiskDefFree(disk); + + if (rc < 0) + return -1; } return 0; - - cleanup: - virDomainDiskDefFree(disk); - return -1; } @@ -457,7 +465,7 @@ xenParseXM(virConfPtr conf, if (xenParseXMOS(conf, def) < 0) goto cleanup; - if (xenParseXMDisk(conf, def) < 0) + if (xenParseXMDiskList(conf, def) < 0) goto cleanup; if (xenParseXMInputDevs(conf, def) < 0) -- 2.14.3
On Mon, May 28, 2018 at 12:28:20AM +0200, Fabiano Fidêncio wrote:
xenParseXMDisk() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParseXMDisk(): it's a new function that keeps the old name. It's responsible for the whole per-disk logic from the old xenParseXMDisk(); - xenParseXMDiskList(): it's basically the old xenParseXMDisk(), but now it just iterates over the list of disks, calling xenParseXMDisk() per each disk.
This patch is basically preparing the ground for the future when typesafe virConf acessors will be used.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_xm.c | 304 +++++++++++++++++++++++++------------------------ 1 file changed, 156 insertions(+), 148 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_xm.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index be50a13909..ef52cf4250 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -263,20 +263,20 @@ xenParseXMDisk(char *entry, int hvm) static int xenParseXMDiskList(virConfPtr conf, virDomainDefPtr def) { + char **disks = NULL, **entries; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list = virConfGetValue(conf, "disk"); + int ret = -1; + int rc; - if (!list || list->type != VIR_CONF_LIST) - return 0; + rc = virConfGetValueStringList(conf, "disk", false, &disks); + if (rc <= 0) + return rc; - for (list = list->list; list; list = list->next) { + for (entries = disks; *entries; entries++) { virDomainDiskDefPtr disk; - int rc; + char *entry = *entries; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - continue; - - if (!(disk = xenParseXMDisk(list->str, hvm))) + if (!(disk = xenParseXMDisk(entry, hvm))) continue; /* Maintain list in sorted order according to target device name */ @@ -284,10 +284,14 @@ xenParseXMDiskList(virConfPtr conf, virDomainDefPtr def) virDomainDiskDefFree(disk); if (rc < 0) - return -1; + goto cleanup; } - return 0; + ret = 0; + + cleanup: + virStringListFree(disks); + return ret; } -- 2.14.3
On Mon, May 28, 2018 at 12:28:21AM +0200, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_xm.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/vmx/vmx.c | 194 +++++++++++++++++++++------------------------------------- 1 file changed, 70 insertions(+), 124 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index df6a58a474..b6df257144 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -722,39 +722,36 @@ virVMXConvertToUTF8(const char *encoding, const char *string) static int -virVMXGetConfigString(virConfPtr conf, const char *name, char **string, - bool optional) +virVMXGetConfigStringHelper(virConfPtr conf, const char *name, char **string, + bool optional) { - virConfValuePtr value; - + int rc; *string = NULL; - value = virConfGetValue(conf, name); - if (value == NULL) { - if (optional) - return 0; + rc = virConfGetValueString(conf, name, string); + if (rc == 1 && *string != NULL) + return 1; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } + if (optional) + return 0; - if (value->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must be a string"), name); - return -1; - } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing essential config entry '%s'"), name); + return -1; +} - if (value->str == NULL) { - if (optional) - return 0; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); + +static int +virVMXGetConfigString(virConfPtr conf, const char *name, char **string, + bool optional) +{ + *string = NULL; + + if (virVMXGetConfigStringHelper(conf, name, string, optional) < 0) return -1; - } - return VIR_STRDUP(*string, value->str); + return 0; } @@ -763,43 +760,26 @@ static int virVMXGetConfigUUID(virConfPtr conf, const char *name, unsigned char *uuid, bool optional) { - virConfValuePtr value; - - value = virConfGetValue(conf, name); + char *string = NULL; + int ret = -1; + int rc; - if (value == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } + rc = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (rc <= 0) + return rc; - if (value->type != VIR_CONF_STRING) { + rc = virUUIDParse(string, uuid); + if (rc < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must be a string"), name); - return -1; - } - - if (value->str == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } + _("Could not parse UUID from string '%s'"), string); + goto cleanup; } - if (virUUIDParse(value->str, uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not parse UUID from string '%s'"), value->str); - return -1; - } + ret = 0; - return 0; + cleanup: + VIR_FREE(string); + return ret; } @@ -808,47 +788,30 @@ static int virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number, long long default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int ret = -1; + int rc; *number = default_; - value = virConfGetValue(conf, name); - if (value == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } - - if (value->type == VIR_CONF_STRING) { - if (value->str == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } + rc = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (rc <= 0) + return rc; - if (STRCASEEQ(value->str, "unlimited")) { - *number = -1; - } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must represent an integer value"), - name); - return -1; - } - } else { + if (STRCASEEQ(string, "unlimited")) { + *number = -1; + } else if (virStrToLong_ll(string, NULL, 10, number) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must be a string"), name); - return -1; + _("Config entry '%s' must represent an integer value"), + name); + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(string); + return ret; } @@ -857,49 +820,32 @@ static int virVMXGetConfigBoolean(virConfPtr conf, const char *name, bool *boolean_, bool default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int ret = -1; + int rc; *boolean_ = default_; - value = virConfGetValue(conf, name); - if (value == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } - - if (value->type == VIR_CONF_STRING) { - if (value->str == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } + rc = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (rc <= 0) + return rc; - if (STRCASEEQ(value->str, "true")) { - *boolean_ = 1; - } else if (STRCASEEQ(value->str, "false")) { - *boolean_ = 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must represent a boolean value " - "(true|false)"), name); - return -1; - } + if (STRCASEEQ(string, "true")) { + *boolean_ = 1; + } else if (STRCASEEQ(string, "false")) { + *boolean_ = 0; } else { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must be a string"), name); - return -1; + _("Config entry '%s' must represent a boolean value " + "(true|false)"), name); + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(string); + return ret; } -- 2.14.3
On Mon, May 28, 2018 at 12:28:22AM +0200, Fabiano Fidêncio wrote:
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/vmx/vmx.c | 194 +++++++++++++++++++++------------------------------------- 1 file changed, 70 insertions(+), 124 deletions(-)
- if (virUUIDParse(value->str, uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not parse UUID from string '%s'"), value->str); - return -1; - } + ret = 0;
- return 0; + cleanup:
'make syntax-check' complains about two spaces in front of a label. I will fix that before pushing. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
+ VIR_FREE(string); + return ret; }
xenParsePCI() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParsePCI(): it's a new function that keeps the old name. It's responsible for the whole per-PCI logic from the old xenParsePCI(); - xenParsePCIList(): it's basically the old xenParsePCI(), but now it just iterates over the list of PCIs, calling xenParsePCI() per each PCI. This patch is basically preparing the ground for the future when typesafe virConf acessors will be used. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 167 ++++++++++++++++++++++++--------------------- 1 file changed, 89 insertions(+), 78 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a2b0708ee3..fc7b0683b8 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -389,92 +389,103 @@ xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) } -static int -xenParsePCI(virConfPtr conf, virDomainDefPtr def) +static virDomainHostdevDefPtr +xenParsePCI(char *entry) { - virConfValuePtr list = virConfGetValue(conf, "pci"); virDomainHostdevDefPtr hostdev = NULL; + char domain[5]; + char bus[3]; + char slot[3]; + char func[2]; + char *key, *nextkey; + int domainID; + int busID; + int slotID; + int funcID; + + domain[0] = bus[0] = slot[0] = func[0] = '\0'; + + /* pci=['0000:00:1b.0','0000:00:13.0'] */ + if (!(key = entry)) + return NULL; + if (!(nextkey = strchr(key, ':'))) + return NULL; + if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Domain %s too big for destination"), key); + return NULL; + } - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - char domain[5]; - char bus[3]; - char slot[3]; - char func[2]; - char *key, *nextkey; - int domainID; - int busID; - int slotID; - int funcID; - - domain[0] = bus[0] = slot[0] = func[0] = '\0'; + key = nextkey + 1; + if (!(nextkey = strchr(key, ':'))) + return NULL; + if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bus %s too big for destination"), key); + return NULL; + } - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skippci; - /* pci=['0000:00:1b.0','0000:00:13.0'] */ - if (!(key = list->str)) - goto skippci; - if (!(nextkey = strchr(key, ':'))) - goto skippci; - if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Domain %s too big for destination"), key); - goto skippci; - } + key = nextkey + 1; + if (!(nextkey = strchr(key, '.'))) + return NULL; + if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Slot %s too big for destination"), key); + return NULL; + } - key = nextkey + 1; - if (!(nextkey = strchr(key, ':'))) - goto skippci; - if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bus %s too big for destination"), key); - goto skippci; - } + key = nextkey + 1; + if (strlen(key) != 1) + return NULL; + if (virStrncpy(func, key, 1, sizeof(func)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Function %s too big for destination"), key); + return NULL; + } - key = nextkey + 1; - if (!(nextkey = strchr(key, '.'))) - goto skippci; - if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Slot %s too big for destination"), key); - goto skippci; - } + if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) + return NULL; + if (virStrToLong_i(bus, NULL, 16, &busID) < 0) + return NULL; + if (virStrToLong_i(slot, NULL, 16, &slotID) < 0) + return NULL; + if (virStrToLong_i(func, NULL, 16, &funcID) < 0) + return NULL; - key = nextkey + 1; - if (strlen(key) != 1) - goto skippci; - if (virStrncpy(func, key, 1, sizeof(func)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Function %s too big for destination"), key); - goto skippci; - } + if (!(hostdev = virDomainHostdevDefNew())) + return NULL; - if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) - goto skippci; - if (virStrToLong_i(bus, NULL, 16, &busID) < 0) - goto skippci; - if (virStrToLong_i(slot, NULL, 16, &slotID) < 0) - goto skippci; - if (virStrToLong_i(func, NULL, 16, &funcID) < 0) - goto skippci; - if (!(hostdev = virDomainHostdevDefNew())) - return -1; - - hostdev->managed = false; - hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; - hostdev->source.subsys.u.pci.addr.domain = domainID; - hostdev->source.subsys.u.pci.addr.bus = busID; - hostdev->source.subsys.u.pci.addr.slot = slotID; - hostdev->source.subsys.u.pci.addr.function = funcID; - - if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { - virDomainHostdevDefFree(hostdev); - return -1; - } + hostdev->managed = false; + hostdev->source.subsys.type = VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI; + hostdev->source.subsys.u.pci.addr.domain = domainID; + hostdev->source.subsys.u.pci.addr.bus = busID; + hostdev->source.subsys.u.pci.addr.slot = slotID; + hostdev->source.subsys.u.pci.addr.function = funcID; - skippci: - list = list->next; + return hostdev; +} + + +static int +xenParsePCIList(virConfPtr conf, virDomainDefPtr def) +{ + virConfValuePtr list = virConfGetValue(conf, "pci"); + + if (!list || list->type != VIR_CONF_LIST) + return 0; + + for (list = list->list; list; list = list->next) { + virDomainHostdevDefPtr hostdev; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + continue; + + if (!(hostdev = xenParsePCI(list->str))) + return -1; + + if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { + virDomainHostdevDefFree(hostdev); + return -1; } } @@ -1126,7 +1137,7 @@ xenParseConfigCommon(virConfPtr conf, return -1; } - if (xenParsePCI(conf, def) < 0) + if (xenParsePCIList(conf, def) < 0) return -1; if (xenParseEmulatedDevices(conf, def) < 0) -- 2.14.3
On Mon, May 28, 2018 at 12:28:23AM +0200, Fabiano Fidêncio wrote:
xenParsePCI() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParsePCI(): it's a new function that keeps the old name. It's responsible for the whole per-PCI logic from the old xenParsePCI(); - xenParsePCIList(): it's basically the old xenParsePCI(), but now it just iterates over the list of PCIs, calling xenParsePCI() per each PCI.
This patch is basically preparing the ground for the future when typesafe virConf acessors will be used.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 167 ++++++++++++++++++++++++--------------------- 1 file changed, 89 insertions(+), 78 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
xenParseVfi() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParseVfi(): it's a new function that keeps the old name. It's responsible for the whole per-Vfi logic from the old xenParseVfi(); - xenParseVfiList(): it's basically the old xenParsePCI(), but now it just iterates over the list of Vfis, calling xenParsePCI() per each Vfi. This patch is basically preparing the ground for the future when typesafe virConf acessors will be used. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 363 ++++++++++++++++++++++++--------------------- 1 file changed, 192 insertions(+), 171 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index fc7b0683b8..45fecbced8 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -846,202 +846,223 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) } -static int -xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) +static virDomainNetDefPtr +xenParseVif(char *entry, const char *vif_typename) { - char *script = NULL; virDomainNetDefPtr net = NULL; - virConfValuePtr list = virConfGetValue(conf, "vif"); + char *script = NULL; + char model[10]; + char type[10]; + char ip[128]; + char mac[18]; + char bridge[50]; + char vifname[50]; + char rate[50]; + char *key; + int rc = 0; + + bridge[0] = '\0'; + mac[0] = '\0'; + ip[0] = '\0'; + model[0] = '\0'; + type[0] = '\0'; + vifname[0] = '\0'; + rate[0] = '\0'; + + key = entry; + while (key) { + char *data; + char *nextkey = strchr(key, ','); + + if (!(data = strchr(key, '='))) + return NULL; + data++; + + if (STRPREFIX(key, "mac=")) { + int len = nextkey ? (nextkey - data) : sizeof(mac) - 1; + if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("MAC address %s too big for destination"), + data); + return NULL; + } + } else if (STRPREFIX(key, "bridge=")) { + int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1; + if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bridge %s too big for destination"), + data); + return NULL; + } + } else if (STRPREFIX(key, "script=")) { + int len = nextkey ? (nextkey - data) : strlen(data); + VIR_FREE(script); + if (VIR_STRNDUP(script, data, len) < 0) + return NULL; + } else if (STRPREFIX(key, "model=")) { + int len = nextkey ? (nextkey - data) : sizeof(model) - 1; + if (virStrncpy(model, data, len, sizeof(model)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Model %s too big for destination"), + data); + return NULL; + } + } else if (STRPREFIX(key, "type=")) { + int len = nextkey ? (nextkey - data) : sizeof(type) - 1; + if (virStrncpy(type, data, len, sizeof(type)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Type %s too big for destination"), + data); + return NULL; + } + } else if (STRPREFIX(key, "vifname=")) { + int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1; + if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Vifname %s too big for destination"), + data); + return NULL; + } + } else if (STRPREFIX(key, "ip=")) { + int len = nextkey ? (nextkey - data) : sizeof(ip) - 1; + if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("IP %s too big for destination"), data); + return NULL; + } + } else if (STRPREFIX(key, "rate=")) { + int len = nextkey ? (nextkey - data) : sizeof(rate) - 1; + if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("rate %s too big for destination"), data); + return NULL; + } + } - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - char model[10]; - char type[10]; - char ip[128]; - char mac[18]; - char bridge[50]; - char vifname[50]; - char rate[50]; - char *key; - - bridge[0] = '\0'; - mac[0] = '\0'; - ip[0] = '\0'; - model[0] = '\0'; - type[0] = '\0'; - vifname[0] = '\0'; - rate[0] = '\0'; - - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skipnic; - - key = list->str; - while (key) { - char *data; - char *nextkey = strchr(key, ','); + while (nextkey && (nextkey[0] == ',' || + nextkey[0] == ' ' || + nextkey[0] == '\t')) + nextkey++; + key = nextkey; + } - if (!(data = strchr(key, '='))) - goto skipnic; - data++; - - if (STRPREFIX(key, "mac=")) { - int len = nextkey ? (nextkey - data) : sizeof(mac) - 1; - if (virStrncpy(mac, data, len, sizeof(mac)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("MAC address %s too big for destination"), - data); - goto skipnic; - } - } else if (STRPREFIX(key, "bridge=")) { - int len = nextkey ? (nextkey - data) : sizeof(bridge) - 1; - if (virStrncpy(bridge, data, len, sizeof(bridge)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Bridge %s too big for destination"), - data); - goto skipnic; - } - } else if (STRPREFIX(key, "script=")) { - int len = nextkey ? (nextkey - data) : strlen(data); - VIR_FREE(script); - if (VIR_STRNDUP(script, data, len) < 0) - goto cleanup; - } else if (STRPREFIX(key, "model=")) { - int len = nextkey ? (nextkey - data) : sizeof(model) - 1; - if (virStrncpy(model, data, len, sizeof(model)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Model %s too big for destination"), - data); - goto skipnic; - } - } else if (STRPREFIX(key, "type=")) { - int len = nextkey ? (nextkey - data) : sizeof(type) - 1; - if (virStrncpy(type, data, len, sizeof(type)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Type %s too big for destination"), - data); - goto skipnic; - } - } else if (STRPREFIX(key, "vifname=")) { - int len = nextkey ? (nextkey - data) : sizeof(vifname) - 1; - if (virStrncpy(vifname, data, len, sizeof(vifname)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Vifname %s too big for destination"), - data); - goto skipnic; - } - } else if (STRPREFIX(key, "ip=")) { - int len = nextkey ? (nextkey - data) : sizeof(ip) - 1; - if (virStrncpy(ip, data, len, sizeof(ip)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("IP %s too big for destination"), data); - goto skipnic; - } - } else if (STRPREFIX(key, "rate=")) { - int len = nextkey ? (nextkey - data) : sizeof(rate) - 1; - if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("rate %s too big for destination"), data); - goto skipnic; - } - } + if (VIR_ALLOC(net) < 0) + goto cleanup; - while (nextkey && (nextkey[0] == ',' || - nextkey[0] == ' ' || - nextkey[0] == '\t')) - nextkey++; - key = nextkey; - } + if (mac[0]) { + if (virMacAddrParse(mac, &net->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; + } + } - if (VIR_ALLOC(net) < 0) - goto cleanup; + if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || + STREQ_NULLABLE(script, "vif-vnic")) { + net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; + } else { + net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; + } - if (mac[0]) { - if (virMacAddrParse(mac, &net->mac) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("malformed mac address '%s'"), mac); - goto cleanup; - } - } + if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0) + goto cleanup; + } + if (ip[0]) { + char **ip_list = virStringSplit(ip, " ", 0); + size_t i; - if (bridge[0] || STREQ_NULLABLE(script, "vif-bridge") || - STREQ_NULLABLE(script, "vif-vnic")) { - net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - } else { - net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; - } + if (!ip_list) + goto cleanup; - if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - if (bridge[0] && VIR_STRDUP(net->data.bridge.brname, bridge) < 0) - goto cleanup; + for (i = 0; ip_list[i]; i++) { + if (virDomainNetAppendIPAddress(net, ip_list[i], 0, 0) < 0) { + virStringListFree(ip_list); + goto cleanup; } - if (ip[0]) { - char **ip_list = virStringSplit(ip, " ", 0); - size_t i; + } + virStringListFree(ip_list); + } - if (!ip_list) - goto cleanup; + if (script && script[0]) { + rc = VIR_STRDUP(net->script, script); + if (rc < 0) + goto cleanup; + } - for (i = 0; ip_list[i]; i++) { - if (virDomainNetAppendIPAddress(net, ip_list[i], 0, 0) < 0) { - virStringListFree(ip_list); - goto cleanup; - } - } - virStringListFree(ip_list); - } + if (model[0]) { + rc = VIR_STRDUP(net->model, model); + if (rc < 0) + goto cleanup; + } - if (script && script[0] && - VIR_STRDUP(net->script, script) < 0) - goto cleanup; + if (!model[0] && type[0] && STREQ(type, vif_typename)) { + rc = VIR_STRDUP(net->model, "netfront"); + if (rc < 0) + goto cleanup; + } - if (model[0] && - VIR_STRDUP(net->model, model) < 0) - goto cleanup; + if (vifname[0]) { + rc = VIR_STRDUP(net->ifname, vifname); + if (rc < 0) + goto cleanup; + } - if (!model[0] && type[0] && STREQ(type, vif_typename) && - VIR_STRDUP(net->model, "netfront") < 0) - goto cleanup; + if (rate[0]) { + virNetDevBandwidthPtr bandwidth; + unsigned long long kbytes_per_sec; - if (vifname[0] && - VIR_STRDUP(net->ifname, vifname) < 0) - goto cleanup; + rc = xenParseSxprVifRate(rate, &kbytes_per_sec); + if (rc < 0) + goto cleanup; - if (rate[0]) { - virNetDevBandwidthPtr bandwidth; - unsigned long long kbytes_per_sec; + rc = VIR_ALLOC(bandwidth); + if (rc < 0) + goto cleanup; + rc = VIR_ALLOC(bandwidth->out); + if (rc < 0) { + VIR_FREE(bandwidth); + goto cleanup; + } - if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0) - goto cleanup; + bandwidth->out->average = kbytes_per_sec; + net->bandwidth = bandwidth; + } - if (VIR_ALLOC(bandwidth) < 0) - goto cleanup; - if (VIR_ALLOC(bandwidth->out) < 0) { - VIR_FREE(bandwidth); - goto cleanup; - } + cleanup: + if (rc < 0) + virDomainNetDefFree(net); + VIR_FREE(script); + return net; +} - bandwidth->out->average = kbytes_per_sec; - net->bandwidth = bandwidth; - } - if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) - goto cleanup; +static int +xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) +{ + virConfValuePtr list = virConfGetValue(conf, "vif"); + + if (!list || list->type != VIR_CONF_LIST) + return 0; - skipnic: - list = list->next; + for (list = list->list; list; list = list->next) { + virDomainNetDefPtr net = NULL; + int rc; + + if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) + continue; + + if (!(net = xenParseVif(list->str, vif_typename))) + return -1; + + rc = VIR_APPEND_ELEMENT(def->nets, def->nnets, net); + if (rc < 0) { virDomainNetDefFree(net); - net = NULL; - VIR_FREE(script); + return -1; } } return 0; - - cleanup: - virDomainNetDefFree(net); - VIR_FREE(script); - return -1; } @@ -1126,10 +1147,10 @@ xenParseConfigCommon(virConfPtr conf, return -1; if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XL)) { - if (xenParseVif(conf, def, "vif") < 0) + if (xenParseVifList(conf, def, "vif") < 0) return -1; } else if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { - if (xenParseVif(conf, def, "netfront") < 0) + if (xenParseVifList(conf, def, "netfront") < 0) return -1; } else { virReportError(VIR_ERR_INVALID_ARG, -- 2.14.3
On Mon, May 28, 2018 at 12:28:24AM +0200, Fabiano Fidêncio wrote:
xenParseVfi() does a lot of stuff and, in order to make things cleaner, let's split it in two new functions: - xenParseVfi(): it's a new function that keeps the old name. It's responsible for the whole per-Vfi logic from the old xenParseVfi(); - xenParseVfiList(): it's basically the old xenParsePCI(), but now it just iterates over the list of Vfis, calling xenParsePCI() per each Vfi.
s/Vfi/Vif/g
This patch is basically preparing the ground for the future when typesafe virConf acessors will be used.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 363 ++++++++++++++++++++++++--------------------- 1 file changed, 192 insertions(+), 171 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index fc7b0683b8..45fecbced8 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -846,202 +846,223 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) }
-static int -xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) +static virDomainNetDefPtr +xenParseVif(char *entry, const char *vif_typename) { - char *script = NULL; virDomainNetDefPtr net = NULL; - virConfValuePtr list = virConfGetValue(conf, "vif"); + char *script = NULL; + char model[10]; + char type[10]; + char ip[128]; + char mac[18]; + char bridge[50]; + char vifname[50]; + char rate[50]; + char *key; + int rc = 0;
The usage of rc adds a lot of non-whitespace changes. Instead, you can declare another NetDefPtr: virDomainNetDefPtr ret = NULL;
+ + bridge[0] = '\0'; + mac[0] = '\0'; + ip[0] = '\0'; + model[0] = '\0'; + type[0] = '\0';
[...]
- goto cleanup; - if (VIR_ALLOC(bandwidth->out) < 0) { - VIR_FREE(bandwidth); - goto cleanup; - }
do: VIR_STEAL_PTR(ret, net);
+ cleanup: + if (rc < 0) + virDomainNetDefFree(net);
And make this free unconditional. Jano
+ VIR_FREE(script); + return net; +}
There are still a few places using virConfGetValue(), checking for the specific type of the pointers and so on. Those places are not going to be converted as: - Using virConfGetValue*() would trigger virReportError() in the current code, which would cause, at least, some misleading messages for whoever has to debug this code path; - Expanding virConfValue*() to support strings as other types (for instance, as boolean or long) does not seem to be the safest path to take. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 197 ++++++++++++++++++++++----------------------- 1 file changed, 96 insertions(+), 101 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 45fecbced8..2ba1a19c39 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -145,31 +145,18 @@ xenConfigCopyStringInternal(virConfPtr conf, char **value, int allowMissing) { - virConfValuePtr val; + int rc; *value = NULL; - if (!(val = virConfGetValue(conf, name))) { - if (allowMissing) - return 0; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was missing"), name); - return -1; - } - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was not a string"), name); - return -1; - } - if (!val->str) { - if (allowMissing) - return 0; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was missing"), name); - return -1; - } + rc = virConfGetValueString(conf, name, value); + if (rc == 1 && *value) + return 0; - return VIR_STRDUP(*value, val->str); + if (allowMissing) + return 0; + + return -1; } @@ -193,43 +180,43 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char **value) static int xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) { - virConfValuePtr val; + char *string = NULL; + int ret = -1; if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Arguments must be non null")); - return -1; + goto cleanup; } - if (!(val = virConfGetValue(conf, name))) { + if (virConfGetValueString(conf, name, &string) < 0) { if (virUUIDGenerate(uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); - return -1; - } else { - return 0; + goto cleanup; } - } - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("config value %s not a string"), name); - return -1; + ret = 0; + goto cleanup; } - if (!val->str) { + if (!string) { virReportError(VIR_ERR_CONF_SYNTAX, _("%s can't be empty"), name); - return -1; + goto cleanup; } - if (virUUIDParse(val->str, uuid) < 0) { + if (virUUIDParse(string, uuid) < 0) { virReportError(VIR_ERR_CONF_SYNTAX, - _("%s not parseable"), val->str); - return -1; + _("%s not parseable"), string); + goto cleanup; } - return 0; + ret = 0; + + cleanup: + VIR_FREE(string); + return ret; } @@ -242,23 +229,16 @@ xenConfigGetString(virConfPtr conf, const char **value, const char *def) { - virConfValuePtr val; + char *string = NULL; - *value = NULL; - if (!(val = virConfGetValue(conf, name))) { - *value = def; - return 0; - } + *value = def; - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was malformed"), name); + if (virConfGetValueString(conf, name, &string) < 0) return -1; - } - if (!val->str) - *value = def; - else - *value = val->str; + + if (string) + *value = string; + return 0; } @@ -469,27 +449,30 @@ xenParsePCI(char *entry) static int xenParsePCIList(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); + char **pcis = NULL, **entries; + int ret = -1; - if (!list || list->type != VIR_CONF_LIST) + if (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0) return 0; - for (list = list->list; list; list = list->next) { + for (entries = pcis; *entries; entries++) { + char *entry = *entries; virDomainHostdevDefPtr hostdev; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - continue; - - if (!(hostdev = xenParsePCI(list->str))) - return -1; + if (!(hostdev = xenParsePCI(entry))) + goto cleanup; if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { virDomainHostdevDefFree(hostdev); - return -1; + goto cleanup; } } - return 0; + ret = 0; + + cleanup: + virStringListFree(pcis); + return ret; } @@ -603,10 +586,11 @@ xenParseCPUFeatures(virConfPtr conf, static int xenParseVfb(virConfPtr conf, virDomainDefPtr def) { + int ret = -1; int val; + char **vfbs = NULL; char *listenAddr = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list; virDomainGraphicsDefPtr graphics = NULL; if (hvm) { @@ -662,17 +646,24 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } if (!hvm && def->graphics == NULL) { /* New PV guests use this format */ - list = virConfGetValue(conf, "vfb"); - if (list && list->type == VIR_CONF_LIST && - list->list && list->list->type == VIR_CONF_STRING && - list->list->str) { + char **entries; + int rc; + + rc = virConfGetValueStringList(conf, "vfb", false, &vfbs); + if (rc <= 0) { + ret = 0; + goto cleanup; + } + + for (entries = vfbs; *entries; entries++) { char vfb[MAX_VFB]; char *key = vfb; + char *entry = *entries; - if (virStrcpyStatic(vfb, list->list->str) == NULL) { + if (virStrcpyStatic(vfb, entry) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("VFB %s too big for destination"), - list->list->str); + entry); goto cleanup; } @@ -745,21 +736,24 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } } - return 0; + ret = 0; cleanup: virDomainGraphicsDefFree(graphics); VIR_FREE(listenAddr); - return -1; + virStringListFree(vfbs); + return ret; } static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { + + char **serials = NULL; const char *str; - virConfValuePtr value = NULL; virDomainChrDefPtr chr = NULL; + int ret = -1; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { if (xenConfigGetString(conf, "parallel", &str, NULL) < 0) @@ -768,8 +762,10 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) !(chr = xenParseSxprChar(str, NULL))) goto cleanup; if (chr) { - if (VIR_ALLOC_N(def->parallels, 1) < 0) + if (VIR_ALLOC_N(def->parallels, 1) < 0) { + virDomainChrDefFree(chr); goto cleanup; + } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL; chr->target.port = 0; @@ -779,8 +775,8 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) } /* Try to get the list of values to support multiple serial ports */ - value = virConfGetValue(conf, "serial"); - if (value && value->type == VIR_CONF_LIST) { + if (virConfGetValueStringList(conf, "serial", false, &serials) == 1) { + char **entries; int portnum = -1; if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { @@ -789,27 +785,21 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) goto cleanup; } - value = value->list; - while (value) { - char *port = NULL; + for (entries = serials; *entries; entries++) { + char *port = *entries; - if ((value->type != VIR_CONF_STRING) || (value->str == NULL)) - goto cleanup; - port = value->str; portnum++; - if (STREQ(port, "none")) { - value = value->next; + if (STREQ(port, "none")) continue; - } if (!(chr = xenParseSxprChar(port, NULL))) goto cleanup; chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; chr->target.port = portnum; - if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) + if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) { + virDomainChrDefFree(chr); goto cleanup; - - value = value->next; + } } } else { /* If domain is not using multiple serial ports we parse data old way */ @@ -819,8 +809,10 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) !(chr = xenParseSxprChar(str, NULL))) goto cleanup; if (chr) { - if (VIR_ALLOC_N(def->serials, 1) < 0) + if (VIR_ALLOC_N(def->serials, 1) < 0) { + virDomainChrDefFree(chr); goto cleanup; + } chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; chr->target.port = 0; def->serials[0] = chr; @@ -838,11 +830,11 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) def->consoles[0]->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN; } - return 0; + ret = 0; cleanup: - virDomainChrDefFree(chr); - return -1; + virStringListFree(serials); + return ret; } @@ -1040,29 +1032,32 @@ xenParseVif(char *entry, const char *vif_typename) static int xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { - virConfValuePtr list = virConfGetValue(conf, "vif"); + char **vifs = NULL, **entries; + int ret = -1; - if (!list || list->type != VIR_CONF_LIST) + if (virConfGetValueStringList(conf, "vif", false, &vifs) <= 0) return 0; - for (list = list->list; list; list = list->next) { + for (entries = vifs; *entries; entries++) { virDomainNetDefPtr net = NULL; + char *entry = *entries; int rc; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - continue; - - if (!(net = xenParseVif(list->str, vif_typename))) - return -1; + if (!(net = xenParseVif(entry, vif_typename))) + goto cleanup; rc = VIR_APPEND_ELEMENT(def->nets, def->nnets, net); if (rc < 0) { virDomainNetDefFree(net); - return -1; + goto cleanup; } } - return 0; + ret = 0; + + cleanup: + virStringListFree(vifs); + return ret; } -- 2.14.3
While converting the functions of xen_common to use typesafe virConf acessors, I've spotted a few memory leaks, which are fixed in this patch. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 2ba1a19c39..6d9ce9bd66 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -458,14 +458,16 @@ xenParsePCIList(virConfPtr conf, virDomainDefPtr def) for (entries = pcis; *entries; entries++) { char *entry = *entries; virDomainHostdevDefPtr hostdev; + int rc; if (!(hostdev = xenParsePCI(entry))) goto cleanup; - if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { - virDomainHostdevDefFree(hostdev); + rc = VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev); + virDomainHostdevDefFree(hostdev); + + if (rc < 0) goto cleanup; - } } ret = 0; @@ -787,6 +789,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) for (entries = serials; *entries; entries++) { char *port = *entries; + int rc; portnum++; if (STREQ(port, "none")) @@ -796,10 +799,11 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) goto cleanup; chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL; chr->target.port = portnum; - if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0) { - virDomainChrDefFree(chr); + rc = VIR_APPEND_ELEMENT(def->serials, def->nserials, chr); + virDomainChrDefFree(chr); + + if (rc < 0) goto cleanup; - } } } else { /* If domain is not using multiple serial ports we parse data old way */ @@ -1047,10 +1051,10 @@ xenParseVifList(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) goto cleanup; rc = VIR_APPEND_ELEMENT(def->nets, def->nnets, net); - if (rc < 0) { - virDomainNetDefFree(net); + virDomainNetDefFree(net); + + if (rc < 0) goto cleanup; - } } ret = 0; -- 2.14.3
On Mon, May 28, 2018 at 12:28:26AM +0200, Fabiano Fidêncio wrote:
While converting the functions of xen_common to use typesafe virConf acessors, I've spotted a few memory leaks, which are fixed in this patch.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 2ba1a19c39..6d9ce9bd66 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -458,14 +458,16 @@ xenParsePCIList(virConfPtr conf, virDomainDefPtr def) for (entries = pcis; *entries; entries++) { char *entry = *entries; virDomainHostdevDefPtr hostdev; + int rc;
if (!(hostdev = xenParsePCI(entry))) goto cleanup;
- if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { - virDomainHostdevDefFree(hostdev); + rc = VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev); + virDomainHostdevDefFree(hostdev); +
If VIR_APPEND_ELEMENT returns 0, hostdev has been added to the domain definition and it should be freed by whoever frees the virDomainDef. Also, on success the arguments of VIR_APPEND_ELEMENT are zeroed, so this change is a no-op. Jano
On Mon, May 28, 2018 at 12:28:19AM +0200, Fabiano Fidêncio wrote:
This patchset finishes the conversion to virConfGetValue* functions, started by Daniel Berrange a few months ago.
Please, mind that although we could make virConfGetValue* functions more generic in order to support numbers and booleans as strings, that doesn't seem the safest path to take. The side-effect of this is that we will have to live with some specific code doing that as part of vmx and xen_common.
Once this patchset gets merged, https://wiki.libvirt.org/page/BiteSizedTasks#Finish_conversion_to_virConfGet... can be removed.
- Changes since v1: All the "values" from virConfGetValueString() are freed
- Changes since v2: All comments from Ján Tomko have been addressed; A few leaks were (possibly) found and they're addressed in the last patch of the series;
Fabiano Fidêncio (7): xen_xm: Split the per-disk logic from xenParseXMDisk() xen_vm: convert to typesafe virConf accessors vmx: convert to typesafe virConf accessors xen_common: Split per-PCI logic from xenParsePCI()
I have pushed the first four patches.
xen_common: Split per-Vfi logic from xenParseVif() xen_common: convert to typesafe virConf acessors
xen_common: Fix a few memory leaks
This one won't be necessary. Jano
On Mon, Jun 11, 2018 at 3:34 PM, Ján Tomko <jtomko@redhat.com> wrote:
On Mon, May 28, 2018 at 12:28:19AM +0200, Fabiano Fidêncio wrote:
This patchset finishes the conversion to virConfGetValue* functions, started by Daniel Berrange a few months ago.
Please, mind that although we could make virConfGetValue* functions more generic in order to support numbers and booleans as strings, that doesn't seem the safest path to take. The side-effect of this is that we will have to live with some specific code doing that as part of vmx and xen_common.
Once this patchset gets merged,
https://wiki.libvirt.org/page/BiteSizedTasks#Finish_conversion_to_virConfGet... can be removed.
- Changes since v1: All the "values" from virConfGetValueString() are freed
- Changes since v2: All comments from Ján Tomko have been addressed; A few leaks were (possibly) found and they're addressed in the last patch of the series;
Fabiano Fidêncio (7): xen_xm: Split the per-disk logic from xenParseXMDisk() xen_vm: convert to typesafe virConf accessors vmx: convert to typesafe virConf accessors xen_common: Split per-PCI logic from xenParsePCI()
I have pushed the first four patches.
xen_common: Split per-Vfi logic from xenParseVif() xen_common: convert to typesafe virConf acessors
Okay, I'll re-work these other 2 and submit a v3.
xen_common: Fix a few memory leaks
This one won't be necessary.
Jano
Thanks for the review! -- Fabiano Fidêncio
participants (2)
-
Fabiano Fidêncio -
Ján Tomko