10:16 a.m.
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
5:50 p.m.
New subject: [libvirt] [PATCH] virsh: Use old API if remote libvirtd does not support new
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:56 p.m.
New subject: [libvirt] [PATCH] virsh: Use old API if remote libvirtd does not support new
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:42 p.m.
New subject: [libvirt] [PATCH] virsh: Use old API if remote libvirtd does not support new
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:12 p.m.
New subject: [libvirt] [PATCH] virsh: Use old API if remote libvirtd does not support new
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:51 p.m.
New subject: [libvirt] [PATCH 1/2] rpc: convert unknown procedures to VIR_ERR_NO_SUPPORT
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
1:51 p.m.
New subject: [libvirt] [PATCH 2/2] Revert "virsh: Use old API if remote libvirtd does not support new"
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
9:47 a.m.
New subject: [libvirt] [PATCH 1/2] rpc: convert unknown procedures to VIR_ERR_NO_SUPPORT
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:34 a.m.
New subject: [libvirt] [PATCH 1/2] rpc: convert unknown procedures to VIR_ERR_NO_SUPPORT
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4814
days inactive
4819
days old
8 comments
3 participants
participants (3)
-
Daniel Veillard -
Eric Blake -
Peter Krempa