4:08 a.m.
Hi all
I try virsh dominfo in upstream libvirt on xen machine,
the commands returns -1 as follows:
[root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3
Id: 1
Name: rhel53rc2_pv_sdb3
UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e
OS Type: linux
State: idle
CPU(s): 2
CPU time: 8.8s
Max memory: 1048576 kB
Used memory: 716800 kB
Autostart: disable
error: this function is not supported by the hypervisor:
virNodeGetSecurityModel
[root@vmi20 ~]# echo $?
1
The explanation of virNodeGetSecurityModel() and
virNodeGetSecurityModel() in libvirt.c is return -2
when hypervisor drivers don't support these operations.
But these functions return -1 in this case, and so
cmdDominfo() in virsh.c returns FALSE.
I make a patch.
- virNodeGetSecurityModel() and virNodeGetSecurityModel()
return -2 when drivers don't supprted these operations.
- In CmdDominfo(), it is no operation when virNodeGetSecurityModel()
and virNodeGetSecurityModel() return -2.
Signed-off-by: Tatsuro Enokura <fj2026af(a)aa.jp.fujitsu.com>
Thanks
Tatsuro Enokura
6:20 a.m.
On Thu, Jun 18, 2009 at 06:08:36PM +0900, Tatsuro Enokura wrote:
Hi all
I try virsh dominfo in upstream libvirt on xen machine,
the commands returns -1 as follows:
Opps, thanks for reporting this problem - surprised it slipped
past our testing for so long !
[root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3
Id: 1
Name: rhel53rc2_pv_sdb3
UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e
OS Type: linux
State: idle
CPU(s): 2
CPU time: 8.8s
Max memory: 1048576 kB
Used memory: 716800 kB
Autostart: disable
error: this function is not supported by the hypervisor:
virNodeGetSecurityModel
[root@vmi20 ~]# echo $?
1
The explanation of virNodeGetSecurityModel() and
virNodeGetSecurityModel() in libvirt.c is return -2
when hypervisor drivers don't support these operations.
But these functions return -1 in this case, and so
cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus.
With the remote driver we only have a boolean success vs fail
status, so there is no way to return 2 different error codes. In
addition already have a way to report methods which are not
supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is
no need for a special '-2' value in any case.
I make a patch.
- virNodeGetSecurityModel() and virNodeGetSecurityModel()
return -2 when drivers don't supprted these operations.
- In CmdDominfo(), it is no operation when virNodeGetSecurityModel()
and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the
VIR_ERR_NO_SUPPORT code and simply ignores that error.
This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and
adding in several missing 'memset()' calls, because the
drivers should not assume the caller has zero'd these
structs.
Regards,
Daniel
Index: src/libvirt.c
===================================================================
RCS file: /data/cvs/libvirt/src/libvirt.c,v
retrieving revision 1.214
diff -u -p -u -r1.214 libvirt.c
--- src/libvirt.c 12 Jun 2009 13:20:13 -0000 1.214
+++ src/libvirt.c 18 Jun 2009 11:14:44 -0000
@@ -4386,11 +4386,11 @@ error:
* @domain: a domain object
* @seclabel: pointer to a virSecurityLabel structure
*
- * Extract security label of an active domain.
+ * Extract security label of an active domain. The 'label' field
+ * in the @seclabel argument will be initialized to the empty
2A+ * string if the domain is not running under a security model.
*
- * Returns 0 in case of success, -1 in case of failure, and -2
- * if the operation is not supported (caller decides if that's
- * an error).
+ * Returns 0 in case of success, -1 in case of failure
*/
int
virDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel)
@@ -4421,10 +4421,11 @@ virDomainGetSecurityLabel(virDomainPtr d
* @conn: a connection object
* @secmodel: pointer to a virSecurityModel structure
*
- * Extract the security model of a hypervisor.
+ * Extract the security model of a hypervisor. The 'model' field
+ * in the @secmodel argument may be initialized to the empty
+ * string if the driver has not activated a security model.
*
- * Returns 0 in case of success, -1 in case of failure, and -2 if the
- * operation is not supported (caller decides if that's an error).
+ * Returns 0 in case of success, -1 in case of failure
*/
int
virNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel)
Index: src/qemu_driver.c
===================================================================
RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
retrieving revision 1.255
diff -u -p -u -r1.255 qemu_driver.c
--- src/qemu_driver.c 16 Jun 2009 15:42:46 -0000 1.255
+++ src/qemu_driver.c 18 Jun 2009 11:14:44 -0000
@@ -3064,6 +3064,8 @@ static int qemudDomainGetSecurityLabel(v
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
+ memset(seclabel, 0, sizeof(*seclabel));
+
if (!vm) {
char uuidstr[VIR_UUID_STRING_BUFLEN];
virUUIDFormat(dom->uuid, uuidstr);
@@ -3121,7 +3123,7 @@ static int qemudNodeGetSecurityModel(vir
qemuDriverLock(driver);
if (!driver->securityDriver) {
- ret = -2;
+ memset(secmodel, 0, sizeof (*secmodel));
goto cleanup;
}
Index: src/remote_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/remote_internal.c,v
retrieving revision 1.157
diff -u -p -u -r1.157 remote_internal.c
--- src/remote_internal.c 12 Jun 2009 13:20:13 -0000 1.157
+++ src/remote_internal.c 18 Jun 2009 11:14:44 -0000
@@ -2320,6 +2320,8 @@ remoteDomainGetSecurityLabel (virDomainP
make_nonnull_domain (&args.dom, domain);
memset (&ret, 0, sizeof ret);
+ memset (seclabel, 0, sizeof (*seclabel));
+
if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_SECURITY_LABEL,
(xdrproc_t) xdr_remote_domain_get_security_label_args, (char *)&args,
(xdrproc_t) xdr_remote_domain_get_security_label_ret, (char *)&ret) ==
-1) {
@@ -2353,6 +2355,8 @@ remoteNodeGetSecurityModel (virConnectPt
remoteDriverLock(priv);
memset (&ret, 0, sizeof ret);
+ memset (secmodel, 0, sizeof (*secmodel));
+
if (call (conn, priv, 0, REMOTE_PROC_NODE_GET_SECURITY_MODEL,
(xdrproc_t) xdr_void, NULL,
(xdrproc_t) xdr_remote_node_get_security_model_ret, (char *)&ret) ==
-1) {
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.210
diff -u -p -u -r1.210 virsh.c
--- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210
+++ src/virsh.c 18 Jun 2009 11:14:44 -0000
@@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd
/* Security model and label information */
memset(&secmodel, 0, sizeof secmodel);
if (virNodeGetSecurityModel(ctl->conn, &secmodel) == -1) {
- virDomainFree(dom);
- return FALSE;
+ if (last_error->code != VIR_ERR_NO_SUPPORT) {
+ virDomainFree(dom);
+ return FALSE;
+ }
} else {
/* Only print something if a security model is active */
if (secmodel.model[0] != '\0') {
--
|: 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 :|
8:58 p.m.
Hi Dan,
Daniel P. Berrange wrote:
> The explanation of virNodeGetSecurityModel() and
> virNodeGetSecurityModel() in libvirt.c is return -2
> when hypervisor drivers don't support these operations.
> But these functions return -1 in this case, and so
> cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus.
With the remote driver we only have a boolean success vs fail
status, so there is no way to return 2 different error codes. In
addition already have a way to report methods which are not
supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is
no need for a special '-2' value in any case.
> I make a patch.
> - virNodeGetSecurityModel() and virNodeGetSecurityModel()
> return -2 when drivers don't supprted these operations.
> - In CmdDominfo(), it is no operation when virNodeGetSecurityModel()
> and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the
VIR_ERR_NO_SUPPORT code and simply ignores that error.
This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and
adding in several missing 'memset()' calls, because the
drivers should not assume the caller has zero'd these
structs.
Ok, I see.
Index: src/virsh.c
===================================================================
RCS file: /data/cvs/libvirt/src/virsh.c,v
retrieving revision 1.210
diff -u -p -u -r1.210 virsh.c
--- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210
+++ src/virsh.c 18 Jun 2009 11:14:44 -0000
@@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd
/* Security model and label information */
memset(&secmodel, 0, sizeof secmodel);
if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) {
- virDomainFree(dom);
- return FALSE;
+ if (last_error->code != VIR_ERR_NO_SUPPORT) {
+ virDomainFree(dom);
+ return FALSE;
+ }
} else {
/* Only print something if a security model is active */
if (secmodel.model[0] != '\0') {
Don't check last_error->code of virDomainGetSecurityLabel()?
should check the same as virNodeGetSecurityModel().
Thanks
Tatsuro Enokura
5:55 a.m.
On Fri, Jun 19, 2009 at 10:58:18AM +0900, Tatsuro Enokura wrote:
Hi Dan,
Daniel P. Berrange wrote:
>>The explanation of virNodeGetSecurityModel() and
>>virNodeGetSecurityModel() in libvirt.c is return -2
>>when hypervisor drivers don't support these operations.
>>But these functions return -1 in this case, and so
>>cmdDominfo() in virsh.c returns FALSE.
>
>This API description about returning -1 vs -2 is totally bogus.
>With the remote driver we only have a boolean success vs fail
>status, so there is no way to return 2 different error codes. In
>addition already have a way to report methods which are not
>supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is
>no need for a special '-2' value in any case.
>
>>I make a patch.
>> - virNodeGetSecurityModel() and virNodeGetSecurityModel()
>> return -2 when drivers don't supprted these operations.
>> - In CmdDominfo(), it is no operation when virNodeGetSecurityModel()
>> and virNodeGetSecurityModel() return -2.
>
>I'm attaching a alternate patch which just checks for the
>VIR_ERR_NO_SUPPORT code and simply ignores that error.
>This should deal with the error scenario you saw with Xen.
>
>I'm also fixing the API description to match reality and
>adding in several missing 'memset()' calls, because the
>drivers should not assume the caller has zero'd these
>structs.
Ok, I see.
>Index: src/virsh.c
>===================================================================
>RCS file: /data/cvs/libvirt/src/virsh.c,v
>retrieving revision 1.210
>diff -u -p -u -r1.210 virsh.c
>--- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210
>+++ src/virsh.c 18 Jun 2009 11:14:44 -0000
>@@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd
> /* Security model and label information */
> memset(&secmodel, 0, sizeof secmodel);
> if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) {
>- virDomainFree(dom);
>- return FALSE;
>+ if (last_error->code != VIR_ERR_NO_SUPPORT) {
>+ virDomainFree(dom);
>+ return FALSE;
>+ }
> } else {
> /* Only print something if a security model is active */
> if (secmodel.model[0] != '\0') {
Don't check last_error->code of virDomainGetSecurityLabel()?
should check the same as virNodeGetSecurityModel().
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel
if virNodeGetSecurityModel() was asuccessfull and the returned secmodel
is not the empty string. In such a scenario I'd expect the call to
virDomainGetSecurityLabel() to always be successful and would want the
user to see any error if it fail
Regards,
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 :|
11:24 p.m.
Hi Dan,
Daniel P. Berrange wrote:
>> Index: src/virsh.c
>> ===================================================================
>> RCS file: /data/cvs/libvirt/src/virsh.c,v
>> retrieving revision 1.210
>> diff -u -p -u -r1.210 virsh.c
>> --- src/virsh.c 3 Jun 2009 12:13:52 -0000 1.210
>> +++ src/virsh.c 18 Jun 2009 11:14:44 -0000
>> @@ -1643,8 +1643,10 @@ cmdDominfo(vshControl *ctl, const vshCmd
>> /* Security model and label information */
>> memset(&secmodel, 0, sizeof secmodel);
>> if (virNodeGetSecurityModel(ctl->conn,&secmodel) == -1) {
>> - virDomainFree(dom);
>> - return FALSE;
>> + if (last_error->code != VIR_ERR_NO_SUPPORT) {
>> + virDomainFree(dom);
>> + return FALSE;
>> + }
>> } else {
>> /* Only print something if a security model is active */
>> if (secmodel.model[0] != '\0') {
> Don't check last_error->code of virDomainGetSecurityLabel()?
> should check the same as virNodeGetSecurityModel().
I don't think that is neccessary. You'll only invoke virDomainGetSecurityLabel
if virNodeGetSecurityModel() was asuccessfull and the returned secmodel
is not the empty string. In such a scenario I'd expect the call to
virDomainGetSecurityLabel() to always be successful and would want the
user to see any error if it fail
OK, I understood.
I try your patch, works fine.
Thanks
Tatsuro Enokura
7:29 p.m.
New subject: [libvirt] [PATCH] fix virsh dominfo returns error when virNodeGetSecurityModel() is not supported.
Hi Dan,
Tatsuro Enokura wrote:
> I don't think that is neccessary. You'll only invoke
> virDomainGetSecurityLabel
> if virNodeGetSecurityModel() was asuccessfull and the returned secmodel
> is not the empty string. In such a scenario I'd expect the call to
> virDomainGetSecurityLabel() to always be successful and would want the
> user to see any error if it fail
OK, I understood.
I try your patch, works fine.
If there is not problem, please commit your patch.
Thanks
Tatsuro Enokura
8:43 a.m.
On Thu, Jun 25, 2009 at 09:29:48AM +0900, Tatsuro Enokura wrote:
Hi Dan,
Tatsuro Enokura wrote:
>> I don't think that is neccessary. You'll only invoke
>> virDomainGetSecurityLabel
>> if virNodeGetSecurityModel() was asuccessfull and the returned secmodel
>> is not the empty string. In such a scenario I'd expect the call to
>> virDomainGetSecurityLabel() to always be successful and would want the
>> user to see any error if it fail
>
> OK, I understood.
>
> I try your patch, works fine.
If there is not problem, please commit your patch.
This was already commited to CVS as far as I can tell,
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/
8:51 a.m.
On Thu, Jun 25, 2009 at 03:43:12PM +0200, Daniel Veillard wrote:
On Thu, Jun 25, 2009 at 09:29:48AM +0900, Tatsuro Enokura wrote:
> Hi Dan,
>
> Tatsuro Enokura wrote:
>>> I don't think that is neccessary. You'll only invoke
>>> virDomainGetSecurityLabel
>>> if virNodeGetSecurityModel() was asuccessfull and the returned secmodel
>>> is not the empty string. In such a scenario I'd expect the call to
>>> virDomainGetSecurityLabel() to always be successful and would want the
>>> user to see any error if it fail
>>
>> OK, I understood.
>>
>> I try your patch, works fine.
>
> If there is not problem, please commit your patch.
This was already commited to CVS as far as I can tell,
I did it about an hour ago but got distracted by lunch before I replied :-)
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 :|
8:08 a.m.
On Thu, Jun 18, 2009 at 12:20:16PM +0100, Daniel P. Berrange wrote:
On Thu, Jun 18, 2009 at 06:08:36PM +0900, Tatsuro Enokura wrote:
> Hi all
>
> I try virsh dominfo in upstream libvirt on xen machine,
> the commands returns -1 as follows:
Opps, thanks for reporting this problem - surprised it slipped
past our testing for so long !
> [root@vmi20 ~]# virsh dominfo rhel53rc2_pv_sdb3
> Id: 1
> Name: rhel53rc2_pv_sdb3
> UUID: 05ba9be8-f4e9-e208-11c7-fc936655cd8e
> OS Type: linux
> State: idle
> CPU(s): 2
> CPU time: 8.8s
> Max memory: 1048576 kB
> Used memory: 716800 kB
> Autostart: disable
> error: this function is not supported by the hypervisor:
> virNodeGetSecurityModel
>
> [root@vmi20 ~]# echo $?
> 1
>
> The explanation of virNodeGetSecurityModel() and
> virNodeGetSecurityModel() in libvirt.c is return -2
> when hypervisor drivers don't support these operations.
> But these functions return -1 in this case, and so
> cmdDominfo() in virsh.c returns FALSE.
This API description about returning -1 vs -2 is totally bogus.
With the remote driver we only have a boolean success vs fail
status, so there is no way to return 2 different error codes. In
addition already have a way to report methods which are not
supported, by giving back a VIR_ERR_NO_SUPPORT code, so there is
no need for a special '-2' value in any case.
> I make a patch.
> - virNodeGetSecurityModel() and virNodeGetSecurityModel()
> return -2 when drivers don't supprted these operations.
> - In CmdDominfo(), it is no operation when virNodeGetSecurityModel()
> and virNodeGetSecurityModel() return -2.
I'm attaching a alternate patch which just checks for the
VIR_ERR_NO_SUPPORT code and simply ignores that error.
This should deal with the error scenario you saw with Xen.
I'm also fixing the API description to match reality and
adding in several missing 'memset()' calls, because the
drivers should not assume the caller has zero'd these
structs.
Looks fine, ACK,
thanks !
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/
5629
days inactive
5636
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Tatsuro Enokura