7:59 a.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 comments from Ján Tomko have been addressed in order to lower the
non-whitespace changes in the first patch;
- Chages since v4:
All comments from Ján Tomko have been addressed (hopefully, they
actually were this time :-));
Fabiano Fidêncio (1):
xen_common: Convert to typesafe virConf acessors
src/xenconfig/xen_common.c | 163 +++++++++++++++++--------------------
1 file changed, 75 insertions(+), 88 deletions(-)
--
2.17.1
7:59 a.m.
New subject: [libvirt] [libvirt PATCH v5 1/1] 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. However, 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 neither the safest nor
the preferential path to take.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 163 +++++++++++++++++--------------------
1 file changed, 75 insertions(+), 88 deletions(-)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index fdca9845aa..c31ebfb270 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -145,31 +145,19 @@ 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;
- }
+ rc = virConfGetValueString(conf, name, value);
+ if (rc == 1 && *value)
+ return 0;
- 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;
- }
+ if (allowMissing)
+ return 0;
- return VIR_STRDUP(*value, val->str);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("config value %s was missing"), name);
+ return -1;
}
@@ -193,7 +181,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char
**value)
static int
xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
{
- virConfValuePtr val;
+ char *string = NULL;
if (!uuid || !name || !conf) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -201,7 +189,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
return -1;
}
- 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"));
@@ -211,24 +199,20 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
}
}
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_CONF_SYNTAX,
- _("config value %s not a string"), name);
- 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 (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_CONF_SYNTAX,
- _("%s not parseable"), val->str);
+ _("%s not parseable"), string);
+ VIR_FREE(string);
return -1;
}
+ VIR_FREE(string);
return 0;
}
@@ -242,23 +226,16 @@ 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)
+ if (!string)
*value = def;
else
- *value = val->str;
+ *value = string;
return 0;
}
@@ -469,27 +446,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 +583,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 +643,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) < 0) {
+ if (virStrcpyStatic(vfb, entry) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("VFB %s too big for destination"),
- list->list->str);
+ entry);
goto cleanup;
}
@@ -745,21 +733,23 @@ 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)
@@ -779,8 +769,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,18 +779,12 @@ 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;
@@ -808,8 +792,6 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = portnum;
if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0)
goto cleanup;
-
- value = value->next;
}
} else {
/* If domain is not using multiple serial ports we parse data old way */
@@ -825,6 +807,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = 0;
def->serials[0] = chr;
def->nserials++;
+ chr = NULL;
}
}
} else {
@@ -838,11 +821,12 @@ 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;
}
@@ -1031,29 +1015,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.17.1
10:04 a.m.
New subject: [libvirt] [libvirt PATCH v5 1/1] xen_common: Convert to typesafe virConf acessors
On 09/11/2018 08:59 AM, Fabiano Fidêncio wrote:
There are still a few places using virConfGetValue(), checking for
the
specific type of the pointers and so on. However, 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 neither the safest nor
the preferential path to take.
Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
---
src/xenconfig/xen_common.c | 163 +++++++++++++++++--------------------
1 file changed, 75 insertions(+), 88 deletions(-)
/me thinks much of the above probably should have gone under the ---
indicating your knowledge of more places, but the decision to not change
them for the stated reasons. That commit message doesn't necessarily
describe the subsequent changes...
Perhaps a generic commit messages such as:
Change to using virConfGetValueString rather than open coding for
instances in which error messages are expected to be generated.
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index fdca9845aa..c31ebfb270 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -145,31 +145,19 @@ 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;
- }
+ rc = virConfGetValueString(conf, name, value);
I believe this should be:
if ((rc = virConfGetValueString(conf, name, value)) < 0)
return -1;
otherwise, we could get to the allowMissing w/ rc == -1 and return 0
even though previously for a VIR_STRDUP failure we'd return -1. We'd
also overwrite the no memory error from VIR_STRDUP failure.
+ if (rc == 1 && *value)
+ return 0;
- 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;
- }
+ if (allowMissing)
+ return 0;
- return VIR_STRDUP(*value, val->str);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("config value %s was missing"), name);
+ return -1;
}
@@ -193,7 +181,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char *name, char
**value)
static int
xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
{
- virConfValuePtr val;
+ char *string = NULL;
if (!uuid || !name || !conf) {
virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -201,7 +189,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
return -1;
}
- if (!(val = virConfGetValue(conf, name))) {
+ if (virConfGetValueString(conf, name, &string) < 0) {
This is not the same check - < 0 is returned on errors (!STRING and
VIR_STRDUP failure).
I think you should use:
rc = virConfGetValueString(conf, name, &string);
and then continue from there with the rc and/or string checks.
if (virUUIDGenerate(uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("Failed to generate UUID"));
@@ -211,24 +199,20 @@ xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char
*uuid)
}
}
- if (val->type != VIR_CONF_STRING) {
- virReportError(VIR_ERR_CONF_SYNTAX,
- _("config value %s not a string"), name);
- return -1;
- }
-
- if (!val->str) {
+ if (!string) {
I think this would be if virConfGetValueString returns 1, but !string
because VIR_STRDUP(*value, NULL) would return NULL in *value.
virReportError(VIR_ERR_CONF_SYNTAX,
_("%s can't be empty"), name);
return -1;
}
- if (virUUIDParse(val->str, uuid) < 0) {
+ if (virUUIDParse(string, uuid) < 0) {
virReportError(VIR_ERR_CONF_SYNTAX,
- _("%s not parseable"), val->str);
+ _("%s not parseable"), string);
+ VIR_FREE(string);
return -1;
}
+ VIR_FREE(string);
return 0;
}
@@ -242,23 +226,16 @@ 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) {
more of the same here as from previous < 0 is error, while ret == 0
would be this case
*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)
+ if (!string)
and if rc == 1 and !string
*value = def;
else
- *value = val->str;
+ *value = string;
And the problem here is that @string would be leaked since it's a
VIR_STRDUP of val->str, the @*def is a const char of something.
return 0;
}
@@ -469,27 +446,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)
Again, not the same checks... Not sure this particular one can be used.
The < 0 is an error path...
I'm imagining many of the rest are similar. Probably should have split
things up into single patches for each method (as painful as it is) so
that it's easier to review and easier to pick and choose what doesn't
work and what does.
BTW: I would do all the virConfGetValueString's first... Then tackle the
virConfGetValueStringList's - since it doesn't cause one to context
switch "too much" between two different API's.
John
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 +583,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 +643,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) < 0) {
+ if (virStrcpyStatic(vfb, entry) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("VFB %s too big for destination"),
- list->list->str);
+ entry);
goto cleanup;
}
@@ -745,21 +733,23 @@ 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)
@@ -779,8 +769,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,18 +779,12 @@ 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;
@@ -808,8 +792,6 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = portnum;
if (VIR_APPEND_ELEMENT(def->serials, def->nserials, chr) < 0)
goto cleanup;
-
- value = value->next;
}
} else {
/* If domain is not using multiple serial ports we parse data old way */
@@ -825,6 +807,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr def, const char
*nativeFormat)
chr->target.port = 0;
def->serials[0] = chr;
def->nserials++;
+ chr = NULL;
}
}
} else {
@@ -838,11 +821,12 @@ 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;
}
@@ -1031,29 +1015,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;
}
6:28 a.m.
New subject: [libvirt] [libvirt PATCH v5 1/1] xen_common: Convert to typesafe virConf acessors
John,
On Fri, Sep 14, 2018 at 5:04 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 09/11/2018 08:59 AM, Fabiano Fidêncio wrote:
> There are still a few places using virConfGetValue(), checking for the
> specific type of the pointers and so on. However, 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 neither the safest nor
> the preferential path to take.
>
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/xenconfig/xen_common.c | 163 +++++++++++++++++--------------------
> 1 file changed, 75 insertions(+), 88 deletions(-)
>
/me thinks much of the above probably should have gone under the ---
indicating your knowledge of more places, but the decision to not change
them for the stated reasons. That commit message doesn't necessarily
describe the subsequent changes...
Perhaps a generic commit messages such as:
Change to using virConfGetValueString rather than open coding for
instances in which error messages are expected to be generated.
Okay!
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index fdca9845aa..c31ebfb270 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -145,31 +145,19 @@ 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;
> - }
> + rc = virConfGetValueString(conf, name, value);
I believe this should be:
if ((rc = virConfGetValueString(conf, name, value)) < 0)
return -1;
otherwise, we could get to the allowMissing w/ rc == -1 and return 0
even though previously for a VIR_STRDUP failure we'd return -1. We'd
also overwrite the no memory error from VIR_STRDUP failure.
Fixed locally, thanks!
> + if (rc == 1 && *value)
> + return 0;
>
> - 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;
> - }
> + if (allowMissing)
> + return 0;
>
> - return VIR_STRDUP(*value, val->str);
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("config value %s was missing"), name);
> + return -1;
> }
>
>
> @@ -193,7 +181,7 @@ xenConfigCopyStringOpt(virConfPtr conf, const char
*name, char **value)
> static int
> xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
> {
> - virConfValuePtr val;
> + char *string = NULL;
>
> if (!uuid || !name || !conf) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -201,7 +189,7 @@ xenConfigGetUUID(virConfPtr conf, const char *name,
unsigned char *uuid)
> return -1;
> }
>
> - if (!(val = virConfGetValue(conf, name))) {
> + if (virConfGetValueString(conf, name, &string) < 0) {
This is not the same check - < 0 is returned on errors (!STRING and
VIR_STRDUP failure).
I think you should use:
rc = virConfGetValueString(conf, name, &string);
and then continue from there with the rc and/or string checks.
Fixed locally, thanks!
> if (virUUIDGenerate(uuid) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("Failed to generate
UUID"));
> @@ -211,24 +199,20 @@ xenConfigGetUUID(virConfPtr conf, const char
*name, unsigned char *uuid)
> }
> }
>
> - if (val->type != VIR_CONF_STRING) {
> - virReportError(VIR_ERR_CONF_SYNTAX,
> - _("config value %s not a string"), name);
> - return -1;
> - }
> -
> - if (!val->str) {
> + if (!string) {
I think this would be if virConfGetValueString returns 1, but !string
because VIR_STRDUP(*value, NULL) would return NULL in *value.
Fixed locally, thanks!
> virReportError(VIR_ERR_CONF_SYNTAX,
> _("%s can't be empty"), name);
> return -1;
> }
>
> - if (virUUIDParse(val->str, uuid) < 0) {
> + if (virUUIDParse(string, uuid) < 0) {
> virReportError(VIR_ERR_CONF_SYNTAX,
> - _("%s not parseable"), val->str);
> + _("%s not parseable"), string);
> + VIR_FREE(string);
> return -1;
> }
>
> + VIR_FREE(string);
> return 0;
> }
>
> @@ -242,23 +226,16 @@ 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) {
more of the same here as from previous < 0 is error, while ret == 0
would be this case
Fixed locally, thanks!
> *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)
> + if (!string)
and if rc == 1 and !string
> *value = def;
> else
> - *value = val->str;
> + *value = string;
And the problem here is that @string would be leaked since it's a
VIR_STRDUP of val->str, the @*def is a const char of something.
Here we can just memcpy() the string content and VIR_FREE() the string.
Would you agree with this approach or do you have something else in mind?
> return 0;
> }
>
> @@ -469,27 +446,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)
Again, not the same checks... Not sure this particular one can be used.
The < 0 is an error path...
I understand your point that we're not doing the same check, but I do
believe that the "<= 0" check here is okay if we want to keep the same
behaviour.
The main issue I see here is that if the list->type is from the wrong type,
virConfGetValueStringList() would end up returning -1 (considering it goes
to the default branch, for instance) and the old code would return 0 for
that case.
So, < 0 is an error path, but I'm not sure how we can differentiate between
the errors we want to return 0 and the ones we want to return -1.
I'm imagining many of the rest are similar. Probably should have split
things up into single patches for each method (as painful as it is) so
that it's easier to review and easier to pick and choose what doesn't
work and what does.
BTW: I would do all the virConfGetValueString's first... Then tackle the
virConfGetValueStringList's - since it doesn't cause one to context
switch "too much" between two different API's.
Sure, I'll submit a new version soon (after having your feedback in the
question above).
John
> 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 +583,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 +643,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) < 0) {
> + if (virStrcpyStatic(vfb, entry) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("VFB %s too big for destination"),
> - list->list->str);
> + entry);
> goto cleanup;
> }
>
> @@ -745,21 +733,23 @@ 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)
> @@ -779,8 +769,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,18 +779,12 @@ 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;
> @@ -808,8 +792,6 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> chr->target.port = portnum;
> if (VIR_APPEND_ELEMENT(def->serials, def->nserials,
chr) < 0)
> goto cleanup;
> -
> - value = value->next;
> }
> } else {
> /* If domain is not using multiple serial ports we parse
data old way */
> @@ -825,6 +807,7 @@ xenParseCharDev(virConfPtr conf, virDomainDefPtr
def, const char *nativeFormat)
> chr->target.port = 0;
> def->serials[0] = chr;
> def->nserials++;
> + chr = NULL;
> }
> }
> } else {
> @@ -838,11 +821,12 @@ 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;
> }
>
>
> @@ -1031,29 +1015,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;
> }
>
>
>
7:51 a.m.
New subject: [libvirt] [libvirt PATCH v5 1/1] xen_common: Convert to typesafe virConf acessors
[...]
> *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)
> + if (!string)
and if rc == 1 and !string
> *value = def;
> else
> - *value = val->str;
> + *value = string;
And the problem here is that @string would be leaked since it's a
VIR_STRDUP of val->str, the @*def is a const char of something.
Here we can just memcpy() the string content and VIR_FREE() the string.
Would you agree with this approach or do you have something else in mind?
memcpy @string into what?
Let's look at a caller:
static int
xenParseEventsActions(virConfPtr conf, virDomainDefPtr def)
{
const char *str = NULL;
if (xenConfigGetString(conf, "on_poweroff", &str, "destroy")
< 0)
return -1;
So **value is essentially a "const char *str = NULL; with no memory or
stack space to memcpy into. I think we could have a "size" problem with
memcpy into str since it'd need to be variable based upon caller and not
necessarily the same size as we found in the virConfGetValueString.
I don't think this one is going to be "clean" - either you have to keep
using the existing mechanism or have all the callers be able to VIR_FREE
the returned string with of course a VIR_STRDUP(*value, def).
> return 0;
> }
>
> @@ -469,27 +446,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)
Again, not the same checks... Not sure this particular one can be used.
The < 0 is an error path...
I understand your point that we're not doing the same check, but I do
believe that the "<= 0" check here is okay if we want to keep the same
behaviour.
The main issue I see here is that if the list->type is from the wrong
type, virConfGetValueStringList() would end up returning -1 (considering
it goes to the default branch, for instance) and the old code would
return 0 for that case.
So, < 0 is an error path, but I'm not sure how we can differentiate
between the errors we want to return 0 and the ones we want to return -1.
I think that became the same conclusion I began reaching, but I was also
trying to keep the non *List context present while reviewing. Once I
came across *List changes and saw some differences, I decided perhaps
it'd be best to punt, ask for the split, and take a fresh look when
round 2 showed up.
Perhaps it's just a process of explaining in the commit or after the ---
why the checks are the same.
John
I'm imagining many of the rest are similar. Probably should have split
things up into single patches for each method (as painful as it is) so
that it's easier to review and easier to pick and choose what doesn't
work and what does.
BTW: I would do all the virConfGetValueString's first... Then tackle the
virConfGetValueStringList's - since it doesn't cause one to context
switch "too much" between two different API's.
Sure, I'll submit a new version soon (after having your feedback in the
question above).
[...]
2294
days inactive
2301
days old
4 comments
2 participants
participants (2)
-
Fabiano Fidêncio -
John Ferlan