[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 <ajia@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 @@ -14444,7 +14444,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
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 <ajia@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 @@ -14444,7 +14444,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
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 Jia<ajia@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 @@ -14444,7 +14444,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) xxxx".
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
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 Jia<ajia@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 @@ -14444,7 +14444,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) xxxx".
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
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 Jia<ajia@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 @@ -14444,7 +14444,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) xxxx".
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
On 08/27/2013 05:54 PM, Peter Krempa wrote:
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 Jia<ajia@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 @@ -14444,7 +14444,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) xxxx". 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; }
Exactly, but I think it's only a temporary solution, the Michal's suggestion is more reasonable, and your solution looks like more intelligent :)
Peter
Michal
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) xxxx".
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
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) xxxx". 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.
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 <ajia@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 @@ -14444,7 +14444,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
participants (4)
-
Alex Jia -
Eric Blake -
Michal Privoznik -
Peter Krempa