[libvirt] [PATCHv2 0/3] 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 Fabiano Fidêncio (3): xen_vm: convert to typesafe virConf accessors vmx: convert to typesafe virConf accessors xen_common: convert to typesafe virConf accessors src/vmx/vmx.c | 196 ++++++-------- src/xenconfig/xen_common.c | 637 ++++++++++++++++++++++----------------------- src/xenconfig/xen_xm.c | 268 ++++++++++--------- 3 files changed, 512 insertions(+), 589 deletions(-) -- 2.14.3
From: Fabiano Fidêncio <fidencio@redhat.com> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_xm.c | 268 ++++++++++++++++++++++++------------------------- 1 file changed, 132 insertions(+), 136 deletions(-) diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 4becb40b4c..fc88ac8238 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def) { virDomainDiskDefPtr disk = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list = virConfGetValue(conf, "disk"); + char **disks = NULL, **entries; - if (list && list->type == VIR_CONF_LIST) { - list = list->list; - while (list) { - char *head; - char *offset; - char *tmp; - const char *src; + if (virConfGetValueStringList(conf, "disk", false, &disks) < 0) + goto cleanup; - if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skipdisk; + for (entries = disks; *entries; entries++) { + char *head = *entries; + char *offset; + char *tmp; + const char *src; - head = list->str; - if (!(disk = virDomainDiskDefNew(NULL))) - return -1; + if (!(disk = virDomainDiskDefNew(NULL))) + return -1; + + /* + * 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; + } + 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) + head = offset + 1; + /* Remove legacy ioemu: junk */ + if (STRPREFIX(head, "ioemu:")) + head = head + 6; + + /* Extract the dest device name */ + if (!(offset = strchr(head, ','))) + goto skipdisk; + + if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) + goto cleanup; + + 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 cleanup; + } + + 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 cleanup; - if (virDomainDiskSetSource(disk, tmp) < 0) { + if (virDomainDiskSetDriver(disk, tmp) < 0) { VIR_FREE(tmp); goto cleanup; } VIR_FREE(tmp); - } - - head = offset + 1; - /* Remove legacy ioemu: junk */ - if (STRPREFIX(head, "ioemu:")) - head = head + 6; - - /* Extract the dest device name */ - if (!(offset = strchr(head, ','))) - goto skipdisk; - if (VIR_ALLOC_N(disk->dst, (offset - head) + 1) < 0) - goto cleanup; + /* Strip the prefix we found off the source file name */ + if (virDomainDiskSetSource(disk, src + len + 1) < 0) + goto cleanup; - 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 cleanup; + src = virDomainDiskGetSource(disk); } - 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 cleanup; - - if (virDomainDiskSetDriver(disk, tmp) < 0) { - VIR_FREE(tmp); - goto cleanup; - } - VIR_FREE(tmp); + /* And the sub-type for tap:XXX: type */ + if (STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap") || + STREQ_NULLABLE(virDomainDiskGetDriver(disk), "tap2")) { + char *driverType; - /* Strip the prefix we found off the source file name */ - if (virDomainDiskSetSource(disk, src + len + 1) < 0) - goto cleanup; + if (!(tmp = strchr(src, ':'))) + goto skipdisk; + len = tmp - src; - src = virDomainDiskGetSource(disk); - } + if (VIR_STRNDUP(driverType, src, len) < 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 (!(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); + 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 cleanup; + /* No source, or driver name, so fix to phy: */ + if (!virDomainDiskGetDriver(disk) && + virDomainDiskSetDriver(disk, "phy") < 0) + goto cleanup; - /* 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'; - } + /* 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 (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; - } + 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; + } - if (STREQ(head, "r") || - STREQ(head, "ro")) - disk->src->readonly = true; - else if ((STREQ(head, "w!")) || - (STREQ(head, "!"))) - disk->src->shared = true; + if (STREQ(head, "r") || + STREQ(head, "ro")) + disk->src->readonly = true; + else if ((STREQ(head, "w!")) || + (STREQ(head, "!"))) + disk->src->shared = true; - /* Maintain list in sorted order according to target device name */ - if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) - goto cleanup; + /* 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; - } + skipdisk: + virDomainDiskDefFree(disk); + disk = NULL; } return 0; cleanup: + virStringListFree(disks); virDomainDiskDefFree(disk); return -1; } -- 2.14.3
On Sat, May 26, 2018 at 11:00:25PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
The S-o-B address should match the one of the author.
--- src/xenconfig/xen_xm.c | 268 ++++++++++++++++++++++++------------------------- 1 file changed, 132 insertions(+), 136 deletions(-)
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 4becb40b4c..fc88ac8238 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def) { virDomainDiskDefPtr disk = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list = virConfGetValue(conf, "disk"); + char **disks = NULL, **entries;
- if (list && list->type == VIR_CONF_LIST) { - list = list->list;
Adding else list = NULL; Would let you move the while below outside of the if body and separate the whitespace changes from the virConfGetValue -> StringList change. Or, even better, the whole per-disk logic can be moved to a separate function.
- while (list) {
- char *head; - char *offset; - char *tmp; - const char *src; + if (virConfGetValueStringList(conf, "disk", false, &disks) < 0) + goto cleanup;
- if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skipdisk; + for (entries = disks; *entries; entries++) { + char *head = *entries; + char *offset; + char *tmp; + const char *src;
- head = list->str; - if (!(disk = virDomainDiskDefNew(NULL))) - return -1; + if (!(disk = virDomainDiskDefNew(NULL))) + return -1; + + /* + * 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;
Using a separate function would also get rid of this unusual label.
+ + 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; + } + VIR_FREE(tmp); + }
[...]
- skipdisk: - list = list->next; - virDomainDiskDefFree(disk); - disk = NULL; - } + skipdisk: + virDomainDiskDefFree(disk); + disk = NULL; }
return 0;
cleanup: + virStringListFree(disks);
The list needs to be freed even in the success path. (NB: this usage of 'cleanup' label is not customary - for code paths returning an error, we use 'error'. 'cleanup' is meant for code shared between the success and error paths) Jano
On Sun, May 27, 2018 at 11:55 AM, Ján Tomko <jtomko@redhat.com> wrote:
On Sat, May 26, 2018 at 11:00:25PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org>
The S-o-B address should match the one of the author.
Fixed, thanks!
--- src/xenconfig/xen_xm.c | 268 ++++++++++++++++++++++++------------------------- 1 file changed, 132 insertions(+), 136 deletions(-)
diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c index 4becb40b4c..fc88ac8238 100644 --- a/src/xenconfig/xen_xm.c +++ b/src/xenconfig/xen_xm.c @@ -112,172 +112,168 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def) { virDomainDiskDefPtr disk = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list = virConfGetValue(conf, "disk"); + char **disks = NULL, **entries;
- if (list && list->type == VIR_CONF_LIST) { - list = list->list;
Adding else list = NULL;
Would let you move the while below outside of the if body and separate the whitespace changes from the virConfGetValue -> StringList change.
Or, even better, the whole per-disk logic can be moved to a separate function.
Right, makes sense.
- while (list) {
- char *head; - char *offset; - char *tmp; - const char *src; + if (virConfGetValueStringList(conf, "disk", false, &disks) < 0) + goto cleanup;
- if ((list->type != VIR_CONF_STRING) || (list->str == NULL)) - goto skipdisk; + for (entries = disks; *entries; entries++) { + char *head = *entries; + char *offset; + char *tmp; + const char *src;
- head = list->str; - if (!(disk = virDomainDiskDefNew(NULL))) - return -1; + if (!(disk = virDomainDiskDefNew(NULL))) + return -1; + + /* + * 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;
Using a separate function would also get rid of this unusual label.
Right.
+ + 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; + } + VIR_FREE(tmp); + }
[...]
- skipdisk: - list = list->next; - virDomainDiskDefFree(disk); - disk = NULL; - } + skipdisk: + virDomainDiskDefFree(disk); + disk = NULL; }
return 0;
cleanup: + virStringListFree(disks);
The list needs to be freed even in the success path.
True!
(NB: this usage of 'cleanup' label is not customary - for code paths returning an error, we use 'error'. 'cleanup' is meant for code shared between the success and error paths)
Aha! Okay, I've decided to follow the "most used" label in the file that had the same intention. I'll adopt your suggestion and we can slowly get rid of the non customary labels whenever touching the old code.
Jano
Thanks for the review! -- Fabiano Fidêncio
From: Fabiano Fidêncio <fidencio@redhat.com> Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/vmx/vmx.c | 196 ++++++++++++++++++++++------------------------------------ 1 file changed, 73 insertions(+), 123 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index df6a58a474..54542c29a6 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -720,41 +720,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 result; *string = NULL; - value = virConfGetValue(conf, name); - if (value == NULL) { - if (optional) - return 0; + result = virConfGetValueString(conf, name, string); + if (result == 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 +758,26 @@ static int virVMXGetConfigUUID(virConfPtr conf, const char *name, unsigned char *uuid, bool optional) { - virConfValuePtr value; + char *string = NULL; + int result; - value = virConfGetValue(conf, name); + result = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (result <= 0) + return result; - 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 (virUUIDParse(string, uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Config entry '%s' must be a string"), name); - return -1; + _("Could not parse UUID from string '%s'"), string); + result = -1; + goto cleanup; } - if (value->str == NULL) { - if (optional) { - return 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing essential config entry '%s'"), name); - return -1; - } - } + result = 0; - if (virUUIDParse(value->str, uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not parse UUID from string '%s'"), value->str); - return -1; - } + cleanup: + VIR_FREE(string); - return 0; + return result; } @@ -808,47 +786,35 @@ static int virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number, long long default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int result; *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; - } - } + result = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (result <= 0) + return result; - 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; - } - } + if (STRCASEEQ(string, "unlimited")) { + *number = -1; + result = 0; + goto cleanup; + } - 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 (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); + result = -1; + goto cleanup; } - return 0; + result = 0; + + cleanup: + VIR_FREE(string); + + return result; } @@ -857,49 +823,33 @@ static int virVMXGetConfigBoolean(virConfPtr conf, const char *name, bool *boolean_, bool default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int result; *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; - } - } + result = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (result <= 0) + return result; - 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; - } - } - - 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); + result = -1; + goto cleanup; } - return 0; + result = 0; + + cleanup: + VIR_FREE(string); + + return result; } -- 2.14.3
On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/vmx/vmx.c | 196 ++++++++++++++++++++++------------------------------------ 1 file changed, 73 insertions(+), 123 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index df6a58a474..54542c29a6 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -808,47 +786,35 @@ static int virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number, long long default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int result;
Usually we use 'ret' as the name of the variable that will eventually be used to return from the function and 'rc' to store the value returned by the function we call (where they return more values than 0/-1): 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; - } - } + result = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (result <= 0) + return result;
This would become: rc = GetStringHelper(); if (rc <= 0) return rc;
- 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; - } - } + if (STRCASEEQ(string, "unlimited")) { + *number = -1; + result = 0; + goto cleanup; + }
- 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 you leave this 'else' here, there's no need to touch 'ret' or goto cleanup above.
+ 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);
+ result = -1;
This adjustment would also be unnecessary.
+ goto cleanup; }
- return 0; + result = 0; +
(NB: while many of the functions in this file use 'result' instead of 'ret', I'd suggest to slowly start replacing the old name instead of striving for consistency. Most of them are the Parse functions that already return a device definition via a pointer argument and the 0/-1 they return is redundant, because it can be replaced by a NULL check on the pointer) Jano Jano
On Sun, May 27, 2018 at 1:02 PM, Ján Tomko <jtomko@redhat.com> wrote:
On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/vmx/vmx.c | 196 ++++++++++++++++++++++------------------------------------ 1 file changed, 73 insertions(+), 123 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index df6a58a474..54542c29a6 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -808,47 +786,35 @@ static int virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number, long long default_, bool optional) { - virConfValuePtr value; + char *string = NULL; + int result;
Usually we use 'ret' as the name of the variable that will eventually be used to return from the function and 'rc' to store the value returned by the function we call (where they return more values than 0/-1):
int ret = -1; int rc;
Right!
*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; - } - } + result = virVMXGetConfigStringHelper(conf, name, &string, optional); + if (result <= 0) + return result;
This would become: rc = GetStringHelper(); if (rc <= 0) return rc;
Okay.
- 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; - } - } + if (STRCASEEQ(string, "unlimited")) { + *number = -1; + result = 0; + goto cleanup; + }
- 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 you leave this 'else' here, there's no need to touch 'ret' or goto cleanup above.
Okay, I'll keep it as it is.
+ 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);
+ result = -1;
This adjustment would also be unnecessary.
+ goto cleanup; }
- return 0; + result = 0; +
(NB: while many of the functions in this file use 'result' instead of 'ret', I'd suggest to slowly start replacing the old name instead of striving for consistency. Most of them are the Parse functions that already return a device definition via a pointer argument and the 0/-1 they return is redundant, because it can be replaced by a NULL check on the pointer)
Right!
Jano
Jano
Thanks for the review! -- Fabiano Fidêncio
From: Fabiano Fidêncio <fidencio@redhat.com> There are still some places using virConfGetValue() and then checking the specific type of the pointers and so on. Those place are not going to be changed as: - Directly using virConfGetValue*() would trigger virReportError() on their current code - Expanding virConfGetValue*() to support strings as other types (as boolean or long) doesn't seem to be the safest path to take. Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 637 ++++++++++++++++++++++----------------------- 1 file changed, 307 insertions(+), 330 deletions(-) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a2b0708ee3..2e8e95f720 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 result; *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; - } + result = virConfGetValueString(conf, name, value); + if (result == 1 && *value != NULL) + return 0; + + if (allowMissing) + return 0; - return VIR_STRDUP(*value, val->str); + return -1; } @@ -193,7 +180,8 @@ 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 result; if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) return -1; } - if (!(val = virConfGetValue(conf, name))) { - if (virUUIDGenerate(uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - return -1; - } else { - return 0; - } - } - - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("config value %s not a string"), name); + if (virConfGetValueString(conf, name, &string) < 0) return -1; - } - if (!val->str) { + if (!string) { virReportError(VIR_ERR_CONF_SYNTAX, _("%s can't be empty"), name); return -1; } - if (virUUIDParse(val->str, uuid) < 0) { + if (virUUIDGenerate(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID")); + result = -1; + goto cleanup; + } + + if (virUUIDParse(string, uuid) < 0) { virReportError(VIR_ERR_CONF_SYNTAX, - _("%s not parseable"), val->str); - return -1; + _("%s not parseable"), string); + result = -1; + goto cleanup; } - return 0; + result = 1; + + cleanup: + VIR_FREE(string); + + return result; } @@ -242,23 +230,15 @@ xenConfigGetString(virConfPtr conf, const char **value, const char *def) { - virConfValuePtr val; + char *string = NULL; - *value = NULL; - if (!(val = virConfGetValue(conf, name))) { + if (virConfGetValueString(conf, name, &string) < 0) { *value = def; - return 0; - } - - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("config value %s was malformed"), name); return -1; } - if (!val->str) - *value = def; - else - *value = val->str; + + *value = string != NULL ? string : def; + return 0; } @@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) static int xenParsePCI(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); virDomainHostdevDefPtr hostdev = NULL; + char **pcis = NULL, **entries; - 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'; - - 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, ':'))) - 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 (!(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 (virConfGetValueStringList(conf, "pci", false, &pcis) <= 0) + return 0; - 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; - } + for (entries = pcis; *entries; entries++) { + 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 = *entries; + + /* pci=['0000:00:1b.0','0000:00:13.0'] */ + if (!(nextkey = strchr(key, ':'))) + continue; + if (virStrncpy(domain, key, (nextkey - key), sizeof(domain)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Domain %s too big for destination"), key); + continue; + } + + key = nextkey + 1; + if (!(nextkey = strchr(key, ':'))) + continue; + if (virStrncpy(bus, key, (nextkey - key), sizeof(bus)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Bus %s too big for destination"), key); + continue; + } + + key = nextkey + 1; + if (!(nextkey = strchr(key, '.'))) + continue; + if (virStrncpy(slot, key, (nextkey - key), sizeof(slot)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Slot %s too big for destination"), key); + continue; + } + + key = nextkey + 1; + if (strlen(key) != 1) + continue; + if (virStrncpy(func, key, 1, sizeof(func)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Function %s too big for destination"), key); + continue; + } + + if (virStrToLong_i(domain, NULL, 16, &domainID) < 0) + continue; + if (virStrToLong_i(bus, NULL, 16, &busID) < 0) + continue; + if (virStrToLong_i(slot, NULL, 16, &slotID) < 0) + continue; + if (virStrToLong_i(func, NULL, 16, &funcID) < 0) + continue; + if (!(hostdev = virDomainHostdevDefNew())) + goto cleanup; - 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; - } + if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, hostdev) < 0) { + virDomainHostdevDefFree(hostdev); + goto cleanup; + } } + virStringListFree(pcis); return 0; + + cleanup: + virStringListFree(pcis); + return -1; } @@ -593,9 +572,9 @@ static int xenParseVfb(virConfPtr conf, virDomainDefPtr def) { int val; + char **vfbs = NULL, **entries; char *listenAddr = NULL; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; - virConfValuePtr list; virDomainGraphicsDefPtr graphics = NULL; if (hvm) { @@ -651,17 +630,18 @@ 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) { + if (virConfGetValueStringList(conf, "vfb", false, &vfbs) <= 0) + return 0; + + 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; } @@ -734,11 +714,13 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) } } + virStringListFree(vfbs); return 0; cleanup: virDomainGraphicsDefFree(graphics); VIR_FREE(listenAddr); + virStringListFree(vfbs); return -1; } @@ -746,11 +728,14 @@ xenParseVfb(virConfPtr conf, virDomainDefPtr def) static int xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) { + char **serials = NULL, **entries; const char *str; virConfValuePtr value = NULL; virDomainChrDefPtr chr = NULL; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { + int portnum = -1; + if (xenConfigGetString(conf, "parallel", &str, NULL) < 0) goto cleanup; if (str && STRNEQ(str, "none") && @@ -768,39 +753,7 @@ 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) { - int portnum = -1; - - if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Multiple serial devices are not supported by xen-xm")); - goto cleanup; - } - - value = value->list; - while (value) { - char *port = NULL; - - if ((value->type != VIR_CONF_STRING) || (value->str == NULL)) - goto cleanup; - port = value->str; - portnum++; - if (STREQ(port, "none")) { - value = value->next; - 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) - goto cleanup; - - value = value->next; - } - } else { + if (virConfGetValueStringList(conf, "serial", false, &serials) <= 0) { /* If domain is not using multiple serial ports we parse data old way */ if (xenConfigGetString(conf, "serial", &str, NULL) < 0) goto cleanup; @@ -815,7 +768,33 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) def->serials[0] = chr; def->nserials++; } + + return 0; + } + + if (STREQ(nativeFormat, XEN_CONFIG_FORMAT_XM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Multiple serial devices are not supported by xen-xm")); + goto cleanup; + } + + for (entries = serials; *entries; entries++) { + char *port = *entries; + + portnum++; + if (STREQ(port, "none")) { + value = value->next; + 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) + goto cleanup; } + virStringListFree(serials); } else { if (VIR_ALLOC_N(def->consoles, 1) < 0) goto cleanup; @@ -831,6 +810,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) cleanup: virDomainChrDefFree(chr); + virStringListFree(serials); return -1; } @@ -838,197 +818,194 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char *nativeFormat) static int xenParseVif(virConfPtr conf, virDomainDefPtr def, const char *vif_typename) { + char **vifs = NULL, **entries; char *script = NULL; virDomainNetDefPtr net = NULL; - virConfValuePtr list = virConfGetValue(conf, "vif"); - - 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, ','); + if (virConfGetValueStringList(conf, "vif", false, &vifs) <= 0) + return 0; - if (!(data = strchr(key, '='))) + for (entries = vifs; *entries; entries++) { + char model[10]; + char type[10]; + char ip[128]; + char mac[18]; + char bridge[50]; + char vifname[50]; + char rate[50]; + char *key = *entries; + + bridge[0] = '\0'; + mac[0] = '\0'; + ip[0] = '\0'; + model[0] = '\0'; + type[0] = '\0'; + vifname[0] = '\0'; + rate[0] = '\0'; + + while (key) { + char *data; + char *nextkey = strchr(key, ','); + + 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; - 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; - } } - - while (nextkey && (nextkey[0] == ',' || - nextkey[0] == ' ' || - nextkey[0] == '\t')) - nextkey++; - key = nextkey; - } - - if (VIR_ALLOC(net) < 0) - goto cleanup; - - if (mac[0]) { - if (virMacAddrParse(mac, &net->mac) < 0) { + } 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, - _("malformed mac address '%s'"), mac); + _("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 (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 (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; + while (nextkey && (nextkey[0] == ',' || + nextkey[0] == ' ' || + nextkey[0] == '\t')) + nextkey++; + key = nextkey; + } - if (!ip_list) - goto cleanup; + if (VIR_ALLOC(net) < 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 (mac[0]) { + if (virMacAddrParse(mac, &net->mac) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("malformed mac address '%s'"), mac); + goto cleanup; } + } - if (script && script[0] && - VIR_STRDUP(net->script, script) < 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 (model[0] && - VIR_STRDUP(net->model, model) < 0) + 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 (!model[0] && type[0] && STREQ(type, vif_typename) && - VIR_STRDUP(net->model, "netfront") < 0) + if (!ip_list) goto cleanup; - if (vifname[0] && - VIR_STRDUP(net->ifname, vifname) < 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 (rate[0]) { - virNetDevBandwidthPtr bandwidth; - unsigned long long kbytes_per_sec; + if (script && script[0] && + VIR_STRDUP(net->script, script) < 0) + goto cleanup; - if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0) - goto cleanup; + if (model[0] && + VIR_STRDUP(net->model, model) < 0) + goto cleanup; - if (VIR_ALLOC(bandwidth) < 0) - goto cleanup; - if (VIR_ALLOC(bandwidth->out) < 0) { - VIR_FREE(bandwidth); - goto cleanup; - } + if (!model[0] && type[0] && STREQ(type, vif_typename) && + VIR_STRDUP(net->model, "netfront") < 0) + goto cleanup; - bandwidth->out->average = kbytes_per_sec; - net->bandwidth = bandwidth; - } + if (vifname[0] && + VIR_STRDUP(net->ifname, vifname) < 0) + goto cleanup; - if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) + if (rate[0]) { + virNetDevBandwidthPtr bandwidth; + unsigned long long kbytes_per_sec; + + if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0) goto cleanup; - skipnic: - list = list->next; - virDomainNetDefFree(net); - net = NULL; - VIR_FREE(script); + if (VIR_ALLOC(bandwidth) < 0) + goto cleanup; + if (VIR_ALLOC(bandwidth->out) < 0) { + VIR_FREE(bandwidth); + goto cleanup; + } + + bandwidth->out->average = kbytes_per_sec; + net->bandwidth = bandwidth; } + + if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) + goto cleanup; + + skipnic: + virDomainNetDefFree(net); + net = NULL; + VIR_FREE(script); } + virStringListFree(vifs); return 0; cleanup: virDomainNetDefFree(net); + virStringListFree(vifs); VIR_FREE(script); return -1; } -- 2.14.3
On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
There are still some places using virConfGetValue() and then checking the specific type of the pointers and so on.
Those place are not going to be changed as: - Directly using virConfGetValue*() would trigger virReportError() on their current code
Is that a problem in xenParseCPUFeatures?
- Expanding virConfGetValue*() to support strings as other types (as boolean or long) doesn't seem to be the safest path to take.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 637 ++++++++++++++++++++++----------------------- 1 file changed, 307 insertions(+), 330 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a2b0708ee3..2e8e95f720 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 result;
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; - } + result = virConfGetValueString(conf, name, value); + if (result == 1 && *value != NULL) + return 0; + + if (allowMissing) + return 0;
- return VIR_STRDUP(*value, val->str); + return -1; }
@@ -193,7 +180,8 @@ 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 result;
int ret = -1;
if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) return -1; }
- if (!(val = virConfGetValue(conf, name))) { - if (virUUIDGenerate(uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - return -1; - } else { - return 0; - } - } - - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("config value %s not a string"), name); + if (virConfGetValueString(conf, name, &string) < 0) return -1; - }
- if (!val->str) { + if (!string) { virReportError(VIR_ERR_CONF_SYNTAX, _("%s can't be empty"), name); return -1; }
- if (virUUIDParse(val->str, uuid) < 0) { + if (virUUIDGenerate(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID"));
+ result = -1;
Redundant if you initialize it to -1;
+ goto cleanup; + } + + if (virUUIDParse(string, uuid) < 0) { virReportError(VIR_ERR_CONF_SYNTAX, - _("%s not parseable"), val->str); - return -1; + _("%s not parseable"), string);
+ result = -1;
Dtto.
+ goto cleanup; }
- return 0; + result = 1;
ret = 0;
+ + cleanup: + VIR_FREE(string);
+
Dropping this line could improve readability - it's already visually separated by the cleanup label and }
+ return result; }
@@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) static int xenParsePCI(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); virDomainHostdevDefPtr hostdev = NULL; + char **pcis = NULL, **entries;
Another function that would benefit from being split. [...]
+ virStringListFree(pcis); return 0; + + cleanup:
Use the ret variable to join these cleanup paths into one.
+ virStringListFree(pcis); + return -1; }
Jano
On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko@redhat.com> wrote:
On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
There are still some places using virConfGetValue() and then checking the specific type of the pointers and so on.
Those place are not going to be changed as: - Directly using virConfGetValue*() would trigger virReportError() on their current code
Is that a problem in xenParseCPUFeatures?
It would, at least, generate one more log, which would be misleading whoever ends up debugging some issue on that codepath later on.
- Expanding virConfGetValue*() to support strings as other types (as boolean or long) doesn't seem to be the safest path to take.
Signed-off-by: Fabiano Fidêncio <fabiano@fidencio.org> --- src/xenconfig/xen_common.c | 637 ++++++++++++++++++++++----------------------- 1 file changed, 307 insertions(+), 330 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index a2b0708ee3..2e8e95f720 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 result;
int rc;
Noted!
*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; - } + result = virConfGetValueString(conf, name, value); + if (result == 1 && *value != NULL) + return 0; + + if (allowMissing) + return 0;
- return VIR_STRDUP(*value, val->str); + return -1; }
@@ -193,7 +180,8 @@ 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 result;
int ret = -1;
Noted!
if (!uuid || !name || !conf) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) return -1; }
- if (!(val = virConfGetValue(conf, name))) { - if (virUUIDGenerate(uuid) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Failed to generate UUID")); - return -1; - } else { - return 0; - } - } - - if (val->type != VIR_CONF_STRING) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("config value %s not a string"), name); + if (virConfGetValueString(conf, name, &string) < 0) return -1; - }
- if (!val->str) { + if (!string) { virReportError(VIR_ERR_CONF_SYNTAX, _("%s can't be empty"), name); return -1; }
- if (virUUIDParse(val->str, uuid) < 0) { + if (virUUIDGenerate(uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Failed to generate UUID"));
+ result = -1;
Redundant if you initialize it to -1;
+ goto cleanup; + } + + if (virUUIDParse(string, uuid) < 0) { virReportError(VIR_ERR_CONF_SYNTAX, - _("%s not parseable"), val->str); - return -1; + _("%s not parseable"), string);
+ result = -1;
Dtto.
+ goto cleanup; }
- return 0; + result = 1;
ret = 0;
+ + cleanup: + VIR_FREE(string);
+
Dropping this line could improve readability - it's already visually separated by the cleanup label and }
+ return result; }
@@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf, virDomainDefPtr def) static int xenParsePCI(virConfPtr conf, virDomainDefPtr def) { - virConfValuePtr list = virConfGetValue(conf, "pci"); virDomainHostdevDefPtr hostdev = NULL; + char **pcis = NULL, **entries;
Another function that would benefit from being split.
[...]
+ virStringListFree(pcis); return 0; + + cleanup:
Use the ret variable to join these cleanup paths into one.
+ virStringListFree(pcis); + return -1; }
Jano
Thanks for the review! -- Fabiano Fidêncio
On Sun, May 27, 2018 at 02:08:58PM +0200, Fabiano Fidêncio wrote:
On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko@redhat.com> wrote:
On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
There are still some places using virConfGetValue() and then checking the specific type of the pointers and so on.
Those place are not going to be changed as: - Directly using virConfGetValue*() would trigger virReportError() on their current code
Is that a problem in xenParseCPUFeatures?
It would, at least, generate one more log, which would be misleading whoever ends up debugging some issue on that codepath later on.
I don't see it. xenConfigGetULong already reports an error when the "maxvcpus" value is malformed. Jano
On Mon, May 28, 2018 at 9:16 AM, Ján Tomko <jtomko@redhat.com> wrote:
On Sun, May 27, 2018 at 02:08:58PM +0200, Fabiano Fidêncio wrote:
On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko@redhat.com> wrote:
On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
From: Fabiano Fidêncio <fidencio@redhat.com>
There are still some places using virConfGetValue() and then checking the specific type of the pointers and so on.
Those place are not going to be changed as: - Directly using virConfGetValue*() would trigger virReportError() on their current code
Is that a problem in xenParseCPUFeatures?
It would, at least, generate one more log, which would be misleading whoever ends up debugging some issue on that codepath later on.
I don't see it. xenConfigGetULong already reports an error when the "maxvcpus" value is malformed.
What xenConfigGetULong does is: val = virConfGetValue() if (val->type == _ULLONG) { ... } else if (val->type == _STRING) { virStrToLong_ul(...) ... and in case of failure, it reports an error; } else { an error is reported } If we simply try to do something similar with virConfGetValue ... we'd end up with: if (virConfGetValueULLong() < 0) { /* here, in case virConfGetValueULLong fails, an error is already reported ... and we don't want it to happen, as the config may come as a string */ if (virConfGetString() ... { } } So, summing up, the main difference is that checking by ULLONG in the current approach doesn't generate any error/failure. While checking for ULLONG wih virConfGetValueULLong() would trigger the error from inside the function. One thing that could be done ... expand virConfGetValueULLong() to also support receiving its value as VIR_CONF_STRING. I've talked to Michal about that and he explicitly mentioned that this may not be the way to go and then I've decided to leave the code as it is. Is it clear? Did I miss something? Best Regards, -- Fabiano Fidêncio
participants (2)
-
Fabiano Fidêncio -
Ján Tomko