Re: [libvirt] [PATCH] udev: Don't let strtoul parse USB busnum and devnum as octal
2010/2/2 Dave Allan dal...@redhat.com: On 02/01/2010 06:10 PM, Matthias Bolte wrote: udevGetUintProperty was called with base set to 0 for busnum and devnum. With base 0 strtoul parses the number as octal if it start with a 0. But busnum and devnum are decimal and udev returns them padded with leading zeros. So strtoul parses them as octal. This works for certain decimal values like 001-007, but fails for values like 008. This is a good change, but I thought it was already fixed a couple of weeks ago. You're sure they're returned decimal and not hex? ACK assuming that's the case. vendor, device and ID_MODEL_ID where changed to be parsed as hex some weeks ago. I based my assumption that busnum and devnum are decimal on the lsusb manpage, because the udev driver output matches the lsusb output. The manpage says busnum and devnum are decimal and it also says that vendor is hex. I just found a libvirt bugreport [1] that confirms my assumption. Also googling for 'lsusb bus X' with X as decimal and hex numbers gives no result with hex numbers like 00A, but gives results for decimal numbers like 010, 011, etc. So I'm pretty sure that busnum and devnum are decimal :) Okay, I pushed this one. [1] https://bugzilla.redhat.com/show_bug.cgi?id=508645 Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity reports this:
Event negative_return_fn: Called negative-returning function
lseek(logfile, 0L, 2)
Event var_assign: NEGATIVE return value of lseek assigned to signed
variable pos
At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path
2877if ((pos = lseek(logfile, 0, SEEK_END)) 0)
2878VIR_WARN(_(Unable to seek to end of logfile: %s),
2879 virStrerror(errno, ebuf, sizeof ebuf));
since later in that same function, a negative pos may
be used like this:
Event negative_returns: Tracked variable pos was passed to a negative
sink. [details]
2930if (qemudWaitForMonitor(conn, driver, vm, pos) 0)
2931goto abort;
2932
which is a legitimate problem, since
qemudWaitForMonitor calls qemudLogReadFD, which calls lseek
with that same pos value:
Event neg_sink_parm_call: Parameter pos passed to negative sink lseek
560 if (lseek(fd, pos, SEEK_SET) 0) {
561 virReportSystemError(conn, errno,
562 _(Unable to seek to %lld in %s),
563 (long long) pos, logfile);
564 close(fd);
565 }
One approach is to detect the negative offset in that final bit
of code and skip the lseek:
From 0ef617935462c314ed0b44bcaa3dd5bf58ccbc1b Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 1 Feb 2010 22:17:44 +0100
Subject: [PATCH] avoid a probable EINVAL from lseek
* src/qemu/qemu_driver.c (qemudLogReadFD): Don't pass a negative
offset (from a preceding failed attempt to seek to EOF) to this use
of lseek.
---
src/qemu/qemu_driver.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22593bf..676a27b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir,
const char* name, off_t p
close(fd);
return -1;
}
-if (lseek(fd, pos, SEEK_SET) 0) {
-virReportSystemError(conn, errno,
+if (pos 0 || lseek(fd, pos, SEEK_SET) 0) {
+ virReportSystemError(conn, pos 0 ? 0 : errno,
_(Unable to seek to %lld in %s),
(long long) pos, logfile);
close(fd);
I was wondering if it wasn't simpler to abort earlier on when
pos 0 was returned from lseek, but after rereading the code I
agree with your patch,
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] Introduce internal QEMU monitor APIs for drive + device hotadd
2010/2/1 Daniel P. Berrange berra...@redhat.com:
The way QEMU is started has been changed to use '-device' and
the new style '-drive' syntax. This needs to be mirrored in
the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs
qemuMonitorAddDevice() and qemuMonitorAddDrive()
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h:
Implement the new monitor APIs
---
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 380bcdc..b2a0c53 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2029,3 +2029,80 @@ error:
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
+ const char *drivestr)
+{
+ char *cmd = NULL;
+ char *reply = NULL;
+ int ret = -1;
+ char *safe_str;
+
+ safe_str = qemuMonitorEscapeArg(drivestr);
+ if (!safe_str) {
+ virReportOOMError(NULL);
+ return -1;
+ }
+
+ ret = virAsprintf(cmd, drive_add dummy %s, safe_str);
dummy looks like a leftover from debugging to me, or does it belong there?
+ if (ret == -1) {
+ virReportOOMError(NULL);
+ goto cleanup;
+ }
+
+ if (qemuMonitorCommand(mon, cmd, reply) 0) {
+ qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+ _(failed to close fd in qemu with '%s'), cmd);
+ goto cleanup;
+ }
+
+ ret = 0;
+
+cleanup:
+ VIR_FREE(cmd);
+ VIR_FREE(reply);
+ VIR_FREE(safe_str);
+ return ret;
+}
+
ACK
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] Make hotplug use new device_add where possible
2010/2/1 Daniel P. Berrange berra...@redhat.com: Since QEMU startup uses the new -device argument, the hotplug code needs todo the same. This converts disk, network and host device hotplug to use the device_add command * src/qemu/qemu_driver.c: Use new device_add monitor APIs whereever possible --- ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/13] Introduce generic virDomainDeviceInfo iterator function
2010/2/1 Daniel P. Berrange berra...@redhat.com: The virDomainDeviceInfoIterate() function will provide a convenient way to iterate over all devices in a domain. * src/conf/domain_conf.c, src/conf/domain_conf.h, src/libvirt_private.syms: Add virDomainDeviceInfoIterate() function. --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/13] Standard internal API syntax for building QEMU command line arguments
2010/2/1 Daniel P. Berrange berra...@redhat.com: All the helper functions for building command line arguments now return a 'char *', instead of acepting a 'char **' or virBufferPtr argument * qemu/qemu_conf.c: Standardize syntax for building args * qemu/qemu_conf.h: Export all functions for building args * qemu/qemu_driver.c: Update for changed syntax for building NIC/hostnet args --- diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 2530813..b6f128f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -188,23 +188,51 @@ int qemudBuildCommandLine (virConnectPtr conn, int *ntapfds, const char *migrateFrom); -int qemuBuildHostNetStr (virConnectPtr conn, - virDomainNetDefPtr net, - char type_sep, - int vlan, - const char *tapfd, - char **str); +/* Legacy, pre device support */ +char * qemuBuildHostNetStr(virConnectPtr conn, + virDomainNetDefPtr net, + char type_sep, + int vlan, + const char *tapfd); -int qemuBuildNicStr (virConnectPtr conn, - virDomainNetDefPtr net, - const char *prefix, - int vlan, - char **str); +/* Current, best practice */ +char * qemuBuildNetDevStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *tapfd); + + +/* Legacy, pre device support */ +char * qemuBuildNicStr(virConnectPtr conn, + virDomainNetDefPtr net, + const char *prefix, + int vlan); + +/* Current, best practice */ +char * qemuBuildNicDevStr(virDomainNetDefPtr net); + +/* Both legacy current support support */ support support? +char *qemuBuildDriveStr(virDomainDiskDefPtr disk, + int bootable, + int qemuCmdFlags); + ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/13] Split out QEMU code for building PCI/USB hostdev arg values
2010/2/1 Daniel P. Berrange berra...@redhat.com: To allow for better code reuse from hotplug methods, the code for generating PCI/USB hostdev arg values is split out into separate methods * qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr and qemuBuildUSBHostdevDevStr --- ACK. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/13] Rewrite way QEMU PCI addresses are allocated
2010/2/1 Daniel P. Berrange berra...@redhat.com:
The current QEMU code allocates PCI addresses incrementally starting
at 4. This is not satisfactory because the user may have given some
addresses in their XML config, which need to be skipped over when
allocating addresses to remaining devices.
It is thus neccessary to maintain a list of already allocated PCI
addresses and then only allocate ones that remain unused. This is
also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating
list of existing PCI addresses, and allocating new addresses.
Refactor address assignment to use this code
* src/qemu/qemu_driver.c: Pull PCI address assignment up into the
qemuStartVMDaemon() method, as a prelude to moving it into the
'define' method. Update list of allocated addresses when connecting
to a running VM at daemon startup.
* tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c,
tests/qemuxml2xmltest.c: Remove USB product test since all
passthrough is done based on address
* tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args,
tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil
unused data files
---
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bbf49fd..9bfd181 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
+
+void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
+{
+ if (!addrs)
+ return;
+
+ virHashFree(addrs-used, qemuDomainPCIAddressSetFreeEntry);
Maybe add addrs-used = NULL after virHashFree to prevent potential
double frees or segfaults due to the dangling pointer.
ACK
Matthias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/13] Remove use of -netdev arg with QEMU
2010/2/1 Daniel P. Berrange berra...@redhat.com: The QEMU 0.12.x tree has the -netdev command line argument, but not corresponding monitor command. We can't enable the former, without the latter since it will break hotplug/unplug. * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Disable -netdev usage until 0.13 at earliest * tests/qemuxml2argvtest.c: Add test for -netdev syntax * tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml: Test data files for -netdev syntax --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/13] Assign PCI addresses before hotplugging devices
2010/2/1 Daniel P. Berrange berra...@redhat.com: PCI disk, disk controllers, net devices and host devices need to have PCI addresses assigned before they are hot-plugged * src/qemu/qemu_conf.c: Add APIs for ensuring a device has an address and releasing unused addresses * src/qemu/qemu_driver.c: Ensure all devices have addresses when hotplugging. --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/13] Disable QEMU monitor IO debugging by default
2010/2/1 Daniel P. Berrange berra...@redhat.com: --- src/qemu/qemu_monitor_text.c | 16 +++- 1 files changed, 15 insertions(+), 1 deletions(-) ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/13] Remove direct storage of hostnet_name vlan
2010/2/1 Daniel P. Berrange berra...@redhat.com: The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged * src/conf/domain_conf.c, src/conf/domain_conf.h: Remove hostnet_name and vlan fields from virNetworkDefPtr * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h, src/qemu/qemu_driver.c: Use a single network alias naming scheme regardless of NIC type or backend type. Determine VLANs from the alias name. * tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-device.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args: Update for new simpler naming scheme --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/13] Tweak USB hostdevice XML handling
2010/2/1 Daniel P. Berrange berra...@redhat.com: When attaching a USB host device based on vendor/product, libvirt will resolve the vendor/product into a device/bus pair. This means that when printing XML we should allow device/bus info to be printed at any time if present * src/conf/domain_conf.c, docs/schemas/domain.rng: Allow USB device bus info alongside vendor/product --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/13] Re-arrange QEMU device alias assignment code
2010/2/1 Daniel P. Berrange berra...@redhat.com: This patch re-arranges the QEMU device alias assignment code to make it easier to call into the same codeblock when performing device hotplug. The new code has the ability to skip over already assigned names to facilitate hotplug * src/qemu/qemu_driver.c: Call qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_conf.h: Export qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_driver.c: Merge the legacy disk/network alias assignment code into the main methods --- ACK Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity reports this:
Event negative_return_fn: Called negative-returning function
lseek(logfile, 0L, 2)
Event var_assign: NEGATIVE return value of lseek assigned to signed
variable pos
At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path
2877if ((pos = lseek(logfile, 0, SEEK_END)) 0)
2878VIR_WARN(_(Unable to seek to end of logfile: %s),
2879 virStrerror(errno, ebuf, sizeof ebuf));
I think it'd be less surprising to just set 'pos = 0' inside the if
branch here, so later code doesn't have to worry about unexpected
negative values.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22593bf..676a27b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir,
const char* name, off_t p
close(fd);
return -1;
}
-if (lseek(fd, pos, SEEK_SET) 0) {
-virReportSystemError(conn, errno,
+if (pos 0 || lseek(fd, pos, SEEK_SET) 0) {
+ virReportSystemError(conn, pos 0 ? 0 : errno,
_(Unable to seek to %lld in %s),
(long long) pos, logfile);
close(fd);
--
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity reports this:
Event negative_return_fn: Called negative-returning function
lseek(logfile, 0L, 2)
Event var_assign: NEGATIVE return value of lseek assigned to signed
variable pos
At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path
2877 if ((pos = lseek(logfile, 0, SEEK_END)) 0)
2878 VIR_WARN(_(Unable to seek to end of logfile: %s),
2879virStrerror(errno, ebuf, sizeof ebuf));
since later in that same function, a negative pos may
be used like this:
Event negative_returns: Tracked variable pos was passed to a negative
sink. [details]
2930 if (qemudWaitForMonitor(conn, driver, vm, pos) 0)
2931 goto abort;
2932
which is a legitimate problem, since
qemudWaitForMonitor calls qemudLogReadFD, which calls lseek
with that same pos value:
Event neg_sink_parm_call: Parameter pos passed to negative sink lseek
560if (lseek(fd, pos, SEEK_SET) 0) {
561virReportSystemError(conn, errno,
562 _(Unable to seek to %lld in %s),
563 (long long) pos, logfile);
564close(fd);
565}
One approach is to detect the negative offset in that final bit
of code and skip the lseek:
From 0ef617935462c314ed0b44bcaa3dd5bf58ccbc1b Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 1 Feb 2010 22:17:44 +0100
Subject: [PATCH] avoid a probable EINVAL from lseek
* src/qemu/qemu_driver.c (qemudLogReadFD): Don't pass a negative
offset (from a preceding failed attempt to seek to EOF) to this use
of lseek.
---
src/qemu/qemu_driver.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22593bf..676a27b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir,
const char* name, off_t p
close(fd);
return -1;
}
-if (lseek(fd, pos, SEEK_SET) 0) {
-virReportSystemError(conn, errno,
+if (pos 0 || lseek(fd, pos, SEEK_SET) 0) {
+ virReportSystemError(conn, pos 0 ? 0 : errno,
_(Unable to seek to %lld in %s),
(long long) pos, logfile);
close(fd);
I was wondering if it wasn't simpler to abort earlier on when
pos 0 was returned from lseek, but after rereading the code I
agree with your patch,
Returning early (failing) after the initial log-lseek failure
did not seem justified, considering other log-related failures
merely get a warning there.
Pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
Daniel P. Berrange wrote:
On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity reports this:
Event negative_return_fn: Called negative-returning function
lseek(logfile, 0L, 2)
Event var_assign: NEGATIVE return value of lseek assigned to signed
variable pos
At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path
2877 if ((pos = lseek(logfile, 0, SEEK_END)) 0)
2878 VIR_WARN(_(Unable to seek to end of logfile: %s),
2879virStrerror(errno, ebuf, sizeof ebuf));
I think it'd be less surprising to just set 'pos = 0' inside the if
branch here, so later code doesn't have to worry about unexpected
negative values.
Oh. I pushed after DV's ACK.
That would let the later code continue, but using (lseek'ing to) an
invalid position. Sounds like it could result in a cascade of
additional errors, or worse, silent malfunction.
But this is largely hypothetical, since failing to lseek-to-EOF
on a valid file descriptor is not likely to happen.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22593bf..676a27b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir,
const char* name, off_t p
close(fd);
return -1;
}
-if (lseek(fd, pos, SEEK_SET) 0) {
-virReportSystemError(conn, errno,
+if (pos 0 || lseek(fd, pos, SEEK_SET) 0) {
+ virReportSystemError(conn, pos 0 ? 0 : errno,
_(Unable to seek to %lld in %s),
(long long) pos, logfile);
close(fd);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
On Tue, Feb 02, 2010 at 10:37:32AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote:
In src/qemu/qemu_driver.c, coverity reports this:
Event negative_return_fn: Called negative-returning function
lseek(logfile, 0L, 2)
Event var_assign: NEGATIVE return value of lseek assigned to signed
variable pos
At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path
2877 if ((pos = lseek(logfile, 0, SEEK_END)) 0)
2878 VIR_WARN(_(Unable to seek to end of logfile: %s),
2879 virStrerror(errno, ebuf, sizeof ebuf));
I think it'd be less surprising to just set 'pos = 0' inside the if
branch here, so later code doesn't have to worry about unexpected
negative values.
Oh. I pushed after DV's ACK.
That would let the later code continue, but using (lseek'ing to) an
invalid position. Sounds like it could result in a cascade of
additional errors, or worse, silent malfunction.
The code which later uses this 'pos' variable, does so in order to skip
over the start of the logfile which it does not want. So by setting pos=0
we make it start at the beginning of the file instead.
But this is largely hypothetical, since failing to lseek-to-EOF
on a valid file descriptor is not likely to happen.
My reasoning is that putting the 'pos 0' check in another separate
method far away from where the error occurs is rather obscure, and
the kind of code that will likely be deleted by mistake in later
refactoring.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 22593bf..676a27b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -558,8 +558,8 @@ qemudLogReadFD(virConnectPtr conn, const char* logDir,
const char* name, off_t p
close(fd);
return -1;
}
-if (lseek(fd, pos, SEEK_SET) 0) {
-virReportSystemError(conn, errno,
+if (pos 0 || lseek(fd, pos, SEEK_SET) 0) {
+ virReportSystemError(conn, pos 0 ? 0 : errno,
_(Unable to seek to %lld in %s),
(long long) pos, logfile);
close(fd);
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
Daniel P. Berrange wrote: On Tue, Feb 02, 2010 at 10:37:32AM +0100, Jim Meyering wrote: Daniel P. Berrange wrote: On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote: In src/qemu/qemu_driver.c, coverity reports this: Event negative_return_fn: Called negative-returning function lseek(logfile, 0L, 2) Event var_assign: NEGATIVE return value of lseek assigned to signed variable pos At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path 2877if ((pos = lseek(logfile, 0, SEEK_END)) 0) 2878VIR_WARN(_(Unable to seek to end of logfile: %s), 2879 virStrerror(errno, ebuf, sizeof ebuf)); I think it'd be less surprising to just set 'pos = 0' inside the if branch here, so later code doesn't have to worry about unexpected negative values. Oh. I pushed after DV's ACK. That would let the later code continue, but using (lseek'ing to) an invalid position. Sounds like it could result in a cascade of additional errors, or worse, silent malfunction. The code which later uses this 'pos' variable, does so in order to skip over the start of the logfile which it does not want. So by setting pos=0 we make it start at the beginning of the file instead. Wouldn't that result in reprocessing (misleadingly) past diagnostics? But this is largely hypothetical, since failing to lseek-to-EOF on a valid file descriptor is not likely to happen. My reasoning is that putting the 'pos 0' check in another separate method far away from where the error occurs is rather obscure, and the kind of code that will likely be deleted by mistake in later refactoring. I agree that this action-at-a-distance is bad. I can add a comment pointing to this discussion. Or,... considering that this type of failure is so unlikely, would you prefer to make the initial lseek evoke failure rather than just a warning? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] avoid a probable EINVAL from lseek
On Tue, Feb 02, 2010 at 11:01:55AM +0100, Jim Meyering wrote: Daniel P. Berrange wrote: On Tue, Feb 02, 2010 at 10:37:32AM +0100, Jim Meyering wrote: Daniel P. Berrange wrote: On Mon, Feb 01, 2010 at 10:18:27PM +0100, Jim Meyering wrote: In src/qemu/qemu_driver.c, coverity reports this: Event negative_return_fn: Called negative-returning function lseek(logfile, 0L, 2) Event var_assign: NEGATIVE return value of lseek assigned to signed variable pos At conditional (1): (pos = lseek(logfile, 0L, 2)) 0 taking true path 2877 if ((pos = lseek(logfile, 0, SEEK_END)) 0) 2878 VIR_WARN(_(Unable to seek to end of logfile: %s), 2879 virStrerror(errno, ebuf, sizeof ebuf)); I think it'd be less surprising to just set 'pos = 0' inside the if branch here, so later code doesn't have to worry about unexpected negative values. Oh. I pushed after DV's ACK. That would let the later code continue, but using (lseek'ing to) an invalid position. Sounds like it could result in a cascade of additional errors, or worse, silent malfunction. The code which later uses this 'pos' variable, does so in order to skip over the start of the logfile which it does not want. So by setting pos=0 we make it start at the beginning of the file instead. Wouldn't that result in reprocessing (misleadingly) past diagnostics? No, all that its doing is skipping over the ARGV that were written to the start of the file, to make future error reporting slightly cleaner. But this is largely hypothetical, since failing to lseek-to-EOF on a valid file descriptor is not likely to happen. My reasoning is that putting the 'pos 0' check in another separate method far away from where the error occurs is rather obscure, and the kind of code that will likely be deleted by mistake in later refactoring. I agree that this action-at-a-distance is bad. I can add a comment pointing to this discussion. Or,... considering that this type of failure is so unlikely, would you prefer to make the initial lseek evoke failure rather than just a warning? I'd really rather we just reset pos to 0 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] JAVA .start() method
Good morning, Testing the new java version (0.4.1) I've noticed that there's no .start() method to start a defined or shuted down domain. Are you working on it or is it a minor feature not to be considered? Thanks in advance, Marc Gonzalez Mateo ma...@ac.upc.edu Dept. Arquitectura de Computadors Universitat Politècnica de Catalunya -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] storage_backend.c: avoid closing a negative file descriptor
This close(fd) is reachable with an fd of -1 via
the goto cleanup just before fd is first set.
While closing(-1) is not a big problem, it is a failing
syscall, and would show up on an strace audit, not to mention
the coverity and maybe-clang warnings.
From c69369c445be53f12ec09a176fd477b9ff16bbff Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 11:11:49 +0100
Subject: [PATCH] storage_backend.c: avoid closing a negative file descriptor
* src/storage/storage_backend.c (virStorageBackendRunProgRegex):
Don't close a negative (read-only) file descriptor.
---
src/storage/storage_backend.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index bc656f2..84eb8aa 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2009 Red Hat, Inc.
+ * Copyright (C) 2007-2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -1326,8 +1326,10 @@ virStorageBackendRunProgRegex(virConnectPtr conn,
if (list)
fclose(list);
-else
-close(fd);
+else {
+if (0 = fd)
+close(fd);
+}
while ((err = waitpid(child, exitstatus, 0) == -1) errno == EINTR);
--
1.7.0.rc1.149.g0b0b7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libvirtd.c: avoid closing a negative socket file descriptor
A lot like the last one... From e32826290b960790b0ec4f50b195f424fa42348f Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 2 Feb 2010 11:27:25 +0100 Subject: [PATCH] libvirtd.c: avoid closing a negative socket file descriptor * daemon/libvirtd.c (qemudListenUnix): Close socket only if non-negative. --- daemon/libvirtd.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 3070dfc..f7df0c6 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1,7 +1,7 @@ /* * libvirtd.c: daemon start of day, guest process i/o management * - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006-2010, 2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -582,7 +582,7 @@ static int qemudListenUnix(struct qemud_server *server, return 0; cleanup: -if (sock-fd) +if (0 = sock-fd) close(sock-fd); VIR_FREE(sock); return -1; -- 1.7.0.rc1.149.g0b0b7 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] JAVA .start() method
2010/2/2 Marc Gonzalez Mateo ma...@ac.upc.edu: Good morning, Testing the new java version (0.4.1) I've noticed that there's no .start() method to start a defined or shuted down domain. Are you working on it or is it a minor feature not to be considered? Thanks in advance, Why do you think there should be a method called start? The Java bindings bind the C API, not the virsh commands. The C API function to start a domain is called virDomainCreate. The Java bindings bind this function to the create method of the Domain class. Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend.c: avoid closing a negative file descriptor
On Tue, Feb 02, 2010 at 11:14:39AM +0100, Jim Meyering wrote:
This close(fd) is reachable with an fd of -1 via
the goto cleanup just before fd is first set.
While closing(-1) is not a big problem, it is a failing
syscall, and would show up on an strace audit, not to mention
the coverity and maybe-clang warnings.
From c69369c445be53f12ec09a176fd477b9ff16bbff Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 11:11:49 +0100
Subject: [PATCH] storage_backend.c: avoid closing a negative file descriptor
* src/storage/storage_backend.c (virStorageBackendRunProgRegex):
Don't close a negative (read-only) file descriptor.
---
src/storage/storage_backend.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index bc656f2..84eb8aa 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2009 Red Hat, Inc.
+ * Copyright (C) 2007-2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -1326,8 +1326,10 @@ virStorageBackendRunProgRegex(virConnectPtr conn,
if (list)
fclose(list);
-else
-close(fd);
+else {
+if (0 = fd)
+close(fd);
+}
Can you put conditionals the normal way around with variable being tested
coming first.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libvirtd.c: avoid closing a negative socket file descriptor
On Tue, Feb 02, 2010 at 11:28:07AM +0100, Jim Meyering wrote: A lot like the last one... From e32826290b960790b0ec4f50b195f424fa42348f Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Tue, 2 Feb 2010 11:27:25 +0100 Subject: [PATCH] libvirtd.c: avoid closing a negative socket file descriptor * daemon/libvirtd.c (qemudListenUnix): Close socket only if non-negative. --- daemon/libvirtd.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 3070dfc..f7df0c6 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1,7 +1,7 @@ /* * libvirtd.c: daemon start of day, guest process i/o management * - * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006-2010, 2010 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -582,7 +582,7 @@ static int qemudListenUnix(struct qemud_server *server, return 0; cleanup: -if (sock-fd) +if (0 = sock-fd) close(sock-fd); VIR_FREE(sock); return -1; This conditional is also written the wrong way around 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] lxc_controller.c: don't ignore failed accept
coverity complained (rightly) about the risk of closing a negative
file descriptor. However, the real problem was the missing test
for a failed accept call. I'm not 100% sure that a failed
accept call deserves to provoke a goto cleanup, but doing that
is consistent with what the nearby code does upon epoll_ctl failure.
From 8bfd81f0a8a9cb3fd9b575e9c2f5ab9969a2910f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 11:55:19 +0100
Subject: [PATCH] lxc_controller.c: don't ignore failed accept
* src/lxc/lxc_controller.c (lxcControllerMain): A failed accept could
lead to passing a negative file descriptor to various other functions,
which would in turn report EBADF, rather that whatever error prompted
the initial failure.
---
src/lxc/lxc_controller.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 6304815..682f874 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -349,6 +349,11 @@ static int lxcControllerMain(int monitor,
if (numEvents 0) {
if (epollEvent.data.fd == monitor) {
int fd = accept(monitor, NULL, 0);
+if (fd 0) {
+virReportSystemError(NULL, errno, %s,
+ _(accept(monitor,...) failed));
+goto cleanup;
+}
if (client != -1) { /* Already connected, so kick new one out
*/
close(fd);
continue;
--
1.7.0.rc1.149.g0b0b7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend.c: avoid closing a negative file descriptor
On Tue, Feb 02, 2010 at 11:14:39AM +0100, Jim Meyering wrote:
This close(fd) is reachable with an fd of -1 via
the goto cleanup just before fd is first set.
While closing(-1) is not a big problem, it is a failing
syscall, and would show up on an strace audit, not to mention
the coverity and maybe-clang warnings.
From c69369c445be53f12ec09a176fd477b9ff16bbff Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 11:11:49 +0100
Subject: [PATCH] storage_backend.c: avoid closing a negative file descriptor
* src/storage/storage_backend.c (virStorageBackendRunProgRegex):
Don't close a negative (read-only) file descriptor.
---
src/storage/storage_backend.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index bc656f2..84eb8aa 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2009 Red Hat, Inc.
+ * Copyright (C) 2007-2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -1326,8 +1326,10 @@ virStorageBackendRunProgRegex(virConnectPtr conn,
if (list)
fclose(list);
-else
-close(fd);
+else {
+if (0 = fd)
+close(fd);
+}
while ((err = waitpid(child, exitstatus, 0) == -1) errno == EINTR);
ACK,
tangential, though I know tests like (0 = fd) are safer than (fd = 0) from
a coding and maintainance perspective, I still find that harder to read
and understand.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage_backend.c: avoid closing a negative file descriptor
Daniel P. Berrange wrote:
On Tue, Feb 02, 2010 at 11:14:39AM +0100, Jim Meyering wrote:
This close(fd) is reachable with an fd of -1 via
the goto cleanup just before fd is first set.
While closing(-1) is not a big problem, it is a failing
syscall, and would show up on an strace audit, not to mention
the coverity and maybe-clang warnings.
From c69369c445be53f12ec09a176fd477b9ff16bbff Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 11:11:49 +0100
Subject: [PATCH] storage_backend.c: avoid closing a negative file descriptor
* src/storage/storage_backend.c (virStorageBackendRunProgRegex):
Don't close a negative (read-only) file descriptor.
---
src/storage/storage_backend.c |8 +---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index bc656f2..84eb8aa 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
/*
* storage_backend.c: internal storage driver backend contract
*
- * Copyright (C) 2007-2009 Red Hat, Inc.
+ * Copyright (C) 2007-2010 Red Hat, Inc.
* Copyright (C) 2007-2008 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -1326,8 +1326,10 @@ virStorageBackendRunProgRegex(virConnectPtr conn,
if (list)
fclose(list);
-else
-close(fd);
+else {
+if (0 = fd)
+close(fd);
+}
Can you put conditionals the normal way around with variable being tested
coming first.
Yeah. Forgot.
Normal is context sensitive.
In many other places where I contribute, normal means and =,
but no (or very few) uses of or =. Definitely an acquired habit.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc_controller.c: don't ignore failed accept
On Tue, Feb 02, 2010 at 11:58:44AM +0100, Jim Meyering wrote: coverity complained (rightly) about the risk of closing a negative file descriptor. However, the real problem was the missing test for a failed accept call. I'm not 100% sure that a failed accept call deserves to provoke a goto cleanup, but doing that is consistent with what the nearby code does upon epoll_ctl failure. This isn't correct because the incoming client can quit between the time of poll() indicating its presence, and accept() trying to process it. This is an expected non-fatal scenario, so it should just be ignored without quitting. epoll_ctl() by comparison is a fatal system error, so has to be handled as an unrecoverable error 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc_controller.c: don't ignore failed accept
Daniel P. Berrange wrote: On Tue, Feb 02, 2010 at 11:58:44AM +0100, Jim Meyering wrote: coverity complained (rightly) about the risk of closing a negative file descriptor. However, the real problem was the missing test for a failed accept call. I'm not 100% sure that a failed accept call deserves to provoke a goto cleanup, but doing that is consistent with what the nearby code does upon epoll_ctl failure. This isn't correct because the incoming client can quit between the time of poll() indicating its presence, and accept() trying to process it. This is an expected non-fatal scenario, so it should just be ignored without quitting. epoll_ctl() by comparison is a fatal system error, so has to be handled as an unrecoverable error Ok, so some errno values are ignorable. Do you also want to ignore the likes of EMFILE, ENFILE, EPERM, etc? Otherwise, I propose to enumerate ignorable-errno values and treat any others as unrecoverable. Do you know which errno values should be ignored? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] xs_internal.c: don't use a negative value as allocation size
As the log message says...
From 6d7abfb983328fbe34bd4404d8fafda100e8098f Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 12:22:17 +0100
Subject: [PATCH] xs_internal.c: don't use a negative value as allocation size
* src/xen/xs_internal.c (xenStoreDomainIntroduced): Don't use -1
as an allocation size upon xenStoreNumOfDomains failure.
(xenStoreDomainReleased): Likewise.
---
src/xen/xs_internal.c |7 ++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c
index 1ca3da0..620c482 100644
--- a/src/xen/xs_internal.c
+++ b/src/xen/xs_internal.c
@@ -1,9 +1,9 @@
/*
* xs_internal.c: access to Xen Store
*
- * Copyright (C) 2006, 2009 Red Hat, Inc.
+ * Copyright (C) 2006, 2009-2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
* Daniel Veillard veill...@redhat.com
*/
@@ -1365,10 +1365,13 @@ int xenStoreDomainIntroduced(virConnectPtr conn,
xenUnifiedPrivatePtr priv = opaque;
retry:
new_domain_cnt = xenStoreNumOfDomains(conn);
+if (new_domain_cnt 0)
+return -1;
+
if( VIR_ALLOC_N(new_domids,new_domain_cnt) 0 ) {
virReportOOMError(NULL);
return -1;
}
nread = xenStoreDoListDomains(conn, priv, new_domids, new_domain_cnt);
@@ -1445,10 +1448,12 @@ int xenStoreDomainReleased(virConnectPtr conn,
if(!priv-activeDomainList-count) return 0;
retry:
new_domain_cnt = xenStoreNumOfDomains(conn);
+if (new_domain_cnt 0)
+return -1;
if( VIR_ALLOC_N(new_domids,new_domain_cnt) 0 ) {
virReportOOMError(NULL);
return -1;
}
--
1.7.0.rc1.149.g0b0b7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/13] Standard internal API syntax for building QEMU command line arguments
On Mon, Feb 01, 2010 at 06:39:30PM +, Daniel P. Berrange wrote: All the helper functions for building command line arguments now return a 'char *', instead of acepting a 'char **' or virBufferPtr argument * qemu/qemu_conf.c: Standardize syntax for building args * qemu/qemu_conf.h: Export all functions for building args * qemu/qemu_driver.c: Update for changed syntax for building NIC/hostnet args --- src/qemu/qemu_conf.c | 341 +++- src/qemu/qemu_conf.h | 56 ++-- src/qemu/qemu_driver.c |6 +- 3 files changed, 211 insertions(+), 192 deletions(-) ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/13] Split out QEMU code for building PCI/USB hostdev arg values
On Mon, Feb 01, 2010 at 06:39:31PM +, Daniel P. Berrange wrote: To allow for better code reuse from hotplug methods, the code for generating PCI/USB hostdev arg values is split out into separate methods * qemu/qemu_conf.h, qemu/qemu_conf.c: Introduce new APis for qemuBuildPCIHostdevPCIDevStr, qemuBuildUSBHostdevUsbDevStr and qemuBuildUSBHostdevDevStr --- src/qemu/qemu_conf.c | 105 + src/qemu/qemu_conf.h | 10 + 2 files changed, 81 insertions(+), 34 deletions(-) ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] Introduce internal QEMU monitor APIs for drive + device hotadd
On Mon, Feb 01, 2010 at 06:39:32PM +, Daniel P. Berrange wrote:
The way QEMU is started has been changed to use '-device' and
the new style '-drive' syntax. This needs to be mirrored in
the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs
qemuMonitorAddDevice() and qemuMonitorAddDrive()
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h:
Implement the new monitor APIs
---
[...]
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+int ret;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuMonitorJSONMakeCommand(drive_add,
+ s:pci_addr, dummy,
+ s:opts, drivestr,
+ NULL);
[...]
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+char *cmd = NULL;
+char *reply = NULL;
+int ret = -1;
+char *safe_str;
+
+safe_str = qemuMonitorEscapeArg(drivestr);
+if (!safe_str) {
+virReportOOMError(NULL);
+return -1;
+}
+
+ret = virAsprintf(cmd, drive_add dummy %s, safe_str);
Like Matthias I'm wondering, it seems to be allowed for network and
drive naming, but still a bit surprizing
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/13] Make hotplug use new device_add where possible
On Mon, Feb 01, 2010 at 06:39:33PM +, Daniel P. Berrange wrote:
Since QEMU startup uses the new -device argument, the hotplug
code needs todo the same. This converts disk, network and
host device hotplug to use the device_add command
* src/qemu/qemu_driver.c: Use new device_add monitor APIs
whereever possible
---
[...]
+if (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) {
+ret = qemuMonitorAddDrive(priv-mon, drivestr);
+if (ret == 0)
+qemuMonitorAddDevice(priv-mon, devstr);
+/* XXX remove the drive upon fail */
Hum, maybe this small TODO should be added, hopefully it's in a later
patch
[...]
+if (ret == 0)
+ret = qemuMonitorAddDevice(priv-mon,
+ devstr);
+/* XXX should call 'drive_del' on error but this does not exist
yet */
ah, that's why ...
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] Introduce internal QEMU monitor APIs for drive + device hotadd
On Tue, Feb 02, 2010 at 03:15:18PM +0100, Daniel Veillard wrote:
On Mon, Feb 01, 2010 at 06:39:32PM +, Daniel P. Berrange wrote:
The way QEMU is started has been changed to use '-device' and
the new style '-drive' syntax. This needs to be mirrored in
the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs
qemuMonitorAddDevice() and qemuMonitorAddDrive()
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h:
Implement the new monitor APIs
---
[...]
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+int ret;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuMonitorJSONMakeCommand(drive_add,
+ s:pci_addr, dummy,
+ s:opts, drivestr,
+ NULL);
[...]
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
+const char *drivestr)
+{
+char *cmd = NULL;
+char *reply = NULL;
+int ret = -1;
+char *safe_str;
+
+safe_str = qemuMonitorEscapeArg(drivestr);
+if (!safe_str) {
+virReportOOMError(NULL);
+return -1;
+}
+
+ret = virAsprintf(cmd, drive_add dummy %s, safe_str);
Like Matthias I'm wondering, it seems to be allowed for network and
drive naming, but still a bit surprizing
This is a really bizarre bit of QEMU :-) Normally you would put a
PCI address in that part of the command. In this cae though, we're
not adding a PCI device, but rather adding a disk on a drive
controller, therefore there is no relevant PCI address we put
in the placeholder 'dummy'.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/13] Rewrite way QEMU PCI addresses are allocated
On Mon, Feb 01, 2010 at 06:39:35PM +, Daniel P. Berrange wrote:
The current QEMU code allocates PCI addresses incrementally starting
at 4. This is not satisfactory because the user may have given some
addresses in their XML config, which need to be skipped over when
allocating addresses to remaining devices.
Ah, right !
It is thus neccessary to maintain a list of already allocated PCI
addresses and then only allocate ones that remain unused. This is
also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating
list of existing PCI addresses, and allocating new addresses.
Refactor address assignment to use this code
* src/qemu/qemu_driver.c: Pull PCI address assignment up into the
qemuStartVMDaemon() method, as a prelude to moving it into the
'define' method. Update list of allocated addresses when connecting
to a running VM at daemon startup.
* tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c,
tests/qemuxml2xmltest.c: Remove USB product test since all
passthrough is done based on address
* tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args,
tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil
unused data files
[...]
+addr = qemuPCIAddressAsString(dev);
+if (!addr)
+return -1;
+
+if (virHashLookup(addrs-used, addr)) {
+qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _(unable to reserve PCI address %s), addr);
+VIR_FREE(addr);
+return -1;
+}
+
+if (virHashAddEntry(addrs-used, addr, addr)) {
+VIR_FREE(addr);
+return -1;
+}
is using a hash table really that simpler in that case ? I doubt we
need an optimized lookup, and this mean always saving to a standardized
string to then do string comparisons instead of directly checking
domain/bus/slot values. But that's a minor point.
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] udev: Don't let strtoul parse USB busnum and devnum as octal
On 02/02/2010 03:52 AM, Matthias Bolte wrote: 2010/2/2 Dave Allandal...@redhat.com: On 02/01/2010 06:10 PM, Matthias Bolte wrote: udevGetUintProperty was called with base set to 0 for busnum and devnum. With base 0 strtoul parses the number as octal if it start with a 0. But busnum and devnum are decimal and udev returns them padded with leading zeros. So strtoul parses them as octal. This works for certain decimal values like 001-007, but fails for values like 008. This is a good change, but I thought it was already fixed a couple of weeks ago. You're sure they're returned decimal and not hex? ACK assuming that's the case. vendor, device and ID_MODEL_ID where changed to be parsed as hex some weeks ago. I based my assumption that busnum and devnum are decimal on the lsusb manpage, because the udev driver output matches the lsusb output. The manpage says busnum and devnum are decimal and it also says that vendor is hex. I just found a libvirt bugreport [1] that confirms my assumption. Also googling for 'lsusb bus X' with X as decimal and hex numbers gives no result with hex numbers like 00A, but gives results for decimal numbers like 010, 011, etc. So I'm pretty sure that busnum and devnum are decimal :) Excellent--many thanks! Dave Okay, I pushed this one. [1] https://bugzilla.redhat.com/show_bug.cgi?id=508645 Matthias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/13] Introduce generic virDomainDeviceInfo iterator function
On Mon, Feb 01, 2010 at 06:39:34PM +, Daniel P. Berrange wrote:
The virDomainDeviceInfoIterate() function will provide a
convenient way to iterate over all devices in a domain.
* src/conf/domain_conf.c, src/conf/domain_conf.h,
src/libvirt_private.syms: Add virDomainDeviceInfoIterate()
function.
---
src/conf/domain_conf.c | 67
+++---
src/conf/domain_conf.h |8 +
src/libvirt_private.syms |1 +
3 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e548d1d..691fc84 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -824,59 +824,82 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr
info)
}
-static void virDomainDeviceInfoClearField(virDomainDeviceInfoPtr info, int
alias, int pciaddr)
+static int virDomainDeviceInfoClearAlias(virDomainDefPtr def
ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque ATTRIBUTE_UNUSED)
{
-if (alias)
-VIR_FREE(info-alias);
-if (pciaddr
-info-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+VIR_FREE(info-alias);
+return 0;
+}
+
+static int virDomainDeviceInfoClearPCIAddress(virDomainDefPtr def
ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+if (info-type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
memset(info-addr, 0, sizeof(info-addr));
info-type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
}
+return 0;
}
-
-static void virDomainDefClearDeviceInfo(virDomainDefPtr def, int alias, int
pciaddr)
+int virDomainDeviceInfoIterate(virDomainDefPtr def,
+ virDomainDeviceInfoCallback cb,
+ void *opaque)
{
int i;
for (i = 0; i def-ndisks ; i++)
-virDomainDeviceInfoClearField(def-disks[i]-info, alias, pciaddr);
+if (cb(def, def-disks[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nnets ; i++)
-virDomainDeviceInfoClearField(def-nets[i]-info, alias, pciaddr);
+if (cb(def, def-nets[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nsounds ; i++)
-virDomainDeviceInfoClearField(def-sounds[i]-info, alias, pciaddr);
+if (cb(def, def-sounds[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nhostdevs ; i++)
-virDomainDeviceInfoClearField(def-hostdevs[i]-info, alias,
pciaddr);
+if (cb(def, def-hostdevs[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nvideos ; i++)
-virDomainDeviceInfoClearField(def-videos[i]-info, alias, pciaddr);
+if (cb(def, def-videos[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-ncontrollers ; i++)
-virDomainDeviceInfoClearField(def-controllers[i]-info, alias,
pciaddr);
+if (cb(def, def-controllers[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nserials ; i++)
-virDomainDeviceInfoClearField(def-serials[i]-info, alias,
pciaddr);
+if (cb(def, def-serials[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nparallels ; i++)
-virDomainDeviceInfoClearField(def-parallels[i]-info, alias,
pciaddr);
+if (cb(def, def-parallels[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nchannels ; i++)
-virDomainDeviceInfoClearField(def-channels[i]-info, alias,
pciaddr);
+if (cb(def, def-channels[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-ninputs ; i++)
-virDomainDeviceInfoClearField(def-inputs[i]-info, alias, pciaddr);
+if (cb(def, def-inputs[i]-info, opaque) 0)
+return -1;
for (i = 0; i def-nfss ; i++)
-virDomainDeviceInfoClearField(def-fss[i]-info, alias, pciaddr);
+if (cb(def, def-fss[i]-info, opaque) 0)
+return -1;
if (def-watchdog)
-virDomainDeviceInfoClearField(def-watchdog-info, alias, pciaddr);
+if (cb(def, def-watchdog-info, opaque) 0)
+return -1;
if (def-console)
-virDomainDeviceInfoClearField(def-console-info, alias, pciaddr);
+if (cb(def, def-console-info, opaque) 0)
+return -1;
+return 0;
}
void virDomainDefClearPCIAddresses(virDomainDefPtr def)
{
-virDomainDefClearDeviceInfo(def, 0, 1);
+virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress,
NULL);
}
void virDomainDefClearDeviceAliases(virDomainDefPtr def)
{
-virDomainDefClearDeviceInfo(def, 1, 0);
+virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
}
diff --git
Re: [libvirt] [PATCH 07/13] Assign PCI addresses before hotplugging devices
On Mon, Feb 01, 2010 at 06:39:36PM +, Daniel P. Berrange wrote: PCI disk, disk controllers, net devices and host devices need to have PCI addresses assigned before they are hot-plugged * src/qemu/qemu_conf.c: Add APIs for ensuring a device has an address and releasing unused addresses * src/qemu/qemu_driver.c: Ensure all devices have addresses when hotplugging. --- src/qemu/qemu_conf.c | 60 ++- src/qemu/qemu_conf.h | 11 +++- src/qemu/qemu_driver.c | 43 -- 3 files changed, 97 insertions(+), 17 deletions(-) [...] cleanup: +if ((ret != 0) +(qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) +(net-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) +qemuDomainPCIAddressReleaseAddr(priv-pciaddrs, net-info) 0) +VIR_WARN0(Unable to release PCI address on NIC); + Wondering to which extend (qemuCmdFlags QEMUD_CMD_FLAG_DEVICE) check shouldn't be handled down in allocation and releasing routines instead of before each invocation, but it's minor. ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/13] Remove use of -netdev arg with QEMU
On Mon, Feb 01, 2010 at 06:39:37PM +, Daniel P. Berrange wrote: The QEMU 0.12.x tree has the -netdev command line argument, but not corresponding monitor command. We can't enable the former, without the latter since it will break hotplug/unplug. Argh, nasty * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Disable -netdev usage until 0.13 at earliest * tests/qemuxml2argvtest.c: Add test for -netdev syntax * tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.args, tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev.xml: Test data files for -netdev syntax ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/13] Remove direct storage of hostnet_name vlan
On Mon, Feb 01, 2010 at 06:39:38PM +, Daniel P. Berrange wrote: The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged ACK, this really simplifies the code, but isn't there a small risk of not being able to properly handle domains say after a libvirt upgrade and restart ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt: python bindings
On 29 janv. 2010, at 10:05, Pierre Riteau wrote: On 28 janv. 2010, at 19:11, Daniel P. Berrange wrote: On Thu, Jan 28, 2010 at 05:52:45PM +, Sharadha Prabhakar (3P) wrote: Hi, I'm writing XenServer APIs for libvirt and trying to test it on virt-Manager. I need to build the libvirt python bindings for that. I tried this for libvirt. ./configure -with-xenapi -with-python You need to use -- not just - with arguments, eg --with-python make make install I suppose this is supposed to build a .libs directory with libvirtmod.so in the ~/libvirt/python/ But I don't seem to have this at all after build. It was a clean build with no errors. I need libvirtmod.so to link with virt-manager. I'm using libvirt version 0.7.4 Can anyone help me with this. Am I missing something here Also make sure you have python-devel RPM installed (or equivalent) and have not disabled shared libraries when running configure. When python-dev is not installed, configure currently (in version 0.7.5) disables the bindings: configure: Could not find python2.5/Python.h, disabling bindings and final output says Python: no When --with-python is provided, shouldn't we error out instead of disabling bindings more or less silently? No reaction? -- Pierre Riteau -- PhD student, Myriads team, IRISA, Rennes, France http://perso.univ-rennes1.fr/pierre.riteau/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/13] Re-arrange QEMU device alias assignment code
On Mon, Feb 01, 2010 at 06:39:39PM +, Daniel P. Berrange wrote: This patch re-arranges the QEMU device alias assignment code to make it easier to call into the same codeblock when performing device hotplug. The new code has the ability to skip over already assigned names to facilitate hotplug * src/qemu/qemu_driver.c: Call qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_conf.h: Export qemuAssignDeviceNetAlias() instead of qemuAssignNetNames * src/qemu/qemu_driver.c: Merge the legacy disk/network alias assignment code into the main methods --- src/qemu/qemu_conf.c | 295 +--- src/qemu/qemu_conf.h |4 +- src/qemu/qemu_driver.c | 14 ++- 3 files changed, 166 insertions(+), 147 deletions(-) ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/13] Disable QEMU monitor IO debugging by default
On Mon, Feb 01, 2010 at 06:39:40PM +, Daniel P. Berrange wrote:
---
src/qemu/qemu_monitor_text.c | 16 +++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index b2a0c53..d6cbea8 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -44,6 +44,8 @@
#define QEMU_CMD_PROMPT \n(qemu)
#define QEMU_PASSWD_PROMPT Password:
+#define DEBUG_IO 0
+
/* Return -1 for error, 0 for success */
typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr mon,
const char *buf,
@@ -67,7 +69,7 @@ typedef int qemuMonitorExtraPromptHandler(qemuMonitorPtr
mon,
int qemuMonitorTextIOProcess(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
const char *data,
- size_t len,
+ size_t len ATTRIBUTE_UNUSED,
qemuMonitorMessagePtr msg)
{
int used = 0;
@@ -79,18 +81,24 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
/* We see the greeting prefix, but not postfix, so pretend we've
not consumed anything. We'll restart when more data arrives. */
if (!offset) {
+#if DEBUG_IO
VIR_DEBUG0(Partial greeting seen, getting out waiting for
more);
+#endif
return 0;
}
used = offset - data + strlen(GREETING_POSTFIX);
+#if DEBUG_IO
VIR_DEBUG0(Discarded monitor greeting);
+#endif
}
/* Don't print raw data in debug because its full of control chars */
/*VIR_DEBUG(Process data %d byts of data [%s], len - used, data +
used);*/
+#if DEBUG_IO
VIR_DEBUG(Process data %d byts of data, (int)(len - used));
+#endif
/* Look for a non-zero reply followed by prompt */
if (msg !msg-finished) {
@@ -138,7 +146,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
/* We might get a prompt for a password before the (qemu) prompt
*/
passwd = strstr(start, PASSWORD_PROMPT);
if (passwd) {
+#if DEBUG_IO
VIR_DEBUG(Seen a passwowrd prompt [%s], data + used);
+#endif
if (msg-passwordHandler) {
int i;
/* Try and handle the prompt */
@@ -176,9 +186,11 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
memcpy(msg-rxBuffer + msg-rxLength, start, want);
msg-rxLength += want;
msg-rxBuffer[msg-rxLength] = '\0';
+#if DEBUG_IO
VIR_DEBUG(Finished %d byte reply [%s], want,
msg-rxBuffer);
} else {
VIR_DEBUG0(Finished 0 byte reply);
+#endif
}
msg-finished = 1;
used += end - (data + used);
@@ -186,7 +198,9 @@ int qemuMonitorTextIOProcess(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
}
}
+#if DEBUG_IO
VIR_DEBUG(Total used %d, used);
+#endif
return used;
}
ACK, this was really filling up full debug logs !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 13/13] Tweak USB hostdevice XML handling
On Mon, Feb 01, 2010 at 06:39:42PM +, Daniel P. Berrange wrote:
When attaching a USB host device based on vendor/product, libvirt
will resolve the vendor/product into a device/bus pair. This means
that when printing XML we should allow device/bus info to be printed
at any time if present
* src/conf/domain_conf.c, docs/schemas/domain.rng: Allow USB device
bus info alongside vendor/product
---
docs/schemas/domain.rng |7 ++-
src/conf/domain_conf.c |5 +++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 827ff6f..bb6d00d 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -1179,7 +1179,12 @@
group
element name=source
choice
-ref name=usbproduct/
+ group
+ ref name=usbproduct/
+ optional
+ ref name=usbaddress/
+ /optional
+ /group
ref name=usbaddress/
element name=address
ref name=pciaddress/
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b434fc5..766993c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5244,11 +5244,12 @@ virDomainHostdevDefFormat(virConnectPtr conn,
def-source.subsys.u.usb.vendor);
virBufferVSprintf(buf, product id='0x%.4x'/\n,
def-source.subsys.u.usb.product);
-} else {
+}
+if (def-source.subsys.u.usb.bus ||
+def-source.subsys.u.usb.device)
virBufferVSprintf(buf, address bus='%d'
device='%d'/\n,
def-source.subsys.u.usb.bus,
def-source.subsys.u.usb.device);
-}
} else if (def-source.subsys.type ==
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
virBufferVSprintf(buf, address domain='0x%.4x'
bus='0x%.2x' slot='0x%.2x' function='0x%.1x'/\n,
def-source.subsys.u.pci.domain,
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/13] Fix QEMU hotplug device alias assignment
On Mon, Feb 01, 2010 at 06:39:41PM +, Daniel P. Berrange wrote: To allow devices to be hot(un-)plugged it is neccessary to ensure they all have a unique device aliases. This fixes the hotplug methods to assign device aliases before invoking the monitor commands which need them * src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Expose methods for assigning device aliases for disks, host devices and controllers * src/qemu/qemu_driver.c: Assign device aliases when hotplugging all types of device * tests/qemuxml2argvdata/qemuxml2argv-hostdev-pci-address-device.args, tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-address-device.args: Update for changed hostdev naming scheme --- src/qemu/qemu_conf.c | 61 +++ src/qemu/qemu_conf.h |3 + src/qemu/qemu_driver.c | 35 +++- .../qemuxml2argv-hostdev-pci-address-device.args |2 +- .../qemuxml2argv-hostdev-usb-address-device.args |2 +- 5 files changed, 74 insertions(+), 29 deletions(-) ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/13] Remove direct storage of hostnet_name vlan
On Tue, Feb 02, 2010 at 03:01:46PM +, Daniel P. Berrange wrote: On Tue, Feb 02, 2010 at 03:58:19PM +0100, Daniel Veillard wrote: On Mon, Feb 01, 2010 at 06:39:38PM +, Daniel P. Berrange wrote: The current way of assigning names to the host network backend and NIC device in QEMU was over complicated, by varying naming scheme based on the NIC model and backend type. This simplifies the naming to simply be 'net0' and 'hostnet0', allowing code to easily determine the host network name and vlan based off the primary device alias name 'net0'. This in turn allows removal of alot of QEMU specific code from the XML parser, and makes it easier to assign new unique names for NICs that are hotplugged ACK, this really simplifies the code, but isn't there a small risk of not being able to properly handle domains say after a libvirt upgrade and restart ? Yes no. Any guests that were running before the upgrade will continue to run fine. You won't be able to hotplug further NICs to those guests until you restart them though. I did try to maintain the ability to hotplug into running guests after an upgrade, but it was getting more complex than I could cope with due to the really wierd naming scheme used by the old code. Okay, yes there is a very small tradeoff there but I think it's the right things to do, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/13] Introduce internal QEMU monitor APIs for drive + device hotadd
On Tue, Feb 02, 2010 at 10:12:08AM +0100, Matthias Bolte wrote:
2010/2/1 Daniel P. Berrange berra...@redhat.com:
The way QEMU is started has been changed to use '-device' and
the new style '-drive' syntax. This needs to be mirrored in
the hotplug code, requiring addition of two new APIs.
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Define APIs
qemuMonitorAddDevice() and qemuMonitorAddDrive()
* src/qemu/qemu_monitor_json.c, src/qemu/qemu_monitor_json.h,
src/qemu/qemu_monitor_text.c, src/qemu/qemu_monitor_text.h:
Implement the new monitor APIs
---
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 380bcdc..b2a0c53 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2029,3 +2029,80 @@ error:
+int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
+ const char *drivestr)
+{
+ char *cmd = NULL;
+ char *reply = NULL;
+ int ret = -1;
+ char *safe_str;
+
+ safe_str = qemuMonitorEscapeArg(drivestr);
+ if (!safe_str) {
+ virReportOOMError(NULL);
+ return -1;
+ }
+
+ ret = virAsprintf(cmd, drive_add dummy %s, safe_str);
dummy looks like a leftover from debugging to me, or does it belong there?
I'm adding a comment about this before committing
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/13] Rewrite way QEMU PCI addresses are allocated
On Tue, Feb 02, 2010 at 10:15:24AM +0100, Matthias Bolte wrote:
2010/2/1 Daniel P. Berrange berra...@redhat.com:
The current QEMU code allocates PCI addresses incrementally starting
at 4. This is not satisfactory because the user may have given some
addresses in their XML config, which need to be skipped over when
allocating addresses to remaining devices.
It is thus neccessary to maintain a list of already allocated PCI
addresses and then only allocate ones that remain unused. This is
also required for domain device hotplug to work properly later.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Add APIs for creating
list of existing PCI addresses, and allocating new addresses.
Refactor address assignment to use this code
* src/qemu/qemu_driver.c: Pull PCI address assignment up into the
qemuStartVMDaemon() method, as a prelude to moving it into the
'define' method. Update list of allocated addresses when connecting
to a running VM at daemon startup.
* tests/qemuxml2argvtest.c, tests/qemuargv2xmltest.c,
tests/qemuxml2xmltest.c: Remove USB product test since all
passthrough is done based on address
* tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.args,
tests/qemuxml2argvdata/qemuxml2argv-hostdev-usb-product.xml: Kil
unused data files
---
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bbf49fd..9bfd181 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
+
+void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
+{
+ if (!addrs)
+ return;
+
+ virHashFree(addrs-used, qemuDomainPCIAddressSetFreeEntry);
Maybe add addrs-used = NULL after virHashFree to prevent potential
double frees or segfaults due to the dangling pointer.
Yep, added that too
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] JAVA .start() method
On 02/02/2010 05:44 AM, Matthias Bolte wrote: 2010/2/2 Marc Gonzalez Mateoma...@ac.upc.edu: Good morning, Testing the new java version (0.4.1) I've noticed that there's no .start() method to start a defined or shuted down domain. Are you working on it or is it a minor feature not to be considered? Thanks in advance, Why do you think there should be a method called start? The Java bindings bind the C API, not the virsh commands. The C API function to start a domain is called virDomainCreate. The Java bindings bind this function to the create method of the Domain class. Matthias As Matthias has said, these are tracking the C API. Having said that, where in the doco would you have liked to have seen this? I will agree that create is not an obvious word to look for if all you have seen is the virsh api. -- bk -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Ensure QEMU DAC security driver is activated at all times
If the primary security driver (SELinux/AppArmour) was disabled
then the secondary QEMU DAC security driver was also disabled.
This is mistaken, because the latter must be active at all times
* src/qemu/qemu_driver.c: Ensure DAC driver is always active
---
src/qemu/qemu_driver.c | 22 --
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16e9b56..a9313e7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -897,26 +897,28 @@ qemudSecurityInit(struct qemud_driver *qemud_drv)
int ret;
virSecurityDriverPtr security_drv;
+qemuSecurityStackedSetDriver(qemud_drv);
+qemuSecurityDACSetDriver(qemud_drv);
+
ret = virSecurityDriverStartup(security_drv,
qemud_drv-securityDriverName);
if (ret == -1) {
VIR_ERROR0(_(Failed to start security driver));
return -1;
}
-/* No security driver wanted to be enabled: just return */
+
+/* No primary security driver wanted to be enabled: just setup
+ * the DAC driver on its own */
if (ret == -2) {
+qemud_drv-securityDriver = qemuDACSecurityDriver;
VIR_INFO0(_(No security driver available));
-return 0;
+} else {
+qemud_drv-securityPrimaryDriver = security_drv;
+qemud_drv-securitySecondaryDriver = qemuDACSecurityDriver;
+qemud_drv-securityDriver = qemuStackedSecurityDriver;
+VIR_INFO(Initialized security driver %s, security_drv-name);
}
-qemuSecurityStackedSetDriver(qemud_drv);
-qemuSecurityDACSetDriver(qemud_drv);
-
-qemud_drv-securityPrimaryDriver = security_drv;
-qemud_drv-securitySecondaryDriver = qemuDACSecurityDriver;
-qemud_drv-securityDriver = qemuStackedSecurityDriver;
-
-VIR_INFO(Initialized security driver %s, security_drv-name);
return 0;
}
--
1.6.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Ensure QEMU DAC security driver is activated at all times
On Tue, Feb 02, 2010 at 04:20:39PM +, Daniel P. Berrange wrote:
If the primary security driver (SELinux/AppArmour) was disabled
then the secondary QEMU DAC security driver was also disabled.
This is mistaken, because the latter must be active at all times
* src/qemu/qemu_driver.c: Ensure DAC driver is always active
---
src/qemu/qemu_driver.c | 22 --
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16e9b56..a9313e7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -897,26 +897,28 @@ qemudSecurityInit(struct qemud_driver *qemud_drv)
int ret;
virSecurityDriverPtr security_drv;
+qemuSecurityStackedSetDriver(qemud_drv);
+qemuSecurityDACSetDriver(qemud_drv);
+
ret = virSecurityDriverStartup(security_drv,
qemud_drv-securityDriverName);
if (ret == -1) {
VIR_ERROR0(_(Failed to start security driver));
return -1;
}
-/* No security driver wanted to be enabled: just return */
+
+/* No primary security driver wanted to be enabled: just setup
+ * the DAC driver on its own */
if (ret == -2) {
+qemud_drv-securityDriver = qemuDACSecurityDriver;
VIR_INFO0(_(No security driver available));
-return 0;
+} else {
+qemud_drv-securityPrimaryDriver = security_drv;
+qemud_drv-securitySecondaryDriver = qemuDACSecurityDriver;
+qemud_drv-securityDriver = qemuStackedSecurityDriver;
+VIR_INFO(Initialized security driver %s, security_drv-name);
}
-qemuSecurityStackedSetDriver(qemud_drv);
-qemuSecurityDACSetDriver(qemud_drv);
-
-qemud_drv-securityPrimaryDriver = security_drv;
-qemud_drv-securitySecondaryDriver = qemuDACSecurityDriver;
-qemud_drv-securityDriver = qemuStackedSecurityDriver;
-
-VIR_INFO(Initialized security driver %s, security_drv-name);
return 0;
}
Okay, understood, ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Remove logging handlers in virExec
On Tue, Jan 12, 2010 at 03:26:26PM -0500, Cole Robinson wrote:
This allows debug statements and raised errors in hook functions to
actually be logged somewhere (stderr). Users can enable debugging in the
daemon and now see more info in /var/log/libvirt/...
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/util/util.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 44a4b2f..23d781d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -334,6 +334,7 @@ __virExec(virConnectPtr conn,
int pipeerr[2] = {-1,-1};
int childout = -1;
int childerr = -1;
+int logprio;
sigset_t oldmask, newmask;
struct sigaction sig_action;
@@ -452,6 +453,11 @@ __virExec(virConnectPtr conn,
of being seen / logged */
virSetErrorFunc(NULL, NULL);
+/* Make sure any hook logging is sent to stderr */
+logprio = virLogGetDefaultPriority();
+virLogReset();
+virLogSetDefaultPriority(logprio);
+
This patch turns out to cause a deadlock in libvirtd. The problem is
that fork() preserves the state of any mutexes which are locked.
So if some thread in libvirtd is currently executing a logging call,
while another thread calls virExec(), that other thread no longer
exists in the child, but its lock is never released. So when the
child then does virLogReset() it deadlocks.
I'm actuall surprised we've not hit this problem before, since any call
to virRaiseError in the child will also eventually run a logging function
which will in turn acquire a lock.
The only way I see to address this, is for the parent process to call
virLogLock(), immediately before fork(), and then virLogUnlock()
afterwards in both parent child. This will ensure that no other thread
can be holding the lock across fork().
https://bugzilla.redhat.com/show_bug.cgi?id=561066
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: Remove logging handlers in virExec
On 02/02/2010 11:54 AM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:26PM -0500, Cole Robinson wrote:
This allows debug statements and raised errors in hook functions to
actually be logged somewhere (stderr). Users can enable debugging in the
daemon and now see more info in /var/log/libvirt/...
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/util/util.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 44a4b2f..23d781d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -334,6 +334,7 @@ __virExec(virConnectPtr conn,
int pipeerr[2] = {-1,-1};
int childout = -1;
int childerr = -1;
+int logprio;
sigset_t oldmask, newmask;
struct sigaction sig_action;
@@ -452,6 +453,11 @@ __virExec(virConnectPtr conn,
of being seen / logged */
virSetErrorFunc(NULL, NULL);
+/* Make sure any hook logging is sent to stderr */
+logprio = virLogGetDefaultPriority();
+virLogReset();
+virLogSetDefaultPriority(logprio);
+
This patch turns out to cause a deadlock in libvirtd. The problem is
that fork() preserves the state of any mutexes which are locked.
So if some thread in libvirtd is currently executing a logging call,
while another thread calls virExec(), that other thread no longer
exists in the child, but its lock is never released. So when the
child then does virLogReset() it deadlocks.
I'm actuall surprised we've not hit this problem before, since any call
to virRaiseError in the child will also eventually run a logging function
which will in turn acquire a lock.
The only way I see to address this, is for the parent process to call
virLogLock(), immediately before fork(), and then virLogUnlock()
afterwards in both parent child. This will ensure that no other thread
can be holding the lock across fork().
https://bugzilla.redhat.com/show_bug.cgi?id=561066
Does this look okay? I haven't tested that it fixes the issue (though I
haven't reproduced the error either).
- Cole
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e5e8860..7573af3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -366,6 +366,8 @@ virLogParseOutputs;
virLogStartup;
virLogShutdown;
virLogReset;
+virLogLock;
+virLogUnlock;
# memory.h
diff --git a/src/util/logging.c b/src/util/logging.c
index 3b3c309..11a4b06 100644
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -133,11 +133,11 @@ static int virLogResetOutputs(void);
*/
virMutex virLogMutex;
-static void virLogLock(void)
+void virLogLock(void)
{
virMutexLock(virLogMutex);
}
-static void virLogUnlock(void)
+void virLogUnlock(void)
{
virMutexUnlock(virLogMutex);
}
diff --git a/src/util/logging.h b/src/util/logging.h
index 49e2f6d..5f61f59 100644
--- a/src/util/logging.h
+++ b/src/util/logging.h
@@ -128,6 +128,8 @@ extern int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data,
* Internal logging API
*/
+extern void virLogLock(void);
+extern void virLogUnlock(void);
extern int virLogStartup(void);
extern int virLogReset(void);
extern void virLogShutdown(void);
diff --git a/src/util/util.c b/src/util/util.c
index cf1290d..901c0d2 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -415,12 +415,19 @@ __virExec(virConnectPtr conn,
childerr = null;
}
+/* Ensure we hold the logging lock, to protect child processes
+ * from deadlocking on another threads inheirited mutex state */
+virLogLock();
+
if ((pid = fork()) 0) {
virReportSystemError(conn, errno,
%s, _(cannot fork child process));
goto cleanup;
}
+/* Unlock for both parent and child process */
+virLogUnlock();
+
if (pid) { /* parent */
close(null);
if (outfd *outfd == -1) {
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Fix wrong nodeinfo-mhz when cpufreq is enabled
Hi, I've written a little patch to fix wrong nodeinfo-mhz when the Linux kernel module cpufreq and a typical governor like ondemand are loaded. nodeinfo-mhz is then too low as libvirt just reads /proc/cpuinfo, entry cpu MHz. This patch reads /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq if existing and corrects nodeinfo-mhz if the value found is higher. Using cpu0 is a crude hack but as discussed on IRC, considering different governors/maximum frequencies per cpu/core would require changing the nodeinfo struct, which would break the API. Some more info: https://bugzilla.redhat.com/show_bug.cgi?id=559762 Please have a careful look at my patch, my last C is quite some time ago. I tried to stick to linuxNodeInfoCPUPopulate, but f.e., checking for *p == '\0' after virStrToLong_ui doesn't work and I'm wondering why scaling_max_freq may not be terminated with by NUL? I also hope the file handling is correct, but please have a look. There is still some todo: Some logging would be helpful, maybe one of you could point out which function best to use? This is my first patch, so hopefully it follows in the next mail ;-) kr, tom -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] * src/nodeinfo.c: Read nodeinfo-mhz from cpufreq device file, if detected.
---
src/nodeinfo.c | 36
1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 7d26b2b..15877ed 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -55,11 +55,14 @@
#ifdef __linux__
#define CPUINFO_PATH /proc/cpuinfo
+#define SCALINGMAXFREQ_PATH
/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
/* NB, these are not static as we need to call them from testsuite */
int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo,
virNodeInfoPtr nodeinfo);
+int linuxNodeInfoConsiderCPUScaling(FILE *scalingmaxfreq, virNodeInfoPtr
nodeinfo);
+
int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr
nodeinfo) {
char line[1024];
@@ -132,6 +135,27 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE
*cpuinfo, virNodeInfoPtr n
return 0;
}
+int linuxNodeInfoConsiderCPUScaling(FILE *scalingmaxfreq, virNodeInfoPtr
nodeinfo) {
+char line[1024];
+
+while (fgets(line, sizeof(line), scalingmaxfreq) != NULL) {
+char *buf = line;
+char *p;
+unsigned int khz;
+
+if (virStrToLong_ui(buf, p, 10, khz) == 0)
+{
+if(khz/1000 nodeinfo-mhz)
+{
+nodeinfo-mhz = khz/1000;
+return 0;
+}
+}
+}
+return 1;
+}
+
+
#endif
int nodeGetInfo(virConnectPtr conn,
@@ -164,6 +188,18 @@ int nodeGetInfo(virConnectPtr conn,
if (ret 0)
return -1;
+FILE *scalingmaxfreq = fopen(SCALINGMAXFREQ_PATH, r);
+if (scalingmaxfreq != NULL) {
+// TODO: some logging information that cpufreq was detected?
+ret = linuxNodeInfoConsiderCPUScaling(scalingmaxfreq, nodeinfo);
+if(ret == 0) {
+// TODO: logging: nodeinfo-mhz was updated
+} else if(ret == 1) {
+// TODO: logging: cpufreq was detected, but information available
didn't make sense
+}
+fclose(scalingmaxfreq);
+}
+
/* Convert to KB. */
nodeinfo-memory = physmem_total () / 1024;
--
1.5.4.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt: python bindings
On 02/02/2010 09:58 AM, Pierre Riteau wrote: On 29 janv. 2010, at 10:05, Pierre Riteau wrote: On 28 janv. 2010, at 19:11, Daniel P. Berrange wrote: On Thu, Jan 28, 2010 at 05:52:45PM +, Sharadha Prabhakar (3P) wrote: Hi, I'm writing XenServer APIs for libvirt and trying to test it on virt-Manager. I need to build the libvirt python bindings for that. I tried this for libvirt. ./configure -with-xenapi -with-python You need to use -- not just - with arguments, eg --with-python make make install I suppose this is supposed to build a .libs directory with libvirtmod.so in the ~/libvirt/python/ But I don't seem to have this at all after build. It was a clean build with no errors. I need libvirtmod.so to link with virt-manager. I'm using libvirt version 0.7.4 Can anyone help me with this. Am I missing something here Also make sure you have python-devel RPM installed (or equivalent) and have not disabled shared libraries when running configure. When python-dev is not installed, configure currently (in version 0.7.5) disables the bindings: configure: Could not find python2.5/Python.h, disabling bindings and final output says Python: no When --with-python is provided, shouldn't we error out instead of disabling bindings more or less silently? Yes, that sounds like the desired behavior. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
On Wed, Jan 27, 2010 at 01:39:13PM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When domain is NULL, don't deref NULL. Instead, just return -1,
as in many other functions in this file, and as this function did
up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 26 Jan 2010 20:17:07 +0100
Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain
argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt
to diagnose an unlikely NULL-domain or NULL-domain-conn error.
---
src/xen/xen_hypervisor.c |7 ++-
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 6d8accc..0257be2 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1,7 +1,7 @@
/*
* xen_internal.c: direct access to Xen hypervisor level
*
- * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc.
+ * Copyright (C) 2005-2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain,
virVcpuInfoPtr info, int maxinfo,
virVcpuInfoPtr ipt;
int nbinfo, i;
-if (domain == NULL || domain-conn == NULL) {
-virXenErrorFunc (domain-conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-invalid argument, 0);
+if (domain == NULL || domain-conn == NULL)
return -1;
-}
I'd rather we just got rid of that check completely - its a left
over from a time when Xen was the only driver these entry points
needed to do full checking. Now all mandatory parameters are checked
in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the domain == NULL
tests. I want to keep this global clean-up patch separate from
the above bug-fixing one.
I'm less confident about removing the daomin-conn tests,
and would be inclined to leave them as-is, or use an assert, if you
want to remove them. If we also remove the daomin-conn == NULL tests,
an added assert is the best way to keep clang/coverity from
complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 27 Jan 2010 13:34:03 +0100
Subject: [PATCH] xen_hypervisor.c: remove all domain == NULL tests, ...
* src/xen/xen_hypervisor.c: Remove all domain == NULL tests.
* src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to
mark each domain parameter as known always to be non-NULL.
---
src/xen/xen_hypervisor.c | 28 ++--
src/xen/xen_hypervisor.h | 44 +---
2 files changed, 43 insertions(+), 29 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 0257be2..994f5ef 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1130,7 +1130,7 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int
*nparams)
char *schedulertype = NULL;
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return NULL;
@@ -1214,7 +1214,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain,
{
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
@@ -1317,7 +1317,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain,
xenUnifiedPrivatePtr priv;
char buf[256];
-if ((domain == NULL) || (domain-conn == NULL)) {
+if (domain-conn == NULL) {
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
domain or conn is NULL, 0);
return -1;
@@ -3062,12 +3062,12 @@ xenHypervisorGetDomMaxMemory(virConnectPtr conn, int
id)
*
* Returns the memory size in kilobytes or 0 in case of error.
*/
-static unsigned long
+static unsigned long ATTRIBUTE_NONNULL (1)
xenHypervisorGetMaxMemory(virDomainPtr domain)
{
xenUnifiedPrivatePtr priv;
-if ((domain == NULL) || (domain-conn == NULL))
+if (domain-conn == NULL)
return 0;
priv = (xenUnifiedPrivatePtr) domain-conn-privateData;
@@ -3176,7 +3176,7 @@
Re: [libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
Daniel P. Berrange wrote:
Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain
argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt
to diagnose an unlikely NULL-domain or NULL-domain-conn error.
...
@@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain,
virVcpuInfoPtr info, int maxinfo,
virVcpuInfoPtr ipt;
int nbinfo, i;
-if (domain == NULL || domain-conn == NULL) {
-virXenErrorFunc (domain-conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
-invalid argument, 0);
+if (domain == NULL || domain-conn == NULL)
return -1;
-}
I'd rather we just got rid of that check completely - its a left
over from a time when Xen was the only driver these entry points
needed to do full checking. Now all mandatory parameters are checked
in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the domain == NULL
tests. I want to keep this global clean-up patch separate from
the above bug-fixing one.
I'm less confident about removing the daomin-conn tests,
and would be inclined to leave them as-is, or use an assert, if you
want to remove them. If we also remove the daomin-conn == NULL tests,
an added assert is the best way to keep clang/coverity from
complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 27 Jan 2010 13:34:03 +0100
Subject: [PATCH] xen_hypervisor.c: remove all domain == NULL tests, ...
* src/xen/xen_hypervisor.c: Remove all domain == NULL tests.
* src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to
mark each domain parameter as known always to be non-NULL.
...
ACK
Thanks.
I'll push those as soon as make distcheck etc. passes.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virStoragePoolSourceListNewSource: avoid unconditional leak
The buffer allocated by VIR_ALLOC here is never used, and
the sole pointer to it is overwritten by this stmt:
source = list-sources[list-nsources++];
From 52cf50def236ab61298ebbba39fcaf89d440f4ec Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 19:59:14 +0100
Subject: [PATCH] virStoragePoolSourceListNewSource: avoid unconditional leak
* src/conf/storage_conf.c (virStoragePoolSourceListNewSource):
Remove an unused (and leaked) allocation.
---
src/conf/storage_conf.c |6 --
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index bd34f5e..62b8394 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1692,17 +1692,11 @@ virStoragePoolSourcePtr
virStoragePoolSourceListNewSource(virConnectPtr conn,
virStoragePoolSourceListPtr list)
{
virStoragePoolSourcePtr source;
-if (VIR_ALLOC(source) 0) {
-virReportOOMError(conn);
-return NULL;
-}
-
if (VIR_REALLOC_N(list-sources, list-nsources+1) 0) {
-VIR_FREE(source);
virReportOOMError(conn);
return NULL;
}
source = list-sources[list-nsources++];
--
1.7.0.rc1.149.g0b0b7
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/6] Add more options for clock sync
This series adds two new options for clock synchronization The two current options are: clock offset='utc'/ clock offset='localtime'/ This introduces a way to set a timezone for localtime clock offset='localtime' timezone='Europe/Paris'/ And a way to set a completely arbitrary time, by giving the number of seconds offset from UTC. clock offset='variable' adjustment='123456'/ It is open to debate whether we need to distinguish this last one via the extra string 'variable', or just add 'adjustment' as an optional attribute for the existing term 'utc'. Opinions welcome -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/6] Change the internal domain conf representation of localtime/utc
The XML will soon be extended to allow more than just a simple
localtime/utc boolean flag. This change replaces the plain
'int localtime' with a separate struct to prepare for future
extension
* src/conf/domain_conf.c, src/conf/domain_conf.h: Add a new
virDomainClockDef structure
* src/libvirt_private.syms: Export virDomainClockOffsetTypeToString
and virDomainClockOffsetTypeFromString
* src/qemu/qemu_conf.c, src/vbox/vbox_tmpl.c, src/xen/xend_internal.c,
src/xen/xm_internal.c: Updated to use new structure for localtime
---
src/conf/domain_conf.c | 20
src/conf/domain_conf.h | 16 +++-
src/libvirt_private.syms |2 ++
src/qemu/qemu_conf.c | 11 +--
src/vbox/vbox_tmpl.c |2 +-
src/xen/xend_internal.c | 16 +++-
src/xen/xm_internal.c| 21 ++---
7 files changed, 72 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 766993c..1daf6f4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -222,6 +222,11 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST,
dynamic,
static)
+VIR_ENUM_IMPL(virDomainClockOffset, VIR_DOMAIN_CLOCK_OFFSET_LAST,
+ utc,
+ localtime);
+
+
#define virDomainReportError(conn, code, fmt...) \
virReportErrorHelper(conn, VIR_FROM_DOMAIN, code, __FILE__,\
__FUNCTION__, __LINE__, fmt)
@@ -3464,9 +3469,16 @@ static virDomainDefPtr
virDomainDefParseXML(virConnectPtr conn,
tmp = virXPathString(conn, string(./clock/@offset), ctxt);
-if (tmp STREQ(tmp, localtime))
-def-localtime = 1;
-VIR_FREE(tmp);
+if (tmp) {
+if ((def-clock.offset = virDomainClockOffsetTypeFromString(tmp)) 0)
{
+virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
+ _(unknown clock offset '%s'), tmp);
+goto error;
+}
+VIR_FREE(tmp);
+} else {
+def-clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC;
+}
def-os.bootloader = virXPathString(conn, string(./bootloader), ctxt);
def-os.bootloaderArgs = virXPathString(conn, string(./bootloader_args),
ctxt);
@@ -5408,7 +5420,7 @@ char *virDomainDefFormat(virConnectPtr conn,
goto cleanup;
virBufferVSprintf(buf, clock offset='%s'/\n,
- def-localtime ? localtime : utc);
+ virDomainClockOffsetTypeToString(def-clock.offset));
if (virDomainLifecycleDefFormat(conn, buf, def-onPoweroff,
on_poweroff) 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0b79e88..5653b18 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -594,6 +594,19 @@ struct _virSecurityLabelDef {
int type;
};
+enum virDomainClockOffsetType {
+VIR_DOMAIN_CLOCK_OFFSET_UTC = 0,
+VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME = 1,
+
+VIR_DOMAIN_CLOCK_OFFSET_LAST,
+};
+
+typedef struct _virDomainClockDef virDomainClockDef;
+typedef virDomainClockDef *virDomainClockDefPtr;
+struct _virDomainClockDef {
+int offset;
+};
+
#define VIR_DOMAIN_CPUMASK_LEN 1024
/* Guest VM main configuration */
@@ -622,7 +635,7 @@ struct _virDomainDef {
char *emulator;
int features;
-int localtime;
+virDomainClockDef clock;
int ngraphics;
virDomainGraphicsDefPtr *graphics;
@@ -914,5 +927,6 @@ VIR_ENUM_DECL(virDomainGraphics)
/* from libvirt.h */
VIR_ENUM_DECL(virDomainState)
VIR_ENUM_DECL(virDomainSeclabel)
+VIR_ENUM_DECL(virDomainClockOffset)
#endif /* __DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e5e8860..e882ae4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -188,6 +188,8 @@ virDomainDefAddDiskControllers;
virDomainDefClearPCIAddresses;
virDomainDefClearDeviceAliases;
virDomainDeviceInfoIterate;
+virDomainClockOffsetTypeToString;
+virDomainClockOffsetTypeFromString;
# domain_event.h
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 389db7b..8109820 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -3312,8 +3312,14 @@ int qemudBuildCommandLine(virConnectPtr conn,
}
}
-if (def-localtime)
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
ADD_ARG_LIT(-localtime);
+else if (def-clock.offset != VIR_DOMAIN_CLOCK_OFFSET_UTC) {
+qemudReportError(conn, NULL, NULL, VIR_ERR_CONFIG_UNSUPPORTED,
+ _(unsupported clock offset '%s'),
+ virDomainClockOffsetTypeToString(def-clock.offset));
+goto error;
+}
if ((qemuCmdFlags QEMUD_CMD_FLAG_NO_REBOOT)
def-onReboot != VIR_DOMAIN_LIFECYCLE_RESTART)
@@ -5127,6 +5133,7 @@ virDomainDefPtr qemuParseCommandLine(virConnectPtr conn,
def-id = -1;
def-memory = def-maxmem
[libvirt] [PATCH 5/6] Allow configurable timezones with QEMU
Allow an arbitrary timezone with QEMU by setting the $TZ environment
variable when launching QEMU
* src/qemu/qemu_conf.c: Set TZ environment variable if a timezone
is requested
* tests/qemuxml2argvtest.c: Add test case for timezones
* tests/qemuxml2argvdata/qemuxml2argv-clock-france.xml,
tests/qemuxml2argvdata/qemuxml2argv-clock-france.args: Data
for timezone tests
---
src/qemu/qemu_conf.c |6 -
.../qemuxml2argv-clock-france.args |1 +
.../qemuxml2argvdata/qemuxml2argv-clock-france.xml | 24
tests/qemuxml2argvtest.c |2 +
4 files changed, 32 insertions(+), 1 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-france.xml
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index bf77f3c..d9bea33 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -2903,7 +2903,7 @@ qemuBuildClockArgStr(virDomainClockDefPtr def)
time_t now = time(NULL);
struct tm nowbits;
-now += def-adjustment;
+now += def-data.adjustment;
gmtime_r(now, nowbits);
virBufferVSprintf(buf, base=%d-%d-%dT%d:%d:%d,
@@ -3381,6 +3381,10 @@ int qemudBuildCommandLine(virConnectPtr conn,
goto error;
}
}
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME
+def-clock.data.timezone) {
+ADD_ENV_PAIR(TZ, def-clock.data.timezone);
+}
if ((qemuCmdFlags QEMUD_CMD_FLAG_NO_REBOOT)
def-onReboot != VIR_DOMAIN_LIFECYCLE_RESTART)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
new file mode 100644
index 000..3c57397
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
@@ -0,0 +1 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test TZ=Europe/Paris
/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor
unix:/tmp/test-monitor,server,nowait -rtc base=localtime -no-acpi -boot c -hda
/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-france.xml
b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.xml
new file mode 100644
index 000..88477b6
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.xml
@@ -0,0 +1,24 @@
+domain type='qemu'
+ nameQEMUGuest1/name
+ uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
+ memory219200/memory
+ currentMemory219200/currentMemory
+ vcpu1/vcpu
+ os
+type arch='i686' machine='pc'hvm/type
+boot dev='hd'/
+ /os
+ clock offset='localtime' timezone='Europe/Paris'/
+ on_poweroffdestroy/on_poweroff
+ on_rebootrestart/on_reboot
+ on_crashdestroy/on_crash
+ devices
+emulator/usr/bin/qemu/emulator
+disk type='block' device='disk'
+ source dev='/dev/HostVG/QEMUGuest1'/
+ target dev='hda' bus='ide'/
+ address type='drive' controller='0' bus='0' unit='0'/
+/disk
+controller type='ide' index='0'/
+ /devices
+/domain
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index dff3e27..23ee5d5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -222,6 +222,8 @@ mymain(int argc, char **argv)
* Can't be enabled since the absolute timestamp changes every time
DO_TEST(clock-variable, QEMUD_CMD_FLAG_RTC);
*/
+DO_TEST(clock-france, QEMUD_CMD_FLAG_RTC);
+
DO_TEST(hugepages, QEMUD_CMD_FLAG_MEM_PATH);
DO_TEST(disk-cdrom, 0);
DO_TEST(disk-cdrom-empty, QEMUD_CMD_FLAG_DRIVE);
--
1.6.6
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] Add new clock mode allowing variable adjustments
This introduces a third option for clock offset synchronization,
that allows an arbitrary / variable adjustment to be set. In
essence the XML contains the time delta in seconds, relative to
UTC.
clock offset='variable' adjustment='123465'/
The difference from 'utc' mode, is that management apps should
track adjustments and preserve them at next reboot.
* docs/schemas/domain.rng: Schema for new clock mode
* src/conf/domain_conf.c, src/conf/domain_conf.h: Parse
new clock time delta
* src/libvirt_private.syms, src/util/xml.c, src/util/xml.h: Add
virXPathLongLong() method
---
docs/schemas/domain.rng | 25 +---
src/conf/domain_conf.c | 14 ++-
src/conf/domain_conf.h |5
src/libvirt_private.syms |1 +
src/util/xml.c | 55 ++
src/util/xml.h |4 +++
6 files changed, 98 insertions(+), 6 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index bb6d00d..5c48a8b 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -297,12 +297,24 @@
define name=clock
optional
element name=clock
-attribute name=offset
- choice
+ choice
+ attribute name=offset
valuelocaltime/value
+ /attribute
+ attribute name=offset
valueutc/value
- /choice
-/attribute
+ /attribute
+ group
+attribute name=offset
+ valuevariable/value
+ /attribute
+ optional
+ attribute name=adjustment
+ ref name=timeDelta/
+ /attribute
+ /optional
+ /group
+ /choice
empty/
/element
/optional
@@ -1526,4 +1538,9 @@
param name='pattern'[a-zA-Z0-9\-_]+/param
/data
/define
+ define name=timeDelta
+data type=string
+ param name=pattern(-|\+)?[0-9]+/param
+/data
+ /define
/grammar
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1daf6f4..0c502b9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -224,7 +224,8 @@ VIR_ENUM_IMPL(virDomainSeclabel, VIR_DOMAIN_SECLABEL_LAST,
VIR_ENUM_IMPL(virDomainClockOffset, VIR_DOMAIN_CLOCK_OFFSET_LAST,
utc,
- localtime);
+ localtime,
+ variable);
#define virDomainReportError(conn, code, fmt...) \
@@ -3479,6 +3480,11 @@ static virDomainDefPtr
virDomainDefParseXML(virConnectPtr conn,
} else {
def-clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC;
}
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) {
+if (virXPathLongLong(conn, ./clock/@adjustment, ctxt,
+ def-clock.adjustment) 0)
+def-clock.adjustment = 0;
+}
def-os.bootloader = virXPathString(conn, string(./bootloader), ctxt);
def-os.bootloaderArgs = virXPathString(conn, string(./bootloader_args),
ctxt);
@@ -5419,8 +5425,12 @@ char *virDomainDefFormat(virConnectPtr conn,
if (virCPUDefFormatBuf(conn, buf, def-cpu, , 0) 0)
goto cleanup;
-virBufferVSprintf(buf, clock offset='%s'/\n,
+virBufferVSprintf(buf, clock offset='%s',
virDomainClockOffsetTypeToString(def-clock.offset));
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) {
+virBufferVSprintf(buf, adjustment='%lld', def-clock.adjustment);
+}
+virBufferAddLit(buf, /\n);
if (virDomainLifecycleDefFormat(conn, buf, def-onPoweroff,
on_poweroff) 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5653b18..4df28e5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -597,6 +597,7 @@ struct _virSecurityLabelDef {
enum virDomainClockOffsetType {
VIR_DOMAIN_CLOCK_OFFSET_UTC = 0,
VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME = 1,
+VIR_DOMAIN_CLOCK_OFFSET_VARIABLE = 2,
VIR_DOMAIN_CLOCK_OFFSET_LAST,
};
@@ -605,6 +606,10 @@ typedef struct _virDomainClockDef virDomainClockDef;
typedef virDomainClockDef *virDomainClockDefPtr;
struct _virDomainClockDef {
int offset;
+
+/* Adjustment in seconds, relative to UTC, when
+ * offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE */
+long long adjustment;
};
#define VIR_DOMAIN_CPUMASK_LEN 1024
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e882ae4..39b7c45 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -625,6 +625,7 @@ virXPathStringLimit;
virXPathBoolean;
virXPathNumber;
virXPathULong;
+virXPathLongLong;
virXPathULongLong;
virXPathLongHex;
virXPathULongHex;
diff --git a/src/util/xml.c b/src/util/xml.c
index 4fa443d..52786d0 100644
--- a/src/util/xml.c
+++ b/src/util/xml.c
@@ -374,6 +374,61 @@ virXPathULongLong(virConnectPtr conn,
return (ret);
}
+/**
+ * virXPathULongLong:
+ * @xpath: the XPath string to evaluate
+ *
[libvirt] [PATCH 3/6] Support variable clock offset mode in QEMU
This allows QEMU guests to be started with an arbitrary clock
offset
The test case can't actually be enabled, since QEMU argv expects
an absolute timestring, and this will obviously change every
time the test runs :-( Hopefully QEMU will allow a relative
time offset in the future.
* src/qemu/qemu_conf.c, src/qemu/qemu_conf.h: Use the -rtc arg
if available to support variable clock offset mode
* tests/qemuhelptest.c: Add QEMUD_CMD_FLAG_RTC for qemu 0.12.1
* qemuxml2argvdata/qemuxml2argv-clock-variable.args,
qemuxml2argvdata/qemuxml2argv-clock-variable.xml,
qemuxml2argvtest.c: Test case, except we can't actually enable
it yet.
---
src/qemu/qemu_conf.c | 75 ++--
src/qemu/qemu_conf.h |1 +
tests/qemuhelptest.c |3 +-
.../qemuxml2argv-clock-variable.args |1 +
.../qemuxml2argv-clock-variable.xml| 24 ++
tests/qemuxml2argvtest.c |4 +
6 files changed, 100 insertions(+), 8 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-variable.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-clock-variable.xml
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 8109820..bf77f3c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1154,6 +1154,9 @@ static unsigned int qemudComputeCmdFlags(const char *help,
flags |= QEMUD_CMD_FLAG_BALLOON;
if (strstr(help, -device))
flags |= QEMUD_CMD_FLAG_DEVICE;
+/* The trailing ' ' is important to avoid a bogus match */
+if (strstr(help, -rtc ))
+flags |= QEMUD_CMD_FLAG_RTC;
/* Keep disabled till we're actually ready to turn on netdev mode
* The plan is todo it in 0.13.0 QEMU, but lets wait see... */
#if 0
@@ -2882,6 +2885,56 @@ error:
}
+static char *
+qemuBuildClockArgStr(virDomainClockDefPtr def)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+switch (def-offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_UTC:
+virBufferAddLit(buf, base=utc);
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+virBufferAddLit(buf, base=localtime);
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: {
+time_t now = time(NULL);
+struct tm nowbits;
+
+now += def-adjustment;
+gmtime_r(now, nowbits);
+
+virBufferVSprintf(buf, base=%d-%d-%dT%d:%d:%d,
+ nowbits.tm_year,
+ nowbits.tm_mon,
+ nowbits.tm_mday,
+ nowbits.tm_hour,
+ nowbits.tm_min,
+ nowbits.tm_sec);
+} break;
+
+default:
+qemudReportError(NULL, NULL, NULL, VIR_ERR_CONFIG_UNSUPPORTED,
+ _(unsupported clock offset '%s'),
+ virDomainClockOffsetTypeToString(def-offset));
+goto error;
+}
+
+if (virBufferError(buf)) {
+virReportOOMError(NULL);
+goto error;
+}
+
+return virBufferContentAndReset(buf);
+
+error:
+virBufferFreeAndReset(buf);
+return NULL;
+}
+
+
static int
qemuBuildCpuArgStr(virConnectPtr conn,
const struct qemud_driver *driver,
@@ -3312,13 +3365,21 @@ int qemudBuildCommandLine(virConnectPtr conn,
}
}
-if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
-ADD_ARG_LIT(-localtime);
-else if (def-clock.offset != VIR_DOMAIN_CLOCK_OFFSET_UTC) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_CONFIG_UNSUPPORTED,
- _(unsupported clock offset '%s'),
- virDomainClockOffsetTypeToString(def-clock.offset));
-goto error;
+if (qemuCmdFlags QEMUD_CMD_FLAG_RTC) {
+const char *rtcopt;
+ADD_ARG_LIT(-rtc);
+if (!(rtcopt = qemuBuildClockArgStr(def-clock)))
+goto error;
+ADD_ARG(rtcopt);
+} else {
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
+ADD_ARG_LIT(-localtime);
+else if (def-clock.offset != VIR_DOMAIN_CLOCK_OFFSET_UTC) {
+qemudReportError(conn, NULL, NULL, VIR_ERR_CONFIG_UNSUPPORTED,
+ _(unsupported clock offset '%s'),
+
virDomainClockOffsetTypeToString(def-clock.offset));
+goto error;
+}
}
if ((qemuCmdFlags QEMUD_CMD_FLAG_NO_REBOOT)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 101f187..f67284b 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -82,6 +82,7 @@ enum qemud_cmd_flags {
QEMUD_CMD_FLAG_SDL = (1 27), /* Is the new -sdl arg available
*/
QEMUD_CMD_FLAG_SMP_TOPOLOGY = (1 28), /* Is
sockets=s,cores=c,threads=t available for -smp? */
QEMUD_CMD_FLAG_NETDEV= (1 29), /* The -netdev flag
netdev_add/remove monitor commands */
+
[libvirt] [PATCH 6/6] Expand docs about clock modes
* formatdomain.html.in: Document new clock options --- docs/formatdomain.html.in | 34 +++--- 1 files changed, 31 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ce49f7d..e47a6b7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -393,9 +393,37 @@ dl dtcodeclock/code/dt - ddThe codeoffset/code attribute takes either utc or -localtime to specify how the guest clock is initialized -in relation to the host OS. + dd + pThe codeoffset/code attribute takes three possible + values, allowing fine grained control over how the guest + clock is synchronized to the host. NB, not all hypervisors + support all modes./p + dl + dtcodeutc/code/dt + dd + The guest clock will always be synchronized to UTC when + booted/dd + dtcodelocaltime/code/dt + dd + The guest clock will be synchronized to the host's configured + timezone when booted. It is possible to override the timezone + by using the codetimezone/code attribute. + /dd + dtcodevariable/code/dt + dd + The guest clock will have an arbitrary offset applied + relative to UTC. The delta relative to UTC is specified + in seconds, using the codeadjustment/code attribute. + The guest is free to adjust the RTC over time an expect + that it will be honoured at next reboot. This is in + contrast to 'utc' mode, where the RTC adjustments are + lost at each reboot. + /dd + /dl + p + NB, at time of writing, only QEMU supports the variable + clock mode, or custom timezones. + /p /dd /dl -- 1.6.6 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/6] Allow a timezone to be specified with localtime clocks
This extends the XML to allow for
clock offset='localtime' timezone='Europe/Paris'/
This is useful if the admin has not configured any timezone on the
host OS, but still wants to synchronize a guest to a specific one.
* src/conf/domain_conf.h, src/conf/domain_conf.c: Support extra
'timezone' attribute on clock configuration
* docs/schemas/domain.rng: Add 'timezone' attribute
* src/xen/xend_internal.c, src/xen/xm_internal.c: Reject configs
with a configurable timezone
---
docs/schemas/domain.rng | 18 +++---
src/conf/domain_conf.c | 25 -
src/conf/domain_conf.h | 12 +---
src/xen/xend_internal.c | 10 --
src/xen/xm_internal.c | 16 +++-
5 files changed, 63 insertions(+), 18 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 5c48a8b..95c7d8e 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -298,9 +298,16 @@
optional
element name=clock
choice
- attribute name=offset
-valuelocaltime/value
- /attribute
+ group
+attribute name=offset
+ valuelocaltime/value
+ /attribute
+ optional
+ attribute name=timezone
+ ref name=timeZone/
+ /attribute
+ /optional
+ /group
attribute name=offset
valueutc/value
/attribute
@@ -1543,4 +1550,9 @@
param name=pattern(-|\+)?[0-9]+/param
/data
/define
+ define name=timeZone
+data type=string
+ param name=pattern[a-zA-Z0-9_\.\+\-/]+/param
+/data
+ /define
/grammar
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0c502b9..c5eb086 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -640,6 +640,9 @@ void virDomainDefFree(virDomainDefPtr def)
VIR_FREE(def-os.bootloader);
VIR_FREE(def-os.bootloaderArgs);
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
+VIR_FREE(def-clock.data.timezone);
+
VIR_FREE(def-name);
VIR_FREE(def-cpumask);
VIR_FREE(def-emulator);
@@ -3480,10 +3483,16 @@ static virDomainDefPtr
virDomainDefParseXML(virConnectPtr conn,
} else {
def-clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC;
}
-if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) {
+switch (def-clock.offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+def-clock.data.timezone = virXPathString(conn,
string(./clock/@timezone), ctxt);
+break;
+
+case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE:
if (virXPathLongLong(conn, ./clock/@adjustment, ctxt,
- def-clock.adjustment) 0)
-def-clock.adjustment = 0;
+ def-clock.data.adjustment) 0)
+def-clock.data.adjustment = 0;
+break;
}
def-os.bootloader = virXPathString(conn, string(./bootloader), ctxt);
@@ -5427,8 +5436,14 @@ char *virDomainDefFormat(virConnectPtr conn,
virBufferVSprintf(buf, clock offset='%s',
virDomainClockOffsetTypeToString(def-clock.offset));
-if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE) {
-virBufferVSprintf(buf, adjustment='%lld', def-clock.adjustment);
+switch (def-clock.offset) {
+case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME:
+if (def-clock.data.timezone)
+virBufferEscapeString(buf, timezone='%s',
def-clock.data.timezone);
+break;
+case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE:
+virBufferVSprintf(buf, adjustment='%lld',
def-clock.data.adjustment);
+break;
}
virBufferAddLit(buf, /\n);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 4df28e5..0261979 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -607,9 +607,15 @@ typedef virDomainClockDef *virDomainClockDefPtr;
struct _virDomainClockDef {
int offset;
-/* Adjustment in seconds, relative to UTC, when
- * offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE */
-long long adjustment;
+union {
+/* Adjustment in seconds, relative to UTC, when
+ * offset == VIR_DOMAIN_CLOCK_OFFSET_VARIABLE */
+long long adjustment;
+
+/* Timezone name, when
+ * offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME */
+char *timezone;
+} data;
};
#define VIR_DOMAIN_CPUMASK_LEN 1024
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 98f6103..c6b5b6b 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -5788,9 +5788,15 @@ xenDaemonFormatSxpr(virConnectPtr conn,
virBufferVSprintf(buf, (on_crash '%s'), tmp);
/* Set localtime here for current XenD (both PV HVM) */
-if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
+if (def-clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME) {
+if (def-clock.data.timezone) {
+virXendError(conn,
Re: [libvirt] [PATCH] util: Remove logging handlers in virExec
On Tue, Feb 02, 2010 at 12:15:01PM -0500, Cole Robinson wrote:
On 02/02/2010 11:54 AM, Daniel P. Berrange wrote:
On Tue, Jan 12, 2010 at 03:26:26PM -0500, Cole Robinson wrote:
This allows debug statements and raised errors in hook functions to
actually be logged somewhere (stderr). Users can enable debugging in the
daemon and now see more info in /var/log/libvirt/...
Signed-off-by: Cole Robinson crobi...@redhat.com
---
src/util/util.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 44a4b2f..23d781d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -334,6 +334,7 @@ __virExec(virConnectPtr conn,
int pipeerr[2] = {-1,-1};
int childout = -1;
int childerr = -1;
+int logprio;
sigset_t oldmask, newmask;
struct sigaction sig_action;
@@ -452,6 +453,11 @@ __virExec(virConnectPtr conn,
of being seen / logged */
virSetErrorFunc(NULL, NULL);
+/* Make sure any hook logging is sent to stderr */
+logprio = virLogGetDefaultPriority();
+virLogReset();
+virLogSetDefaultPriority(logprio);
+
This patch turns out to cause a deadlock in libvirtd. The problem is
that fork() preserves the state of any mutexes which are locked.
So if some thread in libvirtd is currently executing a logging call,
while another thread calls virExec(), that other thread no longer
exists in the child, but its lock is never released. So when the
child then does virLogReset() it deadlocks.
I'm actuall surprised we've not hit this problem before, since any call
to virRaiseError in the child will also eventually run a logging function
which will in turn acquire a lock.
The only way I see to address this, is for the parent process to call
virLogLock(), immediately before fork(), and then virLogUnlock()
afterwards in both parent child. This will ensure that no other thread
can be holding the lock across fork().
https://bugzilla.redhat.com/show_bug.cgi?id=561066
Does this look okay? I haven't tested that it fixes the issue (though I
haven't reproduced the error either).
- Cole
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e5e8860..7573af3 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -366,6 +366,8 @@ virLogParseOutputs;
virLogStartup;
virLogShutdown;
virLogReset;
+virLogLock;
+virLogUnlock;
# memory.h
diff --git a/src/util/logging.c b/src/util/logging.c
index 3b3c309..11a4b06 100644
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -133,11 +133,11 @@ static int virLogResetOutputs(void);
*/
virMutex virLogMutex;
-static void virLogLock(void)
+void virLogLock(void)
{
virMutexLock(virLogMutex);
}
-static void virLogUnlock(void)
+void virLogUnlock(void)
{
virMutexUnlock(virLogMutex);
}
diff --git a/src/util/logging.h b/src/util/logging.h
index 49e2f6d..5f61f59 100644
--- a/src/util/logging.h
+++ b/src/util/logging.h
@@ -128,6 +128,8 @@ extern int virLogDefineOutput(virLogOutputFunc f,
virLogCloseFunc c, void *data,
* Internal logging API
*/
+extern void virLogLock(void);
+extern void virLogUnlock(void);
extern int virLogStartup(void);
extern int virLogReset(void);
extern void virLogShutdown(void);
diff --git a/src/util/util.c b/src/util/util.c
index cf1290d..901c0d2 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -415,12 +415,19 @@ __virExec(virConnectPtr conn,
childerr = null;
}
+/* Ensure we hold the logging lock, to protect child processes
+ * from deadlocking on another threads inheirited mutex state */
+virLogLock();
+
if ((pid = fork()) 0) {
virReportSystemError(conn, errno,
%s, _(cannot fork child process));
goto cleanup;
}
+/* Unlock for both parent and child process */
+virLogUnlock();
+
if (pid) { /* parent */
close(null);
if (outfd *outfd == -1) {
Yeah, i reckon that this should be sufficient. I'll get the BZ reporter
to try this patch
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] PATCH [0/1] Fix locking in udev node device backend
The initial udev node device backend implementation fails to take the lock on the node device driverState struct when adding and removing devices. The following patch adds the appropriate locking. Dave -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/1] Fix locking for udev device add/remove
* The original udev node device backend neglected to lock the driverState
struct containing the device list when adding and removing devices.
---
src/node_device/node_device_udev.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 2bc2d32..be765c4 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1190,7 +1190,9 @@ static int udevRemoveOneDevice(struct udev_device *device)
int ret = 0;
name = udev_device_get_syspath(device);
+nodeDeviceLock(driverState);
dev = virNodeDeviceFindBySysfsPath(driverState-devs, name);
+
if (dev != NULL) {
VIR_DEBUG(Removing device '%s' with sysfs path '%s',
dev-def-name, name);
@@ -1200,6 +1202,7 @@ static int udevRemoveOneDevice(struct udev_device *device)
name);
ret = -1;
}
+nodeDeviceUnlock(driverState);
return ret;
}
@@ -1288,7 +1291,10 @@ static int udevAddOneDevice(struct udev_device *device)
goto out;
}
+nodeDeviceLock(driverState);
dev = virNodeDeviceAssignDef(NULL, driverState-devs, def);
+nodeDeviceUnlock(driverState);
+
if (dev == NULL) {
VIR_ERROR(Failed to create device for '%s', def-name);
virNodeDeviceDefFree(def);
--
1.6.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] JAVA .start() method
I've got confused with the naming yes, as I supposed create was doing the same as it does in virsh api, my mistake. Maybe it would be a good idea to rename that method and others to the virsh naming. thanks a lot guys, 2010/2/2, Bryan Kearney bkear...@redhat.com: On 02/02/2010 05:44 AM, Matthias Bolte wrote: 2010/2/2 Marc Gonzalez Mateoma...@ac.upc.edu: Good morning, Testing the new java version (0.4.1) I've noticed that there's no .start() method to start a defined or shuted down domain. Are you working on it or is it a minor feature not to be considered? Thanks in advance, Why do you think there should be a method called start? The Java bindings bind the C API, not the virsh commands. The C API function to start a domain is called virDomainCreate. The Java bindings bind this function to the create method of the Domain class. Matthias As Matthias has said, these are tracking the C API. Having said that, where in the doco would you have liked to have seen this? I will agree that create is not an obvious word to look for if all you have seen is the virsh api. -- bk -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] interface_conf.c: don't use a negative value as allocation size
Here's one way to solve this.
Another would be to change the way virXPathNodeSet works, but
there are several other uses of it.
From 75108240911a1ad943e0bde8ba9ade92ea216f60 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 2 Feb 2010 20:54:01 +0100
Subject: [PATCH] interface_conf.c: don't use a negative value as allocation size
* src/conf/interface_conf.c (virInterfaceDefParseProtoIPv4): If
virXPathNodeSet returns -1, indicate failure by returning -1 right away.
(virInterfaceDefParseProtoIPv6): Likewise.
---
src/conf/interface_conf.c |6 +-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c
index 8a17ed6..510df81 100644
--- a/src/conf/interface_conf.c
+++ b/src/conf/interface_conf.c
@@ -1,9 +1,9 @@
/*
* interface_conf.c: interfaces XML handling
*
- * Copyright (C) 2006-2009 Red Hat, Inc.
+ * Copyright (C) 2006-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2.1 of the License, or (at your option) any later version.
@@ -318,10 +318,12 @@ virInterfaceDefParseProtoIPv4(virConnectPtr conn,
virInterfaceProtocolDefPtr def
if (ret != 0)
return(ret);
}
nIpNodes = virXPathNodeSet(conn, ./ip, ctxt, ipNodes);
+if (nIpNodes 0)
+return -1;
if (ipNodes == NULL)
return 0;
if (VIR_ALLOC_N(def-ips, nIpNodes) 0) {
virReportOOMError(conn);
@@ -375,10 +377,12 @@ virInterfaceDefParseProtoIPv6(virConnectPtr conn,
virInterfaceProtocolDefPtr def
if (ret != 0)
return(ret);
}
nIpNodes = virXPathNodeSet(conn, ./ip, ctxt, ipNodes);
+if (nIpNodes 0)
+return -1;
if (ipNodes == NULL)
return 0;
if (VIR_ALLOC_N(def-ips, nIpNodes) 0) {
virReportOOMError(conn);
--
1.7.0.rc1.167.gdb08
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Fix locking for udev device add/remove
On Tue, Feb 02, 2010 at 02:54:12PM -0500, David Allan wrote:
* The original udev node device backend neglected to lock the driverState
struct containing the device list when adding and removing devices.
---
src/node_device/node_device_udev.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 2bc2d32..be765c4 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1190,7 +1190,9 @@ static int udevRemoveOneDevice(struct udev_device
*device)
int ret = 0;
name = udev_device_get_syspath(device);
+nodeDeviceLock(driverState);
dev = virNodeDeviceFindBySysfsPath(driverState-devs, name);
+
if (dev != NULL) {
VIR_DEBUG(Removing device '%s' with sysfs path '%s',
dev-def-name, name);
@@ -1200,6 +1202,7 @@ static int udevRemoveOneDevice(struct udev_device
*device)
name);
ret = -1;
}
+nodeDeviceUnlock(driverState);
return ret;
}
@@ -1288,7 +1291,10 @@ static int udevAddOneDevice(struct udev_device *device)
goto out;
}
+nodeDeviceLock(driverState);
dev = virNodeDeviceAssignDef(NULL, driverState-devs, def);
+nodeDeviceUnlock(driverState);
+
if (dev == NULL) {
VIR_ERROR(Failed to create device for '%s', def-name);
virNodeDeviceDefFree(def);
ACK, looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
