Re: [libvirt] Live Migration with Pass-through Devices proposal
Hi Chen-san, > Hi all, > > backgrond: > Live migration is one of the most important features of virtualization > technology. > With regard to recent virtualization techniques, performance of network > I/O is critical. > Current network I/O virtualization (e.g. Para-virtualized I/O, VMDq) has > a significant > performance gap with native network I/O. Pass-through network devices > have near > native performance, however, they have thus far prevented live > migration. No existing > methods solve the problem of live migration with pass-through devices > perfectly. > > There was an idea to solve the problem in website: > https://www.kernel.org/doc/ols/2008/ols2008v2-pages-261-267.pdf > Please refer to above document for detailed information. > > So I think this problem maybe could be solved by using the combination > of existing > technology. the attached was the architecture of migration with > passthrough device > we proposed. and the following steps are we considering to implement: > > - before boot VM, we anticipate to specify two NICs for creating > bonding device > (one plugged and one virtual NIC) in XML. here we can specify > the NIC's mac addresses > in XML, which could facilitate qemu-guest-agent to find the > network interfaces in guest. > > - when boot VM, we monitor the qemu-guest-agent process in guest > OS that > when qemu-guest-agent is available, sending command to > qemu-guest-agent to > let it create the bonding device at once according to the XML > configuration. > here netcf maybe a good tool to create bonding device easily. If I understand correctly, in your scenario, bonding device (bondX) is created by qemu-guest agent dynamicaly at every boot time. Is that right ? If so, I think it's hard for a guest administrator to configure bonding device (such as IP address). How about creating configuration file like /etc/sysconfig/network-scripts/ifcfg-bond0 (in Red Hat environment) to persistent boinding device ? >- if need to do migration, checking the bonding's virtual NIC > whether able to > access the plugged NIC's LAN. otherwise, when the passthroughed > NIC is unplugged, > the network would be broken. I think this check should be done before enslaving each NIC. By the way, how do you check whether both NICs are connected to the same network segment? I wonder if it's difficult ... >- during migration, unplug the passthroughed NIC. then do native > magration. > >- on destination side, check whether need to hotplug new NIC > according to XML. > then hotplug the deivce according the source node in XML, tell > qemu-guest-agent > to switch the hotplugged NIC activities. > > Thanks, > Chen Sincrely, Taku Izumi -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
[...] > The choice of Nehalem, Opteron, etc as CPU models is already supported > in QEMU and influences guest CPU performance. You're not explaining why > we need to introduce multiple CPU values. > It makes no sense to have two different CPU models listed for the same > guest like this. > Hi Daniel, Maybe I misunderstood your meaning. Next I will let you know what we think in detail. libvirt already implements Vcpu hotplug for qemu driver by qemu command 'cpu-add', but Vcpu hot-unplug has not been implemented. qemu prefers to implement Vcpu hotplug by command 'device_add' instead of 'cpu-add', implement Vcpu hotplug by command 'device_del' in future. So, according to the above, our team has the following two methods to implement Vcpu hotplug/un-plug in libvirt. 1) improve the existing API, and use the old command 'setVcpus'. This method will invoke qemu's command 'device_add' instead of 'cpu-add'. This method has three problems: a) can not specific the cpu' model. This will change qemu's design for cpu hotplug. b) can not keep consistent with other devices' hotplug in libvirt. c) influence cpu's hot-unplug. If device_add a cpu without id (i.e. device alias name in libvirt), cpu hot-unplug can not work. 2) Add a new API to support cpu hot-plug/unplug, and leave the the existing API as a legacy. This method will realize the feature by libvirt command 'attach-device', and will allow user to specify the cpu model. And in the future, we want to make struct virCPUDefPtr as a member of the new defined struct for hotplugged cpu device, so that We can allow user to set more details for cpu device, not only cpu model. This method has two advantages. a) keep consistent with other devices' hotplug in libvirt. b) can assign device alias name. This is helpful for cpu hot-unplug. The above is our opinion. May you give us some suggestions? Regards, Zhu -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] cpu: add Freescale ppc64 CPU models
When running Openstack on Freescale ppc64 board, got libvirtError as before: nova.openstack.common.threadgroup libvirtError: XML error: Missing CPU model name. This patch is to add Freescale ppc64 CPU models. Signed-off-by: Olivia Yin --- src/cpu/cpu_map.xml | 36 1 file changed, 36 insertions(+) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index bd9b056..707cf88 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -600,6 +600,7 @@ + @@ -657,5 +658,40 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- 2.1.0.27.g96db324 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] cpu: add Freescale ppc64 CPU models
Signed-off-by: Olivia Yin --- src/cpu/cpu_map.xml | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index bd9b056..c34874e 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1,4 +1,4 @@ - +n @@ -600,6 +600,7 @@ + @@ -657,5 +658,40 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- 2.1.0.27.g96db324 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] LSN-2015-0001: CVE-2015-0236 snapshots and save images leak VNC passwords
Libvirt Security Notice: LSN-2015-0001 == Summary: snapshots and save images leak VNC passwords Reported on: 20150120 Published on: 20150122 Fixed on: 20150122 Reported by: Luyao Huang Patched by: Peter Krempa See also: CVE-2015-0236 Description --- The two interfaces virDomainSnapshotGetXMLDesc and virDomainSaveImageGetXMLDesc would accept the VIR_DOMAIN_XML_SECURE flag in situations where virDomainGetXMLDesc did not, when fine-grained access control lists (ACL) are in use. As a result, a client can use a snapshot or save image to bypass restrictions and gain access to the secured information. Impact -- A client using a read-write connection, and which has the 'domain:read' ACL privilege while lacking 'domain:secure_read', can trigger an information leak of data by using VIR_DOMAIN_XML_SECURE with the affected interfaces. Fortunately, the only data in this category is the value of an optional VNC password. Workaround -- VNC passwords are notoriously weak (they are capped at an 8 byte maximum length; the VNC protocol sends them in plaintext over the network; and FIPS mode execution prohibits the use of a VNC password), so it is recommended that users not create domains with a VNC password in the first place. Domains that do not use VNC passwords do not suffer from information leaks; the use of SPICE connections is recommended not only because it avoids the leak, but also because SPICE provides better features than VNC for a guest graphics device. Furthermore, the leak is only possible when fine-grained ACLs are in use; read-only clients cannot trigger the issue. Therefore, the problem is avoided if no user is granted the 'read' ACL privilege without also having the 'read_secure' privilege. Another mitigation is that the information leak can only occur if a snapshot or save image exists; a user that is denied 'read_secure' is typically also unable to create such an image, so the leak depends on a more privileged user making use of that feature. Affected product Name: libvirt Repository: git://libvirt.org/git/libvirt.git http://libvirt.org/git/?p=libvirt.git Branch: master Broken in: v1.1.0 Broken in: v1.1.1 Broken in: v1.1.2 Broken in: v1.1.3 Broken in: v1.1.4 Broken in: v1.2.0 Broken in: v1.2.1 Broken in: v1.2.2 Broken in: v1.2.3 Broken in: v1.2.4 Broken in: v1.2.5 Broken in: v1.2.6 Broken in: v1.2.7 Broken in: v1.2.8 Broken in: v1.2.9 Broken in: v1.2.10 Broken in: v1.2.11 Fixed in: v1.2.12 Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 03c3c0c874c84dfa51ef17556062b095c6e1c0a3 Fixed by: b1674ad5a97441b7e1bd5f5ebaff498ef2fbb11b Branch: v1.1.0-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: a976724f9a10730e1339628482a283653efdb72c Fixed by: c4c824ec818ce85de049ed5546fa8ce3c8b76e32 Branch: v1.1.1-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 9a2728e1b28b67a682e55d8dd3c0d79e21f0ad37 Fixed by: 2c6fc46d987911e310d30621cd6fc195af102fee Branch: v1.1.2-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 6eec2b830a752c95fc2d971d3daf7626f9701290 Fixed by: 947c969fc248c2324e565b5e4f80a3d11733f12b Branch: v1.1.3-maint Broken in: v1.1.3.1 Broken in: v1.1.3.2 Broken in: v1.1.3.3 Broken in: v1.1.3.4 Broken in: v1.1.3.5 Broken in: v1.1.3.6 Broken in: v1.1.3.7 Broken in: v1.1.3.8 Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: ca840e9c827fefadae2e00875b4a552b990b959f Fixed by: 76d6cc3f24ab545694e77e2eafa981d861b965a4 Branch: v1.1.4-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 43d16684c2018c20db1fba35542eb1d52ecb8d7a Fixed by: 17defce9159c5111e7011e575ba72803a9418086 Branch: v1.2.0-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 9475a25c86f3748e2069af67db69d79864b707b9 Fixed by: 8abca887b19600b6652654a01a78455afd4d8294 Branch: v1.2.1-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: f7c70c20530954c2c1a2ce0d192d01a8f71c0093 Fixed by: 1f348188e0698ef2535c81d5a779189531c5df99 Branch: v1.2.2-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: e99c25ca63c695a63b4c9b91ee956be4fb660772 Fixed by: 8107c1e3694ba4685960ec09868076379718f037 Branch: v1.2.3-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: 4edae3cb9600132e875a5b97cf31089a6c8f4cb2 Fixed by: 94d18e8f6dbe3afdc72b6df13e3eaa8861874a14 Branch: v1.2.4-maint Broken by: e341435e5090677c67a0d3d4ca0393102054841f Fixed by: d406f0858e7e3a6199788d3c64217c69d7702032 Fixed by: 4700507a484aec43b02724893cbed93
Re: [libvirt] nodeinfo test cases for x86_64 AMD CPUs
On 22 Jan 2015, at 13:06, Steffen Persvold wrote:
>
> Hi,
>
> Lately we’ve been puzzled by the nodeinfo returned for AMD 63xx based
> platforms so I checked out libvirt from Git and checked out the testcases in
> test/nodeinfodata.
>
> Currently you have 3 AMD test cases there :
>
> linux-x86_64-test3 :
> AMD 6172 2.1 GHz (MCM, 12 cores per package/socket, 2 numa nodes per
> package/socket, 1 thread/core)
> 4 Sockets, 8 NUMA nodes, 48 CPU cores total.
>
> linux-x86_64-test7 :
> AMD 6174 2.2 GHz (MCM, 12 cores per package/socket, 2 numa nodes per
> package/socket, 1 thread/core)
> 2 Sockets, 4 NUMA nodes, 24 CPU cores total.
>
> linux-x86_64-test8 :
> AMD 6282 SE 2.6 GHz (MCM, 8 CU(core) per package/socket, 2 numa nodes
> per package/socket, 2 threads/CU(core))
> 4 Sockets, 8 NUMA nodes, 64 CPU cores total.
>
>
> However, the “expected” output from each of these are wrong I believe :
>
> % ~/libvirt/tests/nodeinfodata$ cat linux-x86_64-test{3,7,8}.expected
> CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 1, Cores: 6, Threads: 1
> CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 1, Cores: 24, Threads: 1
> CPUs: 64/64, MHz: 2593, Nodes: 1, Sockets: 1, Cores: 64, Threads: 1
>
> In my opinion it should have been :
>
> CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 4, Cores: 12, Threads: 1
> CPUs: 24/24, MHz: 2200, Nodes: 4, Sockets: 2, Cores: 12, Threads: 1
> CPUs: 64/64, MHz: 2593, Nodes: 8, Sockets: 4, Cores: 8, Threads: 2
Let me revise that statement a bit. Further looking at the topology in the
nodeinfodata/linux-test{3,7,8} I believe the following would have been the
correct interpretation of the underlying data :
test3:
CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 4, Cores: 12, Threads: 1
test7:
CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 2, Cores: 12, Threads: 1
test8:
CPUs: 64/64, MHz: 2593, Nodes: 8, Sockets: 4, Cores: 8, Threads: 2
The only difference is test7 which seems to be a case where the NUMA
information isn’t available (but the socket numbering and cores/socket,
threads/socket info is still possible to derive). In this case I believe it’s
still valid to present the CPU topology as described, but with one NUMA cell
only (i.e as if it was an old-fashined two-socket non-numa server).
Any comments ?
Cheers,
--
Steffen Persvold
Chief Architect NumaChip, Numascale AS
Tel: +47 23 16 71 88 Fax: +47 23 16 71 80 Skype: spersvold
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/2] Sync macvtap device modes when guest rxfilter changes
From: Tony Krowiak This patch set provides the code to synchonize some macvtap device modes when the values are changed on the guest's network device. The following modes will by synchronized: * PROMISC * MULTICAST * ALLMULTI I noticed something while testing this patch set that did not occur prior to installing more recent kernel and Qemu distributions. It seems that the VLAN table always has an entry for VLAN ID 0 when the rxfilter is queried during processing of the NIC_RX_FILTER_CHANGED event. That means that the PROMISC flag for macvtap0 will be set. I don't know if this will cause problems, but I thought I'd make note of it in case anybody wants to comment on that. v2 changes: * virnetdev.c * Changed names of virNetDevIs... functions to virNetDevGet... * Consolidated code for getting/setting of device option flags * qemu_driver.c * Replaced "default" case in syncNicRxFilterMultiMode function with VIR_NETDEV_RX_FILTER_MODE_NONE v3 changes: * virnetdev.c * Fixed a syntax-check error in virNetDevGetRxFilter function Tony Krowiak (2): util: Functions for getting/setting device options qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/2] qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED
From: Tony Krowiak
This patch enables synchronization of the host macvtap
device options with the guest device's in response to the
NIC_RX_FILTER_CHANGED event.
The following device options will be synchronized:
* PROMISC
* MULTICAST
* ALLMULTI
Signed-off-by: Tony Krowiak
---
No changes for v3
src/qemu/qemu_driver.c | 92
1 files changed, 92 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc6aae4..47c1b5e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4168,6 +4168,93 @@ syncNicRxFilterHostMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
static void
+syncNicRxFilterPromiscMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+bool promisc;
+bool setpromisc = false;
+
+/* Set macvtap promisc mode to true if the guest has vlans defined */
+/* or synchronize the macvtap promisc mode if different from guest */
+if (guestFilter->vlan.nTable > 0) {
+if (!hostFilter->promiscuous) {
+setpromisc = true;
+promisc = true;
+}
+} else if (hostFilter->promiscuous != guestFilter->promiscuous) {
+setpromisc = true;
+promisc = guestFilter->promiscuous;
+}
+
+if (setpromisc) {
+if (virNetDevSetPromiscuous(ifname, promisc) < 0) {
+VIR_WARN("Couldn't set PROMISC flag to %s for device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ promisc ? "true" : "false", ifname);
+}
+}
+}
+
+
+static void
+syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+switch (guestFilter->multicast.mode) {
+case VIR_NETDEV_RX_FILTER_MODE_ALL:
+if (virNetDevSetRcvAllMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NORMAL:
+if (virNetDevSetRcvMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set multicast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NONE:
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvMulti(ifname, false)) {
+VIR_WARN("Couldn't set multicast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED",
+ ifname);
+}
+break;
+}
+}
+}
+
+
+static void
+syncNicRxFilterDeviceOptions(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+syncNicRxFilterPromiscMode(ifname, guestFilter, hostFilter);
+syncNicRxFilterMultiMode(ifname, guestFilter, hostFilter);
+}
+
+
+static void
syncNicRxFilterMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
@@ -4250,9 +4337,14 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
* attributes to match those of the guest network device:
* - MAC address
* - Multicast MAC address table
+ * - Device options:
+ * - PROMISC
+ * - MULTICAST
+ * - ALLMULTI
*/
syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
syncNicRxFilterMulticast(def->ifname, guestFilter, hostFilter);
+syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
}
endjob:
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/2] util: Functions for getting/setting device options
From: Tony Krowiak
This patch provides the utility functions needed to synchronize
the rxfilter changes made to a guest domain with the corresponding
macvtap devices on the host:
* Get/set PROMISC flag
* Get/set ALLMULTI, MULTICAST
Signed-off-by: Tony Krowiak
---
Changes for v3:
* Fixed a syntax-check error in virNetDevGetRxFilter function
src/libvirt_private.syms |8 ++-
src/util/virnetdev.c | 186 +++---
src/util/virnetdev.h | 24 ++-
3 files changed, 190 insertions(+), 28 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2eec83..8d76f9b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1639,13 +1639,16 @@ virNetDevGetIPv4Address;
virNetDevGetLinkInfo;
virNetDevGetMAC;
virNetDevGetMTU;
+virNetDevGetOnline;
virNetDevGetPhysicalFunction;
+virNetDevGetPromiscuous;
+virNetDevGetRcvAllMulti;
+virNetDevGetRcvMulti;
virNetDevGetRxFilter;
virNetDevGetVirtualFunctionIndex;
virNetDevGetVirtualFunctionInfo;
virNetDevGetVirtualFunctions;
virNetDevGetVLanID;
-virNetDevIsOnline;
virNetDevIsVirtualFunction;
virNetDevLinkDump;
virNetDevReplaceMacAddress;
@@ -1663,6 +1666,9 @@ virNetDevSetMTUFromDevice;
virNetDevSetName;
virNetDevSetNamespace;
virNetDevSetOnline;
+virNetDevSetPromiscuous;
+virNetDevSetRcvAllMulti;
+virNetDevSetRcvMulti;
virNetDevSetupControl;
virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ef96b2b..5d330ce 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -610,17 +610,7 @@ int virNetDevSetName(const char* ifname, const char
*newifname)
#if defined(SIOCSIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
-/**
- * virNetDevSetOnline:
- * @ifname: the interface name
- * @online: true for up, false for down
- *
- * Function to control if an interface is activated (up, true) or not (down,
false)
- *
- * Returns 0 in case of success or -1 on error.
- */
-int virNetDevSetOnline(const char *ifname,
- bool online)
+int virNetDevSetIFFlag(const char *ifname, int flag, bool val)
{
int fd = -1;
int ret = -1;
@@ -637,10 +627,10 @@ int virNetDevSetOnline(const char *ifname,
goto cleanup;
}
-if (online)
-ifflags = ifr.ifr_flags | IFF_UP;
+if (val)
+ifflags = ifr.ifr_flags | flag;
else
-ifflags = ifr.ifr_flags & ~IFF_UP;
+ifflags = ifr.ifr_flags & ~flag;
if (ifr.ifr_flags != ifflags) {
ifr.ifr_flags = ifflags;
@@ -659,8 +649,9 @@ int virNetDevSetOnline(const char *ifname,
return ret;
}
#else
-int virNetDevSetOnline(const char *ifname,
- bool online ATTRIBUTE_UNUSED)
+int virNetDevSetIFFlag(const char *ifname,
+ int flag ATTRIBUTE_UNUSED,
+ bool val ATTRIBUTE_UNUSED)
{
virReportSystemError(ENOSYS,
_("Cannot set interface flags on '%s'"),
@@ -670,18 +661,77 @@ int virNetDevSetOnline(const char *ifname,
#endif
-#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
+
/**
- * virNetDevIsOnline:
+ * virNetDevSetOnline:
* @ifname: the interface name
- * @online: where to store the status
+ * @online: true for up, false for down
*
- * Function to query if an interface is activated (true) or not (false)
+ * Function to control if an interface is activated (up, true) or not (down,
false)
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 in case of success or -1 on error.
*/
-int virNetDevIsOnline(const char *ifname,
- bool *online)
+int virNetDevSetOnline(const char *ifname,
+ bool online)
+{
+
+return virNetDevSetIFFlag(ifname, IFF_UP, online);
+}
+
+/**
+ * virNetDevSetPromiscuous:
+ * @ifname: the interface name
+ * @promiscuous: true for receive all packets, false for do not receive
+ * all packets
+ *
+ * Function to control if an interface is to receive all
+ * packets (receive all, true) or not (do not receive all, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetPromiscuous(const char *ifname,
+bool promiscuous)
+{
+return virNetDevSetIFFlag(ifname, IFF_PROMISC, promiscuous);
+}
+
+/**
+ * virNetDevSetRcvMulti:
+ * @ifname: the interface name
+ * @:receive true for receive multicast packets, false for do not receive
+ * multicast packets
+ *
+ * Function to control if an interface is to receive multicast
+ * packets in which it is interested (receive, true)
+ * or not (do not receive, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetRcvMulti(const char *ifname,
+ bool receive)
+{
+return virNetDevSetIFFlag(ifname, IFF_MULTICAST, receive);
+}
+
+/**
+ * virNetDevSetRcvAllMulti:
+ * @ifname: the interface name
+ * @:receive true for receive all packets, false for do not receive a
[libvirt] [PATCH v2 2/2] qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED
From: Tony Krowiak
This patch enables synchronization of the host macvtap
device options with the guest device's in response to the
NIC_RX_FILTER_CHANGED event.
The following device options will be synchronized:
* PROMISC
* MULTICAST
* ALLMULTI
Signed-off-by: Tony Krowiak
---
Changes for v2 (response to v1 review comments):
* Replaced "default" case in syncNicRxFilterMultiMode function with
VIR_NETDEV_RX_FILTER_MODE_NONE
src/qemu/qemu_driver.c | 92
1 files changed, 92 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc6aae4..47c1b5e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4168,6 +4168,93 @@ syncNicRxFilterHostMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
static void
+syncNicRxFilterPromiscMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+bool promisc;
+bool setpromisc = false;
+
+/* Set macvtap promisc mode to true if the guest has vlans defined */
+/* or synchronize the macvtap promisc mode if different from guest */
+if (guestFilter->vlan.nTable > 0) {
+if (!hostFilter->promiscuous) {
+setpromisc = true;
+promisc = true;
+}
+} else if (hostFilter->promiscuous != guestFilter->promiscuous) {
+setpromisc = true;
+promisc = guestFilter->promiscuous;
+}
+
+if (setpromisc) {
+if (virNetDevSetPromiscuous(ifname, promisc) < 0) {
+VIR_WARN("Couldn't set PROMISC flag to %s for device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ promisc ? "true" : "false", ifname);
+}
+}
+}
+
+
+static void
+syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+switch (guestFilter->multicast.mode) {
+case VIR_NETDEV_RX_FILTER_MODE_ALL:
+if (virNetDevSetRcvAllMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NORMAL:
+if (virNetDevSetRcvMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set multicast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NONE:
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvMulti(ifname, false)) {
+VIR_WARN("Couldn't set multicast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED",
+ ifname);
+}
+break;
+}
+}
+}
+
+
+static void
+syncNicRxFilterDeviceOptions(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+syncNicRxFilterPromiscMode(ifname, guestFilter, hostFilter);
+syncNicRxFilterMultiMode(ifname, guestFilter, hostFilter);
+}
+
+
+static void
syncNicRxFilterMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
@@ -4250,9 +4337,14 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
* attributes to match those of the guest network device:
* - MAC address
* - Multicast MAC address table
+ * - Device options:
+ * - PROMISC
+ * - MULTICAST
+ * - ALLMULTI
*/
syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
syncNicRxFilterMulticast(def->ifname, guestFilter, hostFilter);
+syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
}
endjob:
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: Functions for getting/setting device options
From: Tony Krowiak
This patch provides the utility functions needed to synchronize
the rxfilter changes made to a guest domain with the corresponding
macvtap devices on the host:
* Get/set PROMISC flag
* Get/set ALLMULTI, MULTICAST
---
src/libvirt_private.syms |8 ++-
src/util/virnetdev.c | 186 +++---
src/util/virnetdev.h | 24 ++-
3 files changed, 190 insertions(+), 28 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2eec83..8d76f9b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1639,13 +1639,16 @@ virNetDevGetIPv4Address;
virNetDevGetLinkInfo;
virNetDevGetMAC;
virNetDevGetMTU;
+virNetDevGetOnline;
virNetDevGetPhysicalFunction;
+virNetDevGetPromiscuous;
+virNetDevGetRcvAllMulti;
+virNetDevGetRcvMulti;
virNetDevGetRxFilter;
virNetDevGetVirtualFunctionIndex;
virNetDevGetVirtualFunctionInfo;
virNetDevGetVirtualFunctions;
virNetDevGetVLanID;
-virNetDevIsOnline;
virNetDevIsVirtualFunction;
virNetDevLinkDump;
virNetDevReplaceMacAddress;
@@ -1663,6 +1666,9 @@ virNetDevSetMTUFromDevice;
virNetDevSetName;
virNetDevSetNamespace;
virNetDevSetOnline;
+virNetDevSetPromiscuous;
+virNetDevSetRcvAllMulti;
+virNetDevSetRcvMulti;
virNetDevSetupControl;
virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ef96b2b..2bc1b93 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -610,17 +610,7 @@ int virNetDevSetName(const char* ifname, const char
*newifname)
#if defined(SIOCSIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
-/**
- * virNetDevSetOnline:
- * @ifname: the interface name
- * @online: true for up, false for down
- *
- * Function to control if an interface is activated (up, true) or not (down,
false)
- *
- * Returns 0 in case of success or -1 on error.
- */
-int virNetDevSetOnline(const char *ifname,
- bool online)
+int virNetDevSetIFFlag(const char *ifname, int flag, bool val)
{
int fd = -1;
int ret = -1;
@@ -637,10 +627,10 @@ int virNetDevSetOnline(const char *ifname,
goto cleanup;
}
-if (online)
-ifflags = ifr.ifr_flags | IFF_UP;
+if (val)
+ifflags = ifr.ifr_flags | flag;
else
-ifflags = ifr.ifr_flags & ~IFF_UP;
+ifflags = ifr.ifr_flags & ~flag;
if (ifr.ifr_flags != ifflags) {
ifr.ifr_flags = ifflags;
@@ -659,8 +649,9 @@ int virNetDevSetOnline(const char *ifname,
return ret;
}
#else
-int virNetDevSetOnline(const char *ifname,
- bool online ATTRIBUTE_UNUSED)
+int virNetDevSetIFFlag(const char *ifname,
+ int flag ATTRIBUTE_UNUSED,
+ bool val ATTRIBUTE_UNUSED)
{
virReportSystemError(ENOSYS,
_("Cannot set interface flags on '%s'"),
@@ -670,18 +661,77 @@ int virNetDevSetOnline(const char *ifname,
#endif
-#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
+
/**
- * virNetDevIsOnline:
+ * virNetDevSetOnline:
* @ifname: the interface name
- * @online: where to store the status
+ * @online: true for up, false for down
*
- * Function to query if an interface is activated (true) or not (false)
+ * Function to control if an interface is activated (up, true) or not (down,
false)
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 in case of success or -1 on error.
*/
-int virNetDevIsOnline(const char *ifname,
- bool *online)
+int virNetDevSetOnline(const char *ifname,
+ bool online)
+{
+
+return virNetDevSetIFFlag(ifname, IFF_UP, online);
+}
+
+/**
+ * virNetDevSetPromiscuous:
+ * @ifname: the interface name
+ * @promiscuous: true for receive all packets, false for do not receive
+ * all packets
+ *
+ * Function to control if an interface is to receive all
+ * packets (receive all, true) or not (do not receive all, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetPromiscuous(const char *ifname,
+bool promiscuous)
+{
+return virNetDevSetIFFlag(ifname, IFF_PROMISC, promiscuous);
+}
+
+/**
+ * virNetDevSetRcvMulti:
+ * @ifname: the interface name
+ * @:receive true for receive multicast packets, false for do not receive
+ * multicast packets
+ *
+ * Function to control if an interface is to receive multicast
+ * packets in which it is interested (receive, true)
+ * or not (do not receive, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetRcvMulti(const char *ifname,
+ bool receive)
+{
+return virNetDevSetIFFlag(ifname, IFF_MULTICAST, receive);
+}
+
+/**
+ * virNetDevSetRcvAllMulti:
+ * @ifname: the interface name
+ * @:receive true for receive all packets, false for do not receive all packets
+ *
+ * Function to control if an interface is to receive all multicast
+ * packets (receive, true
[libvirt] [PATCH 2/2] qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED
From: Tony Krowiak
This patch enables synchronization of the host macvtap
device options with the guest device's in response to the
NIC_RX_FILTER_CHANGED event.
The following device options will be synchronized:
* PROMISC
* MULTICAST
* ALLMULTI
---
src/qemu/qemu_driver.c | 92
1 files changed, 92 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc6aae4..47c1b5e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4168,6 +4168,93 @@ syncNicRxFilterHostMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
static void
+syncNicRxFilterPromiscMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+bool promisc;
+bool setpromisc = false;
+
+/* Set macvtap promisc mode to true if the guest has vlans defined */
+/* or synchronize the macvtap promisc mode if different from guest */
+if (guestFilter->vlan.nTable > 0) {
+if (!hostFilter->promiscuous) {
+setpromisc = true;
+promisc = true;
+}
+} else if (hostFilter->promiscuous != guestFilter->promiscuous) {
+setpromisc = true;
+promisc = guestFilter->promiscuous;
+}
+
+if (setpromisc) {
+if (virNetDevSetPromiscuous(ifname, promisc) < 0) {
+VIR_WARN("Couldn't set PROMISC flag to %s for device %s "
+ "while responding to NIC_RX_FILTER_CHANGED",
+ promisc ? "true" : "false", ifname);
+}
+}
+}
+
+
+static void
+syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+switch (guestFilter->multicast.mode) {
+case VIR_NETDEV_RX_FILTER_MODE_ALL:
+if (virNetDevSetRcvAllMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NORMAL:
+if (virNetDevSetRcvMulti(ifname, true)) {
+
+VIR_WARN("Couldn't set multicast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+break;
+
+case VIR_NETDEV_RX_FILTER_MODE_NONE:
+if (virNetDevSetRcvAllMulti(ifname, false)) {
+VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+}
+
+if (virNetDevSetRcvMulti(ifname, false)) {
+VIR_WARN("Couldn't set multicast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED",
+ ifname);
+}
+break;
+}
+}
+}
+
+
+static void
+syncNicRxFilterDeviceOptions(char *ifname, virNetDevRxFilterPtr guestFilter,
+ virNetDevRxFilterPtr hostFilter)
+{
+syncNicRxFilterPromiscMode(ifname, guestFilter, hostFilter);
+syncNicRxFilterMultiMode(ifname, guestFilter, hostFilter);
+}
+
+
+static void
syncNicRxFilterMulticast(char *ifname,
virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
@@ -4250,9 +4337,14 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
* attributes to match those of the guest network device:
* - MAC address
* - Multicast MAC address table
+ * - Device options:
+ * - PROMISC
+ * - MULTICAST
+ * - ALLMULTI
*/
syncNicRxFilterMacAddr(def->ifname, guestFilter, hostFilter);
syncNicRxFilterMulticast(def->ifname, guestFilter, hostFilter);
+syncNicRxFilterDeviceOptions(def->ifname, guestFilter, hostFilter);
}
endjob:
--
1.7.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] util: Functions for getting/setting device options
From: Tony Krowiak
This patch provides the utility functions needed to synchronize
the rxfilter changes made to a guest domain with the corresponding
macvtap devices on the host:
* Get/set PROMISC flag
* Get/set ALLMULTI, MULTICAST
Signed-off-by: Tony Krowiak
---
Changes for v2 (response to review comments for v1):
* Changed names of virNetDevIs... functions
* Consolidated code for getting/setting of device option flags
src/libvirt_private.syms |8 ++-
src/util/virnetdev.c | 186 +++---
src/util/virnetdev.h | 24 ++-
3 files changed, 190 insertions(+), 28 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2eec83..8d76f9b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1639,13 +1639,16 @@ virNetDevGetIPv4Address;
virNetDevGetLinkInfo;
virNetDevGetMAC;
virNetDevGetMTU;
+virNetDevGetOnline;
virNetDevGetPhysicalFunction;
+virNetDevGetPromiscuous;
+virNetDevGetRcvAllMulti;
+virNetDevGetRcvMulti;
virNetDevGetRxFilter;
virNetDevGetVirtualFunctionIndex;
virNetDevGetVirtualFunctionInfo;
virNetDevGetVirtualFunctions;
virNetDevGetVLanID;
-virNetDevIsOnline;
virNetDevIsVirtualFunction;
virNetDevLinkDump;
virNetDevReplaceMacAddress;
@@ -1663,6 +1666,9 @@ virNetDevSetMTUFromDevice;
virNetDevSetName;
virNetDevSetNamespace;
virNetDevSetOnline;
+virNetDevSetPromiscuous;
+virNetDevSetRcvAllMulti;
+virNetDevSetRcvMulti;
virNetDevSetupControl;
virNetDevValidateConfig;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index ef96b2b..2bc1b93 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -610,17 +610,7 @@ int virNetDevSetName(const char* ifname, const char
*newifname)
#if defined(SIOCSIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
-/**
- * virNetDevSetOnline:
- * @ifname: the interface name
- * @online: true for up, false for down
- *
- * Function to control if an interface is activated (up, true) or not (down,
false)
- *
- * Returns 0 in case of success or -1 on error.
- */
-int virNetDevSetOnline(const char *ifname,
- bool online)
+int virNetDevSetIFFlag(const char *ifname, int flag, bool val)
{
int fd = -1;
int ret = -1;
@@ -637,10 +627,10 @@ int virNetDevSetOnline(const char *ifname,
goto cleanup;
}
-if (online)
-ifflags = ifr.ifr_flags | IFF_UP;
+if (val)
+ifflags = ifr.ifr_flags | flag;
else
-ifflags = ifr.ifr_flags & ~IFF_UP;
+ifflags = ifr.ifr_flags & ~flag;
if (ifr.ifr_flags != ifflags) {
ifr.ifr_flags = ifflags;
@@ -659,8 +649,9 @@ int virNetDevSetOnline(const char *ifname,
return ret;
}
#else
-int virNetDevSetOnline(const char *ifname,
- bool online ATTRIBUTE_UNUSED)
+int virNetDevSetIFFlag(const char *ifname,
+ int flag ATTRIBUTE_UNUSED,
+ bool val ATTRIBUTE_UNUSED)
{
virReportSystemError(ENOSYS,
_("Cannot set interface flags on '%s'"),
@@ -670,18 +661,77 @@ int virNetDevSetOnline(const char *ifname,
#endif
-#if defined(SIOCGIFFLAGS) && defined(HAVE_STRUCT_IFREQ)
+
/**
- * virNetDevIsOnline:
+ * virNetDevSetOnline:
* @ifname: the interface name
- * @online: where to store the status
+ * @online: true for up, false for down
*
- * Function to query if an interface is activated (true) or not (false)
+ * Function to control if an interface is activated (up, true) or not (down,
false)
*
- * Returns 0 in case of success or an errno code in case of failure.
+ * Returns 0 in case of success or -1 on error.
*/
-int virNetDevIsOnline(const char *ifname,
- bool *online)
+int virNetDevSetOnline(const char *ifname,
+ bool online)
+{
+
+return virNetDevSetIFFlag(ifname, IFF_UP, online);
+}
+
+/**
+ * virNetDevSetPromiscuous:
+ * @ifname: the interface name
+ * @promiscuous: true for receive all packets, false for do not receive
+ * all packets
+ *
+ * Function to control if an interface is to receive all
+ * packets (receive all, true) or not (do not receive all, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetPromiscuous(const char *ifname,
+bool promiscuous)
+{
+return virNetDevSetIFFlag(ifname, IFF_PROMISC, promiscuous);
+}
+
+/**
+ * virNetDevSetRcvMulti:
+ * @ifname: the interface name
+ * @:receive true for receive multicast packets, false for do not receive
+ * multicast packets
+ *
+ * Function to control if an interface is to receive multicast
+ * packets in which it is interested (receive, true)
+ * or not (do not receive, false)
+ *
+ * Returns 0 in case of success or -1 on error.
+ */
+int virNetDevSetRcvMulti(const char *ifname,
+ bool receive)
+{
+return virNetDevSetIFFlag(ifname, IFF_MULTICAST, receive);
+}
+
+/**
+ * virNetDevSetRcvAllMulti:
+ * @ifname: the int
[libvirt] (no subject)
From: Tony Krowiak This patch set provides the code to synchonize some macvtap device modes when the values are changed on the guest's network device. The following modes will by synchronized: * PROMISC * MULTICAST * ALLMULTI I noticed something while testing this patch set that did not occur prior to installing more recent kernel and Qemu distributions. It seems that the VLAN table always has an entry for VLAN ID 0 when the rxfilter is queried during processing of the NIC_RX_FILTER_CHANGED event. That means that the PROMISC flag for macvtap0 will be set. I don't know if this will cause problems, but I thought I'd make note of it in case anybody wants to comment on that. Tony Krowiak (2): util: Functions for getting/setting device options qemu: change macvtap device options in response to NIC_RX_FILTER_CHANGED -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/6] storage: Attempt error recovery in virStorageBackendDiskCreateVol
During virStorageBackendDiskCreateVol if virStorageBackendDiskReadPartitions
fails, then we were leaving with an error and a partition on the disk for
which there was no corresponding volume and used space on the disk which
could be reclaimable through direct parted activity. On a subsequent restart,
reload, or refresh the volume may magically appear too.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_disk.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 60f8393..0c4126a 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -654,6 +654,13 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
virCheckFlags(0, -1);
+if (!vol->target.path) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("volume target path empty for source path '%s'"),
+ pool->def->source.devices[0].path);
+return -1;
+}
+
if (virFileResolveLink(vol->target.path, &devpath) < 0) {
virReportSystemError(errno,
_("Couldn't read volume target path '%s'"),
@@ -709,7 +716,7 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
static int
-virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
+virStorageBackendDiskCreateVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
{
@@ -756,8 +763,16 @@ virStorageBackendDiskCreateVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
VIR_FREE(vol->target.path);
/* Fetch actual extent info, generate key */
-if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
+if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
+/* Best effort to remove the partition. Ignore any errors
+ * since we could be calling this with vol->target.path == NULL
+ */
+virErrorPtr save_err = virSaveLastError();
+ignore_value(virStorageBackendDiskDeleteVol(conn, pool, vol, 0));
+virSetError(save_err);
+virFreeError(save_err);
goto cleanup;
+}
res = 0;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/6] storage: When delete extended partition, need to refresh pool
When removing a volume that is the extended partition, all the logical
volume partitions that exist within the extended partition will also be
removed, so we need to refresh the pool to have the updated list
Signed-off-by: John Ferlan
---
src/storage/storage_backend_disk.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 233e293..300aab3 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -654,7 +654,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr
pool,
static int
-virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
+virStorageBackendDiskDeleteVol(virConnectPtr conn,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
unsigned int flags)
@@ -721,6 +721,15 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn
ATTRIBUTE_UNUSED,
goto cleanup;
}
+/* If this was the extended partition, then all the logical partitions
+ * are then lost. Make it easy on ourselves and just refresh the pool
+ */
+if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) {
+virStoragePoolObjClearVols(pool);
+if (virStorageBackendDiskRefreshPool(conn, pool) < 0)
+goto cleanup;
+}
+
rc = 0;
cleanup:
VIR_FREE(devpath);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/6] storage: Check the partition name against provided name
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
If the provided volume name doesn't match what parted generated as the
partition name, then return a failure.
Update virsh.pod and formatstorage.html.in to describe the 'name' restriction
for disk pools as well as the usage of the 's .
Signed-off-by: John Ferlan
---
docs/formatstorage.html.in | 12 ++--
src/storage/storage_backend_disk.c | 30 --
tools/virsh.pod| 17 ++---
3 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 933268c..d2e2bb8 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -463,7 +463,13 @@
name
Providing a name for the volume which is unique to the pool.
-This is mandatory when defining a volume. Since
0.4.1
+This is mandatory when defining a volume. For a disk pool, the
+name must be combination of the source device path
+device and next partition number to be created. For example, if
+the source device path is /dev/sdb and there are no
+partitions on the disk, then the name must be sdb1 with the next
+name being sdb2 and so on.
+Since 0.4.1
key
Providing an identifier for the volume which identifies a
single volume. In some cases it's possible to have two distinct keys
@@ -553,7 +559,9 @@
Since 0.4.1
format
Provides information about the pool specific volume format.
-For disk pools it will provide the partition type. For filesystem
+For disk pools it will provide the partition table format type, but is
+not preserved after a pool refresh or libvirtd restart. Use extended
+in order to create an extended disk extent partition. For filesystem
or directory pools it will provide the file format type, eg cow,
qcow, vmdk, raw. If omitted when creating a volume, the pool's
default format will be used. The actual format is specified via
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 300aab3..9f4d76a 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -47,16 +47,23 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
char **const groups,
virStorageVolDefPtr vol)
{
-char *tmp, *devpath;
+char *tmp, *devpath, *partname;
+
+/* Prepended path will be same for all partitions, so we can
+ * strip the path to form a reasonable pool-unique name
+ */
+if ((tmp = strrchr(groups[0], '/')))
+partname = tmp + 1;
+else
+partname = groups[0];
if (vol == NULL) {
+/* This is typically a reload/restart/refresh path where
+ * we're discovering the existing partitions for the pool
+ */
if (VIR_ALLOC(vol) < 0)
return -1;
-/* Prepended path will be same for all partitions, so we can
- * strip the path to form a reasonable pool-unique name
- */
-tmp = strrchr(groups[0], '/');
-if (VIR_STRDUP(vol->name, tmp ? tmp + 1 : groups[0]) < 0 ||
+if (VIR_STRDUP(vol->name, partname) < 0 ||
VIR_APPEND_ELEMENT_COPY(pool->volumes.objs,
pool->volumes.count, vol) < 0) {
virStorageVolDefFree(vol);
@@ -80,6 +87,17 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
return -1;
}
+/* Enforce provided vol->name is the same as what parted created.
+ * We do this after filling target.path so that we have a chance at
+ * deleting the partition with this failure from CreateVol path
+ */
+if (STRNEQ(vol->name, partname)) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("invalid partition name '%s', expected '%s'"),
+ vol->name, partname);
+return -1;
+}
+
if (vol->key == NULL) {
/* XXX base off a unique key of the underlying disk */
if (VIR_STRDUP(vol->key, vol->target.path) < 0)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index abe80c2..1591341 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2983,7 +2983,11 @@ Create and start a pool object from the XML I.
Create and start a pool object I from the raw parameters. If
I<--print-xml> is specified, then print the XML of the pool object
without creating the pool. Otherwise, the pool has the specified
-I.
+I. When using B for a pool of I "disk",
+the existing partitions found on the I<--source-dev path> will be used
+to populate the disk pool. Therefore, it is suggested to use
+B and B with the I<--overwrite> in order
+to properly initialize the disk pool.
[I<--source-host hostname>] provides the source hostname for pools backed
by storage from a remote server (pool t
[libvirt] [PATCH 4/6] storage: Adjust how to refresh extended partition disk data
During virStorageBackendDiskMakeDataVol processing, if we find an extended
partition, then handle it specially when updating the capacity/allocation
rather than calling virStorageBackendUpdateVolInfo.
As it turns out, once a logical partition exists, any attempt to refresh
the pool or after libvirtd restart/reload will result in a failure to open
the extended partition device resulting in the inability to start the pool.
The downside to this is we will lose the and for
the extended partition upon subsequent restart, refresh, reload since the
stat() in virStorageBackendUpdateVolTargetInfoFD will not be called. However,
since it's really only a container and shouldn't directly be used for
storage that seems reasonable.
Therefore, only use the existing code that already had a comment about
getting the allocation wrong for extended partitions for just the setting
of the extended partition data.
Signed-off-by: John Ferlan
---
src/storage/storage_backend.c | 4
src/storage/storage_backend_disk.c | 32 +++-
2 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index b990a82..d7bccfe 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1385,6 +1385,10 @@ virStorageBackendVolOpen(const char *path, struct stat
*sb,
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
+if (errno == ENXIO && noerror) {
+VIR_WARN("ignoring missing fifo '%s'", path);
+return -2;
+}
virReportSystemError(errno, _("cannot open volume '%s'"), path);
return -1;
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 31b6025..233e293 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -110,11 +110,6 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
return -1;
}
-/* Refresh allocation/capacity/perms */
-if (virStorageBackendUpdateVolInfo(vol, true, false,
- VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
-return -1;
-
/* set partition type */
if (STREQ(groups[1], "normal"))
vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_PRIMARY;
@@ -127,10 +122,29 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr
pool,
vol->type = VIR_STORAGE_VOL_BLOCK;
-/* The above gets allocation wrong for
- * extended partitions, so overwrite it */
-vol->target.allocation = vol->target.capacity =
-(vol->source.extents[0].end - vol->source.extents[0].start);
+/* Refresh allocation/capacity/perms
+ *
+ * For an extended partition, virStorageBackendUpdateVolInfo will
+ * return incorrect values for allocation and capacity, so use the
+ * extent information captured above instead.
+ *
+ * Also once a logical partition exists or another primary partition
+ * after an extended partition is created an open on the extended
+ * partition will fail, so pass the NOERROR flag and only error if a
+ * -1 was returned indicating some other error than an open error.
+ */
+if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) {
+if (virStorageBackendUpdateVolInfo(vol, true, false,
+ VIR_STORAGE_VOL_OPEN_DEFAULT |
+ VIR_STORAGE_VOL_OPEN_NOERROR) == -1)
+return -1;
+vol->target.allocation = vol->target.capacity =
+(vol->source.extents[0].end - vol->source.extents[0].start);
+} else {
+if (virStorageBackendUpdateVolInfo(vol, true, false,
+ VIR_STORAGE_VOL_OPEN_DEFAULT) < 0)
+return -1;
+}
if (STRNEQ(groups[2], "metadata"))
pool->def->allocation += vol->target.allocation;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/6] storage: Fix check for partition type for disk backing volumes
While checking the existing partitions in virStorageBackendDiskPartFormat,
the code would erroneously compare the volume target format type (eg, the
virStoragePartedFsType) rather than the source partition type (eg, the
virStorageVolTypeDisk) which is set during virStorageBackendDiskReadPartitions.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_disk.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 0c4126a..31b6025 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -495,8 +495,8 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
if (vol->target.format == VIR_STORAGE_VOL_DISK_EXTENDED) {
/* make sure we don't have a extended partition already */
for (i = 0; i < pool->volumes.count; i++) {
-if (pool->volumes.objs[i]->target.format ==
-VIR_STORAGE_VOL_DISK_EXTENDED) {
+if (pool->volumes.objs[i]->source.partType ==
+VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("extended partition already exists"));
return -1;
@@ -517,8 +517,8 @@ virStorageBackendDiskPartFormat(virStoragePoolObjPtr pool,
case VIR_STORAGE_VOL_DISK_TYPE_LOGICAL:
/* make sure we have a extended partition */
for (i = 0; i < pool->volumes.count; i++) {
-if (pool->volumes.objs[i]->target.format ==
-VIR_STORAGE_VOL_DISK_EXTENDED) {
+if (pool->volumes.objs[i]->source.partType ==
+VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) {
if (virAsprintf(partFormat, "logical %s",
partedFormat) < 0)
return -1;
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCHv2 0/6] Resolve issues in disk pool backend
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
v1: http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html
In my previous attempt to resolve the issue, I created a stateDir and
saved the volume XML for each pool in order to attempt to preserve the
volume "name" and "target format type". However, it was pointed out during
review that having a different "name" was not the original design intention.
The intention was the name would be the partition name rather than something
the user fabricates and expects to be saved/used. Since partition naming
is handled by parted, the control over the name is not ours. Additionally,
a pool refresh or libvirtd restart/reload would use the 'default' of the
source device path and the partition number from the partition table.
The "simple" fix to the issue is in patch 6; however, as I was going through
fixing this I realized a few things and found a few other issues along the
way, namely:
Patches #1 & #2:
After creating the partition if we fail something within the callback to
virStorageBackendDiskMakeDataVol as a result of parsing the partition table
from libvirt_parthelper, then the partition wasn't removed. So, patches
1 & 2 work at moving the DeleteVol code and then calling it if something
in virStorageBackendDiskReadPartitions fails.
Patch #3:
When determining what type of partitions currently exist in the pool we
compared against the target.format which is supposed to be the file system
format type of the partition on the target rather than whether the partition
is a primary, extended, or logical partition on the source. Since we set
the partType on the source while reading the partitions, that's what we
should use.
Patch #4:
During a refresh of the pool, we use virStorageBackendDiskReadPartitions
in order to determine what types of partitions exist; however, if we
have an extended partition and have created either a logical partition
within or another primary partition after the extended one, then the
open() will fail with ENXIO. So I special cased that.
Patch #5:
When we delete an extended partition, any logical partitions that existed
in the pool would still be listed even though as part of the delete
partition logic all the logical partitions were also deleted.
Patch #6:
So here is essentially the replacement for the previous patch series.
Since the theory is one is supposed to know what they are doing and
will provide the correct vol->name, make sure that they do so and cause
a failure if they don't (indicating what the name should be). As an
alternative we could just "overwrite" the name like we did anyway with
pool refresh or libvirtd restart/reload by doing the following instead:
/* Like the reload/restart/refresh path - use the name created by
* parted rather than the API/user provided name
*/
if (STRNEQ(vol->name, partname)) {
VIR_FREE(vol->name);
if (VIR_STRDUP(vol->name, partname) < 0)
return -1;
}
I also added details to the virsh.pod and formatstorage.html.in for
the 'name' and the intersting "side effect" I found using 'virsh
vol-create-as $pool $name $size --format extended'. I'd remove it,
but I fear that someone else found this and has made use of it. The
reality is that '--format' is supposed to be the file system format
of the partition. However, the way it's used in the code will take
what is supposed to be target format and allow creation of an extended
partition. In virStorageBackendDiskPartFormat, if the pool source.format
is DOS and the vol->target.format is VIR_STORAGE_VOL_DISK_EXTENDED, then
we create an extended source partition as long as one doesn't already exist.
John Ferlan (6):
storage: Move virStorageBackendDiskDeleteVol
storage: Attempt error recovery in virStorageBackendDiskCreateVol
storage: Fix check for partition type for disk backing volumes
storage: Adjust how to refresh extended partition disk data
storage: When delete extended partition, need to refresh pool
storage: Check the partition name against provided name
docs/formatstorage.html.in | 12 +-
src/storage/storage_backend.c | 4 +
src/storage/storage_backend_disk.c | 235 +++--
tools/virsh.pod| 17 ++-
4 files changed, 174 insertions(+), 94 deletions(-)
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/6] storage: Move virStorageBackendDiskDeleteVol
Move the API to before virStorageBackendDiskCreateVol in order to be
able to call the DeleteVol API when virStorageBackendDiskReadPartitions
fails so that we don't by chance leave a partition on the disk.
Signed-off-by: John Ferlan
---
src/storage/storage_backend_disk.c | 137 +++--
1 file changed, 69 insertions(+), 68 deletions(-)
diff --git a/src/storage/storage_backend_disk.c
b/src/storage/storage_backend_disk.c
index 3f97fd9..60f8393 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -640,6 +640,75 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr
pool,
static int
+virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virStoragePoolObjPtr pool,
+ virStorageVolDefPtr vol,
+ unsigned int flags)
+{
+char *part_num = NULL;
+char *devpath = NULL;
+char *dev_name, *srcname;
+virCommandPtr cmd = NULL;
+bool isDevMapperDevice;
+int rc = -1;
+
+virCheckFlags(0, -1);
+
+if (virFileResolveLink(vol->target.path, &devpath) < 0) {
+virReportSystemError(errno,
+ _("Couldn't read volume target path '%s'"),
+ vol->target.path);
+goto cleanup;
+}
+
+dev_name = last_component(devpath);
+srcname = last_component(pool->def->source.devices[0].path);
+VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname);
+
+isDevMapperDevice = virIsDevMapperDevice(devpath);
+
+if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Volume path '%s' did not start with parent "
+ "pool source device name."), dev_name);
+goto cleanup;
+}
+
+if (!isDevMapperDevice) {
+part_num = dev_name + strlen(srcname);
+
+if (*part_num == 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse partition number from target "
+ "'%s'"), dev_name);
+goto cleanup;
+}
+
+/* eg parted /dev/sda rm 2 */
+cmd = virCommandNewArgList(PARTED,
+ pool->def->source.devices[0].path,
+ "rm",
+ "--script",
+ part_num,
+ NULL);
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+} else {
+cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath,
NULL);
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+}
+
+rc = 0;
+ cleanup:
+VIR_FREE(devpath);
+virCommandFree(cmd);
+return rc;
+}
+
+
+static int
virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol)
@@ -714,74 +783,6 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn,
return build_func(conn, pool, vol, inputvol, flags);
}
-static int
-virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
- virStoragePoolObjPtr pool,
- virStorageVolDefPtr vol,
- unsigned int flags)
-{
-char *part_num = NULL;
-char *devpath = NULL;
-char *dev_name, *srcname;
-virCommandPtr cmd = NULL;
-bool isDevMapperDevice;
-int rc = -1;
-
-virCheckFlags(0, -1);
-
-if (virFileResolveLink(vol->target.path, &devpath) < 0) {
-virReportSystemError(errno,
- _("Couldn't read volume target path '%s'"),
- vol->target.path);
-goto cleanup;
-}
-
-dev_name = last_component(devpath);
-srcname = last_component(pool->def->source.devices[0].path);
-VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname);
-
-isDevMapperDevice = virIsDevMapperDevice(devpath);
-
-if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Volume path '%s' did not start with parent "
- "pool source device name."), dev_name);
-goto cleanup;
-}
-
-if (!isDevMapperDevice) {
-part_num = dev_name + strlen(srcname);
-
-if (*part_num == 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot parse partition number from target "
- "'%s'"), dev_name);
-goto cleanup;
-}
-
-/* eg parted /dev/sda rm 2 */
-cmd = virCommandNewArgList(PARTED,
- pool->def->source.devices[0].path,
- "rm",
- "--script",
-
Re: [libvirt] [PATCH 2/2] esx_vi: fix possible segfault
On 01/21/2015 06:47 PM, Peter Krempa wrote: On Wed, Jan 21, 2015 at 18:09:28 +0100, Pavel Hrdina wrote: Clang found possible dereference of NULL pointer which is right. Function 'esxVI_LookupTaskInfoByTask' should find a task info. The issue is that we could return 0 and leave 'taksInfo' pointer NULL because if there is no match we simply end the search loop end set 'result' to 0. Every caller count on the fact that if the return value is 0 than it's safe to dereference 'taskInfo'. We should return 0 only in case we found something and the '*taskInfo' is not NULL. Signed-off-by: Pavel Hrdina --- src/esx/esx_vi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a87f2c0..752d32a 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -3298,7 +3298,8 @@ esxVI_LookupTaskInfoByTask(esxVI_Context *ctx, } } -result = 0; +if (*taskInfo) +result = 0; I think a more obvious fix would be to move setting of result to zero into the condition that breaks the loop. In that case, result gets set to 0 only if the entry was found. cleanup: esxVI_String_Free(&propertyNameList); ACK with or without that change. I think that this wasn't discovered due to the fact that the correct entry is always in the list and is always first as we haven't seen reports for spamming the warning. Peter Pushed with the update that you've proposed, thanks. Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] xenapi_driver: fix copy-paste typo
On 01/21/2015 06:29 PM, Peter Krempa wrote: On Wed, Jan 21, 2015 at 18:09:27 +0100, Pavel Hrdina wrote: Clang found that we are passing variable with wrong enum type to 'xenapiCrashExitEnum2virDomainLifecycle' function. This is probably copy-paste typo as the correct variable exists in the code, but it isn't used. Signed-off-by: Pavel Hrdina --- src/xenapi/xenapi_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ACK, safe for release. Peter Pushed, thanks Pavel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/2] systemd: fix build without dbus
The virDBusMethodCall method has a DBusError as one of its
parameters. If the caller wants to pass a non-NULL value
for this, it immediately makes the calling code require
DBus at build time. This has led to breakage of non-DBus
builds several times. It is desirable that only the virdbus.c
file should need WITH_DBUS conditionals, so we must ideally
remove the DBusError parameter from the method.
We can't simply raise a libvirt error, since the whole point
of this parameter is to give the callers a way to check if
the error is one they want to ignore, without having the logs
polluted with an error message. So, we add a virErrorPtr
parameter which the caller can then either ignore or raise
using the new virReportErrorObject method.
This new method is distinct from virSetError in that it
ensures the logging hooks are run.
Signed-off-by: Daniel P. Berrange
---
po/POTFILES.in | 1 -
src/libvirt_private.syms | 1 +
src/util/virdbus.c | 31 --
src/util/virdbus.h | 4 +-
src/util/virerror.c | 104 +--
src/util/virerror.h | 8
src/util/virfirewall.c | 29 +++--
src/util/virsystemd.c| 15 ---
8 files changed, 125 insertions(+), 68 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 26c098f..3064037 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -217,7 +217,6 @@ src/util/virstorageencryption.c
src/util/virstoragefile.c
src/util/virstring.c
src/util/virsysinfo.c
-src/util/virsystemd.c
src/util/virerror.c
src/util/virerror.h
src/util/virtime.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a2eec83..528e93c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1284,6 +1284,7 @@ virErrorInitialize;
virErrorSetErrnoFromLastError;
virLastErrorIsSystemErrno;
virRaiseErrorFull;
+virRaiseErrorObject;
virReportErrorHelper;
virReportOOMErrorFull;
virReportSystemErrorFull;
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index d9665c1..3522ae0 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1522,7 +1522,7 @@ static int
virDBusCall(DBusConnection *conn,
DBusMessage *call,
DBusMessage **replyout,
-DBusError *error)
+virErrorPtr error)
{
DBusMessage *reply = NULL;
@@ -1530,8 +1530,9 @@ virDBusCall(DBusConnection *conn,
int ret = -1;
const char *iface, *member, *path, *dest;
-if (!error)
-dbus_error_init(&localerror);
+dbus_error_init(&localerror);
+if (error)
+memset(error, 0, sizeof(*error));
iface = dbus_message_get_interface(call);
member = dbus_message_get_member(call);
@@ -1545,13 +1546,20 @@ virDBusCall(DBusConnection *conn,
if (!(reply = dbus_connection_send_with_reply_and_block(conn,
call,
VIR_DBUS_METHOD_CALL_TIMEOUT_MILLIS,
-error ? error :
&localerror))) {
+&localerror))) {
PROBE(DBUS_METHOD_ERROR,
"'%s.%s' on '%s' at '%s' error %s: %s",
iface, member, path, dest,
- error ? error->name : localerror.name,
- error ? error->message : localerror.message);
+ localerror.name,
+ localerror.message);
if (error) {
+error->level = VIR_ERR_ERROR;
+error->code = VIR_ERR_DBUS_SERVICE;
+error->domain = VIR_FROM_DBUS;
+if (VIR_STRDUP(error->message, localerror.message) < 0)
+goto cleanup;
+if (VIR_STRDUP(error->str1, localerror.name) < 0)
+goto cleanup;
ret = 0;
} else {
virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member,
@@ -1567,8 +1575,9 @@ virDBusCall(DBusConnection *conn,
ret = 0;
cleanup:
-if (!error)
-dbus_error_free(&localerror);
+if (ret < 0 && error)
+virResetError(error);
+dbus_error_free(&localerror);
if (reply) {
if (ret == 0 && replyout)
*replyout = reply;
@@ -1616,7 +1625,7 @@ virDBusCall(DBusConnection *conn,
*/
int virDBusCallMethod(DBusConnection *conn,
DBusMessage **replyout,
- DBusError *error,
+ virErrorPtr error,
const char *destination,
const char *path,
const char *iface,
@@ -1634,8 +1643,6 @@ int virDBusCallMethod(DBusConnection *conn,
if (ret < 0)
goto cleanup;
-ret = -1;
-
ret = virDBusCall(conn, call, replyout, error);
cleanup:
@@ -1832,7 +1839,7 @@ int virDBusCreateReply(DBusMessage **reply
ATTRIBUTE_UNUSED,
int virDBusCallMethod(DBusConnection *conn ATTRIBUTE_UNUSED,
[libvirt] [PATCH v3 2/2] systemd: avoid string comparisons on dbus error messages
Add a virDBusErrorIsUnknownMethod helper so that callers
don't need todo string comparisons themselves to detect
standard error names.
---
src/libvirt_private.syms | 1 +
src/util/virdbus.c | 9 +
src/util/virdbus.h | 2 ++
src/util/virsystemd.c| 3 +--
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 528e93c..a8cd87f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1244,6 +1244,7 @@ virDBusCreateMethod;
virDBusCreateMethodV;
virDBusCreateReply;
virDBusCreateReplyV;
+virDBusErrorIsUnknownMethod;
virDBusGetSessionBus;
virDBusGetSystemBus;
virDBusHasSystemBus;
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 3522ae0..1cf1eef 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1894,3 +1894,12 @@ void virDBusMessageUnref(DBusMessage *msg
ATTRIBUTE_UNUSED)
/* nothing */
}
#endif /* ! WITH_DBUS */
+
+bool virDBusErrorIsUnknownMethod(virErrorPtr err)
+{
+return err->domain == VIR_FROM_DBUS &&
+err->code == VIR_ERR_DBUS_SERVICE &&
+err->level == VIR_ERR_ERROR &&
+STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
+ err->str1);
+}
diff --git a/src/util/virdbus.h b/src/util/virdbus.h
index e2b8d2b..9e86538 100644
--- a/src/util/virdbus.h
+++ b/src/util/virdbus.h
@@ -74,4 +74,6 @@ void virDBusMessageUnref(DBusMessage *msg);
int virDBusIsServiceEnabled(const char *name);
int virDBusIsServiceRegistered(const char *name);
+
+bool virDBusErrorIsUnknownMethod(virErrorPtr err);
#endif /* __VIR_DBUS_H__ */
diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
index 6de265b..3ac399a 100644
--- a/src/util/virsystemd.c
+++ b/src/util/virsystemd.c
@@ -281,8 +281,7 @@ int virSystemdCreateMachine(const char *name,
goto cleanup;
if (error.level == VIR_ERR_ERROR) {
-if (STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
- error.str1)) {
+if (virDBusErrorIsUnknownMethod(&error)) {
VIR_INFO("CreateMachineWithNetwork isn't supported, switching "
"to legacy CreateMachine method for
systemd-machined");
virResetError(&error);
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Assert with libvirt + xen hvm
We're hitting an assert whenever we try to create an HVM instance under Xen via libvirtd. System is running on Gentoo, package information as follows: app-emulation/xen-4.5.0 USE="api debug flask hvm pam pygrub python qemu screen" app-emulation/xen-tools-4.5.0 USE="api debug flask hvm pam pygrub python qemu screen" app-emulation/libvirt-1.2.11-r2:0/1.2.11 USE="caps libvirtd lvm macvtap nls qemu udev vepa virtualbox xen" The following commands are run in parallel: vmmachine ~ # libvirtd --listen 2015-01-22 16:33:13.596+: 2620: info : libvirt version: 1.2.11 2015-01-22 16:33:13.596+: 2620: error : udevGetDMIData:1607 : Failed to get udev device for syspath '/sys/devices/virtual/dmi/id' or '/sys/class/dmi/id' libvirtd: libxl_fork.c:350: sigchld_installhandler_core: Assertion `((void)"application must negotiate with libxl about SIGCHLD", !(sigchld_saved_action.sa_flags & 4) && (sigchld_saved_action.__sigaction_handler.sa_handler == ((__sighandler_t) 0) || sigchld_saved_action.__sigaction_handler.sa_handler == ((__sighandler_t) 1)))' failed. Aborted vmmachine ~ # VIRSH_DEBUG=0 virsh create xml create: file(optdata): xml libvirt: XML-RPC error : End of file while reading data: Input/output error error: Failed to create domain from xml error: End of file while reading data: Input/output error libvirt: Domain Config error : Requested operation is not valid: A different callback was requested -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Grant access to helpers
On Thursday, January 22, 2015 08:55:07 AM Cedric Bosdonnat wrote: > > Seems like the apparmor profile for libvirtd is pretty wide open, so I'm > > not sure if there will be much of a difference between those two > > settings. I'm also not sure how best to test the functionality of those > > helpers to find out... Jim Fehlig and I just tested this and were able to show that 'ix' is sufficient for the helpers to work properly. Thanks for pointing this out Cedric. Can you just change the patch when you commit? > Jamie, as apparmor expert, do you have any opinion on this? Still would be great to hear Jamie's opinion on this. -Mike -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] util: bitmap: Tolerate NULL bitmaps in virBitmapEqual
On 22.01.2015 11:53, Peter Krempa wrote:
> ---
> src/util/virbitmap.c | 6 ++
> src/util/virbitmap.h | 3 +--
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> index 05c50e4..d5b0035 100644
> --- a/src/util/virbitmap.c
> +++ b/src/util/virbitmap.c
> @@ -504,6 +504,12 @@ bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2)
> virBitmapPtr tmp;
> size_t i;
>
> +if (!b1 && !b2)
> +return true;
> +
> +if (!b1 || !b2)
> +return false;
> +
> if (b1->max_bit > b2->max_bit) {
> tmp = b1;
> b1 = b2;
> diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
> index 565264c..a347f0a 100644
> --- a/src/util/virbitmap.h
> +++ b/src/util/virbitmap.h
> @@ -84,8 +84,7 @@ virBitmapPtr virBitmapNewData(void *data, int len)
> ATTRIBUTE_NONNULL(1);
> int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> -bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2)
> -ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2);
>
> size_t virBitmapSize(virBitmapPtr bitmap)
> ATTRIBUTE_NONNULL(1);
>
There are some places in the code that already call this and do the
checks you're introducing. Can we drop the duplicity then?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] qemu: Fix auto-adding PCI bridge when all slots are reserved
On 01/21/2015 05:50 PM, Erik Skultety wrote:
> Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge
> controller, it worked well when the slots were reserved by devices with
> user defined addresses without any other devices with unspecified PCI
> addresses
> present in the XML.
> However, if another device with unspecified PCI addresses appeared in
> the domain's XML, the same problem occurred as the BZ below references.
>
> This patch fixes the issue by not creating any additional bridges when not
> necessary.
> Now let's say all the buses corresponding to the number of PCI controllers
> present in the domain's XML got fully reserved by devices with user defined
> addresses. If there are no other devices present in the XML, no need to
> create both an additional PCI bus and a bridge controller.
> If however there are still some PCI devices waiting to get a slot assigned
> by qemuAssignDevicePCISlots, this means an additional bus needs to
> be created along with a corresponding bridge controller.
> This scenario now results in an reasonable error instead of generating wrong
> qemu
> command line.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1132900
> ---
> src/qemu/qemu_command.c | 42 ++
> 1 file changed, 34 insertions(+), 8 deletions(-)
>
This breaks the pci-many test case you added before.
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index cdf02a6..99b06ee 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1472,6 +1472,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> int nbuses = 0;
> size_t i;
> int rv;
> +int resflags = 0;
> +bool buses_reserved = false;
> +
If you set this to true by default...
> virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCI;
>
> for (i = 0; i < def->ncontrollers; i++) {
> @@ -1490,17 +1493,22 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> /* 1st pass to figure out how many PCI bridges we need */
> if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
> goto cleanup;
> +
> +for (i = 0; i < nbuses; i++) {
> +if (qemuDomainPCIBusFullyReserved(&addrs->buses[i]))
> +resflags |= 1 << i;
you can do:
if (!fullyReserved(buses[i]))
buses_reserved = false;
Or just rename the bool to something like 'addExtraEmptySlot' and invert the
condition below. (To stay more positive :))
> +}
> +buses_reserved = (resflags && ~((~0) << nbuses));
> +
> if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> goto cleanup;
>
> -for (i = 0; i < addrs->nbuses; i++) {
> -if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) {
> -
> -/* Reserve 1 extra slot for a (potential) bridge */
> -if (virDomainPCIAddressReserveNextSlot(addrs, &info,
> flags) < 0)
> -goto cleanup;
> -}
> -}
> +/* Reserve 1 extra slot for a (potential) bridge only if buses
> + * are not fully reserved yet
> + */
> +if (!buses_reserved &&
> +virDomainPCIAddressReserveNextSlot(addrs, &info, flags) < 0)
> +goto cleanup;
>
> for (i = 1; i < addrs->nbuses; i++) {
> virDomainPCIAddressBusPtr bus = &addrs->buses[i];
> @@ -1532,6 +1540,24 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> goto cleanup;
> }
> +
> +for (i = 0; i < def->ncontrollers; i++) {
> +/* check if every PCI bridge controller's ID is greater than
> + * the bus it is placed onto
> + */
> +virDomainControllerDefPtr cont = def->controllers[i];
> +int idx = cont->idx;
> +int bus = cont->info.addr.pci.bus;
> +if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +cont->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE &&
> +idx <= bus) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("failed to create PCI bridge "
> + "on bus %d: bus is fully reserved"),
> + bus);
> +goto cleanup;
> +}
> +}
This should IMHO be a separate commit, as it does not fix the case where the
devices exactly fill the buses, but when there are too many.
Also, maybe the error message could say something about 'too many devices with
fixed addressess'?
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] qemu: move PCI slot assignment for PIIX3, Q35 into a separate function
On 01/21/2015 05:50 PM, Erik Skultety wrote:
> In order to be able to test for fully reserved PCI buses, assignment of
> PCI slots for integrated devices needs to be moved to a separate function.
> This also might be a good preparation if we decide to add support for
> other chipsets as well.
> ---
> src/qemu/qemu_command.c | 45 ++---
> 1 file changed, 30 insertions(+), 15 deletions(-)
>
This should be done before fixing the bug in 3/5, and it also doesn't compile.
Jan
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 99b06ee..dbcc854 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1494,6 +1494,9 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true)))
> goto cleanup;
>
> +if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0)
> +goto cleanup;
> +
> for (i = 0; i < nbuses; i++) {
> if (qemuDomainPCIBusFullyReserved(&addrs->buses[i]))
> resflags |= 1 << i;
> @@ -1537,6 +1540,8 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> goto cleanup;
>
> if (qemuDomainSupportsPCI(def)) {
> +if (qemuValidateDevicePCISlotsChipsets(def, qemuCaps, addrs) < 0)
> +goto cleanup;
> if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
> goto cleanup;
> }
> @@ -1996,6 +2001,27 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr
> def,
> return ret;
> }
>
> +static int
> +qemuValidateDevicePCISlotsChipsets(virDomainDefPtr def,
> + virQEMUCapsPtr qemuCaps,
> + virDomainPCIAddressSetPtr addrs)
> +{
> +if ((STRPREFIX(def->os.machine, "pc-0.") ||
> +STRPREFIX(def->os.machine, "pc-1.") ||
> +STRPREFIX(def->os.machine, "pc-i440") ||
> +STREQ(def->os.machine, "pc") ||
> +STRPREFIX(def->os.machine, "rhel")) &&
> +qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) {
> +return -1;
> +}
> +
> +if (qemuDomainMachineIsQ35(def) &&
> +qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) {
> +return -1;
> +}
> +
> +return 0;
> +}
>
> /*
> * This assigns static PCI slots to all configured devices.
> @@ -2014,6 +2040,9 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
> * - PIIX3 ISA bridge, IDE controller, something else unknown, USB
> controller (slot 1)
> * - Video (slot 2)
> *
> + * - These integrated devices were already added by
> + *qemuValidateDevicePCISlotsChipsets invoked right before this function
> + *
> * Incrementally assign slots from 3 onwards:
> *
> * - Net
> @@ -2031,27 +2060,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr
> def,
> */
> int
> qemuAssignDevicePCISlots(virDomainDefPtr def,
> - virQEMUCapsPtr qemuCaps,
> + virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED,
> virDomainPCIAddressSetPtr addrs)
> {
> size_t i, j;
> virDomainPCIConnectFlags flags;
> virDevicePCIAddress tmp_addr;
>
> -if ((STRPREFIX(def->os.machine, "pc-0.") ||
> -STRPREFIX(def->os.machine, "pc-1.") ||
> -STRPREFIX(def->os.machine, "pc-i440") ||
> -STREQ(def->os.machine, "pc") ||
> -STRPREFIX(def->os.machine, "rhel")) &&
> -qemuValidateDevicePCISlotsPIIX3(def, qemuCaps, addrs) < 0) {
> -goto error;
> -}
> -
> -if (qemuDomainMachineIsQ35(def) &&
> -qemuDomainValidateDevicePCISlotsQ35(def, qemuCaps, addrs) < 0) {
> -goto error;
> -}
> -
> /* PCI controllers */
> for (i = 0; i < def->ncontrollers; i++) {
> if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
>
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] Const correctnes/random fixes
On 22.01.2015 11:53, Peter Krempa wrote: > Few fixes/tweaks that have accumulated in my memory hotplug branch. They > should > be trivial enough and semantically inert. > > Peter Krempa (5): > conf: Fix comment mentioning actual type of @multi member of > virDevicePCIAddress > qemu: command: Honor const-correctnes in qemuBuildNumaArgStr > util: json: Make argument of virJSONValueArraySize const > schemas: Move definition of 'hexuint' to basictypes > util: bitmap: Tolerate NULL bitmaps in virBitmapEqual > > docs/schemas/basictypes.rng | 6 ++ > docs/schemas/nodedev.rng| 6 -- > src/conf/device_conf.h | 2 +- > src/qemu/qemu_command.c | 2 +- > src/util/virbitmap.c| 6 ++ > src/util/virbitmap.h| 3 +-- > src/util/virjson.c | 2 +- > src/util/virjson.h | 2 +- > 8 files changed, 17 insertions(+), 12 deletions(-) > ACK to 1-4, and safe for freeze. I'd like to see 5/5 removing more lines. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] systemd: don't report an error if the guest is already terminated
On Thu, Jan 22, 2015 at 04:22:36PM +0100, Michal Privoznik wrote: > On 16.01.2015 18:36, Daniel P. Berrange wrote: > > In many cases where we invoke virSystemdTerminateMachine the > > process(es) will have already gone away on their own accord. > > In these cases we log an error message that the machine does > > not exist. We should catch this particular error and simply > > ignore it, so we don't pollute the logs. > > --- > > src/util/virsystemd.c | 25 - > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c > > index 3eea5c2..29a2e63 100644 > > --- a/src/util/virsystemd.c > > +++ b/src/util/virsystemd.c > > @@ -340,18 +340,22 @@ int virSystemdTerminateMachine(const char *name, > > int ret; > > DBusConnection *conn; > > char *machinename = NULL; > > +DBusError error; > > + > > +dbus_error_init(&error); > > This will suffer the same issue we already have in the code in > virSystemdCreateMachine if libvirt is built without DBUS. Yep, this patch will need re-working once my other patch to change virDBusCallMethod to take a virErrorPtr is merged. I'll not push this series until after the release now, since it is a feature rather than bugfix Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs
On 22.01.2015 15:52, Ján Tomko wrote:
> On 01/22/2015 03:21 PM, Michal Privoznik wrote:
>> On 22.01.2015 12:26, Ján Tomko wrote:
>>> Per-cpu stats are only shown for present CPUs in the cgroups,
>>> but we were only parsing the largest CPU number from
>>> /sys/devices/system/cpu/present and looking for stats even for
>>> non-present CPUs.
>>> This resulted in:
>>> internal error: cpuacct parse error
>>> ---
>>> cfg.mk| 2 +-
>>> src/nodeinfo.c| 17 +
>>> src/nodeinfo.h| 1 +
>>> src/util/vircgroup.c | 24 ++--
>>> tests/vircgroupmock.c | 44 ++--
>>> tests/vircgrouptest.c | 47 +++
>>> 6 files changed, 110 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/cfg.mk b/cfg.mk
>>> index 21f83c3..70612f8 100644
>>> --- a/cfg.mk
>>> +++ b/cfg.mk
>>> @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
>>>^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>>>
>>> exclude_file_name_regexp--sc_prohibit_strdup = \
>>> - ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
>>> +
>>> ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>>>
>>> exclude_file_name_regexp--sc_prohibit_close = \
>>>
>>> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
>>> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
>>> index 3c22ebc..3a27c22 100644
>>> --- a/src/nodeinfo.c
>>> +++ b/src/nodeinfo.c
>>> @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void)
>>> }
>>>
>>> virBitmapPtr
>>> +nodeGetPresentCPUBitmap(void)
>>> +{
>>> +int max_present;
>>> +
>>> +if ((max_present = nodeGetCPUCount()) < 0)
>>> +return NULL;
>>> +
>>> +#ifdef __linux__
>>> +if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present"))
>>> +return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH
>>> "/cpu/present");
>>> +#endif
>>> +virReportError(VIR_ERR_NO_SUPPORT, "%s",
>>> + _("non-continuous host cpu numbers not implemented on
>>> this platform"));
>>> +return NULL;
>>> +}
>>> +
>>> +virBitmapPtr
>>> nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
>>> {
>>
>> >From my understanding the nodeGetPresentCPUBitmap() is no different than
>> nodeGetCPUBitmap(). The latter can do everything that the former and
>> more. Can we just use already existing function then?
>>
>
> This functions checks the present CPUs, nodeGetCPUBitmap checks the online
> CPUs.
>
> I don't think splitting their common part into another function would be more
> readable - IMO the fact that both use linuxParseCPUmap should be enough.
Okay, ACK then.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] systemd: don't report an error if the guest is already terminated
On 16.01.2015 18:36, Daniel P. Berrange wrote:
> In many cases where we invoke virSystemdTerminateMachine the
> process(es) will have already gone away on their own accord.
> In these cases we log an error message that the machine does
> not exist. We should catch this particular error and simply
> ignore it, so we don't pollute the logs.
> ---
> src/util/virsystemd.c | 25 -
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c
> index 3eea5c2..29a2e63 100644
> --- a/src/util/virsystemd.c
> +++ b/src/util/virsystemd.c
> @@ -340,18 +340,22 @@ int virSystemdTerminateMachine(const char *name,
> int ret;
> DBusConnection *conn;
> char *machinename = NULL;
> +DBusError error;
> +
> +dbus_error_init(&error);
This will suffer the same issue we already have in the code in
virSystemdCreateMachine if libvirt is built without DBUS.
>
> ret = virDBusIsServiceEnabled("org.freedesktop.machine1");
> if (ret < 0)
> -return ret;
> +goto cleanup;
>
> if ((ret = virDBusIsServiceRegistered("org.freedesktop.systemd1")) < 0)
> -return ret;
> +goto cleanup;
> +
> +ret = -1;
>
> if (!(conn = virDBusGetSystemBus()))
> -return -1;
> +goto cleanup;
>
> -ret = -1;
> if (!(machinename = virSystemdMakeMachineName(name, drivername,
> privileged)))
> goto cleanup;
>
> @@ -368,7 +372,7 @@ int virSystemdTerminateMachine(const char *name,
> VIR_DEBUG("Attempting to terminate machine via systemd");
> if (virDBusCallMethod(conn,
>NULL,
> - NULL,
> + &error,
>"org.freedesktop.machine1",
>"/org/freedesktop/machine1",
>"org.freedesktop.machine1.Manager",
> @@ -377,9 +381,20 @@ int virSystemdTerminateMachine(const char *name,
>machinename) < 0)
> goto cleanup;
>
> +if (dbus_error_is_set(&error) &&
> +!STREQ_NULLABLE("org.freedesktop.machine1.NoSuchMachine",
> +error.name)) {
> +virReportError(VIR_ERR_DBUS_SERVICE,
> + _("TerminateMachine: %s"),
> + error.message ? error.message : _("unknown error"));
> +goto cleanup;
> +}
> +
> ret = 0;
>
> cleanup:
> +dbus_error_free(&error);
> +
> VIR_FREE(machinename);
> return ret;
> }
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/8] Enable NIC reporting to systemd
On 16.01.2015 18:36, Daniel P. Berrange wrote: > This series enables the QEMU and LXC drivers to report the > network interface backends they use to systemd. This gets > then shown to the user in > > # machinectl status lxc-shell > lxc-shell(95449419f969d649d9962566ec42af7d) > Since: Fri 2015-01-16 16:53:37 GMT; 3s ago > Leader: 28085 (sh) >Service: libvirt-lxc; class container > Iface: vnet0 >Address: fe80::216:3eff:fe00:c317%124 > OS: Fedora 21 (Twenty One) > Unit: machine-lxc\x2dshell.scope > └─28085 /bin/sh > > but most fun is that if you add nss-mymachines to the > /etc/nsswitch.conf, the machine names can now be used > as hostnames. > > eg if your guest is 'lxc-shell' this lets you just do > 'ssh lxc-shell' and it'll use the detected link local > address to connect. > > Daniel P. Berrange (8): > qemu: report TAP device indexes to systemd > systemd: don't report an error if the guest is already terminated > lxc: don't build pidfile string multiple times > lxc: re-arrange startup synchronization sequence with controller > lxc: only write XML once for lxc controller > lxc: delay setup of cgroup until we have the init pid > lxc: more logging during startup paths > lxc: report veth device indexes to systemd > > src/conf/domain_conf.c | 2 +- > src/conf/domain_conf.h | 5 ++ > src/lxc/lxc_cgroup.c | 15 +++--- > src/lxc/lxc_cgroup.h | 5 +- > src/lxc/lxc_container.c | 8 +++ > src/lxc/lxc_controller.c | 116 > src/lxc/lxc_process.c| 134 > +++ > src/qemu/qemu_cgroup.c | 13 +++-- > src/qemu/qemu_cgroup.h | 4 +- > src/qemu/qemu_command.c | 27 -- > src/qemu/qemu_command.h | 7 ++- > src/qemu/qemu_driver.c | 7 ++- > src/qemu/qemu_hotplug.c | 4 +- > src/qemu/qemu_process.c | 9 +++- > src/util/virsystemd.c| 25 +++-- > tests/qemuxml2argvtest.c | 5 +- > tests/qemuxmlnstest.c| 6 ++- > 17 files changed, 288 insertions(+), 104 deletions(-) > Looking good. ACK to all, but before pushing we should fix the issue I've raised in 2/8. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: verify proper address family in updates to and
On 01/22/2015 03:47 AM, Ján Tomko wrote: > On 01/19/2015 11:04 PM, Laine Stump wrote: >> By specifying parentIndex in a call to virNetworkUpdate(), it was >> possible to direct libvirt to add a dhcp range or static host of a >> non-matching address family to the element of an . For >> example, given: >> >> >> >> >> you could provide a static host entry with an IPv4 address, and >> specify that it be added to the 2nd element (index 1): >> >> virsh net-update default add ip-dhcp-host --parent-index 1 \ >> '' >> >> This would be happily added with no error (and no concern of any >> possible future consequences). >> >> This patch checks that any dhcp range or host element being added to a >> network ip's subelement has addresses of the same family as the >> ip element they are being added to. >> >> This problem was noticed when looking at the reproduction case for >> https://bugzilla.redhat.com/show_bug.cgi?id=1182486 (but is not a >> solution to that bug). >> --- >> src/conf/network_conf.c | 18 +- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> > ACK > > Although it would be nicer if we could also find the correct element > matching the address when --parent-index is not specified. With the ipv4 > section first I get an error trying to add an ipv6 host: Good point! We do search for the first element that has a , but we don't pay attention to whether or not the address family is the same. I'll whip up another patch to take care of that. > > virsh net-update default add-first ip-dhcp-host ipv6.xml > error: Failed to update network default > error: Requested operation is not valid: the address family of a host entry IP > must match the address family of the dhcp element's parent > > Jan > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs
On 01/22/2015 03:21 PM, Michal Privoznik wrote:
> On 22.01.2015 12:26, Ján Tomko wrote:
>> Per-cpu stats are only shown for present CPUs in the cgroups,
>> but we were only parsing the largest CPU number from
>> /sys/devices/system/cpu/present and looking for stats even for
>> non-present CPUs.
>> This resulted in:
>> internal error: cpuacct parse error
>> ---
>> cfg.mk| 2 +-
>> src/nodeinfo.c| 17 +
>> src/nodeinfo.h| 1 +
>> src/util/vircgroup.c | 24 ++--
>> tests/vircgroupmock.c | 44 ++--
>> tests/vircgrouptest.c | 47 +++
>> 6 files changed, 110 insertions(+), 25 deletions(-)
>>
>> diff --git a/cfg.mk b/cfg.mk
>> index 21f83c3..70612f8 100644
>> --- a/cfg.mk
>> +++ b/cfg.mk
>> @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
>>^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>>
>> exclude_file_name_regexp--sc_prohibit_strdup = \
>> - ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
>> +
>> ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>>
>> exclude_file_name_regexp--sc_prohibit_close = \
>>
>> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
>> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
>> index 3c22ebc..3a27c22 100644
>> --- a/src/nodeinfo.c
>> +++ b/src/nodeinfo.c
>> @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void)
>> }
>>
>> virBitmapPtr
>> +nodeGetPresentCPUBitmap(void)
>> +{
>> +int max_present;
>> +
>> +if ((max_present = nodeGetCPUCount()) < 0)
>> +return NULL;
>> +
>> +#ifdef __linux__
>> +if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present"))
>> +return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH
>> "/cpu/present");
>> +#endif
>> +virReportError(VIR_ERR_NO_SUPPORT, "%s",
>> + _("non-continuous host cpu numbers not implemented on
>> this platform"));
>> +return NULL;
>> +}
>> +
>> +virBitmapPtr
>> nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
>> {
>
>>From my understanding the nodeGetPresentCPUBitmap() is no different than
> nodeGetCPUBitmap(). The latter can do everything that the former and
> more. Can we just use already existing function then?
>
This functions checks the present CPUs, nodeGetCPUBitmap checks the online CPUs.
I don't think splitting their common part into another function would be more
readable - IMO the fact that both use linuxParseCPUmap should be enough.
Jan
> Otherwise looking good.
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs
On 22.01.2015 12:26, Ján Tomko wrote:
> Per-cpu stats are only shown for present CPUs in the cgroups,
> but we were only parsing the largest CPU number from
> /sys/devices/system/cpu/present and looking for stats even for
> non-present CPUs.
> This resulted in:
> internal error: cpuacct parse error
> ---
> cfg.mk| 2 +-
> src/nodeinfo.c| 17 +
> src/nodeinfo.h| 1 +
> src/util/vircgroup.c | 24 ++--
> tests/vircgroupmock.c | 44 ++--
> tests/vircgrouptest.c | 47 +++
> 6 files changed, 110 insertions(+), 25 deletions(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index 21f83c3..70612f8 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
>^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>
> exclude_file_name_regexp--sc_prohibit_strdup = \
> - ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
> +
> ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>
> exclude_file_name_regexp--sc_prohibit_close = \
>
> (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 3c22ebc..3a27c22 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void)
> }
>
> virBitmapPtr
> +nodeGetPresentCPUBitmap(void)
> +{
> +int max_present;
> +
> +if ((max_present = nodeGetCPUCount()) < 0)
> +return NULL;
> +
> +#ifdef __linux__
> +if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present"))
> +return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH
> "/cpu/present");
> +#endif
> +virReportError(VIR_ERR_NO_SUPPORT, "%s",
> + _("non-continuous host cpu numbers not implemented on
> this platform"));
> +return NULL;
> +}
> +
> +virBitmapPtr
> nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
> {
>From my understanding the nodeGetPresentCPUBitmap() is no different than
nodeGetCPUBitmap(). The latter can do everything that the former and
more. Can we just use already existing function then?
Otherwise looking good.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] qemu: Fix job type in qemuDomainGetBlockIoTune
On 01/22/2015 10:20 AM, Peter Krempa wrote: > The function just queries status so there's no need for a MODIFY type > job. > --- > src/qemu/qemu_driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) ACK Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] qemu: Job handling fixes
On 01/22/2015 10:20 AM, Peter Krempa wrote: > While reviewing Martin's reference counting series I've noticed a few qemu API > impls that don't properly handle jobs. > > Peter Krempa (7): > qemu: Fix job handling in qemuDomainPinVcpuFlags > qemu: Fix job handling in qemuDomainPinEmulator > qemu: Fix job handling in qemuDomainSetAutostart > qemu: Fix job handling in qemuDomainSetMemoryParameters > qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags > qemu: Fix job type in qemuDomainGetBlockIoTune > qemu: Fix job handling in qemuDomainSetMetadata > > src/qemu/qemu_driver.c | 135 > +++-- > 1 file changed, 87 insertions(+), 48 deletions(-) > In patches 1-3,5 it's necessary to check if the domain is still alive after getting the job. I suggest moving the BeginJob between the ACL check and virDomainLiveConfigHelperMethod, which does check for domain liveness. ACK with that fix. Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/7] qemu: Fix job handling in qemuDomainSetAutostart
On 01/22/2015 10:20 AM, Peter Krempa wrote: > The code modifies the domain configuration but doesn't take a MODIFY > type job to do so. > > This patch also fixes a few very long lines of code around the touched > parts. > --- > src/qemu/qemu_driver.c | 24 +--- > 1 file changed, 17 insertions(+), 7 deletions(-) > ACK Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/7] qemu: Fix job handling in qemuDomainSetMetadata
On 01/22/2015 10:20 AM, Peter Krempa wrote: > The code modifies the domain configuration but doesn't take a MODIFY > type job to do so. > --- > src/qemu/qemu_driver.c | 5 + > 1 file changed, 5 insertions(+) ACK After getting the job, the virDomainLiveConfigHelperMethod is called inside virDomainObjSetMetadata checking if the domain is alive again. Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] qemu: output error when try to hotplug unsupport console
On 01/22/2015 09:25 PM, Peter Krempa wrote: On Thu, Jan 22, 2015 at 21:20:46 +0800, Luyao Huang wrote: On 01/22/2015 06:19 PM, Peter Krempa wrote: On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1164627 ... ACK, although the rest of the chardev command and hotplug code is a big mess and would benefit from a cleanup/refactor. Yes, i noticed this mess and i will try to fix this in next step. I don't insist on you doing that. It was just an observation from looking through the code. yes, I know that, i am just interested in this part of code :) Although i know there are some thorny problems when do a refactor, and seems this will take some time to do it. Peter Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] CVE-2015-0236: qemu: Check ACLs when dumping security info from snapshots
The ACL check didn't check the VIR_DOMAIN_XML_SECURE flag and the
appropriate permission for it. Found via code inspection while fixing
permissions for save images.
---
src/qemu/qemu_driver.c | 2 +-
src/remote/remote_protocol.x | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c9110f0..bc6aae4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14406,7 +14406,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr
snapshot,
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
return NULL;
-if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, vm->def)
< 0)
+if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, vm->def,
flags) < 0)
goto cleanup;
if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 15694fa..c8162a5 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -4489,6 +4489,7 @@ enum remote_procedure {
* @generate: both
* @priority: high
* @acl: domain:read
+ * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE
*/
REMOTE_PROC_DOMAIN_SNAPSHOT_GET_XML_DESC = 186,
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] CVE-2015-0236: Check ACLs for the VIR_DOMAIN_XML_SECURE flag for snapshots and save images
Patches are pushed according to Eric's ACK on the security list. Peter Krempa (2): CVE-2015-0236: qemu: Check ACLs when dumping security info from save image CVE-2015-0236: qemu: Check ACLs when dumping security info from snapshots src/qemu/qemu_driver.c | 4 ++-- src/remote/remote_protocol.x | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] CVE-2015-0236: qemu: Check ACLs when dumping security info from save image
The ACL check didn't check the VIR_DOMAIN_XML_SECURE flag and the
appropriate permission for it.
---
src/qemu/qemu_driver.c | 2 +-
src/remote/remote_protocol.x | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5994558..c9110f0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6031,7 +6031,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const
char *path,
if (fd < 0)
goto cleanup;
-if (virDomainSaveImageGetXMLDescEnsureACL(conn, def) < 0)
+if (virDomainSaveImageGetXMLDescEnsureACL(conn, def, flags) < 0)
goto cleanup;
ret = qemuDomainDefFormatXML(driver, def, flags);
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index d91fbe0..15694fa 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -4819,6 +4819,7 @@ enum remote_procedure {
* @generate: both
* @priority: high
* @acl: domain:read
+ * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE
*/
REMOTE_PROC_DOMAIN_SAVE_IMAGE_GET_XML_DESC = 235,
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] qemu: output error when try to hotplug unsupport console
On Thu, Jan 22, 2015 at 21:20:46 +0800, Luyao Huang wrote: > > On 01/22/2015 06:19 PM, Peter Krempa wrote: > > On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1164627 > >> ... > > ACK, although the rest of the chardev command and hotplug code is a big > > mess and would benefit from a cleanup/refactor. > > Yes, i noticed this mess and i will try to fix this in next step. I don't insist on you doing that. It was just an observation from looking through the code. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] qemu: output error when try to hotplug unsupport console
On 01/22/2015 06:19 PM, Peter Krempa wrote:
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1164627
When using 'virsh attach-device' to hotplug an unsupported console type
into a qemu guest, the attachment will erroneously allows the
attachment. This patch will check to ensure that only the support target
types are used for a running guest. NB, Although a guest can be defined
with an improper target, startup will cause failure.
I tweaked the subject and commit message as some sentences didn't make
sense.
Thanks for your review and help.
Signed-off-by: Luyao Huang
---
src/qemu/qemu_command.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c041ee7..df87e1e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10067,13 +10067,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
+break;
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
-break;
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unsupported console target type %s"),
+
NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
+goto cleanup;
}
ACK, although the rest of the chardev command and hotplug code is a big
mess and would benefit from a cleanup/refactor.
Yes, i noticed this mess and i will try to fix this in next step.
Pushed.
Peter
Luyao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Disallow emulatorpin when numatune's in effect
https://bugzilla.redhat.com/show_bug.cgi?id=1170492
In one of our previous commits (dc8b7ce7) we've obsoleted
in favor of and others. If old element was passed it was
basically ignored and interesting settings were copied from the new
one. Well with one exception we'd forgotten about: emulatorpin.
Imagine that domain is configured as follows:
6
This is perfectly valid as only old style elements are used. However,
adding new style elements messes up the XML:
6
Since is auto, becomes auto as well. However in
that case we can't guarantee that emulator will be pinned onto
selected nodes.
Signed-off-by: Michal Privoznik
---
src/conf/domain_conf.c | 12 +++-
.../qemuxml2argv-cputune-numatune.xml | 35 ++
.../qemuxml2xmlout-cputune-numatune.xml| 32
tests/qemuxml2xmltest.c| 1 +
4 files changed, 79 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8792f5e..0b8af6d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13173,8 +13173,18 @@ virDomainDefParseXML(xmlDocPtr xml,
ctxt) < 0)
goto error;
-if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask)
+if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask) {
+/* If numatune is used, it obsoletes some older settings
+ * like /domain/vcpu/@placement or
+ * /domain/cputune/emulatorpin. For more info see comment
+ * a few lines above where emulatorpin is parsed. */
def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
+if (def->cputune.emulatorpin) {
+VIR_WARN("Ignore emulatorpin for placement is 'auto'");
+virDomainVcpuPinDefFree(def->cputune.emulatorpin);
+def->cputune.emulatorpin = NULL;
+}
+}
if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
new file mode 100644
index 000..9759b48
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml
@@ -0,0 +1,35 @@
+
+ dummy2
+ 4d92ec27-9ebf-400b-ae91-20c71c647c19
+ 131072
+ 65536
+ 6
+
+
+
+
+
+
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml
new file mode 100644
index 000..b33f57f
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-numatune.xml
@@ -0,0 +1,32 @@
+
+ dummy2
+ 4d92ec27-9ebf-400b-ae91-20c71c647c19
+ 131072
+ 65536
+ 6
+
+
+
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-x86_64
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 4abb303..9ceda58 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -310,6 +310,7 @@ mymain(void)
DO_TEST("blkiotune-device");
DO_TEST("cputune");
DO_TEST("cputune-zero-shares");
+DO_TEST_DIFFERENT("cputune-numatune");
DO_TEST("smp");
DO_TEST("iothreads");
--
2.0.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] nodeinfo test cases for x86_64 AMD CPUs
Hi,
Lately we’ve been puzzled by the nodeinfo returned for AMD 63xx based platforms
so I checked out libvirt from Git and checked out the testcases in
test/nodeinfodata.
Currently you have 3 AMD test cases there :
linux-x86_64-test3 :
AMD 6172 2.1 GHz (MCM, 12 cores per package/socket, 2 numa nodes per
package/socket, 1 thread/core)
4 Sockets, 8 NUMA nodes, 48 CPU cores total.
linux-x86_64-test7 :
AMD 6174 2.2 GHz (MCM, 12 cores per package/socket, 2 numa nodes per
package/socket, 1 thread/core)
2 Sockets, 4 NUMA nodes, 24 CPU cores total.
linux-x86_64-test8 :
AMD 6282 SE 2.6 GHz (MCM, 8 CU(core) per package/socket, 2 numa nodes
per package/socket, 2 threads/CU(core))
4 Sockets, 8 NUMA nodes, 64 CPU cores total.
However, the “expected” output from each of these are wrong I believe :
% ~/libvirt/tests/nodeinfodata$ cat linux-x86_64-test{3,7,8}.expected
CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 1, Cores: 6, Threads: 1
CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 1, Cores: 24, Threads: 1
CPUs: 64/64, MHz: 2593, Nodes: 1, Sockets: 1, Cores: 64, Threads: 1
In my opinion it should have been :
CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 4, Cores: 12, Threads: 1
CPUs: 24/24, MHz: 2200, Nodes: 4, Sockets: 2, Cores: 12, Threads: 1
CPUs: 64/64, MHz: 2593, Nodes: 8, Sockets: 4, Cores: 8, Threads: 2
Atleast that’s more in line with what tools like “lscpu” and “lstopo” would
report.
For example, I have a dual-socket AMD 6376 based system. lscpu reports :
$ lscpu
Architecture: x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):32
On-line CPU(s) list: 0-31
Thread(s) per core:2
Core(s) per socket:8
Socket(s): 2
NUMA node(s): 4
Vendor ID: AuthenticAMD
CPU family:21
Model: 2
Stepping: 0
CPU MHz: 2299.820
BogoMIPS: 4600.03
Virtualization:AMD-V
L1d cache: 16K
L1i cache: 64K
L2 cache: 2048K
L3 cache: 6144K
NUMA node0 CPU(s): 0-7
NUMA node1 CPU(s): 8-15
NUMA node2 CPU(s): 16-23
NUMA node3 CPU(s): 24-31
virsh nodeinfo however :
CPU model: x86_64
CPU(s): 32
CPU frequency: 2299 MHz
CPU socket(s): 1
Core(s) per socket: 32
Thread(s) per core: 1
NUMA cell(s):1
Memory size: 49448656 KiB
Looking more closely at the code in src/nodeinfo.c:linuxNodeInfoCPUPopulate() I
believe it makes some false assumptions.
When iterating over the NUMA nodes, it expects that NUMA nodes can contain more
than one socket, whereas on AMD systems atleast it’s the other way around
(Sockets can contain more than 1 socket). Hence, the topology check at the end
goes all wrong and libvirt uses just a flat topology :
if ((nodeinfo->nodes *
nodeinfo->sockets *
nodeinfo->cores *
nodeinfo->threads) != (nodeinfo->cpus + offline)) {
nodeinfo->nodes = 1;
nodeinfo->sockets = 1;
nodeinfo->cores = nodeinfo->cpus + offline;
nodeinfo->threads = 1;
}
If instead the “fallback” mechanism is used, which iterates over
/sys/devices/system/cpu/cpu%d and finds sockets/cores/threads is used, *and*
you leave nodeinfo->nodes out of the equation then it all makes sense.
Iterating over entries in /sys/devices/system/node/node%d would then only be
used to find nodeinfo->nodes.
I have limited insight into how this would work on say an S390 system, so if my
assumptions here are completely wrong please do tell :)
Cheers,
--
Steffen Persvold
Chief Architect NumaChip, Numascale AS
Tel: +47 23 16 71 88 Fax: +47 23 16 71 80 Skype: spersvold
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs
Per-cpu stats are only shown for present CPUs in the cgroups,
but we were only parsing the largest CPU number from
/sys/devices/system/cpu/present and looking for stats even for
non-present CPUs.
This resulted in:
internal error: cpuacct parse error
---
cfg.mk| 2 +-
src/nodeinfo.c| 17 +
src/nodeinfo.h| 1 +
src/util/vircgroup.c | 24 ++--
tests/vircgroupmock.c | 44 ++--
tests/vircgrouptest.c | 47 +++
6 files changed, 110 insertions(+), 25 deletions(-)
diff --git a/cfg.mk b/cfg.mk
index 21f83c3..70612f8 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
exclude_file_name_regexp--sc_prohibit_strdup = \
- ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
+
^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
exclude_file_name_regexp--sc_prohibit_close = \
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 3c22ebc..3a27c22 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1242,6 +1242,23 @@ nodeGetCPUCount(void)
}
virBitmapPtr
+nodeGetPresentCPUBitmap(void)
+{
+int max_present;
+
+if ((max_present = nodeGetCPUCount()) < 0)
+return NULL;
+
+#ifdef __linux__
+if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present"))
+return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH "/cpu/present");
+#endif
+virReportError(VIR_ERR_NO_SUPPORT, "%s",
+ _("non-continuous host cpu numbers not implemented on this
platform"));
+return NULL;
+}
+
+virBitmapPtr
nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
{
#ifdef __linux__
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index a993c63..047bd5c 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -43,6 +43,7 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems,
int nodeGetMemory(unsigned long long *mem,
unsigned long long *freeMem);
+virBitmapPtr nodeGetPresentCPUBitmap(void);
virBitmapPtr nodeGetCPUBitmap(int *max_id);
int nodeGetCPUCount(void);
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index fe34290..e65617a 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2985,7 +2985,8 @@ static int
virCgroupGetPercpuVcpuSum(virCgroupPtr group,
unsigned int nvcpupids,
unsigned long long *sum_cpu_time,
- unsigned int num)
+ size_t nsum,
+ virBitmapPtr cpumap)
{
int ret = -1;
size_t i;
@@ -2995,7 +2996,7 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
for (i = 0; i < nvcpupids; i++) {
char *pos;
unsigned long long tmp;
-size_t j;
+ssize_t j;
if (virCgroupNewVcpu(group, i, false, &group_vcpu) < 0)
goto cleanup;
@@ -3004,7 +3005,9 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
goto cleanup;
pos = buf;
-for (j = 0; j < num; j++) {
+for (j = virBitmapNextSetBit(cpumap, -1);
+ j >= 0 && j < nsum;
+ j = virBitmapNextSetBit(cpumap, j)) {
if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cpuacct parse error"));
@@ -3042,6 +3045,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
virTypedParameterPtr ent;
int param_idx;
unsigned long long cpu_time;
+virBitmapPtr cpumap = NULL;
/* return the number of supported params */
if (nparams == 0 && ncpus != 0) {
@@ -3052,9 +3056,11 @@ virCgroupGetPercpuStats(virCgroupPtr group,
}
/* To parse account file, we need to know how many cpus are present. */
-if ((total_cpus = nodeGetCPUCount()) < 0)
+if (!(cpumap = nodeGetPresentCPUBitmap()))
return rv;
+total_cpus = virBitmapSize(cpumap);
+
if (ncpus == 0)
return total_cpus;
@@ -3077,7 +3083,11 @@ virCgroupGetPercpuStats(virCgroupPtr group,
need_cpus = MIN(total_cpus, start_cpu + ncpus);
for (i = 0; i < need_cpus; i++) {
-if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) {
+bool present;
+ignore_value(virBitmapGetBit(cpumap, i, &present));
+if (!present) {
+cpu_time = 0;
+} else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cpuacct parse error"));
goto cleanup;
@@ -3097,7 +3107,8 @@ virCgroupGetPercpuStats(virCgroupPtr group,
if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0)
goto cleanup;
-if (virCgroupGet
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Thu, Jan 22, 2015 at 07:02:27PM +0800, Zhu Guihua wrote: > On Thu, 2015-01-22 at 10:06 +, Daniel P. Berrange wrote: > > On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote: > > > On Wed, 2015-01-21 at 09:42 +, Daniel P. Berrange wrote: > > > > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > > > > If you apply the folowing patchset > > > > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > > > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > > > > qemu can support hotplug and hot-unplug cpu device. > > > > > > > > > > So this patch series will make libvirt support hotplug and hot-unplug > > > > > cpu > > > > > device for qemu driver, and now only supports one cpu driver which is > > > > > 'qemu64-x86_64-cpu'. > > > > > > > > > > The cpu device's xml like this: > > > > > > > > > > > > > Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. > > > > It feels like a rather low level QEMU private implementation detail > > > > to me that apps should not need to know or care about. I think libvirt > > > > should always just do the right thing to make cpu hotplug work. > > > > > > > > > > There is a need to use more cpu model. > > > 'qemu64-x86_64-cpu' is only one example, we will realize more driver in > > > future. > > > > Can you give an example of why we will need more than one model ? It seems > > pretty crazy to me that we will need to specify two CPU models for CPUs, > > both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little > > sense from the user / app POV IMHO. > > > > In future, this will become an advanced feature for specific user. If > user may not specific a model, we will specific a default model. > > I think this feature is mainly for test environment. If you want to test > different cpu model's performance, this feature will be essential. The choice of Nehalem, Opteron, etc as CPU models is already supported in QEMU and influences guest CPU performance. You're not explaining why we need to introduce multiple CPU values. It makes no sense to have two different CPU models listed for the same guest like this. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Thu, 2015-01-22 at 10:06 +, Daniel P. Berrange wrote: > On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote: > > On Wed, 2015-01-21 at 09:42 +, Daniel P. Berrange wrote: > > > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > > > If you apply the folowing patchset > > > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > > > qemu can support hotplug and hot-unplug cpu device. > > > > > > > > So this patch series will make libvirt support hotplug and hot-unplug > > > > cpu > > > > device for qemu driver, and now only supports one cpu driver which is > > > > 'qemu64-x86_64-cpu'. > > > > > > > > The cpu device's xml like this: > > > > > > > > > > Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. > > > It feels like a rather low level QEMU private implementation detail > > > to me that apps should not need to know or care about. I think libvirt > > > should always just do the right thing to make cpu hotplug work. > > > > > > > There is a need to use more cpu model. > > 'qemu64-x86_64-cpu' is only one example, we will realize more driver in > > future. > > Can you give an example of why we will need more than one model ? It seems > pretty crazy to me that we will need to specify two CPU models for CPUs, > both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little > sense from the user / app POV IMHO. > In future, this will become an advanced feature for specific user. If user may not specific a model, we will specific a default model. I think this feature is mainly for test environment. If you want to test different cpu model's performance, this feature will be essential. Regards, Zhu > Regards, > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Thu, 2015-01-22 at 10:04 +, Daniel P. Berrange wrote: > On Thu, Jan 22, 2015 at 04:57:39PM +0800, Zhu Guihua wrote: > > On Wed, 2015-01-21 at 09:57 +, Daniel P. Berrange wrote: > > > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > > > If you apply the folowing patchset > > > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > > > qemu can support hotplug and hot-unplug cpu device. > > > > > > > > So this patch series will make libvirt support hotplug and hot-unplug > > > > cpu > > > > device for qemu driver, and now only supports one cpu driver which is > > > > 'qemu64-x86_64-cpu'. > > > > > > Also I'm wondering how this interacts with CPU topology. eg lets say > > > you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, > > > 2 threads. Does this hotplug allow you to plug/unplug individual > > > threads - ie each invididual vCPU, or does it only allow plug/unplug > > > of sockets - ie 8 vCPUs at a time in this topology. > > > > > > > > > > qemu has not interacted with CPU topology so for. > > So now we focus on this feature's realization in a simple way. > > What do you mean by that ? eg which impl I describe above will it support ? > Only allow plug/unplug of sockets. Regards, Zhu > > Regards, > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/5] util: json: Make argument of virJSONValueArraySize const
The function doesn't allow to modify the array in any way, thus the
argument can be const.
---
src/util/virjson.c | 2 +-
src/util/virjson.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 3ffa19f..9f2e1cf 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -797,7 +797,7 @@ virJSONValueIsArray(virJSONValuePtr array)
int
-virJSONValueArraySize(virJSONValuePtr array)
+virJSONValueArraySize(const virJSONValue *array)
{
if (array->type != VIR_JSON_TYPE_ARRAY)
return -1;
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 1996a91..c0aefba 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -105,7 +105,7 @@ int virJSONValueObjectHasKey(virJSONValuePtr object, const
char *key);
virJSONValuePtr virJSONValueObjectGet(virJSONValuePtr object, const char *key);
bool virJSONValueIsArray(virJSONValuePtr array);
-int virJSONValueArraySize(virJSONValuePtr object);
+int virJSONValueArraySize(const virJSONValue *array);
virJSONValuePtr virJSONValueArrayGet(virJSONValuePtr object, unsigned int
element);
virJSONValuePtr virJSONValueArraySteal(virJSONValuePtr object, unsigned int
element);
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] util: bitmap: Tolerate NULL bitmaps in virBitmapEqual
---
src/util/virbitmap.c | 6 ++
src/util/virbitmap.h | 3 +--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 05c50e4..d5b0035 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -504,6 +504,12 @@ bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2)
virBitmapPtr tmp;
size_t i;
+if (!b1 && !b2)
+return true;
+
+if (!b1 || !b2)
+return false;
+
if (b1->max_bit > b2->max_bit) {
tmp = b1;
b1 = b2;
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 565264c..a347f0a 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -84,8 +84,7 @@ virBitmapPtr virBitmapNewData(void *data, int len)
ATTRIBUTE_NONNULL(1);
int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+bool virBitmapEqual(virBitmapPtr b1, virBitmapPtr b2);
size_t virBitmapSize(virBitmapPtr bitmap)
ATTRIBUTE_NONNULL(1);
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] qemu: command: Honor const-correctnes in qemuBuildNumaArgStr
@def is modified in the function indirectly although it's marked as const. --- src/qemu/qemu_command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ebfdd1..b4ac3d9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6637,7 +6637,7 @@ qemuBuildSmpArgStr(const virDomainDef *def, static int qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, -const virDomainDef *def, +virDomainDefPtr def, virCommandPtr cmd, virQEMUCapsPtr qemuCaps, virBitmapPtr nodeset) -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] schemas: Move definition of 'hexuint' to basictypes
Allow reuse of the type.
---
docs/schemas/basictypes.rng | 6 ++
docs/schemas/nodedev.rng| 6 --
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index efc9da4..2bc9c1b 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -14,6 +14,12 @@
+
+
+ (0x)?[0-9a-f]+
+
+
+
[0-9]+
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index c4f402c..13c5402 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -467,12 +467,6 @@
-
-
- (0x)?[0-9a-f]+
-
-
-
([a-fA-F0-9]{2}:){5}[a-fA-F0-9]{2}
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] Const correctnes/random fixes
Few fixes/tweaks that have accumulated in my memory hotplug branch. They should be trivial enough and semantically inert. Peter Krempa (5): conf: Fix comment mentioning actual type of @multi member of virDevicePCIAddress qemu: command: Honor const-correctnes in qemuBuildNumaArgStr util: json: Make argument of virJSONValueArraySize const schemas: Move definition of 'hexuint' to basictypes util: bitmap: Tolerate NULL bitmaps in virBitmapEqual docs/schemas/basictypes.rng | 6 ++ docs/schemas/nodedev.rng| 6 -- src/conf/device_conf.h | 2 +- src/qemu/qemu_command.c | 2 +- src/util/virbitmap.c| 6 ++ src/util/virbitmap.h| 3 +-- src/util/virjson.c | 2 +- src/util/virjson.h | 2 +- 8 files changed, 17 insertions(+), 12 deletions(-) -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/5] conf: Fix comment mentioning actual type of @multi member of virDevicePCIAddress
After refactor to use the virTristateSwitch enum the comment in the
struct was not adjusted.
---
src/conf/device_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index f067a35..7256cdc 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -52,7 +52,7 @@ struct _virDevicePCIAddress {
unsigned int bus;
unsigned int slot;
unsigned int function;
-int multi; /* enum virDomainDeviceAddressPCIMulti */
+int multi; /* virTristateSwitch */
};
typedef struct _virInterfaceLink virInterfaceLink;
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 1/2] qemu: output error when try to hotplug unsupport console
On Thu, Jan 22, 2015 at 10:28:18 +0800, Luyao Huang wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1164627
>
> When using 'virsh attach-device' to hotplug an unsupported console type
> into a qemu guest, the attachment will erroneously allows the
> attachment. This patch will check to ensure that only the support target
> types are used for a running guest. NB, Although a guest can be defined
> with an improper target, startup will cause failure.
I tweaked the subject and commit message as some sentences didn't make
sense.
>
> Signed-off-by: Luyao Huang
> ---
> src/qemu/qemu_command.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c041ee7..df87e1e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10067,13 +10067,17 @@ qemuBuildConsoleChrDeviceStr(char **deviceStr,
> break;
>
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL:
> +break;
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE:
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN:
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_UML:
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LXC:
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_OPENVZ:
> case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST:
> -break;
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported console target type %s"),
> +
> NULLSTR(virDomainChrConsoleTargetTypeToString(chr->targetType)));
> +goto cleanup;
> }
ACK, although the rest of the chardev command and hotplug code is a big
mess and would benefit from a cleanup/refactor.
Pushed.
Peter
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Thu, Jan 22, 2015 at 04:55:02PM +0800, Zhu Guihua wrote: > On Wed, 2015-01-21 at 09:42 +, Daniel P. Berrange wrote: > > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > > If you apply the folowing patchset > > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > > qemu can support hotplug and hot-unplug cpu device. > > > > > > So this patch series will make libvirt support hotplug and hot-unplug cpu > > > device for qemu driver, and now only supports one cpu driver which is > > > 'qemu64-x86_64-cpu'. > > > > > > The cpu device's xml like this: > > > > > > > Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. > > It feels like a rather low level QEMU private implementation detail > > to me that apps should not need to know or care about. I think libvirt > > should always just do the right thing to make cpu hotplug work. > > > > There is a need to use more cpu model. > 'qemu64-x86_64-cpu' is only one example, we will realize more driver in > future. Can you give an example of why we will need more than one model ? It seems pretty crazy to me that we will need to specify two CPU models for CPUs, both "Nehalem" / "Opteron" / etc and this new CPU model. It makes little sense from the user / app POV IMHO. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Thu, Jan 22, 2015 at 04:57:39PM +0800, Zhu Guihua wrote: > On Wed, 2015-01-21 at 09:57 +, Daniel P. Berrange wrote: > > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > > If you apply the folowing patchset > > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > > qemu can support hotplug and hot-unplug cpu device. > > > > > > So this patch series will make libvirt support hotplug and hot-unplug cpu > > > device for qemu driver, and now only supports one cpu driver which is > > > 'qemu64-x86_64-cpu'. > > > > Also I'm wondering how this interacts with CPU topology. eg lets say > > you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, > > 2 threads. Does this hotplug allow you to plug/unplug individual > > threads - ie each invididual vCPU, or does it only allow plug/unplug > > of sockets - ie 8 vCPUs at a time in this topology. > > > > > > qemu has not interacted with CPU topology so for. > So now we focus on this feature's realization in a simple way. What do you mean by that ? eg which impl I describe above will it support ? Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/11] qemu: add support to hotplug memory device
On Thu, Jan 22, 2015 at 16:24:12 +0800, Zhu Guihua wrote: > On Wed, 2015-01-21 at 10:50 +0100, Peter Krempa wrote: > > On Wed, Jan 21, 2015 at 16:20:16 +0800, Zhu Guihua wrote: > > Thank you for pointing out the problems, and we hope the feature could > be supported in libvirt as soon as possible. > > So, > 1) could you tell us your schedule for this feature? I hope the next release (1.2.13) will contain this feature. Libvirt's current release (1.2.12) is scheduled next week, so I'll have a complete dev cycle to add and fix all the bits. > > 2) When can you send the patch series to the community? I'm expecting to be ready to send first version next week. > > 3) If there are any else work we can do for this feature?a Not really, I'm almost ready. There are a few details that need tweaking. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/7] qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags
The code modifies the domain configuration but doesn't take a MODIFY
type job to do so.
---
src/qemu/qemu_driver.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f6b2967..c52821a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9679,6 +9679,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
}
}
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
value_ul = param->value.ul;
@@ -9688,10 +9691,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
unsigned long long val;
if (virCgroupSetCpuShares(priv->cgroup, value_ul) < 0)
-goto cleanup;
+goto endjob;
if (virCgroupGetCpuShares(priv->cgroup, &val) < 0)
-goto cleanup;
+goto endjob;
vm->def->cputune.shares = val;
vm->def->cputune.sharesSpecified = true;
@@ -9700,7 +9703,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&eventMaxNparams,
VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES,
val) < 0)
-goto cleanup;
+goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -9715,7 +9718,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
if ((rc = qemuSetVcpusBWLive(vm, priv->cgroup, value_ul, 0)))
-goto cleanup;
+goto endjob;
vm->def->cputune.period = value_ul;
@@ -9723,7 +9726,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&eventMaxNparams,
VIR_DOMAIN_TUNABLE_CPU_VCPU_PERIOD,
value_ul) < 0)
-goto cleanup;
+goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9735,7 +9738,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
if ((rc = qemuSetVcpusBWLive(vm, priv->cgroup, 0, value_l)))
-goto cleanup;
+goto endjob;
vm->def->cputune.quota = value_l;
@@ -9743,7 +9746,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&eventMaxNparams,
VIR_DOMAIN_TUNABLE_CPU_VCPU_QUOTA,
value_l) < 0)
-goto cleanup;
+goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9756,7 +9759,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
if ((rc = qemuSetEmulatorBandwidthLive(vm, priv->cgroup,
value_ul, 0)))
-goto cleanup;
+goto endjob;
vm->def->cputune.emulator_period = value_ul;
@@ -9764,7 +9767,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&eventMaxNparams,
VIR_DOMAIN_TUNABLE_CPU_EMULATOR_PERIOD,
value_ul) < 0)
-goto cleanup;
+goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9777,7 +9780,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
if ((rc = qemuSetEmulatorBandwidthLive(vm, priv->cgroup,
0, value_l)))
-goto cleanup;
+goto endjob;
vm->def->cputune.emulator_quota = value_l;
@@ -9785,7 +9788,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
&eventMaxNparams,
VIR_DOMAIN_TUNABLE_CPU_EMULATOR_QUOTA,
value_l) < 0)
-goto cleanup;
+goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9794,7 +9797,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
-goto clea
[libvirt] [PATCH 4/7] qemu: Fix job handling in qemuDomainSetMemoryParameters
The code modifies the domain configuration but doesn't take a MODIFY
type job to do so.
---
src/qemu/qemu_driver.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ecb3693..f6b2967 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9043,6 +9043,9 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
}
}
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
#define QEMU_SET_MEM_PARAMETER(FUNC, VALUE)
\
if (set_ ## VALUE) {
\
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
\
@@ -9050,7 +9053,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
virReportSystemError(-rc, _("unable to set memory %s
tunable"), \
#VALUE);
\
\
-goto cleanup;
\
+goto endjob;
\
}
\
vm->def->mem.VALUE = VALUE;
\
}
\
@@ -9078,10 +9081,13 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
-goto cleanup;
+goto endjob;
ret = 0;
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
cleanup:
qemuDomObjEndAPI(&vm);
virObjectUnref(caps);
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/7] qemu: Fix job handling in qemuDomainSetAutostart
The code modifies the domain configuration but doesn't take a MODIFY
type job to do so.
This patch also fixes a few very long lines of code around the touched
parts.
---
src/qemu/qemu_driver.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fa2259a..ecb3693 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8040,35 +8040,45 @@ static int qemuDomainSetAutostart(virDomainPtr dom,
autostart = (autostart != 0);
if (vm->autostart != autostart) {
-if ((configFile = virDomainConfigFile(cfg->configDir, vm->def->name))
== NULL)
-goto cleanup;
-if ((autostartLink = virDomainConfigFile(cfg->autostartDir,
vm->def->name)) == NULL)
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
+if (!(configFile = virDomainConfigFile(cfg->configDir, vm->def->name)))
+goto endjob;
+
+if (!(autostartLink = virDomainConfigFile(cfg->autostartDir,
+ vm->def->name)))
+goto endjob;
+
if (autostart) {
if (virFileMakePath(cfg->autostartDir) < 0) {
virReportSystemError(errno,
_("cannot create autostart directory %s"),
cfg->autostartDir);
-goto cleanup;
+goto endjob;
}
if (symlink(configFile, autostartLink) < 0) {
virReportSystemError(errno,
_("Failed to create symlink '%s to '%s'"),
autostartLink, configFile);
-goto cleanup;
+goto endjob;
}
} else {
-if (unlink(autostartLink) < 0 && errno != ENOENT && errno !=
ENOTDIR) {
+if (unlink(autostartLink) < 0 &&
+errno != ENOENT &&
+errno != ENOTDIR) {
virReportSystemError(errno,
_("Failed to delete symlink '%s'"),
autostartLink);
-goto cleanup;
+goto endjob;
}
}
vm->autostart = autostart;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
}
ret = 0;
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/7] qemu: Job handling fixes
While reviewing Martin's reference counting series I've noticed a few qemu API impls that don't properly handle jobs. Peter Krempa (7): qemu: Fix job handling in qemuDomainPinVcpuFlags qemu: Fix job handling in qemuDomainPinEmulator qemu: Fix job handling in qemuDomainSetAutostart qemu: Fix job handling in qemuDomainSetMemoryParameters qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags qemu: Fix job type in qemuDomainGetBlockIoTune qemu: Fix job handling in qemuDomainSetMetadata src/qemu/qemu_driver.c | 135 +++-- 1 file changed, 87 insertions(+), 48 deletions(-) -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/7] qemu: Fix job handling in qemuDomainSetMetadata
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c3c3e0..ab65e9b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17309,10 +17309,15 @@ qemuDomainSetMetadata(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) +goto cleanup; + ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps, driver->xmlopt, cfg->stateDir, cfg->configDir, flags); +qemuDomainObjEndJob(driver, vm); + cleanup: qemuDomObjEndAPI(&vm); virObjectUnref(caps); -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/7] qemu: Fix job handling in qemuDomainPinEmulator
The code modifies the domain configuration but doesn't take a MODIFY
type job to do so.
---
src/qemu/qemu_driver.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4882cab..fa2259a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5116,17 +5116,20 @@ qemuDomainPinEmulator(virDomainPtr dom,
pid = vm->pid;
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (priv->vcpupids != NULL) {
if (VIR_ALLOC(newVcpuPin) < 0)
-goto cleanup;
+goto endjob;
if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap,
maplen, -1) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to update vcpupin"));
virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum);
-goto cleanup;
+goto endjob;
}
if (virCgroupHasController(priv->cgroup,
@@ -5135,20 +5138,20 @@ qemuDomainPinEmulator(virDomainPtr dom,
* Configure the corresponding cpuset cgroup.
*/
if (virCgroupNewEmulator(priv->cgroup, false,
&cgroup_emulator) < 0)
-goto cleanup;
+goto endjob;
if (qemuSetupCgroupEmulatorPin(cgroup_emulator,
newVcpuPin[0]->cpumask) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("failed to set cpuset.cpus in cgroup"
" for emulator threads"));
-goto cleanup;
+goto endjob;
}
} else {
if (virProcessSetAffinity(pid, pcpumap) < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
_("failed to set cpu affinity for "
"emulator threads"));
-goto cleanup;
+goto endjob;
}
}
@@ -5157,7 +5160,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to delete emulatorpin xml of "
"a running domain"));
-goto cleanup;
+goto endjob;
}
} else {
virDomainVcpuPinDefFree(vm->def->cputune.emulatorpin);
@@ -5170,18 +5173,18 @@ qemuDomainPinEmulator(virDomainPtr dom,
} else {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cpu affinity is not supported"));
-goto cleanup;
+goto endjob;
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
-goto cleanup;
+goto endjob;
str = virBitmapFormat(pcpumap);
if (virTypedParamsAddString(&eventParams, &eventNparams,
&eventMaxparams,
VIR_DOMAIN_TUNABLE_CPU_EMULATORPIN,
str) < 0)
-goto cleanup;
+goto endjob;
event = virDomainEventTunableNewFromDom(dom, eventParams,
eventNparams);
}
@@ -5193,23 +5196,26 @@ qemuDomainPinEmulator(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to delete emulatorpin xml of "
"a persistent domain"));
-goto cleanup;
+goto endjob;
}
} else {
if (virDomainEmulatorPinAdd(persistentDef, cpumap, maplen) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to update or add emulatorpin xml "
"of a persistent domain"));
-goto cleanup;
+goto endjob;
}
}
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
-goto cleanup;
+goto endjob;
}
ret = 0;
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
cleanup:
if (cgroup_emulator)
virCgroupFree(&cgroup_emulator);
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/7] qemu: Fix job type in qemuDomainGetBlockIoTune
The function just queries status so there's no need for a MODIFY type job. --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c52821a..2c3c3e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17041,7 +17041,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; -if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, -- 2.2.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/7] qemu: Fix job handling in qemuDomainPinVcpuFlags
The domain modifies the domain configuration but doesn't take a MODIFY
type job to do it.
---
src/qemu/qemu_driver.c | 32 +++-
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5994558..4882cab 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4825,24 +4825,27 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
if (virBitmapIsAllSet(pcpumap))
doReset = true;
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (priv->vcpupids == NULL) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("cpu affinity is not supported"));
-goto cleanup;
+goto endjob;
}
if (vm->def->cputune.vcpupin) {
newVcpuPin = virDomainVcpuPinDefCopy(vm->def->cputune.vcpupin,
vm->def->cputune.nvcpupin);
if (!newVcpuPin)
-goto cleanup;
+goto endjob;
newVcpuPinNum = vm->def->cputune.nvcpupin;
} else {
if (VIR_ALLOC(newVcpuPin) < 0)
-goto cleanup;
+goto endjob;
newVcpuPinNum = 0;
}
@@ -4850,25 +4853,25 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to update vcpupin"));
virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum);
-goto cleanup;
+goto endjob;
}
/* Configure the corresponding cpuset cgroup before set affinity. */
if (virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_CPUSET)) {
if (virCgroupNewVcpu(priv->cgroup, vcpu, false, &cgroup_vcpu) < 0)
-goto cleanup;
+goto endjob;
if (qemuSetupCgroupVcpuPin(cgroup_vcpu, newVcpuPin, newVcpuPinNum,
vcpu) < 0) {
virReportError(VIR_ERR_OPERATION_INVALID,
_("failed to set cpuset.cpus in cgroup"
" for vcpu %d"), vcpu);
-goto cleanup;
+goto endjob;
}
} else {
if (virProcessSetAffinity(priv->vcpupids[vcpu], pcpumap) < 0) {
virReportError(VIR_ERR_SYSTEM_ERROR,
_("failed to set cpu affinity for vcpu %d"),
vcpu);
-goto cleanup;
+goto endjob;
}
}
@@ -4887,17 +4890,17 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum);
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
-goto cleanup;
+goto endjob;
if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH,
VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) {
-goto cleanup;
+goto endjob;
}
str = virBitmapFormat(pcpumap);
if (virTypedParamsAddString(&eventParams, &eventNparams,
&eventMaxparams, paramField, str) < 0)
-goto cleanup;
+goto endjob;
event = virDomainEventTunableNewFromDom(dom, eventParams,
eventNparams);
}
@@ -4909,7 +4912,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
} else {
if (!persistentDef->cputune.vcpupin) {
if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0)
-goto cleanup;
+goto endjob;
persistentDef->cputune.nvcpupin = 0;
}
if (virDomainVcpuPinAdd(&persistentDef->cputune.vcpupin,
@@ -4920,16 +4923,19 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("failed to update or add vcpupin xml of "
"a persistent domain"));
-goto cleanup;
+goto endjob;
}
}
ret = virDomainSaveConfig(cfg->configDir, persistentDef);
-goto cleanup;
+goto endjob;
}
ret = 0;
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
cleanup:
if (cgroup_vcpu)
virCgroupFree(&cgroup_vcpu);
--
2.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] RFC: Building a virtlogd daemon
On Wed, Jan 21, 2015 at 12:12:14PM +, Daniel P. Berrange wrote: [..snip..] > So I'm intending to create a standalone virtlogd daemon to address this > problem. Similarly to virtlockd, it will be able to re-exec itelf so > that upgrades can be done with no interruption to logging, and libvirtd > will talk to it over a simple RPC protocol. This is a great idea! The naming might cause confusion though, having virtlogd and virtlockd within the same source tree it's easy to get them mixed up since both sound almost the same. Maybe virtloggingd although slightly longer avoids that confusion? Cheers, -- Guido -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Wed, 2015-01-21 at 09:42 +, Daniel P. Berrange wrote: > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > If you apply the folowing patchset > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > qemu can support hotplug and hot-unplug cpu device. > > > > So this patch series will make libvirt support hotplug and hot-unplug cpu > > device for qemu driver, and now only supports one cpu driver which is > > 'qemu64-x86_64-cpu'. > > > > The cpu device's xml like this: > > > > Do we really need to expose this 'qemu64-x86_64-cpu' string to apps. > It feels like a rather low level QEMU private implementation detail > to me that apps should not need to know or care about. I think libvirt > should always just do the right thing to make cpu hotplug work. > There is a need to use more cpu model. 'qemu64-x86_64-cpu' is only one example, we will realize more driver in future. Regards, Zhu > Regards, > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Wed, 2015-01-21 at 09:57 +, Daniel P. Berrange wrote: > On Wed, Jan 21, 2015 at 04:00:52PM +0800, Zhu Guihua wrote: > > If you apply the folowing patchset > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > qemu can support hotplug and hot-unplug cpu device. > > > > So this patch series will make libvirt support hotplug and hot-unplug cpu > > device for qemu driver, and now only supports one cpu driver which is > > 'qemu64-x86_64-cpu'. > > Also I'm wondering how this interacts with CPU topology. eg lets say > you configure a 16 vCPU guest, and set topology to 2 sockets, 4 cores, > 2 threads. Does this hotplug allow you to plug/unplug individual > threads - ie each invididual vCPU, or does it only allow plug/unplug > of sockets - ie 8 vCPUs at a time in this topology. > > qemu has not interacted with CPU topology so for. So now we focus on this feature's realization in a simple way. Regards, Zhu > Regards, > Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 11/12] qemu_monitor_json: sort JSON array of cpu info
On Wed, 2015-01-21 at 10:34 +0100, Peter Krempa wrote: > On Wed, Jan 21, 2015 at 16:01:03 +0800, Zhu Guihua wrote: > > JSON array of cpu info is sorted in order to find thread id of cpu smoothly. > > > > Signed-off-by: Zhu Guihua > > --- > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_monitor_json.c | 31 +-- > > src/util/virbitmap.c | 2 +- > > src/util/virbitmap.h | 2 ++ > > 4 files changed, 33 insertions(+), 3 deletions(-) > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 032e9a9..d2c7c73 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -1042,6 +1042,7 @@ virBitmapFree; > > virBitmapGetBit; > > virBitmapIsAllClear; > > virBitmapIsAllSet; > > +virBitmapIsSet; > > This function is not used anywhere in this patch. Is this (and the other > related hunks) here by accident? It is an accident, it should be in another patch. Thanks, Zhu > > > virBitmapLastSetBit; > > virBitmapNew; > > virBitmapNewCopy; > > Peter > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 09/12] qemu: implement cpu device hotplug on live level
On Wed, 2015-01-21 at 10:27 +0100, Peter Krempa wrote:
> On Wed, Jan 21, 2015 at 16:01:01 +0800, Zhu Guihua wrote:
> > This patch implements live hotplug of a cpu device.
> >
> > Signed-off-by: Zhu Guihua
> > ---
> > src/qemu/qemu_driver.c | 6 ++
> > src/qemu/qemu_hotplug.c | 55
> > +
> > src/qemu/qemu_hotplug.h | 7 +++
> > 3 files changed, 68 insertions(+)
> >
>
> ...
>
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index f6d7667..bff0d14 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -1541,6 +1541,61 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr
> > driver,
> > return ret;
> > }
> >
> > +int
> > +qemuDomainCPUInsert(virDomainDefPtr vmdef,
> > +virDomainCPUDefPtr cpu)
> > +{
> > +if (virDomainCPUFind(vmdef, cpu)) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > + _("cpu already exists"));
> > +return -1;
> > +}
> > +
> > +if (virDomainCPUInsert(vmdef, cpu) < 0)
> > +return -1;
> > +
> > +return 0;
> > +}
> > +
> > +int qemuDomainAttachCPUDevice(virQEMUDriverPtr driver,
> > + virDomainObjPtr vm,
> > + virDomainCPUDefPtr cpu)
> > +{
> > +int ret = -1;
> > +char *devstr = NULL;
> > +qemuDomainObjPrivatePtr priv = vm->privateData;
> > +virDomainDefPtr vmdef = vm->def;
> > +
> > +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > + _("qemu does not support -device"));
> > +goto cleanup;;
> > +}
> > +
> > +if (qemuAssignDeviceCPUAlias(vmdef, cpu, -1) < 0)
> > +goto cleanup;
> > +
> > +if (qemuBuildCPUDeviceStr(&devstr, cpu, priv->qemuCaps) < 0)
> > +goto cleanup;
> > +
> > +if (qemuDomainCPUInsert(vmdef, cpu) < 0)
> > +goto cleanup;
> > +
> > +qemuDomainObjEnterMonitor(driver, vm);
> > +if (devstr && qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> > +qemuDomainObjExitMonitor(driver, vm);
> > +goto cleanup;
> > +}
> > +qemuDomainObjExitMonitor(driver, vm);
>
> This function recently changed it's prototype and requires the return
> value to be checked. Rebasing to upstream will break build.
I will confirm it, thanks.
Regards,
Zhu
>
> > +
> > +ignore_value(virBitmapSetBit(vm->def->apic_id_map, cpu->apic_id));
> > +ret = 0;
> > +
> > + cleanup:
> > +VIR_FREE(devstr);
> > +return ret;
> > +}
> > +
> > static int
> > qemuDomainAttachHostUSBDevice(virQEMUDriverPtr driver,
> >virDomainObjPtr vm,a
>
> Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 08/12] qemu: introduce qemuBuildCPUDeviceStr
On Wed, 2015-01-21 at 10:24 +0100, Peter Krempa wrote:
> On Wed, Jan 21, 2015 at 16:01:00 +0800, Zhu Guihua wrote:
> > qemuBuildCPUDeviceStr being introduced is responsible for creating command
> > line argument for '-device' for given cpu device.
> >
> > Signed-off-by: Zhu Guihua
> > ---
> > src/qemu/qemu_command.c | 50
> > +
> > src/qemu/qemu_command.h | 5 +
> > 2 files changed, 55 insertions(+)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 2ee3e10..824ad29 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -5956,6 +5956,50 @@ static char *qemuBuildTPMDevStr(const virDomainDef
> > *def,
> > }
> >
> >
> > +int
> > +qemuBuildCPUDeviceStr(char **deviceStr,
> > + virDomainCPUDefPtr dev,
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > +virBuffer buf = VIR_BUFFER_INITIALIZER;
> > +
> > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU64_CPU)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("%s not supported in this QEMU binary"),
> > dev->driver);
>
> You blindly assume that the user passed the correct model here and not
> any other invalid string. As I've already said, having this converted to
> an enum makes things easier.
>
> > +goto error;
> > +}
> > +
> > +virBufferAsprintf(&buf, "%s,id=%s,apic-id=%d",
> > + dev->driver, dev->info.alias,
> > + dev->apic_id);
>
> Having a buffer for a single virBufferAsprintf case is a bit overkill.
>
> > +
> > +if (virBufferCheckError(&buf) < 0)
> > +goto error;
> > +
> > +*deviceStr = virBufferContentAndReset(&buf);
> > +return 0;
> > +
> > + error:
> > +virBufferFreeAndReset(&buf);
> > +return -1;
> > +}
> > +
> > +static int
> > +qemuBuildCPUDeviceCommandLine(virCommandPtr cmd,
> > + virDomainCPUDefPtr dev,
> > + virQEMUCapsPtr qemuCaps)
> > +{
> > +char *devstr = NULL;
> > +
> > +if (qemuBuildCPUDeviceStr(&devstr, dev, qemuCaps) < 0)
> > +return -1;
> > +
> > +virCommandAddArgList(cmd, "-device", devstr, NULL);
> > +VIR_FREE(devstr);
> > +return 0;
> > +}
> > +
> > +
> > static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def)
> > {
> > virBuffer buf = VIR_BUFFER_INITIALIZER;
> > @@ -9862,6 +9906,12 @@ qemuBuildCommandLine(virConnectPtr conn,
> > goto error;
> > }
> >
> > +/* add cpu devices */
> > +for (i = 0; i < def->ncpus; i++) {
> > +if (qemuBuildCPUDeviceCommandLine(cmd, def->cpus[i], qemuCaps) < 0)
> > +goto error;
> > +}
>
>
> As I've asked before. The question here is whether this is equivalent
> with just increasing the vCPU count in the "-cpu" argument.
>
> If not it will require a bit more work for setting cgroups, numa pinning
> and other stuff.
Yes, we should require more work.
Regards,
Zhu
>
> If they are equivalent we should use that instead and have the existing
> code do the magic.
>
> Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 03/12] domain_conf: introduce cpu def helpers
On Wed, 2015-01-21 at 10:16 +0100, Peter Krempa wrote:
> On Wed, Jan 21, 2015 at 16:00:55 +0800, Zhu Guihua wrote:
> > virDomainCPUDefFree - free memory allocated
> > virDomainCPUDefParseXML - parse job type
> > virDomainCPUDefFormat - output job type
>
> This patch lacks addition to the RNG schemas that would describe the
> elements that are added and the correct values.
>
> Also lacks change to the docs/formatdomain.html.in
>
>
> >
> > Signed-off-by: Zhu Guihua
> > ---
> > src/conf/domain_conf.c | 100
> > +
> > 1 file changed, 100 insertions(+)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index e036d75..1f05056 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
>
> ...
>
> > +
> > +static virDomainCPUDefPtr
> > +virDomainCPUDefParseXML(xmlNodePtr node, virDomainDefPtr def)
> > +{
> > +char *driver = NULL;
> > +char *apic_id = NULL;
> > +virDomainCPUDefPtr dev;
> > +
> > +if (!(dev = virDomainCPUDefNew()))
> > +return NULL;
> > +
> > +driver = virXMLPropString(node, "driver");
>
> The driver should rather be an enum value that is parsed from the string
> rather than stroing an inline string. This limits the namespace of
> devices libvirt supports but simplifies all matching of the driver later
> on.
>
Yes, it will be an enum value in next version, and it will also support
more driver.
>
> > +if (driver == NULL) {
> > +virReportError(VIR_ERR_XML_ERROR, "%s",
> > + _("missing cpu device driver"));
> > +goto error;
> > +}
> > +dev->driver = driver;
> > +
> > +apic_id = virXMLPropString(node, "apic_id");
> > +
> > +if (!apic_id)
> > +dev->apic_id = virDomainCPUGetFreeApicID(def);
> > +else
> > +dev->apic_id = atoi (apic_id);
>
> Atoi is not allowed, use virStrToLong* instead. Also spaces after
> function name is forbidden.
>
I will change it.
> > +
> > +driver = NULL;
> > +apic_id = NULL;
> > +
> > + cleanup:
> > +VIR_FREE(driver);
> > +VIR_FREE(apic_id);
> > +
> > +return dev;
> > +
> > + error:
> > +virDomainCPUDefFree(dev);
> > +dev = NULL;
> > +goto cleanup;
> > +}
> > +
> > virDomainDeviceDefPtr
> > virDomainDeviceDefParse(const char *xmlStr,
> > const virDomainDef *def,
> > @@ -11187,6 +11253,9 @@ virDomainDeviceDefParse(const char *xmlStr,
> > goto error;
> > break;
> > case VIR_DOMAIN_DEVICE_CPU:
> > +if (!(dev->data.cpu = virDomainCPUDefParseXML(node,
> > (virDomainDefPtr)def)))
> > +goto error;
> > +break;
> > case VIR_DOMAIN_DEVICE_NONE:
> > case VIR_DOMAIN_DEVICE_LAST:
> > break;
> > @@ -18142,6 +18211,31 @@ virDomainChrDefFormat(virBufferPtr buf,
> > }
> >
> > static int
> > +virDomainCPUDefFormat(virBufferPtr buf,
> > + virDomainCPUDefPtr def,
> > + unsigned int flags)
> > +{
> > +char *apic_id = NULL;
> > +
> > +ignore_value(virAsprintf(&apic_id, "%d", def->apic_id));
> > +
> > +virBufferAsprintf(buf, "driver);
> > +
> > +virBufferEscapeString(buf, " apic_id='%s'", apic_id);
>
> Um? Why not virBufferAsprintf(buf, " apic_id='%d', apic_id)?
>
> You won't need the intermediate string. Also it leaks the apic_id
> string.
>
Got it.
>
> > +
> > +virBufferAddLit(buf, ">\n");
> > +virBufferAdjustIndent(buf, 2);
> > +
> > +if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> > +return -1;
> > +
> > +virBufferAdjustIndent(buf, -2);
> > +virBufferAddLit(buf, "\n");
> > +
> > +return 0;
> > +}
> > +
> > +static int
>
> Peter
Regards,
Zhu
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] network: verify proper address family in updates to and
On 01/19/2015 11:04 PM, Laine Stump wrote: > By specifying parentIndex in a call to virNetworkUpdate(), it was > possible to direct libvirt to add a dhcp range or static host of a > non-matching address family to the element of an . For > example, given: > > > > > you could provide a static host entry with an IPv4 address, and > specify that it be added to the 2nd element (index 1): > > virsh net-update default add ip-dhcp-host --parent-index 1 \ > '' > > This would be happily added with no error (and no concern of any > possible future consequences). > > This patch checks that any dhcp range or host element being added to a > network ip's subelement has addresses of the same family as the > ip element they are being added to. > > This problem was noticed when looking at the reproduction case for > https://bugzilla.redhat.com/show_bug.cgi?id=1182486 (but is not a > solution to that bug). > --- > src/conf/network_conf.c | 18 +- > 1 file changed, 17 insertions(+), 1 deletion(-) > ACK Although it would be nicer if we could also find the correct element matching the address when --parent-index is not specified. With the ipv4 section first I get an error trying to add an ipv6 host: virsh net-update default add-first ip-dhcp-host ipv6.xml error: Failed to update network default error: Requested operation is not valid: the address family of a host entry IP must match the address family of the dhcp element's parent Jan signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 01/12] domain_conf: allocate cpu's apic id dynamically
On Wed, 2015-01-21 at 09:56 +0100, Peter Krempa wrote: > On Wed, Jan 21, 2015 at 16:00:53 +0800, Zhu Guihua wrote: > > Add a bitmap apic_idmap to store status of APIC IDs. If you want to use an > > APIC > > ID, you can find a minimum value which has not been used in the bitmap. > > > > Signed-off-by: Zhu Guihua > > --- > > src/conf/domain_conf.c | 15 +++ > > src/conf/domain_conf.h | 3 +++ > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_driver.c | 5 - > > 4 files changed, 23 insertions(+), 1 deletion(-) > > > > ... > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index a2eec83..d08e400 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -426,6 +426,7 @@ virDomainVcpuPinDefFree; > > virDomainVcpuPinDel; > > virDomainVcpuPinFindByVcpu; > > virDomainVcpuPinIsDuplicate; > > +virDomainCPUGetFreeApicID; > > virDomainVideoDefaultRAM; > > virDomainVideoDefaultType; > > virDomainVideoDefFree; > > make syntax-check enforces that this file is ordered by file and then by > function name. Please order this hunk correctly and run make > syntax-check before the submission. > Got it, thanks. Regards, Zhu > Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/12] qemu: add support to hot-plug/unplug cpu device
On Wed, 2015-01-21 at 09:49 +0100, Peter Krempa wrote: > On Wed, Jan 21, 2015 at 16:00:52 +0800, Zhu Guihua wrote: > > If you apply the folowing patchset > > [PATCH v3 0/7] cpu: add device_add foo-x86_64-cpu support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01552.html, > > and [PATCH v2 00/11] cpu: add i386 cpu hot remove support > > https://lists.nongnu.org/archive/html/qemu-devel/2015-01/msg01557.html, > > qemu can support hotplug and hot-unplug cpu device. > > > > So this patch series will make libvirt support hotplug and hot-unplug cpu > > device for qemu driver, and now only supports one cpu driver which is > > 'qemu64-x86_64-cpu'. > > > > The cpu device's xml like this: > > > > > > Libvirt already implements vCPU hotplug and unplug via > virDomainSetVcpusFlags() API and implements this API also for the qemu > driver. Granted that the existing code uses older comands but that can > be tweaked to support the new stuff too. > Now Libvirt only supported vCPU hotplug by qemu command 'cpu-add', but vCPU hot-unplug has not been implemented for qemu driver. > Additionally the new subelement of doesn't seem > necessary: > > - There's only one model that can be plugged there are more model will be plugged in next version. > - Configuring the APIC id doesn't make much sense for users I agree. I will not consider the APIC id in future. > - nothing else can be configured > Yes, only cpu model shoud be configured. > I have following questions though: > > Is there a difference in specifying the cpu via the -device option and > via the legacy -cpu argument? > Of course, -device has more functions than -cpu. > Is there a plan to have more than one cpu "model" that can be plugged? > Yes, all x86 cpu model will be supported. This is an RFC patchset, so the feature is realized in a simple way. > Can more than one cpu model be plugged to the same VM at the same time? > Yes, it can. > In general I don't think it's necessary to add the new device type and > we should rather improve the existing API to support the new device > type. > I think it's necessary to support more cpu model. When user hotplug a cpu, it is essential to point the cpu model. And I think improve the existing API would change too much. The feature is implemented by command 'device_add', so I think it should also keep consistent with the other devices' hotplug. Regards, Zhu > More comments will be inline in the patches. > > Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/2] qemu: add a target type check when hot/cold-plug a Chr device
On 01/22/2015 03:37 PM, Peter Krempa wrote: On Thu, Jan 22, 2015 at 10:28:19 +0800, Luyao Huang wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1164627 Add a func just check the base target type which qemu support. And this check will help to avoid add a qemu unsupport target type chr device to a running guest(hotplug) or to the guest inactive XML (coldplug, will effect next boot) You can get exactly the same behavior adding the device by specifying a new XML containing the device (via virsh edit for example). Unfortunately the unsupported devices can't be checked in the post parse callback as it would make existing VMs with broken config disappear. Additionally even if you forbid known-bad targets you still even with this code can get a failure by adding a SCLP console on a x86 machine: Starting such VM gets you: error: unsupported configuration: sclp console requires QEMU to support s390-sclp This kind of failure depends on the actual qemu process running the VM and can be checked only when the VM is starting. Yes, this check cannot avoid attach a unsupported chr device in all scene (too old qemu or a chr device not support for x86_64), because we cannot do a check to qemu caps in this place. As you said we should check if actual qemu support it when start the vm. And this check only check the target type, and other things have been checked in virDomainChrDefParseXML. Signed-off-by: Luyao Huang --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_hotplug.c | 67 2 files changed, 69 insertions(+) Said this. I don't think it's worth adding the check to the coldplug path. Eww, I thought about this before and i think maybe we can have more check to the Chr device when we do coldplug than we define it (add new check in parse will make guest which have broken settings disappear when update libvirt), although this check cannot cover all the sence. And thanks a lot for your opinion and review! Peter Luyao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 00/11] qemu: add support to hotplug memory device
On Wed, 2015-01-21 at 10:50 +0100, Peter Krempa wrote: > On Wed, Jan 21, 2015 at 16:20:16 +0800, Zhu Guihua wrote: > > Now qemu has already supported memory hotplug, so this patchset will make > > libvirt support hotplug memory device for qemu driver. > > As I'm already working on this I can see a few problems with your > series. > > > > > First, add two parameters slots and maxmem for memory hotplug, and the xml > > format can be like this. > > 102400 > > 1024000 > > > > Second, memory device's xml format can be like this. > > > > Your code states that addr=0 auto-allocates the address, but I don't see > any code that would retrieve it afterwards. Without that the migration > can't work reliably. > > > > > Your code doesn't support specifying source nodes for the memory device. > This wouldn't work with libvirt with numa pinnnig enabled. > > Aditionally the target numa node mode that is configured in the numatune > section is not honored. > > The hugepage backend (memory-backend-file) part in your impl requires > the user to specify the path for the memory device. Libvirt has a lot > code that does this detection and it should be reused. > > > > > I'm pretty far in my effort so I think it's not worth for you to > continue working on this feature. > Thank you for pointing out the problems, and we hope the feature could be supported in libvirt as soon as possible. So, 1) could you tell us your schedule for this feature? 2) When can you send the patch series to the community? 3) If there are any else work we can do for this feature? Regards, Zhu > Peter -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt-test-API][PATCH] Add connection_cpu_models test case
On 01/09/2015 03:58 PM, jiahu wrote:
The connection_cpu_models.py uses getCPUModelNames() to validate
new API virConnectGetCPUModelNames of libvirt.
---
cases/test_connection.conf | 12 +
repos/virconn/connection_cpu_models.py | 82 ++
2 files changed, 94 insertions(+)
create mode 100644 repos/virconn/connection_cpu_models.py
diff --git a/cases/test_connection.conf b/cases/test_connection.conf
index ccde119..e916886 100644
--- a/cases/test_connection.conf
+++ b/cases/test_connection.conf
@@ -29,3 +29,15 @@ virconn:connection_nodeinfo
virconn:connection_version
conn
lxc:///
+
+virconn:connection_cpu_models
+arch
+x86_64
+
+virconn:connection_cpu_models
+arch
+i686
+
+virconn:connection_cpu_models
+arch
+ppc64
diff --git a/repos/virconn/connection_cpu_models.py
b/repos/virconn/connection_cpu_models.py
new file mode 100644
index 000..4588188
--- /dev/null
+++ b/repos/virconn/connection_cpu_models.py
@@ -0,0 +1,82 @@
+#!/usr/bin/env python
+# test getCPUModelNames() API for libvirt
+
+import os
+import libvirt
+
+from xml.dom import minidom
+from libvirt import libvirtError
+from src import sharedmod
+from utils import utils
+
+required_params = ('arch',)
+optional_params = {}
+
+CPU_MAP_FILE = "/usr/share/libvirt/cpu_map.xml"
+
+def get_cpu_archs_from_xml(logger):
+"""
+ return supported cpu archs from cpu_map.xml
+"""
+cpu_archs_from_xml = []
+xml = minidom.parse(CPU_MAP_FILE)
+for arch in xml.getElementsByTagName('arch'):
+cpu_archs_from_xml.append(str(arch.getAttribute('name')))
+return cpu_archs_from_xml
+
+def get_cpu_models_from_xml(arch, logger):
+"""
+ return supported cpu models from cpu_map.xml
+"""
+cpu_models_from_xml = []
+if arch == 'x86_64' or arch == 'i686':
+ real_arch = 'x86'
+else:
+ real_arch = arch
+
+xml = minidom.parse(CPU_MAP_FILE)
+for model in xml.getElementsByTagName('model'):
+if model.parentNode.getAttribute('name') == real_arch:
+cpu_models_from_xml.append(str(model.getAttribute('name')))
+return cpu_models_from_xml
+
+def connection_cpu_models(params):
+"""
+ test API for getCPUModelNames in class virConnect
+"""
+logger = params['logger']
+arch_value = params['arch']
+try:
+logger.info("get cpu archs from cpu_map.xml")
+if not os.path.exists(CPU_MAP_FILE):
+ logger.error("%s is not exist" % CPU_MAP_FILE)
+ return 1
+cpu_archs_from_xml = get_cpu_archs_from_xml(logger)
+logger.info("The supported cpu archs in xml are %s" \
+ % cpu_archs_from_xml)
+cpu_models_from_xml = get_cpu_models_from_xml(arch_value, logger)
+logger.info("The supported cpu models in xml are %s" \
+ % cpu_models_from_xml)
+
+conn = sharedmod.libvirtobj['conn']
+
+cpu_models_from_libvirt = conn.getCPUModelNames(arch_value ,0)
+logger.info("The specified architecture is %s" \
+% arch_value)
+logger.info("The supported cpu models is %s" \
+% cpu_models_from_libvirt)
+
+#compare with cpu_map.xml
+for cpu_model in cpu_models_from_libvirt:
+if cpu_model in cpu_models_from_xml:
+logger.debug("'%s' model: PASS" % cpu_model)
+else:
+logger.debug("'%s' model: FAIL, not in libvirt"\
+ % cpu_model)
+return 1
+logger.debug("check all cpu models: PASS")
+except libvirtError, e:
+logger.error("API error message: %s" % e.message)
+return 1
+
+return 0
ACK and Pushed
Please note to remove the trailing spaces next time. Thanks.
Appending the following two lines into /etc/vimrc could highlights them,
then remove them.
"highlight RedundantSpaces ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/
"
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Grant access to helpers
On Wed, 2015-01-21 at 22:32 -0700, Mike Latimer wrote:
> On Tuesday, January 20, 2015 09:08:04 AM Cedric Bosdonnat wrote:
> > On Mon, 2015-01-19 at 18:25 -0700, Mike Latimer wrote:
> > > Apparmor must not prevent access to required helper programs. The
> > > following
> > >
> > > helpers should be allowed to run in unconfined execution mode:
> > > - libvirt_parthelper
> > > - libvirt_iohelper
> > >
> > > ---
> > >
> > > examples/apparmor/usr.sbin.libvirtd | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/examples/apparmor/usr.sbin.libvirtd
> > > b/examples/apparmor/usr.sbin.libvirtd index 9917836..ab6572a 100644
> > > --- a/examples/apparmor/usr.sbin.libvirtd
> > > +++ b/examples/apparmor/usr.sbin.libvirtd
> > > @@ -57,6 +57,8 @@
> > >
> > >audit deny /sys/kernel/security/apparmor/.* rwxl,
> > >/sys/kernel/security/apparmor/profiles r,
> > >/usr/{lib,lib64}/libvirt/* PUxr,
> > >
> > > + /usr/{lib,lib64}/libvirt/libvirt_parthelper Ux,
> > > + /usr/{lib,lib64}/libvirt/libvirt_iohelper Ux,
> > >
> > >/etc/libvirt/hooks/** rmix,
> > >/etc/xen/scripts/** rmix,
> >
> > Can't we find a way to have them run with inherited profile (ix)?
> > Letting them run completely unprofiled may not be the best solution.
>
> Seems like the apparmor profile for libvirtd is pretty wide open, so I'm not
> sure if there will be much of a difference between those two settings. I'm
> also
> not sure how best to test the functionality of those helpers to find out...
>
> I don't mind if the patch is committed with ix. We can always change it later
> if we find a definitive reason to use Ux. ;)
Jamie, as apparmor expert, do you have any opinion on this?
--
Cedric
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
