10:29 a.m.
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
10:29 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Verify metadata type right away
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
10:50 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Verify metadata type right away
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
10:55 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Verify metadata type right away
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 :|
10:29 a.m.
New subject: [libvirt] [PATCH 2/3] util: XML: Avoid forward function declaration
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
10:51 a.m.
New subject: [libvirt] [PATCH 2/3] util: XML: Avoid forward function declaration
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
3:12 a.m.
New subject: [libvirt] [PATCH 2/3] util: XML: Avoid forward function declaration
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
10:29 a.m.
New subject: [libvirt] [PATCH 3/3] lib: Check conditions for VIR_DOMAIN_BLOCK_REBASE_RELATIVE right away
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
10:53 a.m.
New subject: [libvirt] [PATCH 3/3] lib: Check conditions for VIR_DOMAIN_BLOCK_REBASE_RELATIVE right away
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
11 a.m.
New subject: [libvirt] [PATCH 3/3] lib: Check conditions for VIR_DOMAIN_BLOCK_REBASE_RELATIVE right away
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
3825
days inactive
3826
days old
9 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa