[libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by qemuDomainBlockCommit(), and then will hit error unsupported flags (0x2) in function qemuDomainBlockCommit if users run 'virsh blockcommit' with '--delete' option. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475 Signed-off-by: Alex Jia a...@redhat.com --- src/qemu/qemu_driver.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed29373..8863124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, const char *base_canon = NULL; bool clean_access = false; -virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); +virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; -- 1.7.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/13 09:53, Alex Jia wrote: The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by qemuDomainBlockCommit(), and then will hit error unsupported flags (0x2) in function qemuDomainBlockCommit if users run 'virsh blockcommit' with '--delete' option. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475 Signed-off-by: Alex Jia a...@redhat.com --- src/qemu/qemu_driver.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed29373..8863124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, const char *base_canon = NULL; bool clean_access = false; -virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); +virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; The code doesn't seem to support the BLOCK_COMMIT_DELETE flag. It was probably just added as a future expansion. Eric, could you clarify this please? NACK to this patch: qemuDomainBlockCommit doesn't mention VIR_DOMAIN_BLOCK_COMMIT_DELETE anywhere, nor it uses the flags argument to pass down. Peter signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/2013 04:47 PM, Peter Krempa wrote: On 08/27/13 09:53, Alex Jia wrote: The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by qemuDomainBlockCommit(), and then will hit error unsupported flags (0x2) in function qemuDomainBlockCommit if users run 'virsh blockcommit' with '--delete' option. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475 Signed-off-by: Alex Jiaa...@redhat.com --- src/qemu/qemu_driver.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed29373..8863124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, const char *base_canon = NULL; bool clean_access = false; -virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); +virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; The code doesn't seem to support the BLOCK_COMMIT_DELETE flag. It was Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag now, maybe, only need to raise a friendly error message in here instead of unsupported flags (0x2) . probably just added as a future expansion. Eric, could you clarify this please? NACK to this patch: qemuDomainBlockCommit doesn't mention VIR_DOMAIN_BLOCK_COMMIT_DELETE anywhere, nor it uses the flags argument to pass down. Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 27.08.2013 10:58, Alex Jia wrote: On 08/27/2013 04:47 PM, Peter Krempa wrote: On 08/27/13 09:53, Alex Jia wrote: The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by qemuDomainBlockCommit(), and then will hit error unsupported flags (0x2) in function qemuDomainBlockCommit if users run 'virsh blockcommit' with '--delete' option. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475 Signed-off-by: Alex Jiaa...@redhat.com --- src/qemu/qemu_driver.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed29373..8863124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, const char *base_canon = NULL; bool clean_access = false; -virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); +virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; The code doesn't seem to support the BLOCK_COMMIT_DELETE flag. It was Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag now, maybe, only need to raise a friendly error message in here instead of unsupported flags (0x2) . I agree that this error message is not user-friendly. Bare virsh users know nothing about our flags and their numerical expression. However, I don't think there is a way how to produce Unsupported flag VIR_DOMAIN_BLOCK_COMMIT_DELETE instead of Unsupported flag 0x2 since all we see in the qemuDomainBlockCommit() function is just number. I mean, mapping of flag onto numeric value is not one-to-one function (aka injective function). That is, a value 0x2 can express VIR_DOMAIN_BLOCK_COMMIT_DELETE, VIR_DOMAIN_START_AUTODESTROY, VIR_DUMP_DESTROY, etc. (git grep 1 1, include/). If we want to make it work, we have to introduce an injective function, e.g. virUnsupportedFlags(), which would accept a list (not an ORed value) of all flags that are not supported. Too much effort for not much outcome. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/13 11:37, Michal Privoznik wrote:
On 27.08.2013 10:58, Alex Jia wrote:
On 08/27/2013 04:47 PM, Peter Krempa wrote:
On 08/27/13 09:53, Alex Jia wrote:
The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by
qemuDomainBlockCommit(),
and then will hit error unsupported flags (0x2) in function
qemuDomainBlockCommit if users run 'virsh blockcommit' with
'--delete' option.
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475
Signed-off-by: Alex Jiaa...@redhat.com
---
src/qemu/qemu_driver.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ed29373..8863124 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const
char *path, const char *base,
const char *base_canon = NULL;
bool clean_access = false;
-virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
+virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
+ VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1);
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
The code doesn't seem to support the BLOCK_COMMIT_DELETE flag. It was
Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag
now, maybe, only need to raise a friendly error message in here instead
of unsupported flags (0x2) .
I agree that this error message is not user-friendly. Bare virsh users
know nothing about our flags and their numerical expression. However, I
don't think there is a way how to produce Unsupported flag
VIR_DOMAIN_BLOCK_COMMIT_DELETE instead of Unsupported flag 0x2 since
all we see in the qemuDomainBlockCommit() function is just number. I
mean, mapping of flag onto numeric value is not one-to-one function (aka
injective function). That is, a value 0x2 can express
VIR_DOMAIN_BLOCK_COMMIT_DELETE, VIR_DOMAIN_START_AUTODESTROY,
VIR_DUMP_DESTROY, etc. (git grep 1 1, include/).
If we want to make it work, we have to introduce an injective function,
e.g. virUnsupportedFlags(), which would accept a list (not an ORed
value) of all flags that are not supported. Too much effort for not much
outcome.
Additionally this would require updating the list of unsupported flags
at each function when adding a new flag to keep them in sync or add a
way to autogenerate the list automagically.
It would be nice to have this automatic, but I think that Alex was
suggesting something like
if (flags VIR_DOMAIN_BLOCK_COMMIT_DELETE) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, we don't support this yet);
goto cleanup;
}
Peter
Michal
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/2013 01:53 AM, Alex Jia wrote: The flag VIR_DOMAIN_BLOCK_COMMIT_DELETE is missed by qemuDomainBlockCommit(), and then will hit error unsupported flags (0x2) in function qemuDomainBlockCommit if users run 'virsh blockcommit' with '--delete' option. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1001475 Signed-off-by: Alex Jia a...@redhat.com --- src/qemu/qemu_driver.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ed29373..8863124 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1,7 +1,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base, const char *base_canon = NULL; bool clean_access = false; -virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1); +virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | + VIR_DOMAIN_BLOCK_COMMIT_DELETE, -1); NACK. You can't add the the flag to the list of supported flags unless you also add the support for that flag. This is a case of known future expansion (just as block-resize --shrink) - the API was planned for full capability, but no one has had time yet to implement all of the API for all of the hypervisor drivers. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/2013 03:37 AM, Michal Privoznik wrote: Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag now, maybe, only need to raise a friendly error message in here instead of unsupported flags (0x2) . I agree that this error message is not user-friendly. Bare virsh users know nothing about our flags and their numerical expression. However, I don't think there is a way how to produce Unsupported flag VIR_DOMAIN_BLOCK_COMMIT_DELETE instead of Unsupported flag 0x2 since all we see in the qemuDomainBlockCommit() function is just number. I mean, mapping of flag onto numeric value is not one-to-one function (aka injective function). That is, a value 0x2 can express VIR_DOMAIN_BLOCK_COMMIT_DELETE, VIR_DOMAIN_START_AUTODESTROY, VIR_DUMP_DESTROY, etc. (git grep 1 1, include/). If we want to make it work, we have to introduce an injective function, e.g. virUnsupportedFlags(), which would accept a list (not an ORed value) of all flags that are not supported. Too much effort for not much outcome. Agreed - it may not be the nicest of messages, but it is a CORRECT message, and one that will eventually go away when we actually implement things, so it isn't worth the churn of making a temporarily nicer message just to rip it back out later when things are properly implemented. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add missing VIR_DOMAIN_BLOCK_COMMIT_DELETE flags
On 08/27/2013 07:59 PM, Eric Blake wrote: On 08/27/2013 03:37 AM, Michal Privoznik wrote: Yes, the codes haven't any implementation for BLOCK_COMMIT_DELETE flag now, maybe, only need to raise a friendly error message in here instead of unsupported flags (0x2) . I agree that this error message is not user-friendly. Bare virsh users know nothing about our flags and their numerical expression. However, I don't think there is a way how to produce Unsupported flag VIR_DOMAIN_BLOCK_COMMIT_DELETE instead of Unsupported flag 0x2 since all we see in the qemuDomainBlockCommit() function is just number. I mean, mapping of flag onto numeric value is not one-to-one function (aka injective function). That is, a value 0x2 can express VIR_DOMAIN_BLOCK_COMMIT_DELETE, VIR_DOMAIN_START_AUTODESTROY, VIR_DUMP_DESTROY, etc. (git grep 1 1, include/). If we want to make it work, we have to introduce an injective function, e.g. virUnsupportedFlags(), which would accept a list (not an ORed value) of all flags that are not supported. Too much effort for not much outcome. Agreed - it may not be the nicest of messages, but it is a CORRECT message, and one that will eventually go away when we actually implement things, so it isn't worth the churn of making a temporarily nicer message just to rip it back out later when things are properly implemented. Agree, it isn't worth changing a temporarily nicer message now. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
