11:59 p.m.
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_virConf...
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;
- Changes since v3:
All commentes from Ján Tomko have been addressed in order to lower the
non-whitespace changes in the first patch;
Fabiano Fidêncio (2):
xen_common: Split per-Vif logic from xenParseVif()
xen_common: convert to typesafe virConf acessors
src/xenconfig/xen_common.c | 535 +++++++++++++++++++++++----------------------
1 file changed, 273 insertions(+), 262 deletions(-)
--
2.14.3
11:59 p.m.
New subject: [libvirt] [PATCH v4 1/2] xen_common: Split per-Vif logic from xenParseVif()
xenParseVif() does a lot of stuff and, in order to make things cleaner,
let's split it in two new functions:
- xenParseVif(): it's a new function that keeps the old name. It's
responsible for the whole per-Vif logic from the old xenParseVif();
- xenParseVifList(): it's basically the old xenParsePCI(), but now it
just iterates over the list of Vifs, calling xenParsePCI() per each Vif.
This patch is basically preparing the ground for the future when
typesafe virConf acessors will be used.
Signed-off-by: Fabiano Fidêncio <fabiano(a)fidencio.org>
---
src/xenconfig/xen_common.c | 358 +++++++++++++++++++++++----------------------
1 file changed, 187 insertions(+), 171 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index fc7b0683b8..02765c540b 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -846,202 +846,218 @@ 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");
+ virDomainNetDefPtr ret = NULL;
+ 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;
+
+ 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]) {
+ if (VIR_STRDUP(net->script, script) < 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]) {
+ if (VIR_STRDUP(net->model, model) < 0)
+ goto cleanup;
+ }
- if (script && script[0] &&
- VIR_STRDUP(net->script, script) < 0)
- goto cleanup;
+ if (!model[0] && type[0] && STREQ(type, vif_typename)) {
+ if (VIR_STRDUP(net->model, "netfront") < 0)
+ goto cleanup;
+ }
- if (model[0] &&
- VIR_STRDUP(net->model, model) < 0)
- goto cleanup;
+ if (vifname[0]) {
+ if (VIR_STRDUP(net->ifname, vifname) < 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;
+ if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0)
+ goto cleanup;
- if (rate[0]) {
- virNetDevBandwidthPtr bandwidth;
- unsigned long long kbytes_per_sec;
+ if (VIR_ALLOC(bandwidth) < 0)
+ goto cleanup;
- if (xenParseSxprVifRate(rate, &kbytes_per_sec) < 0)
- goto cleanup;
+ if (VIR_ALLOC(bandwidth->out) < 0) {
+ VIR_FREE(bandwidth);
+ goto cleanup;
+ }
- 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;
+ }
- bandwidth->out->average = kbytes_per_sec;
- net->bandwidth = bandwidth;
- }
+ VIR_STEAL_PTR(ret, net);
- if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0)
- goto cleanup;
+ cleanup:
+ virDomainNetDefFree(net);
+ VIR_FREE(script);
+ return ret;
+}
+
+
+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 +1142,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
10:23 a.m.
New subject: [libvirt] [PATCH v4 1/2] xen_common: Split per-Vif logic from xenParseVif()
On Thu, Jun 14, 2018 at 06:59:52AM +0200, Fabiano Fidêncio wrote:
xenParseVif() does a lot of stuff and, in order to make things
cleaner,
let's split it in two new functions:
- xenParseVif(): it's a new function that keeps the old name. It's
responsible for the whole per-Vif logic from the old xenParseVif();
- xenParseVifList(): it's basically the old xenParsePCI(), but now it
just iterates over the list of Vifs, calling xenParsePCI() per each Vif.
This patch is basically preparing the ground for the future when
typesafe virConf acessors will be used.
Signed-off-by: Fabiano Fidêncio <fabiano(a)fidencio.org>
---
src/xenconfig/xen_common.c | 358 +++++++++++++++++++++++----------------------
1 file changed, 187 insertions(+), 171 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
With the following diff squashed in, to minimize changes:
Jano
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 02765c540b..4a94127da1 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -984,25 +984,21 @@ xenParseVif(char *entry, const char *vif_typename)
virStringListFree(ip_list);
}
- if (script && script[0]) {
- if (VIR_STRDUP(net->script, script) < 0)
- goto cleanup;
- }
+ if (script && script[0] &&
+ VIR_STRDUP(net->script, script) < 0)
+ goto cleanup;
- if (model[0]) {
- if (VIR_STRDUP(net->model, model) < 0)
- goto cleanup;
- }
+ if (model[0] &&
+ VIR_STRDUP(net->model, model) < 0)
+ goto cleanup;
- if (!model[0] && type[0] && STREQ(type, vif_typename)) {
- if (VIR_STRDUP(net->model, "netfront") < 0)
- goto cleanup;
- }
+ if (!model[0] && type[0] && STREQ(type, vif_typename) &&
+ VIR_STRDUP(net->model, "netfront") < 0)
+ goto cleanup;
- if (vifname[0]) {
- if (VIR_STRDUP(net->ifname, vifname) < 0)
- goto cleanup;
- }
+ if (vifname[0] &&
+ VIR_STRDUP(net->ifname, vifname) < 0)
+ goto cleanup;
if (rate[0]) {
virNetDevBandwidthPtr bandwidth;
11:59 p.m.
New subject: [libvirt] [PATCH v4 2/2] xen_common: convert to typesafe virConf acessors
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(a)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 02765c540b..2dc5e62a9e 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;
}
@@ -1035,29 +1027,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
10:46 a.m.
New subject: [libvirt] [PATCH v4 2/2] xen_common: convert to typesafe virConf acessors
On Thu, Jun 14, 2018 at 06:59:53AM +0200, Fabiano Fidêncio wrote:
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(a)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 02765c540b..2dc5e62a9e 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;
+
Here you return -1 without setting an error.
Previously, "config value %s was missing" was reported.
+ 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;
}
There are three changes mixed in the diff of this function
* conversion from directly returning -1/0 to using cleanup/ret
* the change of usage val->str to string
* the actual virConfGetValue -> virConfGetValueString change
While the last two somehow belong together, the first one can be
separated to make the logic changes clear. Separating functional
changes makes the commits easier to digest for future readers, see [0]
Previously, if the value was not present, we would attempt to generate
the UUID. After your change, the UUID is generated if something other
than a string was specified, and if the option is missing, an error
is reported.
- }
- 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;
Before, *value was untouched if the config value was malformed.
- 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;
Rewriting the logic here was not necessary.
+
return 0;
}
@@ -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);
Instead of distributing virDomainChrDefFree all over the function, every
point that assigns it to the domain definition should zero the local
'chr' pointer, either via VIR_APPEND_ELEMENT, or VIR_STEAL_PTR.
Jano
goto cleanup;
-
- value = value->next;
+ }
}
} else {
/* If domain is not using multiple serial ports we parse data old way */
[0]
https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack...
2383
days inactive
2383
days old
4 comments
2 participants
participants (2)
-
Fabiano Fidêncio -
Ján Tomko