[libvirt] [PATCH] LXC fix virCgroupGetValueStr on handling a string terminated with '\n'
Hi there, A cgroup file returns integer value terminated with '\n' and remaining it has sometimes harmful effects, for example it leads virStrToLong_ull failed. The fix just gets rid of '\n' if included. It first has been in virCgroupGetFreezerState, but now merged in virCgroupGetValueStr to cover every functions using virCgroupGetValueStr. I'm not sure that that behavior is from the beginning or changed in a recent kernel, but the fix should work even though anyway. BTW, by the defect I first got the following error and it made me confused. # virsh -c lxc:/// dominfo 4930 Id: 4930 Name: lxc UUID: 084369a0-956a-3010-fc37-ddeb4d627e69 OS Type: exe Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel The really error happens in lxcDomainGetInfo, so I guess something is wrong with internal error propagation. Anyone know this unexpected behavior? Thanks, ozaki-r
From 667efde6ad165479817975c6544f6784e1177a32 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Fri, 16 Oct 2009 00:13:41 +0900 Subject: [PATCH] LXC fix virCgroupGetValueStr on handling a string terminated with '\n'
* src/util/cgroup.c: get rid of '\n' from the return value of virCgroupGetValueStr --- src/util/cgroup.c | 12 +++++------- 1 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index f728a2f..f0e2960 100644 --- a/src/util/cgroup.c+++ b/src/util/cgroup.c @@ -309,6 +309,10 @@ static int virCgroupGetValueStr(virCgroupPtr group, DEBUG("Failed to read %s: %m\n", keypath); rc = -errno; } else { + /* Terminated with '\n' has sometimes harmful effects to the caller */ + char *p = strchr((const char *)value, '\n'); + if (p) *p = '\0'; + rc = 0; } @@ -969,13 +973,7 @@ int virCgroupSetFreezerState(virCgroupPtr group, const char *state) int virCgroupGetFreezerState(virCgroupPtr group, char **state) { - int ret; - ret = virCgroupGetValueStr(group, + return virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPU, "freezer.state", state); - if (ret == 0) { - char *p = strchr(*state, '\n'); - if (p) *p = '\0'; - } - return ret; } -- 1.6.2.5
On Fri, Oct 16, 2009 at 05:05:27PM +0900, Ryota Ozaki wrote:
Hi there,
A cgroup file returns integer value terminated with '\n' and remaining it has sometimes harmful effects, for example it leads virStrToLong_ull failed.
The fix just gets rid of '\n' if included. It first has been in virCgroupGetFreezerState, but now merged in virCgroupGetValueStr to cover every functions using virCgroupGetValueStr.
I'm not sure that that behavior is from the beginning or changed in a recent kernel, but the fix should work even though anyway.
ACK, looks good.
BTW, by the defect I first got the following error and it made me confused.
# virsh -c lxc:/// dominfo 4930 Id: 4930 Name: lxc UUID: 084369a0-956a-3010-fc37-ddeb4d627e69 OS Type: exe Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel
The really error happens in lxcDomainGetInfo, so I guess something is wrong with internal error propagation. Anyone know this unexpected behavior?
This looks rather odd - I can't think of anything which could cause this to happen - virsh is supposed to be filtering out that error message Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Ooop! The patch lost the last modification...so please use the new one, sorry. On Fri, Oct 16, 2009 at 9:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Fri, Oct 16, 2009 at 05:05:27PM +0900, Ryota Ozaki wrote:
BTW, by the defect I first got the following error and it made me confused.
# virsh -c lxc:/// dominfo 4930 Id: 4930 Name: lxc UUID: 084369a0-956a-3010-fc37-ddeb4d627e69 OS Type: exe Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel
The really error happens in lxcDomainGetInfo, so I guess something is wrong with internal error propagation. Anyone know this unexpected behavior?
This looks rather odd - I can't think of anything which could cause this to happen - virsh is supposed to be filtering out that error message
OK, I've looked into a bit deep and found the following sequence happens. In cmdDominfo (virsh.c), if virDomainGetInfo fails, 'ret' is set with FALSE. Even so, further virNodeGetSecurityModel is called too. And if it also fails then VIR_ERR_NO_SUPPORT is set. Normally, VIR_ERR_NO_SUPPORT does not lead cmdDominfo failed, however, 'ret' is previously set with FALSE, then virsh fails. ozaki-r
From 55f11663316a9233987aacbaa1b561d0cea13cf8 Mon Sep 17 00:00:00 2001 From: Ryota Ozaki <ozaki.ryota@gmail.com> Date: Sat, 17 Oct 2009 08:16:34 +0900 Subject: [PATCH] LXC fix virCgroupGetValueStr on handling a string terminated with '¥n'
* src/util/cgroup.c: get rid of '¥n' from the return value of virCgroupGetValueStr --- src/util/cgroup.c | 12 +++++------- 1 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/util/cgroup.c b/src/util/cgroup.c index f728a2f..bdd4eb6 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -309,6 +309,10 @@ static int virCgroupGetValueStr(virCgroupPtr group, DEBUG("Failed to read %s: %m¥n", keypath); rc = -errno; } else { + /* Terminated with '¥n' has sometimes harmful effects to the caller */ + char *p = strchr(*value, '¥n'); + if (p) *p = '¥0'; + rc = 0; } @@ -969,13 +973,7 @@ int virCgroupSetFreezerState(virCgroupPtr group, const char *state) int virCgroupGetFreezerState(virCgroupPtr group, char **state) { - int ret; - ret = virCgroupGetValueStr(group, + return virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPU, "freezer.state", state); - if (ret == 0) { - char *p = strchr(*state, '¥n'); - if (p) *p = '¥0'; - } - return ret; } -- 1.6.2.5
On Sat, Oct 17, 2009 at 09:30:12AM +0900, Ryota Ozaki wrote:
Ooop! The patch lost the last modification...so please use the new one, sorry.
On Fri, Oct 16, 2009 at 9:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Fri, Oct 16, 2009 at 05:05:27PM +0900, Ryota Ozaki wrote:
BTW, by the defect I first got the following error and it made me confused.
# virsh -c lxc:/// dominfo 4930 Id: 4930 Name: lxc UUID: 084369a0-956a-3010-fc37-ddeb4d627e69 OS Type: exe Autostart: disable error: this function is not supported by the hypervisor: virNodeGetSecurityModel
The really error happens in lxcDomainGetInfo, so I guess something is wrong with internal error propagation. Anyone know this unexpected behavior?
This looks rather odd - I can't think of anything which could cause this to happen - virsh is supposed to be filtering out that error message
OK, I've looked into a bit deep and found the following sequence happens.
In cmdDominfo (virsh.c), if virDomainGetInfo fails, 'ret' is set with FALSE. Even so, further virNodeGetSecurityModel is called too. And if it also fails then VIR_ERR_NO_SUPPORT is set. Normally, VIR_ERR_NO_SUPPORT does not lead cmdDominfo failed, however, 'ret' is previously set with FALSE, then virsh fails.
Okidoc, I applied the last patch. Thanks ! I wonder if we also need to fix the logic in virsh.c:cmdDominfo() , maybe by making: if (last_error->code != VIR_ERR_NO_SUPPORT) { into if ((last_error->code != VIR_ERR_NO_SUPPORT) && (ret != FALSE)) { or by resetting last_error as well as setting ret to false when virDomainGetInfo() fails. 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/
On Mon, Oct 19, 2009 at 9:39 PM, Daniel Veillard <veillard@redhat.com> wrote:
On Sat, Oct 17, 2009 at 09:30:12AM +0900, Ryota Ozaki wrote:
OK, I've looked into a bit deep and found the following sequence happens.
In cmdDominfo (virsh.c), if virDomainGetInfo fails, 'ret' is set with FALSE. Even so, further virNodeGetSecurityModel is called too. And if it also fails then VIR_ERR_NO_SUPPORT is set. Normally, VIR_ERR_NO_SUPPORT does not lead cmdDominfo failed, however, 'ret' is previously set with FALSE, then virsh fails.
Okidoc, I applied the last patch. Thanks !
I wonder if we also need to fix the logic in virsh.c:cmdDominfo() , maybe by making:
if (last_error->code != VIR_ERR_NO_SUPPORT) {
into
if ((last_error->code != VIR_ERR_NO_SUPPORT) && (ret != FALSE)) {
or by resetting last_error as well as setting ret to false when virDomainGetInfo() fails.
I'm not sure I understand you are saying, I think it's not sufficient because what we need to consider in the case is that virDomainGetInfo() fails and virNodeGetSecurityModel() fails with VIR_ERR_NO_SUPPORT. So we probably need to save an error of virDomainGetInfo() before calling virNodeGetSecurityModel() and restore it if virNodeGetSecurityModel() fails with VIR_ERR_NO_SUPPORT. Nonetheless, if both functions fails (with except VIR_ERR_NO_SUPPORT), we have to discard either of two error codes because we have one container for them... ozaki-r
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 P. Berrange -
Daniel Veillard -
Ryota Ozaki