Re: [libvirt] [PATCH] Fix crash in virDomainGetVcpuPinInfo python wrapper
ACK - Original Message - From: "Daniel P. Berrange" To: libvir-list@redhat.com Sent: Tuesday, September 3, 2013 11:37:44 AM Subject: [libvirt] [PATCH] Fix crash in virDomainGetVcpuPinInfo python wrapper From: "Daniel P. Berrange" It is possible to jump to the cleanup block before the cpumaps variable gets initialized. This will result in a VIR_FREE of an uninitializer pointer Signed-off-by: Daniel P. Berrange --- python/libvirt-override.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index d16b9a2..cc76c47 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -1769,7 +1769,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED, virDomainPtr domain; PyObject *pyobj_domain, *pycpumaps = NULL; virDomainInfo dominfo; -unsigned char *cpumaps; +unsigned char *cpumaps = NULL; size_t cpumaplen, vcpu, pcpu; unsigned int flags; int i_retval, cpunum; -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5]qemu: add usb-bot scsi controller support
- Original Message -
From: "Daniel P. Berrange"
To: "Guannan Ren"
Cc: libvir-list@redhat.com
Sent: Tuesday, September 3, 2013 5:38:08 AM
Subject: Re: [libvirt] [PATCH 0/5]qemu: add usb-bot scsi controller support
On Mon, Sep 02, 2013 at 05:38:39PM +0800, Guannan Ren wrote:
>>
>> BZ:https://bugzilla.redhat.com/show_bug.cgi?id=917702
>> qemu patch:http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg02200.html
>>
>> These patch aims to add usb-bot SCSI controller support for libvirt.
>> As usb-storage libvirt already supported, usb-bot only supports one
>> SCSI target with SCSI ID 0.
>> The difference is that usb-storage creates SCSI HBA and additionaly
>> either a scsi-disk or a scsi-generic device, usb-bot only creates the
>> SCSI HBA. we can attach a SCSI device to it with another -device.
>>
>> usb-bot supports a single target and a number of luns. The limit is
>> 15 luns (0~15 LUN ID) and they must be contiguous(using lun 0 and 2
>> without 1 doesn't work).
>>
>> Athought usb-bot is a SCSI controller it needs to be attached to a
>> exsiting USB controller for work. So it has to use address of usb type.
>>
>> Libvirt XML:
>>
>>
>>...
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> ...
>>
> How does this work from a hotplug POV. With usb-storage you could
> just hotplug the device. Now it seems we need two separate
> hotplug calls one of the and one for the and
>the reverse.
Yes, I will think about the hotplug.
>>
>> The QEMU commandline:
>>
>> qemu ${other_vm_args} \
>> -device piix3-usb-uhci,id=usb \
>> -device usb-bot,id=scsi0,bus=usb.0,port=1 \
>> -drive
>> file=/var/lib/libvirt/images/disk.qcow2,if=none,id=drive-scsi0-0-0-0,format=raw
>> \
>> -device
>> scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0
>>
>> As the usb-storage creates scsi disk automatically which doesn't let
>> you set scsi-disk properties such as vendor, product, wwn, channel,
>> scsi-id and lun. So QEMU guys prefer usb-bot to usb-storage.
>> So this is the first part of the whole work. Next step will replace
>> usb-storage with usb-bot when disk in xml uses usb bus like
>>
>> <...>
>>
>>
> I'm not really a fan of introducing 2 different ways to configure
> the exact same device.
The idea is to use usb-bot instead of usb-storage in this case if usb-bot
capability is available.
usb-storage automatically expands into two devices, a SCSI controller and a
SCSI device. And user
cannot set any property values to this SCSI device such as vendor, product,
wwn, channel, scsi-id
and lun. usb-bot gives the more flexible configurations.
I think it is to use usb-bot if usb-bot is supported, otherwise still use
usb-storage. The xml
will not change at all.
Guannan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] virt-pki-validate: add --help/--version option
- Original Message - From: "Eric Blake" To: libvir-list@redhat.com Sent: Monday, August 19, 2013 6:43:15 PM Subject: [libvirt] [PATCH 3/3] virt-pki-validate: add --help/--version option Another program gains --help/--version :) * tools/virt-pki-validate.in: Add option parsing. Update documentation to match. * tools/Makefile.am (virt-pki-validate): Substitute version. Signed-off-by: Eric Blake --- tools/Makefile.am | 5 +++-- tools/virt-pki-validate.in | 45 - 2 files changed, 47 insertions(+), 3 deletions(-) ... diff --git a/tools/virt-pki-validate.in b/tools/virt-pki-validate.in index 5d0453f..87fb230 100755 --- a/tools/virt-pki-validate.in +++ b/tools/virt-pki-validate.in @@ -22,6 +22,35 @@ # # Daniel Veillard # + +case $1 in +case $1 in copy-paste error, double case lines. + -h | --h | --he | --hel | --help) +cat <&2 +exit 1 ;; remove ;; here. ACK with these small fixes. -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] virt-xml-validate: add missing schemas
ACK
- Original Message -
From: "Eric Blake"
To: libvir-list@redhat.com
Sent: Monday, August 19, 2013 6:43:14 PM
Subject: [libvirt] [PATCH 2/3] virt-xml-validate: add missing schemas
We were failing to autoprobe which schema to use for several
top-level XML elements.
* tools/virt-xml-validate.in (TYPE): Recognize ,
, and .
Signed-off-by: Eric Blake
---
tools/virt-xml-validate.in | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tools/virt-xml-validate.in b/tools/virt-xml-validate.in
index 9744612..82686f4 100644
--- a/tools/virt-xml-validate.in
+++ b/tools/virt-xml-validate.in
@@ -56,6 +56,9 @@ fi
if [ -z "$TYPE" ]; then
ROOT=`xmllint --stream --debug "$XMLFILE" 2>/dev/null | grep "^0 1 " | awk
'{ print $3 }'`
case "$ROOT" in
+ *domainsnapshot*) # Must come first, since *domain* is a substring
+TYPE="domainsnapshot"
+;;
*domain*)
TYPE="domain"
;;
@@ -74,8 +77,14 @@ if [ -z "$TYPE" ]; then
*device*)
TYPE="nodedev"
;;
+ *filter*)
+TYPE="nwfilter"
+;;
+ *secret*)
+TYPE="secret"
+;;
*)
- echo "$0: cannot determine schema type for $XMLFILE"
+ echo "$0: cannot determine schema type for $XMLFILE" >&2
exit 3
esac
fi
@@ -83,7 +92,7 @@ fi
SCHEMA="@schemadir@/${TYPE}.rng"
if [ ! -f "$SCHEMA" ]; then
- echo "$0: schema $SCHEMA does not exist"
+ echo "$0: schema $SCHEMA does not exist" >&2
exit 4
fi
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virt-xml-validate: add --help/--version option
- Original Message - From: "Eric Blake" Cc: libvir-list@redhat.com Sent: Monday, August 19, 2013 6:58:56 PM Subject: Re: [libvirt] [PATCH 1/3] virt-xml-validate: add --help/--version option On 08/19/2013 04:43 PM, Eric Blake wrote: > All good tools should have --help and --version output :) > > * tools/virt-xml-validate.in: Add option parsing. Output errors > to stderr. Update documentation to match. > * tools/Makefile.am (virt-xml-validate): Substitute version. > > + --) shift ;; > + -*) > +echo "$0: unrecognized option '$1'" >&2 > +exit 1 ;; > +esac > + > XMLFILE="$1" This treats '-' as an unrecognized option (which is a bit misleading, since in POSIX terminology a lone - is NOT an option but an argument). It also raises the question - should we allow for an XMLFILE of '-' to mean reading from stdin, the way POSIX recommends? That is, should we support: virsh dumpxml $dom | virt-xml-validate - grep use "grep -f -" to donate the standard input. As written in its manpage -f is specified by POSIX. If we use -f, the single hyphen can be used as a filename. But as you said in the follow, user can prefix the filename with a path - ./-, or /home/Tim/-, so it is still fine in this case. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] virt-xml-validate: add --help/--version option
ACK - Original Message - From: "Eric Blake" To: libvir-list@redhat.com Sent: Monday, August 19, 2013 6:43:13 PM Subject: [libvirt] [PATCH 1/3] virt-xml-validate: add --help/--version option All good tools should have --help and --version output :) * tools/virt-xml-validate.in: Add option parsing. Output errors to stderr. Update documentation to match. * tools/Makefile.am (virt-xml-validate): Substitute version. Signed-off-by: Eric Blake --- tools/Makefile.am | 5 +++-- tools/virt-xml-validate.in | 44 +--- 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index f85c35c..03c9fd0 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -77,8 +77,9 @@ dist_man8_MANS = virt-sanlock-cleanup.8 endif virt-xml-validate: virt-xml-validate.in Makefile - $(AM_V_GEN)sed -e 's|[@]schemadir@|$(pkgdatadir)/schemas|' < $< > $@ \ - || (rm $@ && exit 1) && chmod +x $@ + $(AM_V_GEN)sed -e 's|[@]schemadir@|$(pkgdatadir)/schemas|g' \ + -e 's|[@]VERSION@|$(VERSION)|g' \ + < $< > $@ || (rm $@ && exit 1) && chmod +x $@ virt-xml-validate.1: virt-xml-validate.in $(top_srcdir)/configure.ac $(AM_V_GEN)$(POD2MAN) --name VIRT-XML-VALIDATE $< $(srcdir)/$@ \ diff --git a/tools/virt-xml-validate.in b/tools/virt-xml-validate.in index 9c584ed..9744612 100644 --- a/tools/virt-xml-validate.in +++ b/tools/virt-xml-validate.in @@ -17,16 +17,39 @@ set -e +case $1 in + -h | --h | --he | --hel | --help) +cat <&2 +exit 1 ;; +esac + XMLFILE="$1" TYPE="$2" if [ -z "$XMLFILE" ]; then - echo "syntax: $0 XMLFILE [TYPE]" + echo "syntax: $0 XMLFILE [TYPE]" >&2 exit 1 fi if [ ! -f "$XMLFILE" ]; then - echo "$0: document $XMLFILE does not exist" + echo "$0: document $XMLFILE does not exist" >&2 exit 2 fi @@ -78,6 +101,7 @@ exit 0 =head1 SYNOPSIS virt-xml-validate XML-FILE [SCHEMA-NAME] + virt-xml-validate OPTION =head1 DESCRIPTION @@ -117,6 +141,20 @@ The schema for the XML format used to declare driver capabilities =back +=head1 OPTIONS + +=over + +=item B<-h, --help> + +Display command line help usage then exit. + +=item B<-V, --version> + +Display version information then exit. + +=back + =head1 EXIT STATUS Upon successful validation, an exit status of 0 will be set. Upon @@ -134,7 +172,7 @@ Alternatively report bugs to your software distributor / vendor. =head1 COPYRIGHT -Copyright (C) 2009-2012 by Red Hat, Inc. +Copyright (C) 2009-2013 by Red Hat, Inc. Copyright (C) 2009 by Daniel P. Berrange =head1 LICENSE -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] udev: fix crash in libudev logging
- Original Message -
From: "Ján Tomko"
To: libvir-list@redhat.com
Sent: Friday, June 7, 2013 7:20:10 AM
Subject: [libvirt] [PATCH] udev: fix crash in libudev logging
VIR_ERROR_INT calls virLogMessage(..., const char *fmt, ...).
Call virLogVMessage(..., const char *fmt, va_list list) instead,
since libudev called us with a va_list object, not a list of arguments.
https://bugzilla.redhat.com/show_bug.cgi?id=969152
---
Without the cast, I was getting:
node_device/node_device_udev.c: In function 'nodeStateInitialize':
node_device/node_device_udev.c:1684:5: error: argument 2 of 'udev_set_log_fn'
might be a candidate for a format attribute [-Werror=missing-format-attribute]
node_device/node_device_udev.c: At top level:
cc1: error: unrecognized command line option
"-Wno-unused-command-line-argument" [-Werror]
Is there a nicer way to get rid of it?
src/libvirt_private.syms | 1 +
src/node_device/node_device_udev.c | 30 +-
2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b93629f..d73a4d0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1486,6 +1486,7 @@ virLogSetBufferSize;
virLogSetDefaultPriority;
virLogSetFromEnv;
virLogUnlock;
+virLogVMessage;
# util/virmacaddr.h
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 620cd58..ffcae16 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -348,15 +348,26 @@ static int udevGenerateDeviceName(struct udev_device
*device,
}
-static void udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
-int priority ATTRIBUTE_UNUSED,
-const char *file,
-int line,
-const char *fn,
-const char *fmt,
-va_list args)
+typedef void (*udevLogFunctionPtr) (struct udev *udev,
+int priority,
+const char *file,
+int line,
+const char *fn,
+const char *format,
+va_list args);
+
+static void
+ATTRIBUTE_FMT_PRINTF(6,0)
+udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
+int priority ATTRIBUTE_UNUSED,
+const char *file,
+int line,
+const char *fn,
+const char *fmt,
+va_list args)
{
-VIR_ERROR_INT(VIR_LOG_FROM_LIBRARY, file, line, fn, fmt, args);
+virLogVMessage(VIR_LOG_FROM_LIBRARY, VIR_LOG_ERROR,
+ file, line, fn, NULL, fmt, args);
}
@@ -1672,7 +1683,8 @@ static int nodeStateInitialize(bool privileged
ATTRIBUTE_UNUSED,
* its return value.
*/
udev = udev_new();
-udev_set_log_fn(udev, udevLogFunction);
+/* cast to get rid of missing-format-attribute warning */
+udev_set_log_fn(udev, (udevLogFunctionPtr) udevLogFunction);
priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
if (priv->udev_monitor == NULL) {
--
I have been trying to fix this bug these days, but I failed to find a good way
to bypass the warning you mentioned above caused by a gcc bug:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28492
I can not find a better way than the casting in your code to compress the
warning.
other than this, there are some points needed to pay attention.
1, the log string output from udev ends with newline character which we could
remove.
2, we can use debug mode for these logs.
The following is the code with my code squashed in your casting code to fix
this bug
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 620cd58..dc989e9 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -348,15 +348,32 @@ static int udevGenerateDeviceName(struct udev_device
*device,
}
-static void udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
-int priority ATTRIBUTE_UNUSED,
-const char *file,
-int line,
-const char *fn,
-const char *fmt,
-va_list args)
+typedef void (*udevLogFunctionPtr)(struct udev *udev,
+ int priority,
+ const char *file,
+ int line,
+ const char *fn,
+ const char *format,
+ va_list args);
+
+static void
+ATTRIBUTE_FMT_PRINTF(6,0)
+udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
+int priority ATTRIBUTE_UNUSED,
+cons
Re: [libvirt] [PATCH] interface: list all interfaces with flags == 0
Impressive. :) - Original Message - From: "Eric Blake" To: "Guannan Ren" Cc: libvir-list@redhat.com Sent: Tuesday, May 21, 2013 10:49:57 PM Subject: Re: [libvirt] [PATCH] interface: list all interfaces with flags == 0 On 05/21/2013 06:49 AM, Guannan Ren wrote: Guannan's logic says when to drop: >>> +if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) && >>> +!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && >>> + (status & NETCF_IFACE_ACTIVE)) || >>> + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && >>> + (status & NETCF_IFACE_INACTIVE >>> +continue; Jirka's logic says when to keep; so it would need a ! around the overall expression if we want to turn it into the condition that represents when to drop. >> if (!MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) || >> (MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && >> (status & NETCF_IFACE_ACTIVE)) || >> (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && >> (status & NETCF_IFACE_INACTIVE))) > yes, currently, there are only one group of flags with two > values(active/inactive), > the way of yours is better to read. > if in future, there are more than one group of flags which are > going to support, > the way of my version will be better. Let's compare what happens for all four combinations of the two flags [active is flag 1, inactive is flag 2]: flags status Guannan's [0 means keep] Jirka's [1 means keep] 0 active 0&&!(0||0)=keep !0||0||0=keep 0 inactive 0&&!(0||0)=keep !0||0||0=keep 1 active 1&&!(1||0)=keep !1||1||0=keep 1 inactive 1&&!(0||0)=drop !1||0||0=drop 2 active 1&&!(0||0)=drop !1||0||0=drop 2 inactive 1&&!(0||1)=keep !1||0||1=keep 3 active 1&&!(1||0)=keep !1||1||0=keep 3 inactive 1&&!(0||1)=keep !1||0||1=keep Ultimately, the two expressions are equivalent (you can use deMorgan's law to prove the equivalence), but I find Jirka's positive logic a bit easier to reason with than Guannan's negative logic, even if Guannan's style copied from how we did it on other API. I do agree that for purposes of adding future filter groups, as well as minimizing nested conditionals, that the logic looks better in terms of drop checks: foreach item: if (filter group 1 drops) continue; if (filter group 2 drops) continue; item was kept by all filters, add it to return rather than logic in terms of keep checks: foreach item: if (filter group 1 keeps) if (filter group 2 keeps) add it to return -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: report useful error failling to destroy domain gracefully
Yes, it belongs to system related error rather than libvirt
operation related.
- Original Message -
From: "Daniel P. Berrange"
To: "Guannan Ren"
Cc: libvir-list@redhat.com
Sent: Friday, May 17, 2013 6:14:56 PM
Subject: Re: [libvirt] [PATCH] qemu: report useful error failling to destroy
domain gracefully
On Fri, May 17, 2013 at 05:32:06PM +0800, Guannan Ren wrote:
> Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=927620
>
> #kill -STOP `pidof qemu-kvm`
> #virsh destroy $guest --graceful
> error: Failed to destroy domain testVM
> error: An error occurred, but the cause is unknown
>
> With --graceful, SIGTERM always is emitted to kill driver
> process, but it won't success till burning out waiting time
> in case of process being stopped.
> But domain destroy without --graceful can work, SIGKILL will
> be emitted to the stopped process after 10 secs which always
> kills a process even one that is currently stopped.
> So report an error after burning out waiting time in this case.
> ---
>
> -VIR_DEBUG("Timed out waiting after SIGKILL to process %lld",
> - (long long)pid);
> +virReportError(VIR_ERR_OPERATION_INVALID,
> + _("Failed to terminate process %lld with SIG%s"),
> + (long long)pid, signame);
OPERATION_INVALID isn't really appropriate for this. I'd raise a
system error, using errno==EBUSY to indicate that it needs to
be re-tried to kill the process
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Should we keep the original vnc behavior even if the underlying qemu changes?
Hi Everybody, Since qemu v1.1.0 upstream commit 8cf364, qemu implemented shared-flag handling the default qemu vnc server behavior from mode 'ignore' to 'allow-exclusive'. The direct effect is as follows for libvirt user: vncviwer client: before: all client could share desktop without any option after: all client need -Shared options for desktop sharing. One client without -Shared option breaks others's connection virt-viewer: before: virt-viewer could support multiple desktop sharing after: only one active connection If we need to keep the original vnc behavior at libvirt level, the '-vnc :1,share=ignore' need to add into qemu commandline. Any ideas about this? Reference RFB Protocol about shared-flag Shared-flag is non-zero (true) if the server should try to share the desktop by leaving other clients connected, zero (false) if it should give exclusive access to this client by disconnecting all other clients From qemu doc: Set display sharing policy. 'allow-exclusive' allows clients to ask for exclusive access. As suggested by the rfb spec this is implemented by dropping other connections. Connecting multiple clients in parallel requires all clients asking for a shared session (vncviewer: -shared switch). This is the default. 'force-shared' disables exclusive client access. Useful for shared desktop sessions, where you don't want someone forgetting specify -shared disconnect everybody else. 'ignore' completely ignores the shared flag and allows everybody connect unconditionally. Doesn't conform to the rfb spec but is traditional qemu behavior. Guannan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
- Original Message -
From: "Eric Blake"
To: "Guan Nan Ren"
Cc: libvir-list@redhat.com
Sent: Tuesday, January 24, 2012 8:56:30 PM
Subject: Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
On 01/24/2012 05:07 AM, Guan Nan Ren wrote:
>> static PyObject *
>> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
>> + PyObject *args) {
>
>> +LIBVIRT_BEGIN_ALLOW_THREADS;
>> +i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
>> +LIBVIRT_END_ALLOW_THREADS;
>
> Why are we getting the existing parameters, instead of just blindly
> constructing params based on args? I think that's a waste of effort,
> considering that libvirt will already reject things if the user passes
> in unknown key names.
I finally realized _why_ we do it - that's because we want to pass the
correct types to libvirt.c, but python is not strongly typed. That is,
if libvirt is expecting a particular named value to be
VIR_TYPED_PARAM_ULLONG, but the python code passes '1', we should be
able to properly convert that python value to C's 1ULL, rather than
rejecting the python code for using an incorrect type, since there is
not quite a notion of incorrect type in python.
There is another reason for this, we couldn't determine which
one or more parameter will be set in case of multiple parameters,
unless we hard-code the parameter name to compare. So the a little of overhead
caused by the loop and reassigning new value to given parameter based on
arg is acceptable.
That means that either we hard-code the list of all known parameter
names and their type, or we make things done via a runtime query by
getting all parameters to learn their types before formatting the user's
settings back into something that libvirt will parse.
It may also mean that libvirt itself should be a bit nicer about things
- now that we have virTypedParam.c, maybe that function should be taught
to do some type conversions (if the user passes in VIR_TYPED_PARAM_INT,
but the code wanted VIR_TYPED_PARAM_ULLONG, libvirt could do the type
conversion on the user's behalf); if libvirt is made smarter, then the
python glue code might be simpler. But there's still the issue of
back-compat - a newer python library talking to an older libvirt that
didn't do automatic type conversion would still be stuck needing to
query for types first.
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
Thanks for these comments.
I will refactor the loop code, rethink about other codes and make v3 patch.
Guannan Ren
- Original Message -
From: "Eric Blake"
To: "Guannan Ren"
Cc: libvir-list@redhat.com
Sent: Saturday, January 21, 2012 11:04:15 PM
Subject: Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
On 01/19/2012 02:16 AM, Guannan Ren wrote:
> *virDomainSetNumaParameters
> *virDomainGetNumaParameters
> ---
> python/libvirt-override-api.xml | 13 +++
> python/libvirt-override.c | 186
> +++
> 2 files changed, 199 insertions(+), 0 deletions(-)
>
> static PyObject *
> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args) {
> +LIBVIRT_BEGIN_ALLOW_THREADS;
> +i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
> +LIBVIRT_END_ALLOW_THREADS;
Why are we getting the existing parameters, instead of just blindly
constructing params based on args? I think that's a waste of effort,
considering that libvirt will already reject things if the user passes
in unknown key names. Besides,
> +
> +if (i_retval < 0) {
> +free(params);
> +return VIR_PY_INT_FAIL;
> +}
> +
> +/* convert to a Python tuple of long objects */
> +for (i = 0; i < nparams; i++) {
> +PyObject *key, *val;
> +key = libvirt_constcharPtrWrap(params[i].field);
> +val = PyDict_GetItem(info, key);
> +Py_DECREF(key);
> +
> +if (val == NULL)
> +continue;
this says that you will silently discard any unknown keys passed in from
the python code, whereas libvirt would noisily reject unknown keys. I
don't think the python code should behave differently from the C code.
> +
> +switch (params[i].type) {
> +case VIR_TYPED_PARAM_INT:
> +params[i].value.i = (int)PyInt_AS_LONG(val);
> +break;
We're starting to repeat this code sequence in several places - we ought
to refactor things to share this conversion to and from python types and
virTypedParameter, so that all of the glue code can share the same
conversions (and fixing bugs in one will fix all of them at the same time).
> +static PyObject *
> +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args) {
Indentation; also, we've lately been sticking the { of a function on
column 1:
static PyObject *
libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
{
> +
> +if (i_retval < 0)
> +return VIR_PY_NONE;
I think this is okay; it tells python that the C call successfully
reported an error, and that the caller can then query for the last
libvirt error.
> +
> +if ((params = malloc(sizeof(*params)*nparams)) == NULL)
> +return VIR_PY_NONE;
Bad - we didn't raise any libvirt error, so the user wouldn't be able to
tell why we returned NONE. Rather, we should really be calling return
PyErr_NoMemory() in situations where we failed to malloc(), so that
python can correctly raise a memory exception. (You were just
copying-and-pasting; the problem is pervasive throughout this file).
> +
> +LIBVIRT_BEGIN_ALLOW_THREADS;
> +i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
> +LIBVIRT_END_ALLOW_THREADS;
> +
> +if (i_retval < 0) {
> +free(params);
> +return VIR_PY_NONE;
> +}
Okay.
> +
> +/* convert to a Python tuple of long objects */
> +if ((info = PyDict_New()) == NULL) {
> +free(params);
Memory leak - params might include VIR_TYPED_PARAM_STRING contents, so
we need to call virTypedParameterArrayClear before freeing params.
> +return VIR_PY_NONE;
Bad - PyDict_New already raised the python exception for no memory, but
we silently discarded it by returning NONE instead of NULL. We should
really be propagating any python errors back up the call chain.
> +}
> +
> +for (i = 0 ; i < nparams ; i++) {
> +PyObject *key, *val;
> +
> +switch (params[i].type) {
> +case VIR_TYPED_PARAM_INT:
> +val = PyInt_FromLong((long)params[i].value.i);
> +break;
Again, worth refactoring code to share this loop.
> +
> +default:
> +free(params);
Another memory leak if params had any string contents.
> +Py_DECREF(info);
> +return VIR_PY_NONE;
> +}
> +
> +key = libvirt_constcharPtrWrap(params[i].field);
> +PyDict_SetItem(info, key, val);
Bad - we should be checking for failure of PyDict_SetItem, and
propagating that failure back to the caller.
> +}
> +free(params);
Another memory leak.
> +return(info);
> +}
> +
--
Eric Blake ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
l
Re: [libvirt] [test-API][PATCH 2/2] Fix problem of a logger instance
ack
Guannan Ren
- Original Message -
From: "Wayne Sun"
To: libvir-list@redhat.com
Sent: Friday, January 20, 2012 1:34:14 PM
Subject: [libvirt] [test-API][PATCH 2/2] Fix problem of a logger instance
---
utils/Python/format.py |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/utils/Python/format.py b/utils/Python/format.py
index 5de8eb0..9c119dd 100644
--- a/utils/Python/format.py
+++ b/utils/Python/format.py
@@ -33,7 +33,7 @@ class Format(object):
def print_string(self, msg, env_logger):
"""Only print a simple string"""
-env_logger(msg)
+env_logger.info(msg)
self.write_log('\n%s' %msg)
def print_start(self, msg, env_logger):
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [test-API][PATCH 1/2] Fix compatibility problem on python 2.4
ack
The rpartition string methods family is from Python 2.5.
Guannan Ren
- Original Message -
From: "Wayne Sun"
To: libvir-list@redhat.com
Sent: Friday, January 20, 2012 1:34:13 PM
Subject: [libvirt] [test-API][PATCH 1/2] Fix compatibility problem on python
2.4
* switch rpartition with rsplit
* adjust the style
---
generator.py | 15 +++
1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/generator.py b/generator.py
index 9a2ed06..6108963 100644
--- a/generator.py
+++ b/generator.py
@@ -124,10 +124,14 @@ class FuncGen(object):
for i in range(loop_number):
case_ref_name = self.cases_ref_names[i]
-pkg_casename = case_ref_name.rpartition(":")[0]
-funcname = case_ref_name.rpartition(":")[-1]
+pkg_casename = case_ref_name.rsplit(":", 1)[0]
+funcname = case_ref_name.rsplit(":", 1)[-1]
+
+if "_clean" not in funcname:
+cleanoper = 0
+else:
+cleanoper = 1
-cleanoper = 0 if "_clean" not in funcname else 1
if not cleanoper:
self.fmt.print_start(pkg_casename, env_logger)
@@ -182,7 +186,10 @@ class FuncGen(object):
if not cleanoper:
self.fmt.print_end(pkg_casename, ret, env_logger)
else:
-self.fmt.print_string(21*" " + "Done" if clean_ret < 1
else 21*" " + "Fail", env_logger)
+if clean_ret < 1:
+self.fmt.print_string(21*" " + "Done", env_logger)
+else:
+self.fmt.print_string(21*" " + "Fail", env_logger)
end_time = time.strftime("%Y-%m-%d %H:%M:%S")
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs
- Original Message -
From: "Alex Jia"
To: "Guan Nan Ren"
Cc: libvir-list@redhat.com
Sent: Thursday, January 19, 2012 1:39:58 PM
Subject: Re: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs
On 01/19/2012 11:29 AM, Alex Jia wrote:
> On 01/19/2012 09:38 AM, Guan Nan Ren wrote:
>>Hi
>>
>> Anybody could give a hand on reviewing this patch,
>> I appreciate it.
>> Chinese New Year is coming, best wishes to this community :)
>>
>>Guannan Ren
> Happy New Year!
>> - Original Message -
>> From: "Guannan Ren"
>> To: libvir-list@redhat.com
>> Sent: Monday, January 16, 2012 6:58:06 PM
>> Subject: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs
>>
>> *virDomainSetNumaParameters
>> *virDomainGetNumaParameters
>> ---
>> python/libvirt-override-api.xml | 13 +++
>> python/libvirt-override.c | 186
>> +++
>> 2 files changed, 199 insertions(+), 0 deletions(-)
>>
>> diff --git a/python/libvirt-override-api.xml
>> b/python/libvirt-override-api.xml
>> index 704fee9..e09290c 100644
>> --- a/python/libvirt-override-api.xml
>> +++ b/python/libvirt-override-api.xml
>> @@ -248,6 +248,19 @@
>>
>>
>>
>> +
>> +Change the NUMA tunables
>> +
>> +
>> +
A copy-paste error, s/memory/numa/.
>> +
>> +
>> +
>> +Get the NUMA parameters
>> +
Save as above, s/I/O/numa/.
>> +
>> +
>> +
>>
>> list the storage pools, stores the pointers to the names in
>> @names
>>
>> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
>> index d2aad0f..27ad1d8 100644
>> --- a/python/libvirt-override.c
>> +++ b/python/libvirt-override.c
>> @@ -1000,6 +1000,190 @@ libvirt_virDomainGetMemoryParameters(PyObject
>> *self ATTRIBUTE_UNUSED,
>> }
>>
>> static PyObject *
>> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
>> + PyObject *args) {
>> +virDomainPtr domain;
>> +PyObject *pyobj_domain, *info;
>> +int i_retval;
>> +int nparams = 0, i;
>> +unsigned int flags;
>> +virTypedParameterPtr params;
>> +
>> +if (!PyArg_ParseTuple(args,
>> + (char *)"OOi:virDomainSetNumaParameters",
>> +&pyobj_domain,&info,&flags))
>> +return(NULL);
>> +domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
>> +
>> +LIBVIRT_BEGIN_ALLOW_THREADS;
>> +i_retval = virDomainGetNumaParameters(domain, NULL,&nparams,
>> flags);
>> +LIBVIRT_END_ALLOW_THREADS;
>> +
>> +if (i_retval< 0)
>> +return VIR_PY_INT_FAIL;
>> +
>> +if ((params = malloc(sizeof(*params)*nparams)) == NULL)
>> +return VIR_PY_INT_FAIL;
>> +
>> +LIBVIRT_BEGIN_ALLOW_THREADS;
>> +i_retval = virDomainGetNumaParameters(domain, params,&nparams,
>> flags);
>> +LIBVIRT_END_ALLOW_THREADS;
>> +
>> +if (i_retval< 0) {
>> +free(params);
>> +return VIR_PY_INT_FAIL;
>> +}
>> +
>> +/* convert to a Python tuple of long objects */
>> +for (i = 0; i< nparams; i++) {
>> +PyObject *key, *val;
>> +key = libvirt_constcharPtrWrap(params[i].field);
>> +val = PyDict_GetItem(info, key);
>> +Py_DECREF(key);
>> +
>> +if (val == NULL)
>> +continue;
>> +
>> +switch (params[i].type) {
>> +case VIR_TYPED_PARAM_INT:
>> +params[i].value.i = (int)PyInt_AS_LONG(val);
>> +break;
>> +
>> +case VIR_TYPED_PARAM_UINT:
>> +params[i].value.ui = (unsigned int)PyInt_AS_LONG(val);
>> +break;
>> +
>> +case VIR_TYPED_PARAM_LLONG:
>> +params[i].value.l = (long long)PyLong_AsLongLong(val);
>> +break;
>> +
>> +case VIR_TYPED_PARAM_ULLONG:
>> +params[i].value.ul = (unsigned long
>> long)PyLong_AsLongLong(val);
>> +break;
>> +
>> +case VIR_TYPED_PARAM_DOUBLE:
>> +params[i].value.d = (double)PyFloat_AsDouble(val);
>> +break;
>> +
>> +case VIR_TYPED_PARAM_BOOLEAN:
>> +{
>> +/* Hack - Python's definition of Py_True breaks strict
>
Re: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs
Hi
Anybody could give a hand on reviewing this patch,
I appreciate it.
Chinese New Year is coming, best wishes to this community :)
Guannan Ren
- Original Message -
From: "Guannan Ren"
To: libvir-list@redhat.com
Sent: Monday, January 16, 2012 6:58:06 PM
Subject: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs
*virDomainSetNumaParameters
*virDomainGetNumaParameters
---
python/libvirt-override-api.xml | 13 +++
python/libvirt-override.c | 186 +++
2 files changed, 199 insertions(+), 0 deletions(-)
diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml
index 704fee9..e09290c 100644
--- a/python/libvirt-override-api.xml
+++ b/python/libvirt-override-api.xml
@@ -248,6 +248,19 @@
+
+ Change the NUMA tunables
+
+
+
+
+
+
+ Get the NUMA parameters
+
+
+
+
list the storage pools, stores the pointers to the names in
@names
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index d2aad0f..27ad1d8 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1000,6 +1000,190 @@ libvirt_virDomainGetMemoryParameters(PyObject *self
ATTRIBUTE_UNUSED,
}
static PyObject *
+libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
+ PyObject *args) {
+virDomainPtr domain;
+PyObject *pyobj_domain, *info;
+int i_retval;
+int nparams = 0, i;
+unsigned int flags;
+virTypedParameterPtr params;
+
+if (!PyArg_ParseTuple(args,
+ (char *)"OOi:virDomainSetNumaParameters",
+ &pyobj_domain, &info, &flags))
+return(NULL);
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (i_retval < 0)
+return VIR_PY_INT_FAIL;
+
+if ((params = malloc(sizeof(*params)*nparams)) == NULL)
+return VIR_PY_INT_FAIL;
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (i_retval < 0) {
+free(params);
+return VIR_PY_INT_FAIL;
+}
+
+/* convert to a Python tuple of long objects */
+for (i = 0; i < nparams; i++) {
+PyObject *key, *val;
+key = libvirt_constcharPtrWrap(params[i].field);
+val = PyDict_GetItem(info, key);
+Py_DECREF(key);
+
+if (val == NULL)
+continue;
+
+switch (params[i].type) {
+case VIR_TYPED_PARAM_INT:
+params[i].value.i = (int)PyInt_AS_LONG(val);
+break;
+
+case VIR_TYPED_PARAM_UINT:
+params[i].value.ui = (unsigned int)PyInt_AS_LONG(val);
+break;
+
+case VIR_TYPED_PARAM_LLONG:
+params[i].value.l = (long long)PyLong_AsLongLong(val);
+break;
+
+case VIR_TYPED_PARAM_ULLONG:
+params[i].value.ul = (unsigned long long)PyLong_AsLongLong(val);
+break;
+
+case VIR_TYPED_PARAM_DOUBLE:
+params[i].value.d = (double)PyFloat_AsDouble(val);
+break;
+
+case VIR_TYPED_PARAM_BOOLEAN:
+{
+/* Hack - Python's definition of Py_True breaks strict
+ * aliasing rules, so can't directly compare
+ */
+PyObject *hacktrue = PyBool_FromLong(1);
+params[i].value.b = hacktrue == val ? 1: 0;
+Py_DECREF(hacktrue);
+}
+break;
+
+case VIR_TYPED_PARAM_STRING:
+params[i].value.s = PyString_AsString(val);
+break;
+
+default:
+free(params);
+return VIR_PY_INT_FAIL;
+}
+}
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainSetNumaParameters(domain, params, nparams, flags);
+LIBVIRT_END_ALLOW_THREADS;
+if (i_retval < 0) {
+free(params);
+return VIR_PY_INT_FAIL;
+}
+
+free(params);
+return VIR_PY_INT_SUCCESS;
+}
+
+static PyObject *
+libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
+ PyObject *args) {
+virDomainPtr domain;
+PyObject *pyobj_domain, *info;
+int i_retval;
+int nparams = 0, i;
+unsigned int flags;
+virTypedParameterPtr params;
+
+if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainGetNumaParameters",
+ &pyobj_domain, &flags))
+return(NULL);
+domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (i_retval < 0)
+return VIR_P
