11:32 a.m.
Some patches of the previous version are pushed already.
However, there are some new too.
Michal Privoznik (3):
conf: Always format seclabel's model
virSecurityLabelDefParseXML: Rework
virSecurityLabelDef: use enum type for @type
src/conf/domain_conf.c | 129 +++++++++++----------
src/security/security_dac.c | 2 +-
src/util/virseclabel.h | 2 +-
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 +++++
tests/qemuxml2xmltest.c | 1 +
5 files changed, 101 insertions(+), 61 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
--
1.8.5.5
11:32 a.m.
New subject: [libvirt] [PATCH v3 1/3] conf: Always format seclabel's model
https://bugzilla.redhat.com/show_bug.cgi?id=1113860
We've always done that. Well, until 990e46c45. Point is, if we don't
format model, we may lose a domain on libvirtd restart. If the
seclabel is implicit however, we should skip it's formatting.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 34 +++++++++++++++-------
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 ++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
3 files changed, 52 insertions(+), 11 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b91ccf7..7b90903 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4614,8 +4614,23 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* For the model 'none' none of the following labels is going to be
* present. Hence, return now. */
- if (STREQ_NULLABLE(def->model, "none"))
+ if (STREQ_NULLABLE(def->model, "none")) {
+ if (flags & VIR_DOMAIN_XML_INACTIVE) {
+ /* Fix older configurations */
+ def->type = VIR_DOMAIN_SECLABEL_NONE;
+ def->relabel = false;
+ } else {
+ if (def->type != VIR_DOMAIN_SECLABEL_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported type='%s' to model
'none'"),
+ virDomainSeclabelTypeToString(def->type));
+ goto error;
+ }
+ /* combination of relabel='yes' and type='static'
+ * is checked a few lines above. */
+ }
return def;
+ }
/* Only parse label, if using static labels, or
* if the 'live' VM XML is requested
@@ -14690,8 +14705,7 @@ virDomainEventActionDefFormat(virBufferPtr buf,
static void
virSecurityLabelDefFormat(virBufferPtr buf,
- virSecurityLabelDefPtr def,
- unsigned flags)
+ virSecurityLabelDefPtr def)
{
const char *sectype = virDomainSeclabelTypeToString(def->type);
@@ -14701,19 +14715,17 @@ virSecurityLabelDefFormat(virBufferPtr buf,
if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT)
return;
- /* To avoid backward compatibility issues, suppress DAC labels that are
- * automatically generated.
+ /* To avoid backward compatibility issues, suppress DAC and 'none' labels
+ * that are automatically generated.
*/
- if (STREQ_NULLABLE(def->model, "dac") && def->implicit)
+ if ((STREQ_NULLABLE(def->model, "dac") ||
+ STREQ_NULLABLE(def->model, "none")) && def->implicit)
return;
virBufferAsprintf(buf, "<seclabel type='%s'",
sectype);
- /* When generating state XML do include the model */
- if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS ||
- STRNEQ_NULLABLE(def->model, "none"))
- virBufferEscapeString(buf, " model='%s'", def->model);
+ virBufferEscapeString(buf, " model='%s'", def->model);
if (def->type == VIR_DOMAIN_SECLABEL_NONE) {
virBufferAddLit(buf, "/>\n");
@@ -17923,7 +17935,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferAddLit(buf, "</devices>\n");
for (n = 0; n < def->nseclabels; n++)
- virSecurityLabelDefFormat(buf, def->seclabels[n], flags);
+ virSecurityLabelDefFormat(buf, def->seclabels[n]);
if (def->namespaceData && def->ns.format) {
if ((def->ns.format)(buf, def->namespaceData) < 0)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
new file mode 100644
index 0000000..cec59f8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
@@ -0,0 +1,28 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <memballoon model='virtio'/>
+ </devices>
+ <seclabel type='none' model='none'/>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 26e3cad..9f919de 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -307,6 +307,7 @@ mymain(void)
DO_TEST_FULL("seclabel-static-labelskip", false, WHEN_ACTIVE);
DO_TEST("seclabel-none");
DO_TEST("seclabel-dac-none");
+ DO_TEST("seclabel-dynamic-none");
DO_TEST("numad-static-vcpu-no-numatune");
DO_TEST("disk-scsi-lun-passthrough-sgio");
--
1.8.5.5
6:55 p.m.
New subject: [libvirt] [PATCH v3 1/3] conf: Always format seclabel's model
On 07/11/2014 03:32 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1113860
We've always done that. Well, until 990e46c45. Point is, if we don't
format model, we may lose a domain on libvirtd restart. If the
seclabel is implicit however, we should skip it's formatting.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 34 +++++++++++++++-------
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 ++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
3 files changed, 52 insertions(+), 11 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:32 a.m.
New subject: [libvirt] [PATCH 2/3] virSecurityLabelDefParseXML: Rework
Instead of allocating the virSecurityLabelDef structure ourselves, we
can utilize virSecurityLabelDefNew which even sets the default values
for us.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 103 ++++++++++++++++++++++++-------------------------
1 file changed, 50 insertions(+), 53 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7b90903..de60cd2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4553,91 +4553,87 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
unsigned int flags)
{
char *p;
- virSecurityLabelDefPtr def = NULL;
+ virSecurityLabelDefPtr seclabel = NULL;
- if (VIR_ALLOC(def) < 0)
+ p = virXPathStringLimit("string(./@model)",
+ VIR_SECURITY_MODEL_BUFLEN - 1, ctxt);
+
+ if (!(seclabel = virSecurityLabelDefNew(p)))
goto error;
+ /* set seclabelault value */
+ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
+
p = virXPathStringLimit("string(./@type)",
- VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
- if (p == NULL) {
- def->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
- } else {
- def->type = virDomainSeclabelTypeFromString(p);
- VIR_FREE(p);
- if (def->type <= 0) {
+ VIR_SECURITY_LABEL_BUFLEN - 1, ctxt);
+ if (p) {
+ seclabel->type = virDomainSeclabelTypeFromString(p);
+ if (seclabel->type <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("invalid security type"));
+ _("invalid security type '%s'"), p);
goto error;
}
}
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
+ seclabel->type == VIR_DOMAIN_SECLABEL_NONE)
+ seclabel->relabel = false;
+
+ VIR_FREE(p);
p = virXPathStringLimit("string(./@relabel)",
VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
- if (p != NULL) {
+ if (p) {
if (STREQ(p, "yes")) {
- def->relabel = true;
+ seclabel->relabel = true;
} else if (STREQ(p, "no")) {
- def->relabel = false;
+ seclabel->relabel = false;
} else {
virReportError(VIR_ERR_XML_ERROR,
_("invalid security relabel value %s"), p);
- VIR_FREE(p);
goto error;
}
- VIR_FREE(p);
- if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
- !def->relabel) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("dynamic label type must use resource
relabeling"));
- goto error;
- }
- if (def->type == VIR_DOMAIN_SECLABEL_NONE &&
- def->relabel) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("resource relabeling is not compatible
with 'none' label type"));
- goto error;
- }
- } else {
- if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
- def->type == VIR_DOMAIN_SECLABEL_NONE)
- def->relabel = false;
- else
- def->relabel = true;
}
- /* Always parse model */
- p = virXPathStringLimit("string(./@model)",
- VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
- def->model = p;
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
+ !seclabel->relabel) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("dynamic label type must use resource
relabeling"));
+ goto error;
+ }
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE &&
+ seclabel->relabel) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("resource relabeling is not compatible with
'none' label type"));
+ goto error;
+ }
/* For the model 'none' none of the following labels is going to be
* present. Hence, return now. */
- if (STREQ_NULLABLE(def->model, "none")) {
+ if (STREQ_NULLABLE(seclabel->model, "none")) {
if (flags & VIR_DOMAIN_XML_INACTIVE) {
/* Fix older configurations */
- def->type = VIR_DOMAIN_SECLABEL_NONE;
- def->relabel = false;
+ seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
+ seclabel->relabel = false;
} else {
- if (def->type != VIR_DOMAIN_SECLABEL_NONE) {
+ if (seclabel->type != VIR_DOMAIN_SECLABEL_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported type='%s' to model
'none'"),
- virDomainSeclabelTypeToString(def->type));
+ virDomainSeclabelTypeToString(seclabel->type));
goto error;
}
/* combination of relabel='yes' and type='static'
* is checked a few lines above. */
}
- return def;
+ return seclabel;
}
/* Only parse label, if using static labels, or
* if the 'live' VM XML is requested
*/
- if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
(!(flags & VIR_DOMAIN_XML_INACTIVE) &&
- def->type != VIR_DOMAIN_SECLABEL_NONE)) {
+ seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
p = virXPathStringLimit("string(./label[1])",
VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
if (p == NULL) {
@@ -4646,13 +4642,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
goto error;
}
- def->label = p;
+ seclabel->label = p;
}
/* Only parse imagelabel, if requested live XML with relabeling */
- if (def->relabel &&
+ if (seclabel->relabel &&
(!(flags & VIR_DOMAIN_XML_INACTIVE) &&
- def->type != VIR_DOMAIN_SECLABEL_NONE)) {
+ seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
p = virXPathStringLimit("string(./imagelabel[1])",
VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
if (p == NULL) {
@@ -4660,20 +4656,21 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
"%s", _("security imagelabel is
missing"));
goto error;
}
- def->imagelabel = p;
+ seclabel->imagelabel = p;
}
/* Only parse baselabel for dynamic label type */
- if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
p = virXPathStringLimit("string(./baselabel[1])",
VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
- def->baselabel = p;
+ seclabel->baselabel = p;
}
- return def;
+ return seclabel;
error:
- virSecurityLabelDefFree(def);
+ VIR_FREE(p);
+ virSecurityLabelDefFree(seclabel);
return NULL;
}
--
1.8.5.5
6:54 p.m.
New subject: [libvirt] [PATCH 2/3] virSecurityLabelDefParseXML: Rework
On 07/11/2014 03:32 AM, Michal Privoznik wrote:
Instead of allocating the virSecurityLabelDef structure ourselves,
we
can utilize virSecurityLabelDefNew which even sets the default values
for us.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 103 ++++++++++++++++++++++++-------------------------
1 file changed, 50 insertions(+), 53 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7b90903..de60cd2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4553,91 +4553,87 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
unsigned int flags)
{
char *p;
- virSecurityLabelDefPtr def = NULL;
+ virSecurityLabelDefPtr seclabel = NULL;
Looks like you did a search-and-replace s/def/seclabel/...
- if (VIR_ALLOC(def) < 0)
+ p = virXPathStringLimit("string(./@model)",
+ VIR_SECURITY_MODEL_BUFLEN - 1, ctxt);
+
+ if (!(seclabel = virSecurityLabelDefNew(p)))
goto error;
+ /* set seclabelault value */
...but didn't pay attention to what got replaced :)
- if (p != NULL) {
+ if (p) {
if (STREQ(p, "yes")) {
- def->relabel = true;
+ seclabel->relabel = true;
I'm guessing that this assignment is now redundant with the fact that it
already defaults to this value; but I'm okay leaving it to make the code
obvious.
ACK with the typo fix.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:32 a.m.
New subject: [libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type
There's this trend in libvirt of using enum types wherever possible.
Now that I'm at virSecurityLabelDef let's rework @type item of the
structure so we don't have to typecast it elsewhere.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++++--
src/security/security_dac.c | 2 +-
src/util/virseclabel.h | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index de60cd2..b3a0ec8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4567,12 +4567,14 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
p = virXPathStringLimit("string(./@type)",
VIR_SECURITY_LABEL_BUFLEN - 1, ctxt);
if (p) {
- seclabel->type = virDomainSeclabelTypeFromString(p);
- if (seclabel->type <= 0) {
+ int type; /* virDomainSeclabelType */
+ type = virDomainSeclabelTypeFromString(p);
+ if (type <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("invalid security type '%s'"), p);
goto error;
}
+ seclabel->type = type;
}
if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 4d2a9d6..b4bfc57 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1125,7 +1125,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr,
return rc;
}
- switch ((virDomainSeclabelType) seclabel->type) {
+ switch (seclabel->type) {
case VIR_DOMAIN_SECLABEL_STATIC:
if (seclabel->label == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h
index 94c4dfc..abf9510 100644
--- a/src/util/virseclabel.h
+++ b/src/util/virseclabel.h
@@ -39,7 +39,7 @@ struct _virSecurityLabelDef {
char *label; /* security label string */
char *imagelabel; /* security image label string */
char *baselabel; /* base name of label string */
- int type; /* virDomainSeclabelType */
+ virDomainSeclabelType type; /* seclabel @type */
bool relabel; /* true (default) for allowing relabels */
bool implicit; /* true if seclabel is auto-added */
};
--
1.8.5.5
6:59 p.m.
New subject: [libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type
On 07/11/2014 03:32 AM, Michal Privoznik wrote:
There's this trend in libvirt of using enum types wherever
possible.
Now that I'm at virSecurityLabelDef let's rework @type item of the
structure so we don't have to typecast it elsewhere.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++++--
src/security/security_dac.c | 2 +-
src/util/virseclabel.h | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
I'm not quite as sure about this one. This solves the issue of how to
detect errors when parsing strings to enum, but required the use of an
intermediate variable which in turn made the patch a net gain in lines
of code. If someone forgets to use the intermediate variable for
parsing, this backfires. On the other hand, parsing string to enum
should be done in just one location, and that's the location touched by
this patch. I'm 50-50 on whether to take this, so I'd like someone else
to chime in with an opinion.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:09 a.m.
New subject: [libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type
On 11.07.2014 18:59, Eric Blake wrote:
On 07/11/2014 03:32 AM, Michal Privoznik wrote:
> There's this trend in libvirt of using enum types wherever possible.
> Now that I'm at virSecurityLabelDef let's rework @type item of the
> structure so we don't have to typecast it elsewhere.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/domain_conf.c | 6 ++++--
> src/security/security_dac.c | 2 +-
> src/util/virseclabel.h | 2 +-
> 3 files changed, 6 insertions(+), 4 deletions(-)
I'm not quite as sure about this one. This solves the issue of how to
detect errors when parsing strings to enum, but required the use of an
intermediate variable which in turn made the patch a net gain in lines
of code. If someone forgets to use the intermediate variable for
parsing, this backfires. On the other hand, parsing string to enum
should be done in just one location, and that's the location touched by
this patch. I'm 50-50 on whether to take this, so I'd like someone else
to chime in with an opinion.
I hear you. This patch is just a refactor. It does not add anything useful nor solve any
issue. It's okay if dropped. But the more I think about our vir*TypeFromString() the
more I feel we should do something about it. How about making it follow our typical
function return pattern:
int func() {
virMyFavourite x;
const char *string;
if (virMyFavouriteTypeFromString(string, &x) < 0) {
virReportError("unknown value: %s", string);
goto error;
}
That is, we need this diff:
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 95d1ff9..40075e9 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -407,15 +407,20 @@ virParseVersionString(const char *str, unsigned long *version,
int virEnumFromString(const char *const*types,
unsigned int ntypes,
- const char *type)
+ const char *type,
+ int *val)
{
size_t i;
if (!type)
return -1;
- for (i = 0; i < ntypes; i++)
- if (STREQ(types[i], type))
- return i;
+ for (i = 0; i < ntypes; i++) {
+ if (STREQ(types[i], type)) {
+ if (val)
+ *val = i;
+ return 0;
+ }
+ }
return -1;
}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 2bb74e2..670b2e1 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -73,7 +73,8 @@ char *virIndexToDiskName(int idx, const char *prefix);
int virEnumFromString(const char *const*types,
unsigned int ntypes,
- const char *type);
+ const char *type,
+ int *val);
const char *virEnumToString(const char *const*types,
unsigned int ntypes,
@@ -87,10 +88,10 @@ const char *virEnumToString(const char *const*types,
ARRAY_CARDINALITY(name ## TypeList), \
type); \
} \
- int name ## TypeFromString(const char *type) { \
+ int name ## TypeFromString(const char *type, name *val) { \
return virEnumFromString(name ## TypeList, \
ARRAY_CARDINALITY(name ## TypeList), \
- type); \
+ type, (int *) val); \
}
# define VIR_ENUM_DECL(name) \
And then a tons of follow up patches. Or even make virEnumString() report the error (that
could save a lot of virReportError() calls).
Michal
6:49 p.m.
New subject: [libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type
On 07/14/2014 03:09 AM, Michal Privoznik wrote:
On 11.07.2014 18:59, Eric Blake wrote:
> On 07/11/2014 03:32 AM, Michal Privoznik wrote:
>> There's this trend in libvirt of using enum types wherever possible.
>> Now that I'm at virSecurityLabelDef let's rework @type item of the
>> structure so we don't have to typecast it elsewhere.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 6 ++++--
>> src/security/security_dac.c | 2 +-
>> src/util/virseclabel.h | 2 +-
>> 3 files changed, 6 insertions(+), 4 deletions(-)
>
> I'm not quite as sure about this one. This solves the issue of how to
> detect errors when parsing strings to enum, but required the use of an
> intermediate variable which in turn made the patch a net gain in lines
> of code. If someone forgets to use the intermediate variable for
> parsing, this backfires. On the other hand, parsing string to enum
> should be done in just one location, and that's the location touched by
> this patch. I'm 50-50 on whether to take this, so I'd like someone else
> to chime in with an opinion.
I hear you. This patch is just a refactor. It does not add anything useful nor solve any
issue. It's okay if dropped. But the more I think about our vir*TypeFromString() the
more I feel we should do something about it. How about making it follow our typical
function return pattern:
int func() {
virMyFavourite x;
const char *string;
if (virMyFavouriteTypeFromString(string, &x) < 0) {
virReportError("unknown value: %s", string);
goto error;
}
Yeah, making the API failsafe by separating error return from value
setting is what we've done elsewhere (such as virStrToLong_*). It's a
bigger, more invasive change, but may be worth it. And if we do it, we
can then start sticking enum values inside non-public structs.
That is, we need this diff:
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 95d1ff9..40075e9 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -407,15 +407,20 @@ virParseVersionString(const char *str, unsigned long *version,
int virEnumFromString(const char *const*types,
unsigned int ntypes,
- const char *type)
+ const char *type,
+ int *val)
}
\
- int name ## TypeFromString(const char *type) { \
+ int name ## TypeFromString(const char *type, name *val) { \
return virEnumFromString(name ## TypeList, \
ARRAY_CARDINALITY(name ## TypeList), \
- type); \
+ type, (int *) val); \
}
# define VIR_ENUM_DECL(name) \
And then a tons of follow up patches. Or even make virEnumString() report the error (that
could save a lot of virReportError() calls).
Or even make the error reporting an optional bool parameter.
It's also a huge patch; I wonder if we can come up with an alternative
name (VIR_ENUM_DECL2 expands to name ## FromString, leaving
VIR_ENUM_DECL expanding to name ## TypeFromString - also nice that it's
a shorter name since it's usage will be longer), where we can then focus
on just a few conversions per patch, until all patches are using the new
name and finally delete the old name, rather than redefining
VIR_ENUM_DECL up front and having to do all the refactoring in a single
patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3722
days inactive
3725
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Michal Privoznik