[libvirt] [PATCH] virsh: Use old API if remote libvirtd does not support new
Commit ffe28ab74b821c916ec4ba8efb5c992454e4bd24 introduced regression while comunicating with older libvirtd command 'domblkstat' used the new API and did not check for VIR_ERR_RPC error code signalling the remote server does not support this API and did not fall back to older API. Thereafter 'domblkstat' ended with "error: unknown procedure: 243". --- footnote: Will not apply along with: [libvirt] [PATCH v5] virsh: Add more human-friendly output of domblkstat command as that path does fix this bug. tools/virsh.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3b060bf..430168c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1090,7 +1090,8 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) * then. */ if (rc < 0) { - if (last_error->code != VIR_ERR_NO_SUPPORT) { + if (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_RPC) { virshReportError(ctl); goto cleanup; } else { -- 1.7.3.4
On 09/13/2011 09:16 AM, Peter Krempa wrote:
Commit ffe28ab74b821c916ec4ba8efb5c992454e4bd24 introduced regression while comunicating with older libvirtd command 'domblkstat' used the new API and did not check for VIR_ERR_RPC error code signalling the remote server does not support this API and did not fall back to older API. Thereafter 'domblkstat' ended with "error: unknown procedure: 243". ---
ACK.
footnote: Will not apply along with: [libvirt] [PATCH v5] virsh: Add more human-friendly output of domblkstat command as that path does fix this bug.
Apply this first, then rebase that patch into a v6. Then the human-friendly patch is separate, rather than pushing two fixes in one patch (that is, upstream should have two patches, while for back-porting purposes, it might make sense to backport just this).
tools/virsh.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 3b060bf..430168c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1090,7 +1090,8 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) * then. */ if (rc< 0) { - if (last_error->code != VIR_ERR_NO_SUPPORT) { + if (last_error->code != VIR_ERR_NO_SUPPORT&& + last_error->code != VIR_ERR_RPC) {
Hmm, I wonder if any of my recent snapshot patches also need to check for VIR_ERR_RPC (for example, see cmdUndefine, which checks for VIR_ERR_NO_SUPPORT but not VIR_ERR_RPC). Yep, I'm starting to convince myself that they do - if you have a new client talking to a pre-0.8.0 server, then virDomainSnapshotNum would fail with VIR_ERR_RPC rather than VIR_ERR_NO_SUPPORT. Or, maybe this means that we have a bug in our RPC code - when the local side gets VIR_ERR_RPC from the remote side due to unknown rpc number, should the local side be translating that into VIR_ERR_NO_SUPPORT instead of passing it back to the user as VIR_ERR_RPC? But only for unknown rpc numbers; for other rpc errors, VIR_ERR_RPC still makes sense. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/13/2011 04:50 PM, Eric Blake wrote:
On 09/13/2011 09:16 AM, Peter Krempa wrote:
Commit ffe28ab74b821c916ec4ba8efb5c992454e4bd24 introduced regression while comunicating with older libvirtd command 'domblkstat' used the new
s/comunicating/communicationg/
API and did not check for VIR_ERR_RPC error code signalling the remote server does not support this API and did not fall back to older API. Thereafter 'domblkstat' ended with "error: unknown procedure: 243". ---
ACK.
footnote: Will not apply along with: [libvirt] [PATCH v5] virsh: Add more human-friendly output of domblkstat command as that path does fix this bug.
Apply this first, then rebase that patch into a v6.
This patch is now pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/13/2011 04:50 PM, Eric Blake wrote:
On 09/13/2011 09:16 AM, Peter Krempa wrote:
Commit ffe28ab74b821c916ec4ba8efb5c992454e4bd24 introduced regression while comunicating with older libvirtd command 'domblkstat' used the new API and did not check for VIR_ERR_RPC error code signalling the remote server does not support this API and did not fall back to older API. Thereafter 'domblkstat' ended with "error: unknown procedure: 243". ---
Or, maybe this means that we have a bug in our RPC code - when the local side gets VIR_ERR_RPC from the remote side due to unknown rpc number, should the local side be translating that into VIR_ERR_NO_SUPPORT instead of passing it back to the user as VIR_ERR_RPC? But only for unknown rpc numbers; for other rpc errors, VIR_ERR_RPC still makes sense.
I've confirmed that we have a regression :(. Talking to an 0.8.8 server gives VIR_ERR_NO_SUPPORT when issuing the "unknown procedure: 243" message; it is only 0.9.3 and newer where we switched to the new rpc handling where we get VIR_ERR_RPC instead. I'm looking into how best to repair this regression, so that clients need not check for VIR_ERR_RPC everywhere. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/14/2011 11:42 AM, Eric Blake wrote:
I've confirmed that we have a regression :(. Talking to an 0.8.8 server gives VIR_ERR_NO_SUPPORT when issuing the "unknown procedure: 243" message; it is only 0.9.3 and newer where we switched to the new rpc handling where we get VIR_ERR_RPC instead. I'm looking into how best to repair this regression, so that clients need not check for VIR_ERR_RPC everywhere.
More info from gdb. In both cases, I tested virsh client from latest libvirt.git, and had a breakpoint at virNetClientProgramDispatchError. With 0.8.8 server, err looks like: (gdb) p err $5 = {code = 39, domain = 13, message = 0x657cb0, level = 2, dom = 0x0, str1 = 0x657de0, str2 = 0x0, str3 = 0x0, int1 = 0, int2 = 0, net = 0x0} but with 0.9.4, it looks like: (gdb) p err $2 = {code = 39, domain = 7, message = 0x657d50, level = 2, dom = 0x0, str1 = 0x657e70, str2 = 0x657f90, str3 = 0x0, int1 = -1, int2 = -1, net = 0x0} That difference in domain (VIR_FROM_REMOTE vs. VIR_FROM_RPC) is sufficient to bypass our rewriting rules later in the function which convert VIR_ERR_RPC back to VIR_ERR_NO_SUPPORT. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Libvirt special-cases a specific VIR_ERR_RPC from the remote driver back into VIR_ERR_NO_SUPPORT on the client, so that clients can handle missing rpc functions the same whether the hypervisor driver is local or remote. However, commit c1b22644 introduced a regression: VIR_FROM_THIS changed from VIR_FROM_REMOTE to VIR_FROM_RPC, so the special casing no longer works if the server uses the newer error domain. * src/rpc/virnetclientprogram.c (virNetClientProgramDispatchError): Also cater to 0.9.3 and newer. --- src/rpc/virnetclientprogram.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/virnetclientprogram.c b/src/rpc/virnetclientprogram.c index a07b744..33fa507 100644 --- a/src/rpc/virnetclientprogram.c +++ b/src/rpc/virnetclientprogram.c @@ -151,7 +151,7 @@ virNetClientProgramDispatchError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, break; } - if (err.domain == VIR_FROM_REMOTE && + if ((err.domain == VIR_FROM_REMOTE || err.domain == VIR_FROM_RPC) && err.code == VIR_ERR_RPC && err.level == VIR_ERR_ERROR && err.message && -- 1.7.4.4
This reverts commit 799912fa05b8c3aa37bd04c57b196755f3f70552; now that the rpc regression is fixed, virsh no longer needs the special case here. --- tools/virsh.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3c6e65a..aee1842 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1090,8 +1090,7 @@ cmdDomblkstat (vshControl *ctl, const vshCmd *cmd) * then. */ if (rc < 0) { - if (last_error->code != VIR_ERR_NO_SUPPORT && - last_error->code != VIR_ERR_RPC) { + if (last_error->code != VIR_ERR_NO_SUPPORT) { virshReportError(ctl); goto cleanup; } else { -- 1.7.4.4
On 09/14/2011 12:51 PM, Eric Blake wrote:
Libvirt special-cases a specific VIR_ERR_RPC from the remote driver back into VIR_ERR_NO_SUPPORT on the client, so that clients can handle missing rpc functions the same whether the hypervisor driver is local or remote. However, commit c1b22644 introduced a regression: VIR_FROM_THIS changed from VIR_FROM_REMOTE to VIR_FROM_RPC, so the special casing no longer works if the server uses the newer error domain.
* src/rpc/virnetclientprogram.c (virNetClientProgramDispatchError): Also cater to 0.9.3 and newer. --- src/rpc/virnetclientprogram.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetclientprogram.c b/src/rpc/virnetclientprogram.c index a07b744..33fa507 100644 --- a/src/rpc/virnetclientprogram.c +++ b/src/rpc/virnetclientprogram.c @@ -151,7 +151,7 @@ virNetClientProgramDispatchError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, break; }
- if (err.domain == VIR_FROM_REMOTE&& + if ((err.domain == VIR_FROM_REMOTE || err.domain == VIR_FROM_RPC)&&
This is a regression fix, and I got approval on IRC, so I'm pushing now to be part of rc3: <eblake> danpb: have you seen this rpc regression bug fix? https://www.redhat.com/archives/libvir-list/2011-September/msg00547.html <supybot> Title: [libvirt] [PATCH 1/2] rpc: convert unknown procedures to VIR_ERR_NO_SUPP (at www.redhat.com) <danpb> eblake: yeah looks ok -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Sep 16, 2011 at 08:47:00AM -0600, Eric Blake wrote:
On 09/14/2011 12:51 PM, Eric Blake wrote:
Libvirt special-cases a specific VIR_ERR_RPC from the remote driver back into VIR_ERR_NO_SUPPORT on the client, so that clients can handle missing rpc functions the same whether the hypervisor driver is local or remote. However, commit c1b22644 introduced a regression: VIR_FROM_THIS changed from VIR_FROM_REMOTE to VIR_FROM_RPC, so the special casing no longer works if the server uses the newer error domain.
* src/rpc/virnetclientprogram.c (virNetClientProgramDispatchError): Also cater to 0.9.3 and newer. --- src/rpc/virnetclientprogram.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/rpc/virnetclientprogram.c b/src/rpc/virnetclientprogram.c index a07b744..33fa507 100644 --- a/src/rpc/virnetclientprogram.c +++ b/src/rpc/virnetclientprogram.c @@ -151,7 +151,7 @@ virNetClientProgramDispatchError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, break; }
- if (err.domain == VIR_FROM_REMOTE&& + if ((err.domain == VIR_FROM_REMOTE || err.domain == VIR_FROM_RPC)&&
This is a regression fix, and I got approval on IRC, so I'm pushing now to be part of rc3:
<eblake> danpb: have you seen this rpc regression bug fix? https://www.redhat.com/archives/libvir-list/2011-September/msg00547.html <supybot> Title: [libvirt] [PATCH 1/2] rpc: convert unknown procedures to VIR_ERR_NO_SUPP (at www.redhat.com) <danpb> eblake: yeah looks ok
Okay, formal ACK anyway ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Daniel Veillard -
Eric Blake -
Peter Krempa