[libvirt] [PATCH 0/3] Few cleanups pointed out in reviews
Peter Krempa (3): conf: Verify metadata type right away util: XML: Avoid forward function declaration lib: Check conditions for VIR_DOMAIN_BLOCK_REBASE_RELATIVE right away src/conf/domain_conf.c | 10 ++-------- src/libvirt.c | 17 +++++++++++++---- src/qemu/qemu_driver.c | 7 ------- src/util/virxml.c | 5 ----- 4 files changed, 15 insertions(+), 24 deletions(-) -- 2.0.0
Verify the desired metadata type in the libvirt.c dispatcher rather than in the bottom level executor function. --- src/conf/domain_conf.c | 10 ++-------- src/libvirt.c | 10 ++++++---- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70f1103..a2b0f23 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19601,10 +19601,7 @@ virDomainObjGetMetadata(virDomainObjPtr vm, goto cleanup; break; - default: - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("unknown metadata type")); - goto cleanup; + case VIR_DOMAIN_METADATA_LAST: break; } @@ -19683,10 +19680,7 @@ virDomainDefSetMetadata(virDomainDefPtr def, } break; - default: - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("unknown metadata type")); - goto cleanup; + case VIR_DOMAIN_METADATA_LAST: break; } diff --git a/src/libvirt.c b/src/libvirt.c index 316fdf0..6bf260a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -10178,8 +10178,9 @@ virDomainSetMetadata(virDomainPtr domain, virCheckNonNullArgGoto(key, error); break; default: - /* For future expansion */ - break; + virReportInvalidArg(type, + _("unsupported metadata type '%d'"), type); + goto error; } if (conn->driver->domainSetMetadata) { @@ -10255,8 +10256,9 @@ virDomainGetMetadata(virDomainPtr domain, virCheckNonNullArgGoto(uri, error); break; default: - /* For future expansion */ - break; + virReportInvalidArg(type, + _("unsupported metadata type '%d'"), type); + goto error; } conn = domain->conn; -- 2.0.0
On 07/08/2014 09:29 AM, Peter Krempa wrote:
Verify the desired metadata type in the libvirt.c dispatcher rather than in the bottom level executor function. --- src/conf/domain_conf.c | 10 ++-------- src/libvirt.c | 10 ++++++---- 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 70f1103..a2b0f23 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19601,10 +19601,7 @@ virDomainObjGetMetadata(virDomainObjPtr vm, goto cleanup; break;
- default: - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("unknown metadata type")); - goto cleanup; + case VIR_DOMAIN_METADATA_LAST: break;
This part is nice for one reason: it cleans up a dead code issue (the break was previously unreachable). But I'm not sure if it is right to hoist the validation into the caller...
+++ b/src/libvirt.c @@ -10178,8 +10178,9 @@ virDomainSetMetadata(virDomainPtr domain, virCheckNonNullArgGoto(key, error); break; default: - /* For future expansion */ - break; + virReportInvalidArg(type, + _("unsupported metadata type '%d'"), type); + goto error; }
...because doing this makes it impossible for an older client to manually set a newer key of a newer server. That is, we intentionally don't reject unknown flags in libvirt.c, so that: func(0x4) -> old libvirt.so that doesn't know flag 4 -> new libvirtd that does works, rather than dying client-side. On the same vein, virTypedParameterValidateSet rejects strings when the server is too old, but does NOT reject unknown parameter type extensions (which would allow an extension that does not affect wire protocol to go through an old client to a new server). I'd like Dan to chime in on this one. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Jul 08, 2014 at 09:50:50AM -0600, Eric Blake wrote:
On 07/08/2014 09:29 AM, Peter Krempa wrote:
+++ b/src/libvirt.c @@ -10178,8 +10178,9 @@ virDomainSetMetadata(virDomainPtr domain, virCheckNonNullArgGoto(key, error); break; default: - /* For future expansion */ - break; + virReportInvalidArg(type, + _("unsupported metadata type '%d'"), type); + goto error; }
...because doing this makes it impossible for an older client to manually set a newer key of a newer server. That is, we intentionally don't reject unknown flags in libvirt.c, so that:
func(0x4) -> old libvirt.so that doesn't know flag 4 -> new libvirtd that does
works, rather than dying client-side. On the same vein, virTypedParameterValidateSet rejects strings when the server is too old, but does NOT reject unknown parameter type extensions (which would allow an extension that does not affect wire protocol to go through an old client to a new server).
I'd like Dan to chime in on this one.
Yes, this is the same scenario as verifying 'unsigned int flags'. We explicitly only verify in the driver impl, not libvirt.c, so that old client library can still talk to new server if desired without getting a bogus error. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Recursive functions apparently don't need them, but I originally thought they do. --- src/util/virxml.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/util/virxml.c b/src/util/virxml.c index 34af64a..cc4a85c 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -935,11 +935,6 @@ typedef int (*virXMLForeachCallback)(xmlNodePtr node, static int virXMLForeachNode(xmlNodePtr root, virXMLForeachCallback cb, - void *opaque); - -static int -virXMLForeachNode(xmlNodePtr root, - virXMLForeachCallback cb, void *opaque) { xmlNodePtr next; -- 2.0.0
On 07/08/2014 09:29 AM, Peter Krempa wrote:
Recursive functions apparently don't need them, but I originally thought they do. --- src/util/virxml.c | 5 ----- 1 file changed, 5 deletions(-)
ACK. When it comes to static functions, it's only mutually recursive function pairs that need a forward declaration of at least one of the two functions. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/08/14 17:51, Eric Blake wrote:
On 07/08/2014 09:29 AM, Peter Krempa wrote:
Recursive functions apparently don't need them, but I originally thought they do. --- src/util/virxml.c | 5 ----- 1 file changed, 5 deletions(-)
ACK. When it comes to static functions, it's only mutually recursive function pairs that need a forward declaration of at least one of the two functions.
Pushed; Thanks. Peter
VIR_DOMAIN_BLOCK_REBASE_RELATIVE works only when @base is specified. Check it right in libvirt.c as it's not expected to change across hypervisors. --- src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 6bf260a..edf2f8b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19817,6 +19817,13 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, goto error; } + if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportInvalidArg(base, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid " + "only with non-null base")); + goto error; + } + if (conn->driver->domainBlockRebase) { int ret; ret = conn->driver->domainBlockRebase(dom, disk, base, bandwidth, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fd68324..b077950 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15055,13 +15055,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; } - if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid only " - "with non-null base")); - goto cleanup; - } - priv = vm->privateData; if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { async = true; -- 2.0.0
On 07/08/2014 09:29 AM, Peter Krempa wrote:
VIR_DOMAIN_BLOCK_REBASE_RELATIVE works only when @base is specified. Check it right in libvirt.c as it's not expected to change across hypervisors. --- src/libvirt.c | 7 +++++++ src/qemu/qemu_driver.c | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 6bf260a..edf2f8b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19817,6 +19817,13 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, goto error; }
+ if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportInvalidArg(base, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid " + "only with non-null base")); + goto error; + }
I'd shorten this to: if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) virCheckNonNullArgGoto(params, error); for consistency with other validity checks in this file. I'd also like to see this restriction mentioned in the libvirt.c docs. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/08/2014 09:53 AM, Eric Blake wrote:
+ if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { + virReportInvalidArg(base, "%s", + _("flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE is valid " + "only with non-null base")); + goto error; + }
I'd shorten this to:
if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) virCheckNonNullArgGoto(params, error);
too much copy-and-paste; I meant: virCheckNonNullArgGoto(base, error);
for consistency with other validity checks in this file.
I'd also like to see this restriction mentioned in the libvirt.c docs.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa