[libvirt] [PATCH 1/2] libxl: add acpi slic table support
From: Ivan Kardykov
Libxl driver did not support setup additional acpi firmware to xen
guest. It is necessary to activate OEM Windows installs. This patch
allow to define in OS section acpi table param (which supported domain
common schema).
Signed-off-by: Ivan Kardykov
[added info to docs/formatdomain.html.in]
Signed-off-by: Marek Marczykowski-Górecki
---
docs/formatdomain.html.in | 3 ++-
src/libxl/libxl_conf.c| 5 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fcb7c59c00..de612ae870 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -363,7 +363,8 @@
The table element contains a fully-qualified path
to the ACPI table. The type attribute contains the
ACPI table type (currently only slic is supported)
-Since 1.3.5 (QEMU only)
+Since 1.3.5 (QEM)
+Since 5.8.0 (Xen)
Container boot
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 766a726ebc..c1e248d98c 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -506,6 +506,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
def->features[VIR_DOMAIN_FEATURE_ACPI] ==
VIR_TRISTATE_SWITCH_ON);
+/* copy SLIC table path to acpi_firmware */
+if (def->os.slic_table &&
+VIR_STRDUP(b_info->u.hvm.acpi_firmware, def->os.slic_table) <
0)
+return -1;
+
if (def->nsounds > 0) {
/*
* Use first sound device. man xl.cfg(5) describes soundhw as
--
2.21.0
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] tests: libxl: ACPI slic table test
Signed-off-by: Marek Marczykowski-Górecki
---
.../fullvirt-acpi-slic.json | 54 +++
.../fullvirt-acpi-slic.xml| 32 +++
tests/libxlxml2domconfigtest.c| 2 +
3 files changed, 88 insertions(+)
create mode 100644 tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
create mode 100644 tests/libxlxml2domconfigdata/fullvirt-acpi-slic.xml
diff --git a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
new file mode 100644
index 00..5d85d75af5
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.json
@@ -0,0 +1,54 @@
+{
+"c_info": {
+"type": "hvm",
+"name": "XenGuest2",
+"uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+},
+"b_info": {
+"max_vcpus": 1,
+"avail_vcpus": [
+0
+],
+"max_memkb": 592896,
+"target_memkb": 403456,
+"shadow_memkb": 5656,
+"sched_params": {
+},
+"nested_hvm": "False",
+"type.hvm": {
+"pae": "True",
+"apic": "True",
+"acpi": "True",
+"acpi_firmware": "/path/to/slic.dat",
+"nographic": "True",
+"vnc": {
+"enable": "False"
+},
+"sdl": {
+"enable": "False"
+},
+"spice": {
+
+},
+"boot": "c",
+"rdm": {
+
+}
+},
+"arch_arm": {
+
+}
+},
+"disks": [
+{
+"pdev_path": "/dev/HostVG/XenGuest2",
+"vdev": "hda",
+"backend": "phy",
+"format": "raw",
+"removable": 1,
+"readwrite": 1
+}
+],
+"on_reboot": "restart",
+"on_crash": "restart"
+}
diff --git a/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.xml
b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.xml
new file mode 100644
index 00..017fdb5062
--- /dev/null
+++ b/tests/libxlxml2domconfigdata/fullvirt-acpi-slic.xml
@@ -0,0 +1,32 @@
+
+ XenGuest2
+ c7a5fdb2-cdaf-9455-926a-d65c16db1809
+ 592896
+ 403456
+ 1
+
+hvm
+
+ /path/to/slic.dat
+
+
+
+
+
+
+
+
+ destroy
+ restart
+ restart
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index 3b3f63403e..120796b110 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -212,6 +212,8 @@ mymain(void)
DO_TEST("fullvirt-cpuid-legacy-nest");
# endif
+DO_TEST("fullvirt-acpi-slic");
+
# ifdef LIBXL_HAVE_BUILDINFO_GRANT_LIMITS
DO_TEST("max-gntframes-hvm");
# endif
--
2.21.0
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] qemu: explicitly delete standard tap devices only on platforms that require it
On 9/6/19 11:46 AM, Daniel P. Berrangé wrote: On Fri, Sep 06, 2019 at 11:37:12AM -0400, Laine Stump wrote: On 9/6/19 5:16 AM, Daniel P. Berrangé wrote: On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote: libvirt creates its tap devices without the IFF_PERSIST flag, so they will be automatically deleted when qemu is finished with them. In the case of tap devices created outside of libvirt, if the creating entity wants the devices to be deleted, it will also omit IFF_PERSIST, but if it wants them to remain (e.g. for re-use), then it will use IFF_PERSIST when creating the device. Back when support was added for autocreation by libvirt of tap devices for (commit 9c17d665), code was mistakenly put in qemuProcessStop to always delete tap devices for type='ethernet'. This should only be done on platforms that have VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only FreeBSD). This isn't right. The tap devices should *always* be deleted as we don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST itself. (So you're saying this is a security issue that was coincidentally fixed by commit 9c17d665?) I'm not sure I'd call it a security issue - more of a reliability issue - since its really just a denial of service at most. We just want to be sure we're not leaving anything behind when QEMU quits. Interesting point. But I wonder if it's also problematic that (in the case of a tap device created by someone else (not libvirt) who purposefully set IFF_PERSIST) QEMU could mistakenly/maliciously *clear* IFF_PERSIST. I guess there's really nothing we could do about that though, since the device would already be deleted by the time we found out about it... I found this bit of code specifically because I was creating tap devices with IFF_PERSIST set (that's just what "ip tuntap add" does), and they were disappearing after each use, which may or may not be what the user wants - another case of "someone" clearing IFF_PERSIST, but in this case it is *us*. If they don't want the device deleted, then they can set managed=no now with your patch series. Right. In the case of anyway. There *could* be cases where someone is using a pre-existing tap device with (that works, although only by the coincidence that TUNSETIFF will create the specified device if it doesn't exist, or just return a handle to any existing device), but I suppose anyone already doing that will already be accustomed to the current (since 2016) behavior. And I'm trying to think of a good reason why somebody would want to use a pre-existing tap device but still rely on libvirt to connect it to the bridge, and I can't think of anything, so the number of new people who would want to do this is probably vanishingly small. TL;DR - I'm convinced. I think for consistency I will make a patch that does the same thing both for shutting qemuProcessStop() and qemuDomainRemoveNetDevice() (hot-unplug). (Really it looks like a good reorganization is in order - there are multiple bits that cycle through all the netdefs to shutdown various things, and they all should just happen in one place, if for no other reason than so that we can sequester them off in qemu_interface.c, provide a simple API, then make that into its own module. Or "block", some would say :-) And as a matter of fact I can't see a way to even force macvtap devices to be deleted by an unprivileged process - when I had libvirt try to do the standard delete, it would fail. So having this unconditional forced delete of all standard tap devices both causes an unexpected behavior for some users, as well as creating an inconsistency between tap and macvtap behavior (standard taps are always deleted, macvtaps are never deleted). We don't currently support pre-createds macvtap, so we can fix this inconsistency so that it works the way tap devs do. Right. And since the only way to use a pre-created macvtap is with managed=no, we've done that (again, as long as QEMU doesn't clear IFF_PERSIST on standard taps). (This reminds me of another inconsistency I saw while looking at this, but then later forgot - virNetDevTapDelete() is *never * called in the case of hot-unplug. So if you think that we should be unconditionally deleting all taps after use regardless of the previous state of IFF_PERSIST of pre-created taps, then we should also be doing it for hot-unplug.) So how about if we remember the setting of IFF_PERSIST prior to starting QEMU, and restore it to its previous state afterwards? That would make behavior more what was expected / consistent with macvtap. I don't think we need rmemeber IFF_PERSIST when we have the "managed" flag now. Yeah, the only situation that would warrant intervention would be to try and recover from QEMU maliciously clearing IFF_PERSIST, but by the time libvirt gets control it's too late anyway - the tap is already deleted. As always, thanks for the reviews! -- libvir-list mailing list libvir-l
Re: [libvirt] [PATCH 4/4] qemu_capabilities: Temporarily disable dbus-vmstate capability
[cc-ing Marc-André]
On Fri, Sep 06, 2019 at 04:25:19PM +0200, Michal Privoznik wrote:
The qemu side is not merged in yet, so there is a chance that the
interface will change. Don't detect the capability just yet then.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 136999ad0d..7be026ae12 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1129,7 +1129,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "virtio-serial-pci-non-transitional", QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL },
{ "max-x86_64-cpu", QEMU_CAPS_X86_MAX_CPU },
{ "bochs-display", QEMU_CAPS_DEVICE_BOCHS_DISPLAY },
-{ "dbus-vmstate", QEMU_CAPS_DBUS_VMSTATE },
};
static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
If
1) dbus-vmstate is required for migration of a domain using a slirp-helper and
2) we enable this automatically for existing XML configs with
if
a) QEMU has QEMU_CAPS_NET_SOCKET_DGRAM
b) the slirp-helper process is installed on the system
then we've effectively made their machines unmigratable
So if we want to enable it automatically, we should add:
c) QEMU has QEMU_CAPS_DBUS_VMSTATE
regardless of whether dbus-vmstate gets merged into QEMU until libvirt's
next release
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] qemu: explicitly delete standard tap devices only on platforms that require it
On Fri, Sep 06, 2019 at 11:37:12AM -0400, Laine Stump wrote: > On 9/6/19 5:16 AM, Daniel P. Berrangé wrote: > > On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote: > > > libvirt creates its tap devices without the IFF_PERSIST flag, so they > > > will be automatically deleted when qemu is finished with them. In the > > > case of tap devices created outside of libvirt, if the creating entity > > > wants the devices to be deleted, it will also omit IFF_PERSIST, but if > > > it wants them to remain (e.g. for re-use), then it will use > > > IFF_PERSIST when creating the device. > > > > > > Back when support was added for autocreation by libvirt of tap devices > > > for (commit 9c17d665), code was mistakenly > > > put in qemuProcessStop to always delete tap devices for > > > type='ethernet'. This should only be done on platforms that have > > > VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only > > > FreeBSD). > > This isn't right. The tap devices should *always* be deleted as we > > don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST > > itself. > > > (So you're saying this is a security issue that was coincidentally fixed by > commit 9c17d665?) I'm not sure I'd call it a security issue - more of a reliability issue - since its really just a denial of service at most. We just want to be sure we're not leaving anything behind when QEMU quits. > Interesting point. But I wonder if it's also problematic that (in the case > of a tap device created by someone else (not libvirt) who purposefully set > IFF_PERSIST) QEMU could mistakenly/maliciously *clear* IFF_PERSIST. I guess > there's really nothing we could do about that though, since the device would > already be deleted by the time we found out about it... > > > I found this bit of code specifically because I was creating tap devices > with IFF_PERSIST set (that's just what "ip tuntap add" does), and they were > disappearing after each use, which may or may not be what the user wants - > another case of "someone" clearing IFF_PERSIST, but in this case it is *us*. If they don't want the device deleted, then they can set managed=no now with your patch series. > And as a matter of fact I can't see a way to even force macvtap devices to > be deleted by an unprivileged process - when I had libvirt try to do the > standard delete, it would fail. So having this unconditional forced delete > of all standard tap devices both causes an unexpected behavior for some > users, as well as creating an inconsistency between tap and macvtap behavior > (standard taps are always deleted, macvtaps are never deleted). We don't currently support pre-createds macvtap, so we can fix this inconsistency so that it works the way tap devs do. > (This reminds me of another inconsistency I saw while looking at this, but > then later forgot - virNetDevTapDelete() is *never * called in the case of > hot-unplug. So if you think that we should be unconditionally deleting all > taps after use regardless of the previous state of IFF_PERSIST of > pre-created taps, then we should also be doing it for hot-unplug.) > > > So how about if we remember the setting of IFF_PERSIST prior to starting > QEMU, and restore it to its previous state afterwards? That would make > behavior more what was expected / consistent with macvtap. I don't think we need rmemeber IFF_PERSIST when we have the "managed" flag now. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] lib: Grab write lock when modifying list of domains
On Fri, Sep 06, 2019 at 04:25:17PM +0200, Michal Privoznik wrote: In some places where virDomainObjListForEach() is called the passed callback calls virDomainObjListRemoveLocked(). Well, this is unsafe, because the former only grabs a read lock but the latter modifies the list. I've identified the following unsafe calls: - qemuProcessReconnectAll() - libxlReconnectDomains() The rest seem to be safe. Signed-off-by: Michal Privoznik --- src/bhyve/bhyve_driver.c| 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/virdomainobjlist.c | 12 ++-- src/conf/virdomainobjlist.h | 1 + src/libxl/libxl_driver.c| 11 +++ src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_process.c | 3 ++- src/vmware/vmware_driver.c | 2 +- 8 files changed, 25 insertions(+), 12 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] qemu: explicitly delete standard tap devices only on platforms that require it
On 9/6/19 5:16 AM, Daniel P. Berrangé wrote: On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote: libvirt creates its tap devices without the IFF_PERSIST flag, so they will be automatically deleted when qemu is finished with them. In the case of tap devices created outside of libvirt, if the creating entity wants the devices to be deleted, it will also omit IFF_PERSIST, but if it wants them to remain (e.g. for re-use), then it will use IFF_PERSIST when creating the device. Back when support was added for autocreation by libvirt of tap devices for (commit 9c17d665), code was mistakenly put in qemuProcessStop to always delete tap devices for type='ethernet'. This should only be done on platforms that have VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only FreeBSD). This isn't right. The tap devices should *always* be deleted as we don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST itself. (So you're saying this is a security issue that was coincidentally fixed by commit 9c17d665?) Interesting point. But I wonder if it's also problematic that (in the case of a tap device created by someone else (not libvirt) who purposefully set IFF_PERSIST) QEMU could mistakenly/maliciously *clear* IFF_PERSIST. I guess there's really nothing we could do about that though, since the device would already be deleted by the time we found out about it... I found this bit of code specifically because I was creating tap devices with IFF_PERSIST set (that's just what "ip tuntap add" does), and they were disappearing after each use, which may or may not be what the user wants - another case of "someone" clearing IFF_PERSIST, but in this case it is *us*. And as a matter of fact I can't see a way to even force macvtap devices to be deleted by an unprivileged process - when I had libvirt try to do the standard delete, it would fail. So having this unconditional forced delete of all standard tap devices both causes an unexpected behavior for some users, as well as creating an inconsistency between tap and macvtap behavior (standard taps are always deleted, macvtaps are never deleted). (This reminds me of another inconsistency I saw while looking at this, but then later forgot - virNetDevTapDelete() is *never * called in the case of hot-unplug. So if you think that we should be unconditionally deleting all taps after use regardless of the previous state of IFF_PERSIST of pre-created taps, then we should also be doing it for hot-unplug.) So how about if we remember the setting of IFF_PERSIST prior to starting QEMU, and restore it to its previous state afterwards? That would make behavior more what was expected / consistent with macvtap. To change the topic a bit - I actually find it strange that some of the IF flags can be modified by an unprivileged process and some can't. From what I've seen (if I'm remembering my experiments correctly - I didn't take notes, but the implementation is based on the following observations), an unprivileged process can set/clear: IFF_VNET_HDR IFF_PERSIST but can't set/clear IFF_MULTI_QUEUE IFF_UP I can't really see a way to categorize the implications of these flags to justify the difference - each looks just as important/potentially disruptive/not as the other. (Also it's not possible to change the MAC address). And to top that off, the method of getting a handle to a standard tap device is via opening /dev/tun/tap and then calling TUNSETIFF (which magically makes the handle you have to /dev/tun/tap be a handle to the specific tap device), and if you request the wrong setting of an "unmodifiable" IF flag in the TUNSETIFF ifreq struct, it will fail. For example, if the tap was created with IFF_MULTI_QUEUE and libvirt doesn't set IFF_MULTI_QUEUE in the TUNSETIFF, the ioctl will fail (and the same for the opposite setting of the flags). For IFF_VNET_HDR, though, the value it was created with is essentially ignored, and the setting given in libvirt's TUNSETIFF is honored. The result is that we can't just "leave all the flags alone", we have to make sure some are set the same as at tap creation, and others are set as we want them to be (regardless of how the tap was created). (The list of which flags can/can't be set by an unprivileged process is at least consistent between tap and macvtap, although it's problematic that you can clear IFF_PERSIST for a tap (effectively deleting it), but *can't* do an RTM_DELLINK on a macvtap). I went in assuming that *none* of the flags would be changeable, and I could just not set any of them, but was sorely disappointed - I have to set IFF_VNET_HDR appropriately or performance of virtio devices will suffer, and I have to set IFF_MULTI_QUEUE appropriately or the TUNSETIFF will fail completely. This mistake has been corrected, along with the unnecessary check for non-null net->ifname (it must always be non-null), and erroneous VIR_FREE
[libvirt] [PATCH] Revert "dbus: correctly build reply message"
This reverts commit 39dded7bb61444bb608fadd3f82f6fe93d08fd0e.
This commit broke virpolkittest on Ubuntu 18 which has an old
dbus (v1.12.2). Any other distro with the recent one works
(v1.12.16) which hints its a bug in dbus somewhere. Revert the
commit to stop tickling it.
Signed-off-by: Michal Privoznik
---
src/util/virdbus.c | 18 ++
src/util/virdbus.h | 6 ++
tests/virfirewalltest.c | 9 +++--
tests/virpolkittest.c | 3 +--
4 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 64513eef14..b0ac8d7055 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1456,7 +1456,6 @@ int virDBusCreateMethod(DBusMessage **call,
/**
* virDBusCreateReplyV:
- * @msg: the message to reply to
* @reply: pointer to be filled with a method reply message
* @types: type signature for following method arguments
* @args: method arguments
@@ -1469,14 +1468,13 @@ int virDBusCreateMethod(DBusMessage **call,
* as variadic args. See virDBusCreateMethodV for a
* description of this parameter.
*/
-int virDBusCreateReplyV(DBusMessage *msg,
-DBusMessage **reply,
+int virDBusCreateReplyV(DBusMessage **reply,
const char *types,
va_list args)
{
int ret = -1;
-if (!(*reply = dbus_message_new_method_return(msg))) {
+if (!(*reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN))) {
virReportOOMError();
goto cleanup;
}
@@ -1495,7 +1493,6 @@ int virDBusCreateReplyV(DBusMessage *msg,
/**
* virDBusCreateReply:
- * @msg: the message to reply to
* @reply: pointer to be filled with a method reply message
* @types: type signature for following method arguments
* @...: method arguments
@@ -1503,15 +1500,14 @@ int virDBusCreateReplyV(DBusMessage *msg,
* See virDBusCreateReplyV for a description of the
* behaviour of this method.
*/
-int virDBusCreateReply(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReply(DBusMessage **reply,
const char *types, ...)
{
va_list args;
int ret;
va_start(args, types);
-ret = virDBusCreateReplyV(msg, reply, types, args);
+ret = virDBusCreateReplyV(reply, types, args);
va_end(args);
return ret;
@@ -1815,8 +1811,7 @@ int virDBusCreateMethodV(DBusMessage **call
ATTRIBUTE_UNUSED,
return -1;
}
-int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED,
-DBusMessage **reply ATTRIBUTE_UNUSED,
+int virDBusCreateReplyV(DBusMessage **reply ATTRIBUTE_UNUSED,
const char *types ATTRIBUTE_UNUSED,
va_list args ATTRIBUTE_UNUSED)
{
@@ -1825,8 +1820,7 @@ int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED,
return -1;
}
-int virDBusCreateReply(DBusMessage *msg ATTRIBUTE_UNUSED,
- DBusMessage **reply ATTRIBUTE_UNUSED,
+int virDBusCreateReply(DBusMessage **reply ATTRIBUTE_UNUSED,
const char *types ATTRIBUTE_UNUSED, ...)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virdbus.h b/src/util/virdbus.h
index 0303e91045..083c074d59 100644
--- a/src/util/virdbus.h
+++ b/src/util/virdbus.h
@@ -52,11 +52,9 @@ int virDBusCreateMethodV(DBusMessage **call,
const char *member,
const char *types,
va_list args);
-int virDBusCreateReply(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReply(DBusMessage **reply,
const char *types, ...);
-int virDBusCreateReplyV(DBusMessage *msg,
-DBusMessage **reply,
+int virDBusCreateReplyV(DBusMessage **reply,
const char *types,
va_list args);
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index e5eeb52175..78685a3bf4 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -150,8 +150,7 @@
VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
if (nargs == 1 &&
STREQ(type, "ipv4") &&
STREQ(args[0], "-L")) {
-if (virDBusCreateReply(message,
- &reply,
+if (virDBusCreateReply(&reply,
"s", TEST_FILTER_TABLE_LIST) < 0)
goto error;
} else if (nargs == 3 &&
@@ -159,13 +158,11 @@
VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
STREQ(args[0], "-t") &&
STREQ(args[1], "nat") &&
STREQ(args[2], "-L")) {
-if (virDBusCreateReply(message,
- &reply,
+if (virDBusCreateReply(&reply,
"s", TEST_NAT_TABLE_LIS
Re: [libvirt] [PATCH 0/3] Fix memlock limit during hotplug of mdev devices
On 9/6/19 10:29 AM, Daniel Henrique Barboza wrote: > Hi, > > > I've thought about the issue you're fixing. Have you considered/tried > to handle the MemLockLimit upper in the call hierarchy with your new > qemuDomainAdjustMaxMemLockHostdev() function? Both > qemuDomainAttachMediatedDevice() and qemuDomainAttachHostPCIDevice() > are called from qemuDomainAttachHostDevice(). In theory, a call to That's a good point. I did start going down that path, but that was before I created qemuDomainAdjustMaxMemLockHostdev() and didn't like the way things were turning out. I will revisit that, since it would certainly be better to have them adjusted in one place, rather than in the different qemuDomainAttach* routines that are affected now (or may be in the future). > qemuDomainAdjustMaxMemLockHostdev() at the start of AttachHostDevice() > would cover all hostdev types of the function. Then you can undo the > memLock in the 'error' label there if something goes wrong. > > It is possible to even argue about the be possibility of calling > qemuDomainAdjustMaxMemLock() at the start of the generic hotplug > function, qemuDomainAttachDeviceLive(), using the same principle - > temporarily add the device in the vmdef, define the new mem limit > assuming that everything will be OK, undo the adjustment if ret != 0. > This would cover the case of memory hotplug, which needs RLIMIT > changes as well and uses the same qemuDomainAdjustMaxMemLock() > function to do it.> That would be excellent. > > This is more of a future work though. For now I believe it's appropriate > to fix vfio-ccw hotplug ASAP. > I agree. I have a few other things at the moment, but will look into these ideas after I get them sorted out. Thanks for the reviews, and the suggestions! - Eric > > Thanks, > > > DHB > > > > On 9/3/19 5:09 PM, Eric Farman wrote: >> The routine qemuDomainGetMemLockLimitBytes() has a couple tests >> to determine what the maximum limit of locked memory should be. >> If I start a domain without any vfio stuff, /proc/$PID/limits says >> the limit is 64KiB. If I start a guest with a vfio-ccw hostdev, >> the limit is now $GUEST_MEMORY + 1GiB. It doesn't matter if I >> have one hostdev or a thousand; it's always 1GiB more than the >> configured amount of guest memory. >> >> If I start a guest without any vfio devices, and hotplug that same >> vfio-ccw hostdev, the limit remains at 64KiB and I start getting >> I/O errors in the guest. This makes sense, since some of the I/O >> chains are long enough to exceed that page limit, and the host >> starts throwing errors. >> >> There is already code that adjusts this limit during the hotplug >> of vfio-pci devices, so patch 1 refactors that code so that it >> can be re-used on the mdev hotplug path in patch 3. >> >> Patch 2, meanwhile, adds some cleanup that I think is missing in >> the vfio-pci path, based on my read of the mdev path. >> >> $ grep locked /proc/83543/limits >> Max locked memory 65536 65536 >> bytes >> $ virsh attach-device guest scratch-ca8b.xml >> Device attached successfully >> >> $ grep locked /proc/83543/limits >> Max locked memory 3221225472 3221225472 >> bytes >> >> >> Eric Farman (3): >> qemu: Refactor the max memlock routine >> qemu: Reset the maximum locked memory on hotplug fail >> qemu: Adjust max memlock on mdev hotplug >> >> src/qemu/qemu_domain.c | 30 ++ >> src/qemu/qemu_domain.h | 2 ++ >> src/qemu/qemu_hotplug.c | 22 -- >> 3 files changed, 44 insertions(+), 10 deletions(-) >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] qemu_slirp: Drop unused variable in qemuSlirpStart()
On Fri, Sep 06, 2019 at 04:25:18PM +0200, Michal Privoznik wrote: The @cmdstr variable is not used really. Signed-off-by: Michal Privoznik --- src/qemu/qemu_slirp.c | 1 - 1 file changed, 1 deletion(-) This is a trivial build breaker fix and can be pushed right away regardless of the rest of the series. Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] virdomainobjlist: Documnet virDomainObjListForEach()
On Fri, Sep 06, 2019 at 04:25:16PM +0200, Michal Privoznik wrote: Signed-off-by: Michal Privoznik --- s/Documnet/Document/ in the commit summary src/conf/virdomainobjlist.c | 13 + 1 file changed, 13 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/3] Fix memlock limit during hotplug of mdev devices
Hi, I've thought about the issue you're fixing. Have you considered/tried to handle the MemLockLimit upper in the call hierarchy with your new qemuDomainAdjustMaxMemLockHostdev() function? Both qemuDomainAttachMediatedDevice() and qemuDomainAttachHostPCIDevice() are called from qemuDomainAttachHostDevice(). In theory, a call to qemuDomainAdjustMaxMemLockHostdev() at the start of AttachHostDevice() would cover all hostdev types of the function. Then you can undo the memLock in the 'error' label there if something goes wrong. It is possible to even argue about the be possibility of calling qemuDomainAdjustMaxMemLock() at the start of the generic hotplug function, qemuDomainAttachDeviceLive(), using the same principle - temporarily add the device in the vmdef, define the new mem limit assuming that everything will be OK, undo the adjustment if ret != 0. This would cover the case of memory hotplug, which needs RLIMIT changes as well and uses the same qemuDomainAdjustMaxMemLock() function to do it. This is more of a future work though. For now I believe it's appropriate to fix vfio-ccw hotplug ASAP. Thanks, DHB On 9/3/19 5:09 PM, Eric Farman wrote: The routine qemuDomainGetMemLockLimitBytes() has a couple tests to determine what the maximum limit of locked memory should be. If I start a domain without any vfio stuff, /proc/$PID/limits says the limit is 64KiB. If I start a guest with a vfio-ccw hostdev, the limit is now $GUEST_MEMORY + 1GiB. It doesn't matter if I have one hostdev or a thousand; it's always 1GiB more than the configured amount of guest memory. If I start a guest without any vfio devices, and hotplug that same vfio-ccw hostdev, the limit remains at 64KiB and I start getting I/O errors in the guest. This makes sense, since some of the I/O chains are long enough to exceed that page limit, and the host starts throwing errors. There is already code that adjusts this limit during the hotplug of vfio-pci devices, so patch 1 refactors that code so that it can be re-used on the mdev hotplug path in patch 3. Patch 2, meanwhile, adds some cleanup that I think is missing in the vfio-pci path, based on my read of the mdev path. $ grep locked /proc/83543/limits Max locked memory 6553665536bytes $ virsh attach-device guest scratch-ca8b.xml Device attached successfully $ grep locked /proc/83543/limits Max locked memory 3221225472 3221225472 bytes Eric Farman (3): qemu: Refactor the max memlock routine qemu: Reset the maximum locked memory on hotplug fail qemu: Adjust max memlock on mdev hotplug src/qemu/qemu_domain.c | 30 ++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_hotplug.c | 22 -- 3 files changed, 44 insertions(+), 10 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: Set backing file name for LOOP_GET_STATUS64 queries.
On Mon, Sep 02, 2019 at 02:00:27PM -0300, jcfara...@gmail.com wrote:
> From: Julio Faracco
>
> This is an issue for LXC loop devices when you are trying to get loop
> devices info using `ioctl`. Modern apps uses `/sys/dev/block` to grab
> information about devices, but if you use the method mention you won't
> be able to retrive the associated file with that loop device. See
> example below from cryptsetup sources:
>
> static char *_ioctl_backing_file(const char *loop)
> {
> struct loop_info64 lo64 = {0};
> int loop_fd;
>
> loop_fd = open(loop, O_RDONLY);
> if (loop_fd < 0)
> return NULL;
>
> if (ioctl(loop_fd, LOOP_GET_STATUS64, &lo64) < 0) {
> close(loop_fd);
> return NULL;
> }
>
> lo64.lo_file_name[LO_NAME_SIZE-2] = '*';
> lo64.lo_file_name[LO_NAME_SIZE-1] = 0;
>
> close(loop_fd);
> return strdup((char*)lo64.lo_file_name);
> }
>
> It will return an empty string because lo_file_name was not set.
> Function `virFileLoopDeviceOpenSearch()` is using `ioctl` to query data,
> but it is not checking `lo_file_name` field.
>
> v1: I accidentally committed two wrong lines.
This type of message should be put after the "---" lines...
>
> Signed-off-by: Julio Faracco
> ---
Here, so that it doesn't get into git history when the patch
is applied
> src/util/virfile.c | 8
> 1 file changed, 8 insertions(+)
Reviewed-by: Daniel P. Berrangé
and pushed to git
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] lib: Grab write lock when modifying list of domains
In some places where virDomainObjListForEach() is called the
passed callback calls virDomainObjListRemoveLocked(). Well, this
is unsafe, because the former only grabs a read lock but the
latter modifies the list.
I've identified the following unsafe calls:
- qemuProcessReconnectAll()
- libxlReconnectDomains()
The rest seem to be safe.
Signed-off-by: Michal Privoznik
---
src/bhyve/bhyve_driver.c| 2 +-
src/bhyve/bhyve_process.c | 2 +-
src/conf/virdomainobjlist.c | 12 ++--
src/conf/virdomainobjlist.h | 1 +
src/libxl/libxl_driver.c| 11 +++
src/lxc/lxc_process.c | 4 ++--
src/qemu/qemu_process.c | 3 ++-
src/vmware/vmware_driver.c | 2 +-
8 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index e2c1b00080..c52def7607 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -114,7 +114,7 @@ bhyveAutostartDomains(bhyveConnPtr driver)
struct bhyveAutostartData data = { driver, conn };
-virDomainObjListForEach(driver->domains, bhyveAutostartDomain, &data);
+virDomainObjListForEach(driver->domains, false, bhyveAutostartDomain,
&data);
virObjectUnref(conn);
}
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 4dab6e5e54..5fea2eb51c 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -466,7 +466,7 @@ virBhyveProcessReconnectAll(bhyveConnPtr driver)
data.driver = driver;
data.kd = kd;
-virDomainObjListForEach(driver->domains, virBhyveProcessReconnect, &data);
+virDomainObjListForEach(driver->domains, false, virBhyveProcessReconnect,
&data);
kvm_close(kd);
}
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 11fd68745b..2eff6768c2 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -818,25 +818,33 @@ virDomainObjListHelper(void *payload,
/**
* virDomainObjListForEach:
* @doms: Pointer to the domain object list
+ * @modify: Whether to lock @doms for modify operation
* @callback: callback to run over each domain on the list
* @opaque: opaque data to pass to @callback
*
* For every domain on the list (@doms) run @callback on it. If
* @callback fails (i.e. returns a negative value), the iteration
- * carries still on until all domains are visited.
+ * carries still on until all domains are visited. Moreover, if
+ * @callback wants to modify the list of domains (@doms) then
+ * @modify must be set to true.
*
* Returns: 0 on success,
* -1 otherwise.
*/
int
virDomainObjListForEach(virDomainObjListPtr doms,
+bool modify,
virDomainObjListIterator callback,
void *opaque)
{
struct virDomainListIterData data = {
callback, opaque, 0,
};
-virObjectRWLockRead(doms);
+
+if (modify)
+virObjectRWLockWrite(doms);
+else
+virObjectRWLockRead(doms);
virHashForEach(doms->objs, virDomainObjListHelper, &data);
virObjectRWUnlock(doms);
return data.ret;
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index 7d71bc54d0..da5ec8a57c 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -91,6 +91,7 @@ typedef int (*virDomainObjListIterator)(virDomainObjPtr dom,
void *opaque);
int virDomainObjListForEach(virDomainObjListPtr doms,
+bool modify,
virDomainObjListIterator callback,
void *opaque);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d0396e4781..5e14aa4fbe 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -497,7 +497,7 @@ libxlReconnectDomain(virDomainObjPtr vm,
static void
libxlReconnectDomains(libxlDriverPrivatePtr driver)
{
-virDomainObjListForEach(driver->domains, libxlReconnectDomain, driver);
+virDomainObjListForEach(driver->domains, true, libxlReconnectDomain,
driver);
}
static int
@@ -800,10 +800,12 @@ libxlStateInitialize(bool privileged,
NULL, NULL) < 0)
goto error;
-virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+virDomainObjListForEach(libxl_driver->domains, false,
+libxlAutostartDomain,
libxl_driver);
-virDomainObjListForEach(libxl_driver->domains, libxlDomainManagedSaveLoad,
+virDomainObjListForEach(libxl_driver->domains, false,
+libxlDomainManagedSaveLoad,
libxl_driver);
return VIR_DRV_STATE_INIT_COMPLETE;
@@ -832,7 +834,8 @@ libxlStateReload(void)
libxl_driver->xmlopt,
NULL, libxl_driver);
-virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain,
+v
[libvirt] [PATCH 4/4] qemu_capabilities: Temporarily disable dbus-vmstate capability
The qemu side is not merged in yet, so there is a chance that the
interface will change. Don't detect the capability just yet then.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_capabilities.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 136999ad0d..7be026ae12 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1129,7 +1129,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "virtio-serial-pci-non-transitional", QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL
},
{ "max-x86_64-cpu", QEMU_CAPS_X86_MAX_CPU },
{ "bochs-display", QEMU_CAPS_DEVICE_BOCHS_DISPLAY },
-{ "dbus-vmstate", QEMU_CAPS_DBUS_VMSTATE },
};
static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] qemu_slirp: Drop unused variable in qemuSlirpStart()
The @cmdstr variable is not used really.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_slirp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
index 24df1e1a20..716b73759d 100644
--- a/src/qemu/qemu_slirp.c
+++ b/src/qemu/qemu_slirp.c
@@ -298,7 +298,6 @@ qemuSlirpStart(qemuSlirpPtr slirp,
{
VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
VIR_AUTOPTR(virCommand) cmd = NULL;
-VIR_AUTOFREE(char *) cmdstr = NULL;
VIR_AUTOFREE(char *) pidfile = NULL;
VIR_AUTOFREE(char *) dbus_path = NULL;
VIR_AUTOFREE(char *) dbus_addr = NULL;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] virdomainobjlist: Documnet virDomainObjListForEach()
Signed-off-by: Michal Privoznik --- src/conf/virdomainobjlist.c | 13 + 1 file changed, 13 insertions(+) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d640da6205..11fd68745b 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -815,6 +815,19 @@ virDomainObjListHelper(void *payload, } +/** + * virDomainObjListForEach: + * @doms: Pointer to the domain object list + * @callback: callback to run over each domain on the list + * @opaque: opaque data to pass to @callback + * + * For every domain on the list (@doms) run @callback on it. If + * @callback fails (i.e. returns a negative value), the iteration + * carries still on until all domains are visited. + * + * Returns: 0 on success, + * -1 otherwise. + */ int virDomainObjListForEach(virDomainObjListPtr doms, virDomainObjListIterator callback, -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] Couple of qemu fixups
The first issue (patch 2/4) is an issue I've found by code investigation. The second (patch 3/4) fixes a build problem and the last patch disables detection of a feature that is not merged in QEMU just yet. Michal Prívozník (4): virdomainobjlist: Documnet virDomainObjListForEach() lib: Grab write lock when modifying list of domains qemu_slirp: Drop unused variable in qemuSlirpStart() qemu_capabilities: Temporarily disable dbus-vmstate capability src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c| 2 +- src/conf/virdomainobjlist.c | 23 ++- src/conf/virdomainobjlist.h | 1 + src/libxl/libxl_driver.c | 11 +++ src/lxc/lxc_process.c| 4 ++-- src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_slirp.c| 1 - src/vmware/vmware_driver.c | 2 +- 10 files changed, 37 insertions(+), 13 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: Fix libxlDomainPMSuspendForDuration domain active check
On Fri, Sep 06, 2019 at 04:12:55PM +0200, Marek Marczykowski-Górecki wrote: > virDomainObjCheckActive() returns -1 if domain is not active, not 0. > > Fixes cb50436c6f "libxl: implement virDomainPM* functions" > Signed-off-by: Marek Marczykowski-Górecki > --- > src/libxl/libxl_driver.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé and pushed to git Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libxl: Fix libxlDomainPMSuspendForDuration domain active check
virDomainObjCheckActive() returns -1 if domain is not active, not 0. Fixes cb50436c6f "libxl: implement virDomainPM* functions" Signed-off-by: Marek Marczykowski-Górecki --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d0396e4781..215471fa0d 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1459,7 +1459,7 @@ libxlDomainPMSuspendForDuration(virDomainPtr dom, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; -if (!virDomainObjCheckActive(vm)) +if (virDomainObjCheckActive(vm) < 0) goto endjob; /* Unlock virDomainObjPtr to not deadlock with even handler, which will try -- 2.21.0 signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: Adjust max memlock on mdev hotplug
On 9/3/19 5:09 PM, Eric Farman wrote:
When starting a domain, we use the presence of a vfio-pci or
mdev hostdev to determine if the memlock maximum needs to be
increased. But if we hotplug either of these devices, only the
vfio-pci path gets that love. This means that attaching a, say,
vfio-ccw device will appear to succeed but the device may be
unusable as the guest may see I/O errors on long CCW chains.
The host, meanwhile, would be flooded with these messages:
vfio_pin_page_external: Task qemu-system-s39 (11584) RLIMIT_MEMLOCK (65536)
exceeded
Let's adjust the maximum memlock value in the mdev hotplug path,
so that the domain has the same value as if it were started with
one or more mdev devices in its configuration.
(Note: I started trying to refactor qemuDomainAdjustMaxMemLockHostdev()
to address the "A better way to handle this would be nice" comment,
but the result was getting way too involved for the problem I was
trying to fix. Maybe another day.)
Signed-off-by: Eric Farman
---
Reviewed-by: Daniel Henrique Barboza
src/qemu/qemu_hotplug.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 979e97b608..ae7c5395d2 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2755,6 +2755,7 @@ qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver,
bool teardowncgroup = false;
bool teardownlabel = false;
bool teardowndevice = false;
+bool teardownmemlock = false;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_HOSTDEV,
{ .hostdev = hostdev } };
@@ -2798,6 +2799,10 @@ qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver,
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
goto cleanup;
+if (qemuDomainAdjustMaxMemLockHostdev(vm, hostdev) < 0)
+goto cleanup;
+teardownmemlock = true;
+
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorAddDevice(priv->mon, devstr);
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
@@ -2813,6 +2818,8 @@ qemuDomainAttachMediatedDevice(virQEMUDriverPtr driver,
ret = 0;
cleanup:
if (ret < 0) {
+if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0)
+VIR_WARN("Unable to reset maximum locked memory on hotplug fail");
if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
VIR_WARN("Unable to remove host device cgroup ACL on hotplug
fail");
if (teardownlabel &&
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] qemu: Reset the maximum locked memory on hotplug fail
On 9/3/19 5:09 PM, Eric Farman wrote:
If attaching a PCI hostdev fails, there are several things that
need to be un-done as part of the cleanup. One thing that is
not done is re-calculating/re-setting the maximum amount of locked
memory for the domain, since we may have changed that.
Let's fix that, just to ensure everything is back the way it was.
Signed-off-by: Eric Farman
---
Ouch. This is a bug fix that should be pushed regardless of the rest of the
series, IMHO.
Reviewed-by: Daniel Henrique Barboza
src/qemu/qemu_hotplug.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 24e75e49be..979e97b608 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1469,6 +1469,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
bool teardowncgroup = false;
bool teardownlabel = false;
bool teardowndevice = false;
+bool teardownmemlock = false;
int backend;
VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
virQEMUDriverGetConfig(driver);
unsigned int flags = 0;
@@ -1510,6 +1511,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
if (qemuDomainAdjustMaxMemLockHostdev(vm, hostdev) < 0)
goto error;
+teardownmemlock = true;
if (qemuDomainNamespaceSetupHostdev(vm, hostdev) < 0)
goto error;
@@ -1577,6 +1579,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
if (teardowndevice &&
qemuDomainNamespaceTeardownHostdev(vm, hostdev) < 0)
VIR_WARN("Unable to remove host device from /dev");
+if (teardownmemlock && qemuDomainAdjustMaxMemLock(vm) < 0)
+VIR_WARN("Unable to reset maximum locked memory on hotplug fail");
if (releaseaddr)
qemuDomainReleaseDeviceAddress(vm, info);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemu: Refactor the max memlock routine
On 9/3/19 5:09 PM, Eric Farman wrote:
Let's pull this hunk out into a function, so it can be reused
in another codepath that needs to do the same thing.
Signed-off-by: Eric Farman
---
Reviewed-by: Daniel Henrique Barboza
src/qemu/qemu_domain.c | 30 ++
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_hotplug.c | 11 +--
3 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c7eb0b5e9a..a63a035328 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11723,6 +11723,36 @@ qemuDomainAdjustMaxMemLock(virDomainObjPtr vm)
return ret;
}
+
+/**
+ * qemuDomainAdjustMaxMemLockHostdev:
+ * @vm: domain
+ * @hostdev: device
+ *
+ * Temporarily add the hostdev to the domain definition. This is needed
+ * because qemuDomainAdjustMaxMemLock() requires the hostdev to be already
+ * part of the domain definition, but other functions like
+ * qemuAssignDeviceHostdevAlias() expect it *not* to be there.
+ * A better way to handle this would be nice
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+int
+qemuDomainAdjustMaxMemLockHostdev(virDomainObjPtr vm,
+ virDomainHostdevDefPtr hostdev)
+{
+int ret = 0;
+
+vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
+if (qemuDomainAdjustMaxMemLock(vm) < 0)
+ret = -1;
+
+vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
+
+return ret;
+}
+
+
/**
* qemuDomainHasVcpuPids:
* @vm: Domain object
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index d097f23342..456c623336 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -931,6 +931,8 @@ void qemuDomainUpdateCurrentMemorySize(virDomainObjPtr vm);
unsigned long long qemuDomainGetMemLockLimitBytes(virDomainDefPtr def);
int qemuDomainAdjustMaxMemLock(virDomainObjPtr vm);
+int qemuDomainAdjustMaxMemLockHostdev(virDomainObjPtr vm,
+ virDomainHostdevDefPtr hostdev);
int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
virQEMUCapsPtr qemuCaps,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 63acb9c451..24e75e49be 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1508,17 +1508,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
break;
}
-/* Temporarily add the hostdev to the domain definition. This is needed
- * because qemuDomainAdjustMaxMemLock() requires the hostdev to be already
- * part of the domain definition, but other functions like
- * qemuAssignDeviceHostdevAlias() used below expect it *not* to be there.
- * A better way to handle this would be nice */
-vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
-if (qemuDomainAdjustMaxMemLock(vm) < 0) {
-vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
+if (qemuDomainAdjustMaxMemLockHostdev(vm, hostdev) < 0)
goto error;
-}
-vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
if (qemuDomainNamespaceSetupHostdev(vm, hostdev) < 0)
goto error;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: update threading info about domain object refs
On 9/4/19 2:23 PM, Jonathon Jongsma wrote:
Since commit fd9ef3b31e, virDomainFindByUUIDRef() no longer exists and
all virDomainObjListFindBy*() functions now increment the reference
count.
Signed-off-by: Jonathon Jongsma
---
src/qemu/THREADS.txt | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
Reviewed-by: Daniel Henrique Barboza
diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt
index d17f3f4e0c..aa428fda6a 100644
--- a/src/qemu/THREADS.txt
+++ b/src/qemu/THREADS.txt
@@ -25,27 +25,17 @@ There are a number of locks on various objects
* virDomainObjPtr
-Will be locked after calling any of the virDomainObjListFindBy{ID,Name,UUID}
-methods. However, preferred method is qemuDomObjFromDomain() that uses
-virDomainFindByUUIDRef() which also increases the reference counter and
-finds the domain in the domain list without blocking all other lookups.
-When the domain is locked and the reference increased, the preferred way of
-decrementing the reference counter and unlocking the domain is using the
+Will be locked and the reference counter will be increased after calling
+any of the virDomainObjListFindBy{ID,Name,UUID} methods. The preferred way
+of decrementing the reference counter and unlocking the domain is using the
virDomainObjEndAPI() function.
Lock must be held when changing/reading any variable in the virDomainObjPtr
-If the lock needs to be dropped & then re-acquired for a short period of
-time, the reference count must be incremented first using
virDomainObjRef().
-There is no need to increase the reference count if qemuDomObjFromDomain()
-was used for looking up the domain. In this case there is one reference
-already added by that function.
-
This lock must not be held for anything which sleeps/waits (i.e. monitor
commands).
-
* qemuMonitorPrivatePtr: Job conditions
Since virDomainObjPtr lock must not be held during sleeps, the job
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 16/23] qemu: add a flag to the cookie to prevent slirp-helper setup
On 8/8/19 10:55 AM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> For VM started and migrated/saved without slirp-helpers, let's prevent
> the automatic setup (as it would fail to migrate otherwise).
>
> Signed-off-by: Marc-André Lureau
> ---
> src/qemu/qemu_domain.c | 30 --
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 8
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ff83d1c024..4b49203738 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6971,6 +6971,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> bool start_paused,
> qemuDomainAsyncJob asyncJob)
> {
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> int ret = -1;
> bool restored = false;
> virObjectEventPtr event;
> @@ -7011,6 +7012,9 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
> qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
> goto cleanup;
>
> +if (!cookie->slirpHelper)
> +priv->disableSlirp = true;
> +
Coverity lets me know that the above will need to have a "cookie &&" in
the if statement (similar to the lines just above it and of course the
check just below for @cookie being NULL...
John
> if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL,
> asyncJob, "stdio", *fd, path, NULL,
> VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
> @@ -16654,6 +16658,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> virCPUDefFree(priv->origCPU);
> VIR_STEAL_PTR(priv->origCPU, origCPU);
> }
> +
> +if (cookie && !cookie->slirpHelper)
> +priv->disableSlirp = true;
hmm... yeah, just like this ;-)
> +
> } else {
> /* Transitions 2, 3 */
> load:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util: Set backing file name for LOOP_GET_STATUS64 queries.
For further reference:
https://github.com/lxc/lxc/commit/a70c9e85a6d8ac1b75d6705d2373fd9c7b567240
Em seg, 2 de set de 2019 às 14:00, escreveu:
>
> From: Julio Faracco
>
> This is an issue for LXC loop devices when you are trying to get loop
> devices info using `ioctl`. Modern apps uses `/sys/dev/block` to grab
> information about devices, but if you use the method mention you won't
> be able to retrive the associated file with that loop device. See
> example below from cryptsetup sources:
>
> static char *_ioctl_backing_file(const char *loop)
> {
> struct loop_info64 lo64 = {0};
> int loop_fd;
>
> loop_fd = open(loop, O_RDONLY);
> if (loop_fd < 0)
> return NULL;
>
> if (ioctl(loop_fd, LOOP_GET_STATUS64, &lo64) < 0) {
> close(loop_fd);
> return NULL;
> }
>
> lo64.lo_file_name[LO_NAME_SIZE-2] = '*';
> lo64.lo_file_name[LO_NAME_SIZE-1] = 0;
>
> close(loop_fd);
> return strdup((char*)lo64.lo_file_name);
> }
>
> It will return an empty string because lo_file_name was not set.
> Function `virFileLoopDeviceOpenSearch()` is using `ioctl` to query data,
> but it is not checking `lo_file_name` field.
>
> v1: I accidentally committed two wrong lines.
>
> Signed-off-by: Julio Faracco
> ---
> src/util/virfile.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 81a3c096eb..dbfe74e24f 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -781,6 +781,14 @@ int virFileLoopDeviceAssociate(const char *file,
> memset(&lo, 0, sizeof(lo));
> lo.lo_flags = LO_FLAGS_AUTOCLEAR;
>
> +/* Set backing file name for LOOP_GET_STATUS64 queries */
> +if (virStrncpy((char *) lo.lo_file_name, file,
> + strlen(file), LO_NAME_SIZE) < 0) {
> +virReportSystemError(errno,
> + _("Unable to set backing file %s"), file);
> +goto cleanup;
> +}
> +
> if ((fsfd = open(file, O_RDWR)) < 0) {
> virReportSystemError(errno,
> _("Unable to open %s"), file);
> --
> 2.20.1
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dockerfiles PATCH] refresh: Update Dockerfiles due to new "locales" dependency
Let's refresh the archived Dockerfiles as "locales" dependency has to be added to all Debian, Ubuntu, and Fedora files. Signed-off-by: Fabiano Fidêncio --- This commit is available in the following branch: https://gitlab.com/fidencio/libvirt-dockerfiles/tree/wip/locales Here's the libvirt-jenkins-ci counter part: https://www.redhat.com/archives/libvir-list/2019-September/msg00270.html --- buildenv-libosinfo-debian-10.zip | Bin 559 -> 567 bytes buildenv-libosinfo-debian-sid.zip | Bin 559 -> 567 bytes buildenv-libosinfo-fedora-29.zip | Bin 492 -> 502 bytes buildenv-libosinfo-fedora-30.zip | Bin 555 -> 564 bytes buildenv-libosinfo-fedora-rawhide.zip | Bin 512 -> 522 bytes buildenv-libvirt-debian-10-cross-aarch64.zip | Bin 887 -> 892 bytes buildenv-libvirt-debian-10-cross-armv6l.zip| Bin 880 -> 885 bytes buildenv-libvirt-debian-10-cross-armv7l.zip| Bin 884 -> 890 bytes buildenv-libvirt-debian-10-cross-i686.zip | Bin 883 -> 889 bytes buildenv-libvirt-debian-10-cross-mips.zip | Bin 878 -> 885 bytes buildenv-libvirt-debian-10-cross-mips64el.zip | Bin 892 -> 896 bytes buildenv-libvirt-debian-10-cross-mipsel.zip| Bin 882 -> 887 bytes buildenv-libvirt-debian-10-cross-ppc64le.zip | Bin 890 -> 896 bytes buildenv-libvirt-debian-10-cross-s390x.zip | Bin 883 -> 888 bytes buildenv-libvirt-debian-10.zip | Bin 773 -> 778 bytes buildenv-libvirt-debian-9-cross-aarch64.zip| Bin 896 -> 901 bytes buildenv-libvirt-debian-9-cross-armv6l.zip | Bin 887 -> 892 bytes buildenv-libvirt-debian-9-cross-armv7l.zip | Bin 893 -> 898 bytes buildenv-libvirt-debian-9-cross-mips.zip | Bin 886 -> 892 bytes buildenv-libvirt-debian-9-cross-mips64el.zip | Bin 898 -> 903 bytes buildenv-libvirt-debian-9-cross-mipsel.zip | Bin 889 -> 895 bytes buildenv-libvirt-debian-9-cross-ppc64le.zip| Bin 898 -> 903 bytes buildenv-libvirt-debian-9-cross-s390x.zip | Bin 890 -> 895 bytes buildenv-libvirt-debian-9.zip | Bin 777 -> 782 bytes buildenv-libvirt-debian-sid-cross-aarch64.zip | Bin 886 -> 891 bytes buildenv-libvirt-debian-sid-cross-armv6l.zip | Bin 879 -> 885 bytes buildenv-libvirt-debian-sid-cross-armv7l.zip | Bin 884 -> 890 bytes buildenv-libvirt-debian-sid-cross-i686.zip | Bin 883 -> 889 bytes buildenv-libvirt-debian-sid-cross-mips.zip | Bin 877 -> 884 bytes buildenv-libvirt-debian-sid-cross-mips64el.zip | Bin 892 -> 896 bytes buildenv-libvirt-debian-sid-cross-mipsel.zip | Bin 882 -> 887 bytes buildenv-libvirt-debian-sid-cross-ppc64le.zip | Bin 890 -> 896 bytes buildenv-libvirt-debian-sid-cross-s390x.zip| Bin 883 -> 888 bytes buildenv-libvirt-debian-sid.zip| Bin 773 -> 778 bytes buildenv-libvirt-fedora-29.zip | Bin 694 -> 706 bytes buildenv-libvirt-fedora-30.zip | Bin 809 -> 821 bytes buildenv-libvirt-fedora-rawhide.zip| Bin 714 -> 726 bytes buildenv-libvirt-ubuntu-16.zip | Bin 784 -> 788 bytes buildenv-libvirt-ubuntu-18.zip | Bin 783 -> 790 bytes 39 files changed, 0 insertions(+), 0 deletions(-) diff --git a/buildenv-libosinfo-debian-10.zip b/buildenv-libosinfo-debian-10.zip index 4a057eb..6413bb9 100644 Binary files a/buildenv-libosinfo-debian-10.zip and b/buildenv-libosinfo-debian-10.zip differ diff --git a/buildenv-libosinfo-debian-sid.zip b/buildenv-libosinfo-debian-sid.zip index 86cf9dd..b077868 100644 Binary files a/buildenv-libosinfo-debian-sid.zip and b/buildenv-libosinfo-debian-sid.zip differ diff --git a/buildenv-libosinfo-fedora-29.zip b/buildenv-libosinfo-fedora-29.zip index 8fac1a5..185000f 100644 Binary files a/buildenv-libosinfo-fedora-29.zip and b/buildenv-libosinfo-fedora-29.zip differ diff --git a/buildenv-libosinfo-fedora-30.zip b/buildenv-libosinfo-fedora-30.zip index 10f89e9..f3a0914 100644 Binary files a/buildenv-libosinfo-fedora-30.zip and b/buildenv-libosinfo-fedora-30.zip differ diff --git a/buildenv-libosinfo-fedora-rawhide.zip b/buildenv-libosinfo-fedora-rawhide.zip index 7cf81df..8d2d603 100644 Binary files a/buildenv-libosinfo-fedora-rawhide.zip and b/buildenv-libosinfo-fedora-rawhide.zip differ diff --git a/buildenv-libvirt-debian-10-cross-aarch64.zip b/buildenv-libvirt-debian-10-cross-aarch64.zip index 8f2dbbc..ae285b9 100644 Binary files a/buildenv-libvirt-debian-10-cross-aarch64.zip and b/buildenv-libvirt-debian-10-cross-aarch64.zip differ diff --git a/buildenv-libvirt-debian-10-cross-armv6l.zip b/buildenv-libvirt-debian-10-cross-armv6l.zip index 774ff27..bb4adda 100644 Binary files a/buildenv-libvirt-debian-10-cross-armv6l.zip and b/buildenv-libvirt-debian-10-cross-armv6l.zip differ diff --git a/buildenv-libvirt-debian-10-cross-armv7l.zip b/buildenv-libvirt-debian-10-cross-armv7l.zip index 893c929..7bf00e1 100644 Binary files a/buildenv-libvirt-debian-10-cross-armv7l.zip and b/buildenv-libvirt-debi
Re: [libvirt] [jenkins-ci PATCH] guests: Include "locales" by default
> + locales: > +CentOS: glibc-common Sorry, CentOS should be CentOS7. Sent a v2 with this one fixed. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH v2] guests: Include "locales" by default
osinfo-db tests require "en_US.UTF-8" locale to be set. Unfortunately, our containers do not contain the needed locale file. After a discussion on libosinfo mailing list[0], it's been agreed on having the locale as part of our libvirt-jenkins-ci's base packages. [0]: https://www.redhat.com/archives/libosinfo/2019-September/msg00011.html Signed-off-by: Fabiano Fidêncio --- guests/vars/mappings.yml | 6 ++ guests/vars/projects/base.yml | 1 + 2 files changed, 7 insertions(+) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index c5a7824..bc6ea69 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -431,6 +431,12 @@ mappings: rpm: libxslt-devel cross-policy-deb: foreign + locales: +CentOS7: glibc-common +deb: locales +Fedora: glibc-langpack-en +FreeBSD: + lsof: default: lsof diff --git a/guests/vars/projects/base.yml b/guests/vars/projects/base.yml index a7e572b..2a84029 100644 --- a/guests/vars/projects/base.yml +++ b/guests/vars/projects/base.yml @@ -16,6 +16,7 @@ packages: - glibc - libtool - libtoolize + - locales - lsof - net-tools - make -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [jenkins-ci PATCH] guests: Include "locales" by default
osinfo-db tests require "en_US.UTF-8" locale to be set. Unfortunately, our containers do not contain the needed locale file. After a discussion on libosinfo mailing list[0], it's been agreed on having the locale as part of our libvirt-jenkins-ci's base packages. [0]: https://www.redhat.com/archives/libosinfo/2019-September/msg00011.html Signed-off-by: Fabiano Fidêncio --- guests/vars/mappings.yml | 6 ++ guests/vars/projects/base.yml | 1 + 2 files changed, 7 insertions(+) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index c5a7824..90ac611 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -431,6 +431,12 @@ mappings: rpm: libxslt-devel cross-policy-deb: foreign + locales: +CentOS: glibc-common +deb: locales +Fedora: glibc-langpack-en +FreeBSD: + lsof: default: lsof diff --git a/guests/vars/projects/base.yml b/guests/vars/projects/base.yml index a7e572b..2a84029 100644 --- a/guests/vars/projects/base.yml +++ b/guests/vars/projects/base.yml @@ -16,6 +16,7 @@ packages: - glibc - libtool - libtoolize + - locales - lsof - net-tools - make -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 00/23] Use a slirp helper process
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Hi, SLIRP networking can be running in a separate process. This allows for stricter security policies for QEMU & SLIRP, as SLIRP is notoriously not very safe (discussed on ML, various CVEs, and even the code says so explicitly in the comments), yet people rely on it for various reasons. With this series, for a network interface "user", libvirt will: - check the slirp-helper presence and capabilites (see [1]) - setup a socket pair between qemu and the helper - use -net socket - setup migration thanks to dbus-vmstate There are no changes required to domain configuration to benefit it. "guestfwd" isn't supported at this point, but it is known to be in a broken state with libvirt+qemu anyway. The dbus-vmstate is being proposed to QEMU. The libslirp-rs slirp-helper hasn't yet received a release. The current DBus p2p mode works ok, but is a hack. This is due to poor DBus support in Rust, and also relatively poor DBus p2p mode support in libdbus. fwiw, I have been working on an alternative rust-only implementation of a slirp-helper that will also follow [1], but I am now wondering if netstack or vpnkit could do the job. [1] https://gitlab.freedesktop.org/slirp/libslirp-rs/blob/master/src/bin/README.rst Marc-André Lureau (23): Add .editorconfig tests: fix xml2xml tpm-emulator.xml test dbus: correctly build reply message qemu: replace logCtxt with qemuDomainLogAppendMessage() qemu: add socket datagram capability qemu: add dbus-vmstate capability qemu: reset VM id after external devices stop qemu-security: add qemuSecurityCommandRun() qemu: add dbus-vmstate domain-conf: add network def private data qemu: add qemuDomainNetworkPrivate qemu-conf: add configurable slirp-helper location qemu-conf: add slirp state dir qemu: add slirp helper unit qemu-domain: save and restore slirp state qemu: add a flag to the cookie to prevent slirp-helper setup qemu-migration: prevent migration if dbus-vmstate is required qemu-migration: prevent migration if slirp cannot be migrated qemu-extdevice: prepare, start and stop slirp-helper qemu-command: use -net socket,fd= with slirp-helper qemu-process: prepare slirp-helper qemu-hotplug: handle hotplugging of slirp-helper tests: add slirp-helper qemuxml2argv test .editorconfig | 21 + m4/virt-driver-qemu.m4| 5 + src/conf/domain_conf.c| 21 +- src/conf/domain_conf.h| 6 + src/qemu/Makefile.inc.am | 4 + src/qemu/libvirtd_qemu.aug| 1 + src/qemu/qemu.conf| 3 + src/qemu/qemu_alias.c | 17 + src/qemu/qemu_alias.h | 3 + src/qemu/qemu_capabilities.c | 8 + src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 118 - src/qemu/qemu_command.h | 6 +- src/qemu/qemu_conf.c | 11 +- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_dbus.c | 94 src/qemu/qemu_dbus.h | 42 ++ src/qemu/qemu_domain.c| 220 - src/qemu/qemu_domain.h| 20 + src/qemu/qemu_driver.c| 8 + src/qemu/qemu_extdevice.c | 82 ++-- src/qemu/qemu_extdevice.h | 10 +- src/qemu/qemu_hotplug.c | 112 - src/qemu/qemu_hotplug.h | 11 + src/qemu/qemu_interface.c | 27 ++ src/qemu/qemu_interface.h | 4 + src/qemu/qemu_migration.c | 19 + src/qemu/qemu_monitor.c | 13 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_process.c | 24 +- src/qemu/qemu_security.c | 22 + src/qemu/qemu_security.h | 6 + src/qemu/qemu_slirp.c | 448 ++ src/qemu/qemu_slirp.h | 81 src/qemu/qemu_tpm.c | 4 +- src/qemu/test_libvirtd_qemu.aug.in| 1 + src/util/virdbus.c| 18 +- src/util/virdbus.h| 6 +- .../caps_4.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + .../caps_4.0.0.riscv32.xml| 1 + .../caps_4.0.0.riscv64.xml| 1 + .../qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + .../caps_4.0.0.x86_64.xml | 1 + .../caps_4.1.0.x86_64.xml | 1 + .../net-user.x86_64-4.0.0.args
Re: [libvirt] [PATCH v2 17/23] qemu-migration: prevent migration if dbus-vmstate is required
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Signed-off-by: Marc-André Lureau --- src/qemu/qemu_migration.c | 7 +++ 1 file changed, 7 insertions(+) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 18/23] qemu-migration: prevent migration if slirp cannot be migrated
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Signed-off-by: Marc-André Lureau --- src/qemu/qemu_migration.c | 12 1 file changed, 12 insertions(+) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 09/23] qemu: add dbus-vmstate
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Add dbusVMStates to keep a list of dbus-vmstate objects needed for
migration. They are populated on the command line during start or
qemuDBusVMStateAdd/Remove() will hotplug them as needed.
Signed-off-by: Marc-André Lureau
---
src/qemu/Makefile.inc.am | 2 +
src/qemu/qemu_alias.c| 17
src/qemu/qemu_alias.h| 3 ++
src/qemu/qemu_command.c | 83 +++
src/qemu/qemu_command.h | 3 ++
src/qemu/qemu_dbus.c | 94
src/qemu/qemu_dbus.h | 42 ++
src/qemu/qemu_domain.c | 14 ++
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_hotplug.c | 75
src/qemu/qemu_hotplug.h | 11 +
11 files changed, 346 insertions(+)
create mode 100644 src/qemu/qemu_dbus.c
create mode 100644 src/qemu/qemu_dbus.h
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 254ba07dc0..94dd0e56ff 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -13,6 +13,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_capabilities.h \
qemu/qemu_command.c \
qemu/qemu_command.h \
+ qemu/qemu_dbus.c \
+ qemu/qemu_dbus.h \
qemu/qemu_domain.c \
qemu/qemu_domain.h \
qemu/qemu_domain_address.c \
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 585cc972ba..fc5246bc7f 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -843,3 +843,20 @@ qemuDomainGetUnmanagedPRAlias(const char *parentalias)
return ret;
}
+
+char *
+qemuAliasDBusVMStateFromId(const char *id)
+{
+char *ret;
+int i;
This needs to be size_t rather than int.
+
+if (virAsprintf(&ret, "dbus-vms-%s", id) < 0)
+return NULL;
+
+for (i = 0; ret[i]; i++) {
+if (ret[i] == ':')
+ret[i] = '_';
+}
+
+return ret;
+}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index aaac09a1d1..ae2fce16bc 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -95,3 +95,6 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias)
const char *qemuDomainGetManagedPRAlias(void);
char *qemuDomainGetUnmanagedPRAlias(const char *parentalias);
+
+char *qemuAliasDBusVMStateFromId(const char *id)
+ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 71a36ff63a..4357aa2fe1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -27,6 +27,7 @@
#include "qemu_interface.h"
#include "qemu_alias.h"
#include "qemu_security.h"
+#include "qemu_dbus.h"
#include "qemu_block.h"
#include "cpu/cpu.h"
#include "dirname.h"
@@ -10386,6 +10387,85 @@ qemuBuildManagedPRCommandLine(virCommandPtr cmd,
}
+static virJSONValuePtr
+qemuBuildDBusVMStateInfoPropsInternal(const char *alias,
+ const char *addr)
+{
+virJSONValuePtr ret = NULL;
+
+if (qemuMonitorCreateObjectProps(&ret,
+ "dbus-vmstate", alias,
+ "s:addr", addr, NULL) < 0)
+return NULL;
+
+return ret;
+}
+
+
+virJSONValuePtr
+qemuBuildDBusVMStateInfoProps(const char *id,
+ const char *addr)
+{
+VIR_AUTOFREE(char *) alias = qemuAliasDBusVMStateFromId(id);
+
+if (!alias)
+return NULL;
+
+return qemuBuildDBusVMStateInfoPropsInternal(alias, addr);
+}
+
+
+typedef struct qemuBuildDBusVMStateCommandLineData {
+virCommandPtr cmd;
+} qemuBuildDBusVMStateCommandLineData;
+
+
+static int
+qemuBuildDBusVMStateCommandLineEach(void *payload,
+const void *id,
+void *user_data)
+{
+qemuBuildDBusVMStateCommandLineData *data = user_data;
+qemuDBusVMStatePtr vms = payload;
+VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+VIR_AUTOPTR(virJSONValue) props = NULL;
+
+if (!(props = qemuBuildDBusVMStateInfoProps(id, vms->addr)))
+return -1;
+
+if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0)
+return -1;
+
+virCommandAddArg(data->cmd, "-object");
+virCommandAddArgBuffer(data->cmd, &buf);
+
+return 0;
+}
+
+static int
+qemuBuildDBusVMStateCommandLine(virCommandPtr cmd,
+qemuDomainObjPrivatePtr priv)
+{
+qemuBuildDBusVMStateCommandLineData data = {
+.cmd = cmd,
+};
+
+if (virHashSize(priv->dbusVMStates) == 0)
+return 0;
+
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("dbus-vmstate object is not supported by this QEMU
binary"));
+return 0;
+}
+
+if (virHashForEach(priv->dbusVMStates, qemuBuildDBusVMStateCommandLineEach,
&data) < 0)
+return -1;
+
+return 0;
+}
+
+
/**
* qemuBuildC
Re: [libvirt] [PATCH v2 07/23] qemu: reset VM id after external devices stop
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau pid filenames (from swtpm and other helpers from this series) are based on VM shortname, which is derived from VM id. If the id is reset to early, the state filenames will not be found. Signed-off-by: Marc-André Lureau --- src/qemu/qemu_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/23] domain-conf: add network def private data
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Signed-off-by: Marc-André Lureau
> ---
> src/conf/domain_conf.c | 21 -
> src/conf/domain_conf.h | 6 ++
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0456369d55..fb0904177f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2454,6 +2454,7 @@ virDomainNetDefFree(virDomainNetDefPtr def)
> if (!def)
> return;
> virDomainNetDefClear(def);
> +virObjectUnref(def->privateData);
> VIR_FREE(def);
> }
>
> @@ -11441,7 +11442,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> VIR_AUTOFREE(char *) trustGuestRxFilters = NULL;
> VIR_AUTOFREE(char *) vhost_path = NULL;
>
> -if (VIR_ALLOC(def) < 0)
> +if (!(def = virDomainNetDefNew(xmlopt)))
> return NULL;
>
> ctxt->node = node;
> @@ -14337,6 +14338,24 @@ virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt)
> }
>
>
> +virDomainNetDefPtr
> +virDomainNetDefNew(virDomainXMLOptionPtr xmlopt)
> +{
> +virDomainNetDefPtr def = NULL;
> +
> +if (VIR_ALLOC(def) < 0)
> +return NULL;
> +
> +if (xmlopt && xmlopt->privateData.networkNew &&
> +!(def->privateData = xmlopt->privateData.networkNew())) {
> +VIR_FREE(def);
> +def = NULL;
This call to 'def = NULL' is not needed. VIR_FREE() does that for us. However,
I actually prefer using virDomainNetDefFree() as that is more fool proof if
somebody ever allocs something else in @def after VIR_ALLOC() and before these
lines.
> +}
> +
> +return def;
> +}
> +
> +
> /* Parse the XML definition for a graphics device */
> static virDomainGraphicsDefPtr
> virDomainGraphicsDefParseXML(virDomainXMLOptionPtr xmlopt,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 57ca2a8ad1..9bd196b53c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1018,6 +1018,7 @@ struct _virDomainNetDef {
> unsigned int mtu;
> virNetDevCoalescePtr coalesce;
> virDomainVirtioOptionsPtr virtio;
> +virObjectPtr privateData;
> };
>
> typedef enum {
> @@ -2711,6 +2712,7 @@ struct _virDomainXMLPrivateDataCallbacks {
> virDomainXMLPrivateDataNewFuncchrSourceNew;
> virDomainXMLPrivateDataNewFuncvsockNew;
> virDomainXMLPrivateDataNewFuncgraphicsNew;
> +virDomainXMLPrivateDataNewFuncnetworkNew;
> virDomainXMLPrivateDataFormatFunc format;
> virDomainXMLPrivateDataParseFunc parse;
> /* following function shall return a pointer which will be used as the
> @@ -2894,6 +2896,10 @@ virDomainChrDefPtr
> virDomainChrDefNew(virDomainXMLOptionPtr xmlopt);
>
> virDomainGraphicsDefPtr
> virDomainGraphicsDefNew(virDomainXMLOptionPtr xmlopt);
> +
> +virDomainNetDefPtr
> +virDomainNetDefNew(virDomainXMLOptionPtr xmlopt);
This function must be exposed in libvirt_private.syms too.
I've identified other places where we VIR_ALLOC() a virDomainNetDef structure:
bhyveParsePCINet(), xenParseVif(), lxcCreateNetDef() and vboxDumpNetwork()
In some of them we already have xmlopt available, in others we might pass NULL
safely:
diff --git c/src/bhyve/bhyve_parse_command.c w/src/bhyve/bhyve_parse_command.c
index 490381688c..7d460e9824 100644
--- c/src/bhyve/bhyve_parse_command.c
+++ w/src/bhyve/bhyve_parse_command.c
@@ -501,7 +501,7 @@ bhyveParsePCINet(virDomainDefPtr def,
const char *separator = NULL;
const char *mac = NULL;
-if (VIR_ALLOC(net) < 0)
+if (!(net = virDomainNetDefNew(xmlopt)))
goto cleanup;
/* As we only support interface type='bridge' and cannot
diff --git c/src/libvirt_private.syms w/src/libvirt_private.syms
index a34d92f5ef..f1fe7259f9 100644
--- c/src/libvirt_private.syms
+++ w/src/libvirt_private.syms
@@ -478,6 +478,7 @@ virDomainNetDefActualToNetworkPort;
virDomainNetDefClear;
virDomainNetDefFormat;
virDomainNetDefFree;
+virDomainNetDefNew;
virDomainNetDefToNetworkPort;
virDomainNetFind;
virDomainNetFindByName;
diff --git c/src/libxl/xen_common.c w/src/libxl/xen_common.c
index 7eb52c8c84..d327f03d73 100644
--- c/src/libxl/xen_common.c
+++ w/src/libxl/xen_common.c
@@ -1234,7 +1234,7 @@ xenParseVif(char *entry, const char *vif_typename)
key = nextkey;
}
-if (VIR_ALLOC(net) < 0)
+if (!(net = virDomainNetDefNew(NULL)))
goto cleanup;
if (mac[0]) {
diff --git c/src/lxc/lxc_native.c w/src/lxc/lxc_native.c
index b4c6e790d8..018eec6977 100644
--- c/src/lxc/lxc_native.c
+++ w/src/lxc/lxc_native.c
@@ -359,7 +359,7 @@ lxcCreateNetDef(const char *type,
virDomainNetDefPtr net = NULL;
virMacAddr macAddr;
-if (VIR_ALLOC(net) < 0)
+if (!(net = virDomainNetDefNew(NULL)))
goto error;
if (STREQ_NULLABLE(flag, "up"))
diff --git c/src/vbox/vbox_common.c w/src/vbox/vbox_common.c
index 4
Re: [libvirt] [PATCH v2 13/23] qemu-conf: add slirp state dir
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_conf.c | 4
src/qemu/qemu_conf.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4b84cb6dea..7d2e84b5bb 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -216,6 +216,9 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
if (virAsprintf(&cfg->swtpmStateDir, "%s/swtpm", cfg->stateDir) < 0)
goto error;
+if (virAsprintf(&cfg->slirpStateDir, "%s/slirp", cfg->stateDir) < 0)
+goto error;
+
if (!(cfg->configBaseDir = virGetUserConfigDirectory()))
goto error;
Missing initialization for @privileged == true case. Although, if you do
this outside of this if() statement, then you don't need to worry.
@@ -335,6 +338,7 @@ static void virQEMUDriverConfigDispose(void *obj)
VIR_FREE(cfg->swtpmLogDir);
VIR_FREE(cfg->stateDir);
VIR_FREE(cfg->swtpmStateDir);
+VIR_FREE(cfg->slirpStateDir);
VIR_FREE(cfg->libDir);
VIR_FREE(cfg->cacheDir);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index a85ae50e14..8473d6d4ca 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -96,6 +96,7 @@ struct _virQEMUDriverConfig {
char *swtpmLogDir;
char *stateDir;
char *swtpmStateDir;
+char *slirpStateDir;
/* These two directories are ones QEMU processes use (so must match
* the QEMU user/group */
char *libDir;
Also, what is missing is the dir creation and chown() that should be
done in qemuStateInitialize().
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 04/23] qemu: replace logCtxt with qemuDomainLogAppendMessage()
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Once QEMU is started, the qemuDomainLogContext is owned by it, and can
no longer be used from libvirt. Instead, use
qemuDomainLogAppendMessage() which will redirect the log.
This is not strictly necessary for swtpm, but the following patches
are going to reuse qemuExtDeviceLogCommand().
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_extdevice.c | 35 ++-
src/qemu/qemu_extdevice.h | 5 +++--
src/qemu/qemu_tpm.c | 4 ++--
3 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index af52466421..0aa050cb0a 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -36,39 +36,24 @@
VIR_LOG_INIT("qemu.qemu_extdevice");
int
-qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt,
+qemuExtDeviceLogCommand(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
virCommandPtr cmd,
const char *info)
{
-int ret = -1;
-char *timestamp = NULL;
-char *logline = NULL;
-int logFD;
+VIR_AUTOFREE(char *) timestamp = virTimeStringNow();
+VIR_AUTOFREE(char *) cmds = virCommandToString(cmd, false);
-logFD = qemuDomainLogContextGetWriteFD(logCtxt);
-
-if ((timestamp = virTimeStringNow()) == NULL)
-goto cleanup;
-
-if (virAsprintf(&logline, "%s: Starting external device: %s\n",
-timestamp, info) < 0)
-goto cleanup;
-
-if (safewrite(logFD, logline, strlen(logline)) < 0)
-goto cleanup;
-
-virCommandWriteArgLog(cmd, logFD);
-
-ret = 0;
-
- cleanup:
-VIR_FREE(timestamp);
-VIR_FREE(logline);
+if (!timestamp || !cmds)
+return -1;
-return ret;
+return qemuDomainLogAppendMessage(driver, vm,
+ _("%s: Starting external device:
%s\n%s\n"),
+ timestamp, info, cmds);
}
+
/*
* qemuExtDevicesInitPaths:
*
diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
index 5a53c79f38..cdd20c28ef 100644
--- a/src/qemu/qemu_extdevice.h
+++ b/src/qemu/qemu_extdevice.h
@@ -23,10 +23,11 @@
#include "qemu_conf.h"
#include "qemu_domain.h"
-int qemuExtDeviceLogCommand(qemuDomainLogContextPtr logCtxt,
+int qemuExtDeviceLogCommand(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
virCommandPtr cmd,
const char *info)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4)
ATTRIBUTE_RETURN_CHECK;
int qemuExtDevicesPrepareHost(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 98fe8a38b4..7537091e90 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -834,7 +834,7 @@ qemuExtTPMCleanupHost(virDomainDefPtr def)
static int
qemuExtTPMStartEmulator(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-qemuDomainLogContextPtr logCtxt,
+qemuDomainLogContextPtr logCtxt ATTRIBUTE_UNUSED,
I'd go a step further and remove logCtxt from the rest of the functions.
But that can be done in a separate commit.
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 08/23] qemu-security: add qemuSecurityCommandRun()
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Add a generic way to run a command through the security management.
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_security.c | 22 ++
src/qemu/qemu_security.h | 6 ++
2 files changed, 28 insertions(+)
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 3cd6d9bd3d..f8b53e06b3 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -632,3 +632,25 @@ qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr driver,
virSecurityManagerTransactionAbort(driver->securityManager);
return ret;
}
+
+
+int
+qemuSecurityCommandRun(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virCommandPtr cmd,
+ int *exitstatus,
+ int *cmdret)
+{
+if (virSecurityManagerSetChildProcessLabel(driver->securityManager,
+ vm->def, cmd) < 0)
+return -1;
+
+if (virSecurityManagerPreFork(driver->securityManager) < 0)
+return -1;
+
+*cmdret = virCommandRun(cmd, exitstatus);
+
+virSecurityManagerPostFork(driver->securityManager);
+
+return 0;
+}
diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h
index 68e377f418..8cf4ab0721 100644
--- a/src/qemu/qemu_security.h
+++ b/src/qemu/qemu_security.h
@@ -101,6 +101,12 @@ int qemuSecurityRestoreSavedStateLabel(virQEMUDriverPtr
driver,
virDomainObjPtr vm,
const char *savefile);
+int qemuSecurityCommandRun(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virCommandPtr cmd,
+ int *exitstatus,
+ int *cmdret);
+
/* Please note that for these APIs there is no wrapper yet. Do NOT blindly add
* new APIs here. If an API can touch a file add a proper wrapper instead.
*/
Since this is copied from qemuSecurityStartTPMEmulator() I'd expect some
lines to be removed there. And also document what this function does and
describe arguments.
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 02/23] tests: fix xml2xml tpm-emulator.xml test
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau It is failing, because it ends up being parsed with version='default' and expects '1.2' instead. Signed-off-by: Marc-André Lureau --- tests/qemuxml2argvdata/tpm-emulator.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 05/23] qemu: add socket datagram capability
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Datagram socket is available since qemu 4.0, commit
fdec16e3c2a614e2861f3086b05d444b5d8c3406 ("net/socket: learn to talk
with a unix dgram socket").
Required for slirp-helper communication.
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_capabilities.c | 6 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 +
9 files changed, 14 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 405bc3f288..4cb135cd93 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -536,6 +536,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 335 */
"bochs-display",
+ "net-socket-dgram",
);
@@ -4389,6 +4390,11 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps)
ARCH_IS_PPC64(qemuCaps->arch)) {
virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_MAX_CPU_COMPAT);
}
+
+/* -net socket,fd= with dgram socket (for ex, with slirp helper) */
+if (qemuCaps->version >= 3001092) {
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM);
+}
For some weird reasons we don't allow curly braces around a single line
body. Also, I know this is available since 3.1.92 because that's how
qemu advertises its versions towards the end of release cycle, but I'd
rather see 4.0.0 encoded - that's what is going to match version from a
package anyway.
Or even better, isn't there a way for libvirt to detect this using some
monitor sorcery?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 14/23] qemu: add slirp helper unit
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
The unit provides the functions associated with a slirp-helper:
- probing / checking capabilities
- opening the socketpair
- starting / stoping the helper
- registering for dbus-vmstate migration
Signed-off-by: Marc-André Lureau
---
src/qemu/Makefile.inc.am | 2 +
src/qemu/qemu_domain.h | 4 +
src/qemu/qemu_slirp.c| 448 +++
src/qemu/qemu_slirp.h| 81 +++
4 files changed, 535 insertions(+)
create mode 100644 src/qemu/qemu_slirp.c
create mode 100644 src/qemu/qemu_slirp.h
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 94dd0e56ff..505b418fa5 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -58,6 +58,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_security.h \
qemu/qemu_qapi.c \
qemu/qemu_qapi.h \
+ qemu/qemu_slirp.c \
+ qemu/qemu_slirp.h \
qemu/qemu_tpm.c \
qemu/qemu_tpm.h \
$(NULL)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 560b01d80a..7293c87d7c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -509,6 +509,10 @@ struct _qemuDomainGraphicsPrivate {
};
+typedef struct _qemuSlirp qemuSlirp;
+typedef struct _qemuSlirp *qemuSlirpPtr;
+
+
I understand why you need this here, but it feels wrong. This must go
into qemu_slirp.h and then from qemu_domain.h you include qemu_slirp.h.
But that creates a circular dependency via qemu_process.h. But since you
don'y really need qemuProcessIncomingDefPtr in qemu_slirp.h you only
need to know if there's an incoming migration the qemuSlirpStart()
argument can be turned into a boolean type and thus you qemu_process.h
include can be dropped and the circular include is gone.
#define QEMU_DOMAIN_NETWORK_PRIVATE(dev)\
((qemuDomainNetworkPrivatePtr) (dev)->privateData)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
new file mode 100644
index 00..36370b6be0
--- /dev/null
+++ b/src/qemu/qemu_slirp.c
@@ -0,0 +1,448 @@
+/*
+ * qemu_slirp.c: QEMU Slirp support
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ *
Re: [libvirt] [PATCH v2 06/23] qemu: add dbus-vmstate capability
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau This object is being proposed to qemu upstream "Add dbus-vmstate object". It handles data migration of external processes. Signed-off-by: Marc-André Lureau --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 03/23] dbus: correctly build reply message
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau dbus_message_new() does not construct correct replies by itself, it is recommended to use dbus_message_new_method_return() instead. Signed-off-by: Marc-André Lureau --- src/util/virdbus.c | 18 -- src/util/virdbus.h | 6 -- tests/virfirewalltest.c | 9 ++--- tests/virpolkittest.c | 3 ++- 4 files changed, 24 insertions(+), 12 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 01/23] Add .editorconfig
On 8/8/19 4:54 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Consistent code style across editors. Signed-off-by: Marc-André Lureau --- .editorconfig | 21 + 1 file changed, 21 insertions(+) create mode 100644 .editorconfig Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 11/23] qemu: add qemuDomainNetworkPrivate
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_domain.c | 39 +++
src/qemu/qemu_domain.h | 12
2 files changed, 51 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 806dbfd1f8..7315fe103e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1269,6 +1269,44 @@ qemuDomainGraphicsPrivateDispose(void *obj)
}
+static virClassPtr qemuDomainNetworkPrivateClass;
+static void qemuDomainNetworkPrivateDispose(void *obj);
+
+
+static int
+qemuDomainNetworkPrivateOnceInit(void)
+{
+if (!VIR_CLASS_NEW(qemuDomainNetworkPrivate, virClassForObject()))
+return -1;
+
+return 0;
+}
+
+
+VIR_ONCE_GLOBAL_INIT(qemuDomainNetworkPrivate);
+
+
+static virObjectPtr
+qemuDomainNetworkPrivateNew(void)
+{
+qemuDomainNetworkPrivatePtr priv;
+
+if (qemuDomainNetworkPrivateInitialize() < 0)
+return NULL;
+
+if (!(priv = virObjectNew(qemuDomainNetworkPrivateClass)))
+return NULL;
+
+return (virObjectPtr) priv;
+}
+
+
+static void
+qemuDomainNetworkPrivateDispose(void *obj ATTRIBUTE_UNUSED)
+{
+}
+
+
/* qemuDomainSecretPlainSetup:
* @secinfo: Pointer to secret info
* @usageType: The virSecretUsageType
@@ -3428,6 +3466,7 @@ virDomainXMLPrivateDataCallbacks
virQEMUDriverPrivateDataCallbacks = {
.chrSourceNew = qemuDomainChrSourcePrivateNew,
.vsockNew = qemuDomainVsockPrivateNew,
.graphicsNew = qemuDomainGraphicsPrivateNew,
+.networkNew = qemuDomainNetworkPrivateNew,
.parse = qemuDomainObjPrivateXMLParse,
.format = qemuDomainObjPrivateXMLFormat,
.getParseOpaque = qemuDomainObjPrivateXMLGetParseOpaque,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 851fb98f42..560b01d80a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -509,6 +509,18 @@ struct _qemuDomainGraphicsPrivate {
};
+#define QEMU_DOMAIN_NETWORK_PRIVATE(dev)\
+((qemuDomainNetworkPrivatePtr) (dev)->privateData)
We don't try to align backslashes anymore.
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 19/23] qemu-extdevice: prepare, start and stop slirp-helper
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
If a slirp-helper is associated with a network interface,
prepare/start/stop the process via qemu-extdevice.
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_extdevice.c | 47 +--
src/qemu/qemu_extdevice.h | 5 +++--
src/qemu/qemu_process.c | 4 ++--
3 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index 0aa050cb0a..179552a3ae 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -23,6 +23,7 @@
#include "qemu_extdevice.h"
#include "qemu_domain.h"
#include "qemu_tpm.h"
+#include "qemu_slirp.h"
#include "viralloc.h"
#include "virlog.h"
@@ -87,14 +88,24 @@ qemuExtDevicesInitPaths(virQEMUDriverPtr driver,
*/
int
qemuExtDevicesPrepareHost(virQEMUDriverPtr driver,
- virDomainDefPtr def)
+ virDomainObjPtr vm)
{
-int ret = 0;
+virDomainDefPtr def = vm->def;
+size_t i;
-if (def->tpm)
-ret = qemuExtTPMPrepareHost(driver, def);
+if (def->tpm &&
+qemuExtTPMPrepareHost(driver, def) < 0)
+return -1;
-return ret;
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+
+if (slirp && qemuSlirpOpen(slirp, driver, def) < 0)
+return -1;
+}
+
+return 0;
}
@@ -114,15 +125,26 @@ int
qemuExtDevicesStart(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainLogContextPtr logCtxt,
-bool incomingMigration)
+qemuProcessIncomingDefPtr incoming)
No need for this change.
{
+virDomainDefPtr def = vm->def;
int ret = 0;
+size_t i;
if (qemuExtDevicesInitPaths(driver, vm->def) < 0)
return -1;
if (vm->def->tpm)
-ret = qemuExtTPMStart(driver, vm, logCtxt, incomingMigration);
+ret = qemuExtTPMStart(driver, vm, logCtxt, incoming != NULL);
+
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+
+if (slirp &&
+qemuSlirpStart(slirp, vm, driver, net, false, incoming) < 0)
+return -1;
+}
return ret;
}
@@ -132,11 +154,22 @@ void
qemuExtDevicesStop(virQEMUDriverPtr driver,
virDomainObjPtr vm)
{
+virDomainDefPtr def = vm->def;
+size_t i;
+
if (qemuExtDevicesInitPaths(driver, vm->def) < 0)
return;
if (vm->def->tpm)
qemuExtTPMStop(driver, vm);
+
+for (i = 0; i < def->nnets; i++) {
+virDomainNetDefPtr net = def->nets[i];
+qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+
+if (slirp)
+qemuSlirpStop(slirp, vm, driver, net, false);
+}
}
diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h
index cdd20c28ef..a526308c51 100644
--- a/src/qemu/qemu_extdevice.h
+++ b/src/qemu/qemu_extdevice.h
@@ -22,6 +22,7 @@
#include "qemu_conf.h"
#include "qemu_domain.h"
+#include "qemu_process.h"
int qemuExtDeviceLogCommand(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -31,7 +32,7 @@ int qemuExtDeviceLogCommand(virQEMUDriverPtr driver,
ATTRIBUTE_RETURN_CHECK;
int qemuExtDevicesPrepareHost(virQEMUDriverPtr driver,
- virDomainDefPtr def)
+ virDomainObjPtr vm)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ATTRIBUTE_RETURN_CHECK;
@@ -42,7 +43,7 @@ void qemuExtDevicesCleanupHost(virQEMUDriverPtr driver,
int qemuExtDevicesStart(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainLogContextPtr logCtxt,
-bool incomingMigration)
+qemuProcessIncomingDefPtr incoming)
This change is not needed and thus so the include of qemu_process.h is
needless too.
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_RETURN_CHECK;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f8d740979d..d41ee0f25b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6602,7 +6602,7 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
goto cleanup;
VIR_DEBUG("Preparing external devices");
-if (qemuExtDevicesPrepareHost(driver, vm->def) < 0)
+if (qemuExtDevicesPrepareHost(driver, vm) < 0)
goto cleanup;
if (qemuProcessPrepareSEVGuestInput(vm) < 0)
@@ -6776,7 +6776,7 @@ qemuProcessLaunch(virConnectPtr conn,
if (qemuProcessGenID(vm, flags) < 0)
goto cleanup;
-if (qemuExtDevicesStart(driver, vm, logCtxt, incoming !=
Re: [libvirt] [PATCH v2 16/23] qemu: add a flag to the cookie to prevent slirp-helper setup
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau For VM started and migrated/saved without slirp-helpers, let's prevent the automatic setup (as it would fail to migrate otherwise). Signed-off-by: Marc-André Lureau --- src/qemu/qemu_domain.c | 30 -- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_driver.c | 8 3 files changed, 38 insertions(+), 2 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 12/23] qemu-conf: add configurable slirp-helper location
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau A slirp helper is a process that provides user-mode networking through a unix domain socket. It is expected to follow the following specification: https://gitlab.freedesktop.org/slirp/libslirp-rs/blob/master/src/bin/README.rst Signed-off-by: Marc-André Lureau --- m4/virt-driver-qemu.m4 | 5 + src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 +++ src/qemu/qemu_conf.c | 7 ++- src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 17 insertions(+), 1 deletion(-) Yo Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 20/23] qemu-command: use -net socket, fd= with slirp-helper
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
If a slirp-helper is associated with a network interface (after
probing & preparing succesfully), pass the socket fd to QEMU and use
"-net socket,fd=".
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_command.c | 35 +--
src/qemu/qemu_command.h | 3 ++-
src/qemu/qemu_hotplug.c | 4 +++-
3 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4357aa2fe1..90e61a336e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -28,6 +28,7 @@
#include "qemu_alias.h"
#include "qemu_security.h"
#include "qemu_dbus.h"
+#include "qemu_slirp.h"
#include "qemu_block.h"
#include "cpu/cpu.h"
#include "dirname.h"
@@ -4008,7 +4009,8 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
char **tapfd,
size_t tapfdSize,
char **vhostfd,
-size_t vhostfdSize)
+size_t vhostfdSize,
+const char *slirpfd)
{
bool is_tap = false;
virBuffer buf = VIR_BUFFER_INITIALIZER;
@@ -4079,6 +4081,12 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
break;
case VIR_DOMAIN_NET_TYPE_USER:
+if (slirpfd) {
+virBufferAsprintf(&buf, "socket,fd=%s,",
+ slirpfd);
+break;
+}
No, please don't put a break at random places within 'case' bodies.
+
virBufferAddLit(&buf, "user,");
for (i = 0; i < net->guestIP.nips; i++) {
const virNetDevIPAddr *ip = net->guestIP.ips[i];
@@ -8634,10 +8642,10 @@ qemuInterfaceVhostuserConnect(virQEMUDriverPtr driver,
static int
qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
virLogManagerPtr logManager,
virSecurityManagerPtr secManager,
virCommandPtr cmd,
- virDomainDefPtr def,
virDomainNetDefPtr net,
virQEMUCapsPtr qemuCaps,
unsigned int bootindex,
@@ -8646,6 +8654,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
size_t *nnicindexes,
int **nicindexes)
{
+virDomainDefPtr def = vm->def;
int ret = -1;
char *nic = NULL;
char *host = NULL;
@@ -8656,9 +8665,11 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
size_t vhostfdSize = 0;
char **tapfdName = NULL;
char **vhostfdName = NULL;
+VIR_AUTOFREE(char *) slirpfdName = NULL;
virDomainNetType actualType = virDomainNetGetActualType(net);
virNetDevBandwidthPtr actualBandwidth;
bool requireNicdev = false;
+qemuSlirpPtr slirp;
size_t i;
@@ -8884,6 +8895,16 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
goto cleanup;
}
+slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+if (slirp && !standalone) {
+int slirpfd = qemuSlirpGetFD(slirp);
+virCommandPassFD(cmd, slirpfd,
+ VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+if (virAsprintf(&slirpfdName, "%d", slirpfd) < 0)
+goto cleanup;
+}
+
+
for (i = 0; i < tapfdSize; i++) {
if (qemuSecuritySetTapFDLabel(driver->securityManager,
def, tapfd[i]) < 0)
@@ -8908,7 +8929,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
if (!(host = qemuBuildHostNetStr(net, driver,
tapfdName, tapfdSize,
- vhostfdName, vhostfdSize)))
+ vhostfdName, vhostfdSize,
+ slirpfdName)))
goto cleanup;
virCommandAddArgList(cmd, "-netdev", host, NULL);
@@ -8976,10 +8998,10 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
*/
static int
qemuBuildNetCommandLine(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
virLogManagerPtr logManager,
virSecurityManagerPtr secManager,
virCommandPtr cmd,
-virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virNetDevVPortProfileOp vmop,
bool standalone,
@@ -8990,6 +9012,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
size_t i;
int last_good_net = -1;
virErrorPtr originalError = NULL;
+virDomainDefPtr def = vm->def;
if (def->nnets) {
unsigned int bootNet = 0;
@@ -9005,7 +9028,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver,
for (i = 0; i < def->nnets
Re: [libvirt] [PATCH v2 15/23] qemu-domain: save and restore slirp state
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Save & restore the slirp helper PID associated with a network
interface & the probed features.
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_domain.c | 137 +
src/qemu/qemu_domain.h | 2 +-
2 files changed, 138 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7315fe103e..b6f7e8bacc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -32,6 +32,7 @@
#include "qemu_migration.h"
#include "qemu_migration_params.h"
#include "qemu_security.h"
+#include "qemu_slirp.h"
#include "qemu_extdevice.h"
#include "qemu_blockjob.h"
#include "viralloc.h"
@@ -1304,6 +1305,11 @@ qemuDomainNetworkPrivateNew(void)
static void
qemuDomainNetworkPrivateDispose(void *obj ATTRIBUTE_UNUSED)
{
+qemuDomainNetworkPrivatePtr priv = obj;
+
+if (priv->slirp) {
+qemuSlirpFree(priv->slirp);
Needless NULL check.
+}
}
@@ -2630,6 +2636,63 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
}
+static bool
+qemuDomainHasSlirp(virDomainObjPtr vm)
+{
+size_t i;
+
+for (i = 0; i < vm->def->nnets; i++) {
+virDomainNetDefPtr net = vm->def->nets[i];
+
+if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
+return true;
+}
+
+return false;
+}
+
+
+static int
+qemuDomainObjPrivateXMLFormatSlirp(virBufferPtr buf,
+ virDomainObjPtr vm)
+{
+size_t i;
+
+if (!qemuDomainHasSlirp(vm))
+return 0;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+
+for (i = 0; i < vm->def->nnets; i++) {
+virDomainNetDefPtr net = vm->def->nets[i];
+qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp;
+size_t j;
+
+if (!slirp)
+continue;
+
+virBufferAsprintf(buf, "\n",
+ net->info.alias, slirp->pid);
+
+virBufferAdjustIndent(buf, 2);
+for (j = 0; j < QEMU_SLIRP_FEATURE_LAST; j++) {
+if (qemuSlirpHasFeature(slirp, j)) {
+virBufferAsprintf(buf, "\n",
+ qemuSlirpFeatureTypeToString(j));
+}
+}
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+
+
+return 0;
+}
+
static int
qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
virDomainObjPtr vm)
@@ -2731,6 +2794,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0)
return -1;
+if (qemuDomainObjPrivateXMLFormatSlirp(buf, vm) < 0)
+return -1;
+
return 0;
}
@@ -3258,6 +3324,46 @@ qemuDomainObjPrivateXMLParseJob(virDomainObjPtr vm,
}
+static int
+qemuDomainObjPrivateXMLParseSlirpFeatures(xmlXPathContextPtr ctxt,
+ size_t h,
+ qemuSlirpPtr slirp)
We tend to pass xmlNodePtr here among with ctxt and then use
VIR_XPATH_NODE_AUTORESTORE(ctxt).
+{
+VIR_AUTOFREE(char *) path = NULL;
+VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
+size_t i;
+int n;
+
+if (virAsprintf(&path, "./slirp/helper[%ld]/feature", h + 1) < 0)
+return -1;
+
+if ((n = virXPathNodeSet(path, ctxt, &nodes)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("failed to parse slirp-helper features"));
+return -1;
+}
+
+for (i = 0; i < n; i++) {
+VIR_AUTOFREE(char *) str = virXMLPropString(nodes[i], "name");
+int feature;
+
+if (!str)
+continue;
+
+feature = qemuSlirpFeatureTypeFromString(str);
+if (feature < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown slirp feature %s"), str);
+return -1;
+}
+
+qemuSlirpSetFeature(slirp, feature);
+}
+
+return 0;
+}
+
+
static int
qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
virDomainObjPtr vm,
@@ -3396,6 +3502,37 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
}
VIR_FREE(nodes);
+if ((n = virXPathNodeSet("./slirp/helper", ctxt, &nodes)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to parse slirp helper list"));
+goto error;
+}
+if (n > 0) {
I know you put this check here to be consistent with the rest of the
code, but that code needs to alloc something in case of n > 0 and only
after that it execs for ().
+for (i = 0; i < n; i++) {
+VIR_AUTOFREE(char *) alias = virXMLPropString(nodes[i], "alias");
+VIR_AUTOFREE(char *) pid = virXMLPropString(nodes[i], "pid");
+VIR_AUTOP
Re: [libvirt] [PATCH v2 23/23] tests: add slirp-helper qemuxml2argv test
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Signed-off-by: Marc-André Lureau --- .../net-user.x86_64-4.0.0.args| 34 +++ tests/qemuxml2argvtest.c | 16 + tests/testutilsqemu.h | 1 + 3 files changed, 51 insertions(+) create mode 100644 tests/qemuxml2argvdata/net-user.x86_64-4.0.0.args I needed to regenerate the .args file so that it is wrapped correctly, but that's a minor issue. Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 21/23] qemu-process: prepare slirp-helper
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau When the network interface is of "user" type, and QEMU has the "-net socket,fd=" datagram support, call qemuInterfacePrepareSlirp() to probe and associate a slirp-helper with the interface. The usage of automated slirp-helper can be prevented with disableSlirp (in particular when resuming a VM that didn't start with slirp-helper before). Signed-off-by: Marc-André Lureau --- src/qemu/qemu_interface.c | 27 +++ src/qemu/qemu_interface.h | 4 src/qemu/qemu_process.c | 16 +--- 3 files changed, 44 insertions(+), 3 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 22/23] qemu-hotplug: handle hotplugging of slirp-helper
On 8/8/19 4:55 PM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
Signed-off-by: Marc-André Lureau
---
src/qemu/qemu_hotplug.c | 33 ++---
src/qemu/qemu_monitor.c | 13 ++---
src/qemu/qemu_monitor.h | 3 ++-
3 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 43c3f0755b..fcbf7a8aa9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1136,6 +1136,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_NET, { .net = net } };
virErrorPtr originalError = NULL;
VIR_AUTOFREE(char *) slirpfdName = NULL;
+int slirpfd = -1;
char **tapfdName = NULL;
int *tapfd = NULL;
size_t tapfdSize = 0;
@@ -1315,7 +1316,26 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
break;
case VIR_DOMAIN_NET_TYPE_USER:
-/* No preparation needed. */
+if (!priv->disableSlirp &&
+virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NET_SOCKET_DGRAM)) {
+qemuSlirpPtr slirp = qemuInterfacePrepareSlirp(driver, net);
+
+if (!slirp)
+break;
+
+QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
+
+if (qemuSlirpOpen(slirp, driver, vm->def) < 0 ||
+qemuSlirpStart(slirp, vm, driver, net, true, NULL) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("Failed to start slirp"));
+goto cleanup;
+}
+
+slirpfd = qemuSlirpGetFD(slirp);
+if (virAsprintf(&slirpfdName, "slirpfd-%s", net->info.alias) < 0)
+goto cleanup;
+}
break;
case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -1391,7 +1411,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
if (qemuMonitorAddNetdev(priv->mon, netstr,
tapfd, tapfdName, tapfdSize,
- vhostfd, vhostfdName, vhostfdSize) < 0) {
+ vhostfd, vhostfdName, vhostfdSize,
+ slirpfd, slirpfdName) < 0) {
ignore_value(qemuDomainObjExitMonitor(driver, vm));
virDomainAuditNet(vm, NULL, net, "attach", false);
goto try_remove;
@@ -1506,6 +1527,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
VIR_FREE(charDevAlias);
virObjectUnref(conn);
virDomainCCWAddressSetFree(ccwaddrs);
+VIR_FORCE_CLOSE(slirpfd);
return ret;
@@ -1516,6 +1538,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
virErrorPreserveLast(&originalError);
if (virAsprintf(&netdev_name, "host%s", net->info.alias) >= 0) {
qemuDomainObjEnterMonitor(driver, vm);
+if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
+qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver,
net, true);
This can be done outside of EnterMonitor(). qemuSlirpStop() will enter
the monitor on its own.
if (charDevPlugged &&
qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
VIR_WARN("Failed to remove associated chardev %s", charDevAlias);
@@ -2201,7 +2225,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
if (guestfwd) {
if (qemuMonitorAddNetdev(priv->mon, devstr,
- NULL, NULL, 0, NULL, NULL, 0) < 0)
+ NULL, NULL, 0, NULL, NULL, 0, -1, NULL) < 0)
goto exit_monitor;
} else {
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
@@ -4674,6 +4698,9 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto cleanup;
+if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
+qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver,
net, true);
+
virDomainAuditNet(vm, net, NULL, "detach", true);
for (i = 0; i < vm->def->nnets; i++) {
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a880da3ab6..84af24958a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2844,15 +2844,17 @@ int
qemuMonitorAddNetdev(qemuMonitorPtr mon,
const char *netdevstr,
int *tapfd, char **tapfdName, int tapfdSize,
- int *vhostfd, char **vhostfdName, int vhostfdSize)
+ int *vhostfd, char **vhostfdName, int vhostfdSize,
+ int slirpfd, char *slirpfdName)
{
int ret = -1;
size_t i = 0, j = 0;
VIR_DEBUG("netdevstr=%s tapfd=%p tapfdName=%p tapfdSize=%d"
- "vhostfd=%p vhostfdName=%p vhostfdSize=%d",
+ "vhostfd=%p vhostfdName=%p vhostfdSize=%d"
+ "slirpfd=%d slirpfdName=%s",
netdevstr, tapfd, tapfdName, tapfdSize,
- vhostfd, vhostfdName, vhostfdSize);
Re: [libvirt] [PATCH 1/2] test_driver: implement virDomainPMSuspendForDuration
On Tue, Sep 3, 2019 at 8:17 PM Daniel Henrique Barboza
wrote:
>
>
>
> On 9/3/19 2:00 PM, Daniel Henrique Barboza wrote:
> >
> >
> > On 8/13/19 1:19 PM, Ilias Stamatis wrote:
> >> Signed-off-by: Ilias Stamatis
> >> ---
> >> src/test/test_driver.c | 79 ++
> >> 1 file changed, 79 insertions(+)
> >>
> >> [...]
> >> +
> >> +virDomainObjSetState(vm, VIR_DOMAIN_PMSUSPENDED,
> >> + VIR_DOMAIN_PMSUSPENDED_UNKNOWN);
> >> +event_suspend = virDomainEventLifecycleNewFromObj(vm,
> >> + VIR_DOMAIN_EVENT_PMSUSPENDED,
> >> + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED);
> >> +virObjectEventStateQueue(privconn->eventState, event_suspend);
> >> +
> >> +if (target == VIR_NODE_SUSPEND_TARGET_DISK) {
> >> +testDomainShutdownState(dom, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> >> +event_shutdown = virDomainEventLifecycleNewFromObj(vm,
> >> + VIR_DOMAIN_EVENT_STOPPED,
> >> + VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
> >> +if (!vm->persistent)
> >> +virDomainObjListRemove(privconn->domains, vm);
> >> +}
> >> +
> >> +ret = 0;
> >> + cleanup:
> >> +virDomainObjEndAPI(&vm);
> >> +virObjectEventStateQueue(privconn->eventState, event_shutdown);
> >
> >
> > Unless you're OK with passing a NULL here, you need to check if
> > 'event_shutdown' is NULL at this point, since there's no guarantee that
> > you set the var with something else in the 'if
> > target==VIR_NODE_SUSPEND_TARGET_DISK'
> > conditional.
> >
> > I'll take a guess here and say that this is unintended, thus it's best
> > to move this
> > 'virObjectEventStateQueue()' call that uses 'event_shutdown' inside
> > the "if"
> > right before the cleanup label.
>
> Just saw that patch 2/2 did something similar and decided to get more
> informed.
>
> Checking what virObjectEventStateQueue() does, I see that it's a wrapper to
> virObjectEventStateQueueRemote(), which is a no-op if the second
> argument - in this case, event_shutdown - is NULL. This means that the
> usage above does not harm, which is good.
>
> I'd still prefer to avoid using virObjectEventStateQueue() like you're doing
> here, especially when it's easier to simply move the function call inside
> the "if" in which you defined a non-NULL value to event_shutdown.
>
> However, I see this same use of virObjectEventStateQueue() across the
> board in test_driver.c. That said, I'll assume that this question was
> already
> dealt with in the first time this code pattern was introduced in the
> code, and
> this use is ok.
I think not only in test_driver.c. I took a quick look and it's also
like that in some places in the QEMU driver. Plus I would say this
"pattern" is in general common in the codebase with all these free*()
functions on the cleanup sections which do the same thing - always
check for NULL first.
Additionally, if you want to check for NULL in that case you need to
do it explicitly because virDomainEventLifecycleNewFromObj might also
return NULL. This results in adding extra lines of code. Merely moving
the virObjectEventStateQueue above wouldn't change much. So, since
virObjectEventStateQueue already protects us against NULL I think
maybe it's fine to leave it like that?
In general I'm neutral to this, so feel free to alter it before merging.
Thanks for the review!
Ilias
>
> Rest of the code looks fine, so:
>
>
> Reviewed-by: Daniel Henrique Barboza
>
>
>
>
>
>
> >
> >
> >
> > Thanks,
> >
> >
> > DHB
> >
> >> +return ret;
> >> +}
> >> +
> >> +
> >> #define TEST_TOTAL_CPUTIME 48772617035LL
> >>
> >> static int
> >> @@ -9415,6 +9493,7 @@ static virHypervisorDriver testHypervisorDriver
> >> = {
> >> .domainSendKey = testDomainSendKey, /* 5.5.0 */
> >> .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
> >> .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
> >> +.domainPMSuspendForDuration = testDomainPMSuspendForDuration, /*
> >> 5.7.0 */
> >> .domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */
> >> .domainSendProcessSignal = testDomainSendProcessSignal, /*
> >> 5.5.0 */
> >> .connectGetCPUModelNames = testConnectGetCPUModelNames, /*
> >> 1.1.3 */
> >> --
> >> 2.22.0
> >>
> >> --
> >> libvir-list mailing list
> >> libvir-list@redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> >
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote: > Implicitly the query depth is limited by the length of the QAPI schema > query, but 'alternate' and 'array' QAPI meta-types don't consume a part > of the query string thus a loop on such types would get our traversal > code stuck in an infinite loop. Prevent this from happening by limiting > the nesting depth to 1000. > > Signed-off-by: Peter Krempa > --- > src/qemu/qemu_qapi.c | 17 + > 1 file changed, 17 insertions(+) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
On Fri, Sep 06, 2019 at 09:50:41 +0100, Daniel Berrange wrote:
> On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote:
> > Implicitly the query depth is limited by the length of the QAPI schema
> > query, but 'alternate' and 'array' QAPI meta-types don't consume a part
> > of the query string thus a loop on such types would get our traversal
> > code stuck in an infinite loop. Prevent this from happening by limiting
> > the nesting depth to 1000.
>
> I'm not too clear on what 'depth' is applying to here ? Is this
> the level of nesting in the JSON compound types we're following,
> or is it something else ?
>
> I ask because YAJL limits JSON nesting to only 128. So 1000 is
> almost an order of magnitude larger.
This is not about JSON/YAJL limits. The QAPI schema is flattened and
cross-referenced by type names ('name' field in the schema). Our
code for 'alternate' and 'array' looks up the corresponding type and
moves to it when processing the query string without processing any part
of input.
This means that if you create a mallicious or broken QAPI
schema where the array member type (element-type) would be exactly the
same as the type name of the array [1] our query string evaluator
would be stuck in an infinite loop. The number is crazy big, because it
only needs to prevent from loops of 'alternate' and 'arrays'. All other
cases consume an element from the query string and thus the bounds are
limited by what you are attempting to query.
[1]
query-qmp-schema:
Normal case:
{
"name": "[14]",
"element-type": "14",
"meta-type": "array"
},
broken looping case:
{
"name": "[14]",
"element-type": "[14]",
"meta-type": "array"
},
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/9] qemu: explicitly delete standard tap devices only on platforms that require it
On Tue, Aug 27, 2019 at 09:46:38PM -0400, Laine Stump wrote:
> libvirt creates its tap devices without the IFF_PERSIST flag, so they
> will be automatically deleted when qemu is finished with them. In the
> case of tap devices created outside of libvirt, if the creating entity
> wants the devices to be deleted, it will also omit IFF_PERSIST, but if
> it wants them to remain (e.g. for re-use), then it will use
> IFF_PERSIST when creating the device.
>
> Back when support was added for autocreation by libvirt of tap devices
> for (commit 9c17d665), code was mistakenly
> put in qemuProcessStop to always delete tap devices for
> type='ethernet'. This should only be done on platforms that have
> VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP #defined (which is only
> FreeBSD).
This isn't right. The tap devices should *always* be deleted as we
don't trust that QEMU hasn't (possibly maliciously) set IFF_PERSIST
itself.
>
> This mistake has been corrected, along with the unnecessary check for
> non-null net->ifname (it must always be non-null), and erroneous
> VIR_FREE of net->ifname.
There could be a risk of net->ifname being NULL if qemuProcessStart
fails early in startup before all tap devices have finished being
created IIUC.
>
> Signed-off-by: Laine Stump
> ---
> src/qemu/qemu_process.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 11c1ba8fb9..3449abf2ec 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7548,10 +7548,10 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> cfg->stateDir));
> break;
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> -if (net->managed_tap != VIR_TRISTATE_BOOL_NO && net->ifname) {
> +#ifdef VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP
> +if (net->managed_tap != VIR_TRISTATE_BOOL_NO)
> ignore_value(virNetDevTapDelete(net->ifname,
> net->backend.tap));
> -VIR_FREE(net->ifname);
> -}
> +#endif
> break;
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/9] qemu: support unmanaged macvtap devices with
On Tue, Aug 27, 2019 at 09:46:37PM -0400, Laine Stump wrote: > Traditionally, macvtap devices are supported using type='direct'>, but that type requires specifying a source device name > and macvtap mode which can't be altered after the initial device > creation (and may not even be available to the management software > that's creating the XML config to feed to libvirt). > > But the attributes in the are essentially describing how the > device will be connected to the network, and if libvirt is to be > supplied with the name of a macvtap device that has already been > created, that device will also already be connected to the network > (and the connection can't be changed). Thus it seems more appropriate > to use type='ethernet', which was created explicitly for this purpose > - for devices that have already been (or will be) connected to the > external network by someone/something outside of libvirt. The fact > that it is a *macv*tap rather than a contentional tap device is just a > detail. > > This patch supports using an existing macvtap device with type='ethernet'> by checking the supplied target dev name to see if it > is a macvtap device and, when this is the case, calling > virNetDevMacVLanTapOpen() instead of virNetDevTapCreate(). For > consistency, this is only done when target managed='no'. > > Resolves: https://bugzilla.redhat.com/1723367 (partially) > Signed-off-by: Laine Stump > --- > src/qemu/qemu_interface.c | 15 --- > 1 file changed, 12 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/9] qemu: support unmanaged target tap dev for
On Tue, Aug 27, 2019 at 09:46:36PM -0400, Laine Stump wrote: > If managed='no', then the tap device must already exist, and setting > of MAC address and online status (IFF_UP) is skipped. > > NB: we still set IFF_VNET_HDR and IFF_MULTI_QUEUE as appropriate, > because those bits must be properly set in the TUNSETIFF we use to set > the tap device name of the handle we've opened - if IFF_VNET_HDR has > not been set and we set it the request will be honored even when > running libvirtd unprivileged; if IFF_MULTI_QUEUE is requested to be > different than how it was created, that will result in an error from > the kernel. This means that you don't need to pay attention to > IFF_VNET_HDR when creating the tap devices, but you *do* need to set > IFF_MULTI_QUEUE if you're going to use multiple queues for your tap > device. > > NB2: /dev/vhost-net normally has permissions 600, so it can't be > opened by an unprivileged process. This would normally cause a warning > message when using a virtio net device from an unprivileged > libvirtd. I've found that setting the permissions for /dev/vhost-net > permits unprivileged libvirtd to use vhost-net for virtio devices, but > have no idea what sort of security implications that has. I haven't > changed libvrit's code to avoid *attempting* to open /dev/vhost-net - > if you are concerned about the security of opening up permissions of > /dev/vhost-net (probably a good idea at least until we ask someone who > knows about the code) then add to the interface > definition and you'll avoid the warning message. > > Note that virNetDevTapCreate() is the correct function to call in the > case of an existing device, because the same ioctl() that creates a > new tap device will also open an existing tap device. > > Resolves: https://bugzilla.redhat.com/1723367 (partially) > Signed-off-by: Laine Stump > --- > src/qemu/qemu_interface.c | 77 +++ > src/qemu/qemu_process.c | 2 +- > src/util/virnetdev.h | 2 +- > .../net-eth-unmanaged-tap.args| 32 > tests/qemuxml2argvmock.c | 16 +++- > tests/qemuxml2argvtest.c | 1 + > 6 files changed, 96 insertions(+), 34 deletions(-) > create mode 100644 tests/qemuxml2argvdata/net-eth-unmanaged-tap.args Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/9] conf: new "managed" attribute for target dev of
On Tue, Aug 27, 2019 at 09:46:35PM -0400, Laine Stump wrote: > Although has always been able to use an > existing tap device, this is just a coincidence due to the fact that > the same ioctl is used to create a new tap device or get a handle to > an existing device. > > Even then, once we have the handle to the device, we still insist on > doing extra setup to it (setting the MAC address and IFF_UP). That > *might* be okay if libvirtd is running as a privileged process, but if > libvirtd is running as an unprivileged user, those attempted > modifications to the tap device will fail (yes, even if the tap is set > to be owned by the user running libvirtd). We could avoid this if we > knew that the device already existed, but as stated above, an existing > device and new device are both accessed in the same manner, and > anyway, we need to preserve existing behavior for those who are > already using pre-existing devices with privileged libvirtd (and > allowing/expecting libvirt to configure the pre-existing device). > > In order to cleanly support the idea of using a pre-existing and > pre-configured tap device, this patch introduces a new optional > attribute "managed" for the interface element. This > attribute is only valid for (since all > other interface types have mandatory config that doesn't apply in the > case where we expect the tap device to be setup before we > get it). The syntax would look something like this: > > > > ... > > > This patch just adds managed to the grammar and parser for , > but has no functionality behind it. > > (NB: when managed='no' (the default when not specified is 'yes'), the > target dev is always a name explicitly provided, so we don't > auto-remove it from the config just because it starts with "vnet" > (VIR_NET_GENERATED_TAP_PREFIX); this makes it possible to use the > same pattern of names that libvirt itself uses when it automatically > creates the tap devices.) > > Signed-off-by: Laine Stump > --- > docs/formatdomain.html.in | 48 + > docs/schemas/domaincommon.rng | 5 ++ > src/conf/domain_conf.c| 51 +++ > src/conf/domain_conf.h| 1 + > .../net-eth-unmanaged-tap.xml | 35 + > .../net-eth-unmanaged-tap.xml | 40 +++ > tests/qemuxml2xmltest.c | 1 + > 7 files changed, 160 insertions(+), 21 deletions(-) > create mode 100644 tests/qemuxml2argvdata/net-eth-unmanaged-tap.xml > create mode 100644 tests/qemuxml2xmloutdata/net-eth-unmanaged-tap.xml Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/9] conf: use virXMLFormatElement for interface
On Tue, Aug 27, 2019 at 09:46:34PM -0400, Laine Stump wrote: > This will simplify addition of another attribute to the element > > Signed-off-by: Laine Stump > --- > src/conf/domain_conf.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/9] qemu: reorganize qemuInterfaceEthernetConnect()
On Tue, Aug 27, 2019 at 09:46:33PM -0400, Laine Stump wrote:
> This just moves around a few things in qemuInterfaceConnect() with no
> functional difference (except that a few failures that would have
> previously resulted in a "success" audit log will now properly produce
> a "fail" audit). The change is so that adding support for unmanaged
> tap/macvtap devices will be more easily reviewable.
>
> Signed-off-by: Laine Stump
> ---
> src/qemu/qemu_interface.c | 69 ---
> 1 file changed, 35 insertions(+), 34 deletions(-)
>
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index 72ed51cb1f..1e3b7f0d06 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -414,6 +414,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
> bool template_ifname = false;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> const char *tunpath = "/dev/net/tun";
> +const char *auditdev = tunpath;
>
> if (net->backend.tap) {
> tunpath = net->backend.tap;
> @@ -424,43 +425,39 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
> }
> }
>
> -if (!net->ifname ||
> -STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
> -strchr(net->ifname, '%')) {
> -VIR_FREE(net->ifname);
> -if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_TAP_PREFIX "%d") < 0)
> -goto cleanup;
> -/* avoid exposing vnet%d in getXMLDesc or error outputs */
> -template_ifname = true;
> -}
> -
> if (virDomainNetIsVirtioModel(net))
> tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>
> -if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
> - tap_create_flags) < 0) {
> -virDomainAuditNetDevice(def, net, tunpath, false);
> -goto cleanup;
> -}
> -
> -virDomainAuditNetDevice(def, net, tunpath, true);
> -
> -/* The tap device's MAC address cannot match the MAC address
> - * used by the guest. This results in "received packet on
> - * vnetX with own address as source address" error logs from
> - * the kernel.
> - */
> -virMacAddrSet(&tapmac, &net->mac);
> -if (tapmac.addr[0] == 0xFE)
> -tapmac.addr[0] = 0xFA;
> -else
> -tapmac.addr[0] = 0xFE;
> -
> -if (virNetDevSetMAC(net->ifname, &tapmac) < 0)
> -goto cleanup;
> -
> -if (virNetDevSetOnline(net->ifname, true) < 0)
> -goto cleanup;
> + if (!net->ifname ||
> + STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||
> + strchr(net->ifname, '%')) {
> + VIR_FREE(net->ifname);
> + if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_TAP_PREFIX "%d") < 0)
> + goto cleanup;
> + /* avoid exposing vnet%d in getXMLDesc or error outputs */
> + template_ifname = true;
> + }
> + if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
> + tap_create_flags) < 0) {
> + goto cleanup;
> + }
> +
> + /* The tap device's MAC address cannot match the MAC address
> +* used by the guest. This results in "received packet on
> +* vnetX with own address as source address" error logs from
> +* the kernel.
> +*/
> + virMacAddrSet(&tapmac, &net->mac);
> + if (tapmac.addr[0] == 0xFE)
> + tapmac.addr[0] = 0xFA;
> + else
> + tapmac.addr[0] = 0xFE;
> +
> + if (virNetDevSetMAC(net->ifname, &tapmac) < 0)
> + goto cleanup;
> +
> + if (virNetDevSetOnline(net->ifname, true) < 0)
> + goto cleanup;
>
Indentation here is all off-by-1 which makes the diffstat bigger
> if (net->script &&
> virNetDevRunEthernetScript(net->ifname, net->script) < 0)
> @@ -477,11 +474,15 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
> goto cleanup;
> }
>
> +virDomainAuditNetDevice(def, net, auditdev, true);
> +
> ret = 0;
>
> cleanup:
> if (ret < 0) {
> size_t i;
> +
> +virDomainAuditNetDevice(def, net, auditdev, false);
> for (i = 0; i < tapfdSize && tapfd[i] >= 0; i++)
> VIR_FORCE_CLOSE(tapfd[i]);
> if (template_ifname)
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/9] util: make a couple virNetDevMacVlan*() functions public
On Tue, Aug 27, 2019 at 09:46:32PM -0400, Laine Stump wrote: > In virNetDevMacVLanOpen(), The "retries" arg has been removed and the > value hardcoded as 10, since previously the function was only called > from one place, so it was always 10. > > Signed-off-by: Laine Stump > --- > src/util/virnetdevmacvlan.c | 12 +--- > src/util/virnetdevmacvlan.h | 9 + > 2 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/9] util: new function virNetDevMacVLanIsMacvtap()
On Tue, Aug 27, 2019 at 09:46:31PM -0400, Laine Stump wrote: > This function returns T if the given name is a macvtap device. This is > determined by 1) getting the ifindex of the device with that name (if > there is one), and 2) checking for existence of /dev/tapXX, where "XX" > is the ifindex learned in (1). > > It's also possible to learn this by getting a netlink dump of the > interface and parsing through it to look for some attributes, but that > is complicated to figure out, takes longer to execute, and I'm lazy. > > Signed-off-by: Laine Stump > --- > src/libvirt_private.syms| 3 +++ > src/util/virnetdevmacvlan.c | 23 +++ > src/util/virnetdevmacvlan.h | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index a34d92f5ef..afea00b629 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2523,10 +2523,13 @@ virNetDevMacVLanCreate; > virNetDevMacVLanCreateWithVPortProfile; > virNetDevMacVLanDelete; > virNetDevMacVLanDeleteWithVPortProfile; > +virNetDevMacVLanIsMacvtap; > virNetDevMacVLanModeTypeFromString; > virNetDevMacVLanReleaseName; > virNetDevMacVLanReserveName; > virNetDevMacVLanRestartWithVPortProfile; > +virNetDevMacVLanTapOpen; > +virNetDevMacVLanTapSetup; Accidentally included > virNetDevMacVLanVPortProfileRegisterCallback; With that removed Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
On Fri, Sep 06, 2019 at 10:33:15AM +0200, Peter Krempa wrote:
> Implicitly the query depth is limited by the length of the QAPI schema
> query, but 'alternate' and 'array' QAPI meta-types don't consume a part
> of the query string thus a loop on such types would get our traversal
> code stuck in an infinite loop. Prevent this from happening by limiting
> the nesting depth to 1000.
I'm not too clear on what 'depth' is applying to here ? Is this
the level of nesting in the JSON compound types we're following,
or is it something else ?
I ask because YAJL limits JSON nesting to only 128. So 1000 is
almost an order of magnitude larger.
>
> Signed-off-by: Peter Krempa
> ---
> src/qemu/qemu_qapi.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
> index 0226d6c659..93fcae0d44 100644
> --- a/src/qemu/qemu_qapi.c
> +++ b/src/qemu/qemu_qapi.c
> @@ -74,9 +74,23 @@ struct virQEMUQAPISchemaTraverseContext {
> virHashTablePtr schema;
> char **queries;
> virJSONValuePtr returnType;
> +size_t depth;
> };
>
>
> +static int
> +virQEMUQAPISchemaTraverseContextValidateDepth(struct
> virQEMUQAPISchemaTraverseContext *ctxt)
> +{
> +if (ctxt->depth++ > 1000) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("possible loop in QMP schema"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
> static void
> virQEMUQAPISchemaTraverseContextInit(struct virQEMUQAPISchemaTraverseContext
> *ctxt,
> char **queries,
> @@ -329,6 +343,9 @@ virQEMUQAPISchemaTraverse(const char *baseName,
> const char *metatype;
> size_t i;
>
> +if (virQEMUQAPISchemaTraverseContextValidateDepth(ctxt) < 0)
> +return -2;
> +
> if (!(cur = virHashLookup(ctxt->schema, baseName)))
> return -2;
>
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: maintain user alias for video type 'none'
On Fri, Sep 06, 2019 at 09:58:32AM +0200, Erik Skultety wrote: > On Thu, Sep 05, 2019 at 11:17:38AM -0500, Jonathon Jongsma wrote: > > After parsing a video device with a model type of > > VIR_DOMAIN_VIDEO_TYPE_NONE, all device info is cleared (see > > virDomainDefPostParseVideo()) in order to avoid formatting any > > auto-generated values for the XML. Subsequently, however, an alias is > > generated for the video device (e.g. 'video0'), which results in an > > alias property being formatted in the XML output anyway. This creates > > confusion if the user has explicitly provided an alias for the video > > device since the alias will change. > > > > To avoid this, don't clear the user-defined alias for video devices of > > type "none". > > > > See https://bugzilla.redhat.com/show_bug.cgi?id=1720612 > > s/See// - we had a discussion about using/not using "Resolves: " across > the commit messages; right now, the custom is to mention just the plain URL. 'Resolves:' is just redundant noise - people only do it out of habit because Red Hat's own internal dev tools for RHEL require that for beurocratic reasons. In any case, the most important rule is that the commit message should fully describe the problem. ie it should be possible to understand the commit without opening the BZ link at all. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: qapi: Limit traversal depth for QAPI schema queries
Implicitly the query depth is limited by the length of the QAPI schema
query, but 'alternate' and 'array' QAPI meta-types don't consume a part
of the query string thus a loop on such types would get our traversal
code stuck in an infinite loop. Prevent this from happening by limiting
the nesting depth to 1000.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_qapi.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/src/qemu/qemu_qapi.c b/src/qemu/qemu_qapi.c
index 0226d6c659..93fcae0d44 100644
--- a/src/qemu/qemu_qapi.c
+++ b/src/qemu/qemu_qapi.c
@@ -74,9 +74,23 @@ struct virQEMUQAPISchemaTraverseContext {
virHashTablePtr schema;
char **queries;
virJSONValuePtr returnType;
+size_t depth;
};
+static int
+virQEMUQAPISchemaTraverseContextValidateDepth(struct
virQEMUQAPISchemaTraverseContext *ctxt)
+{
+if (ctxt->depth++ > 1000) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("possible loop in QMP schema"));
+return -1;
+}
+
+return 0;
+}
+
+
static void
virQEMUQAPISchemaTraverseContextInit(struct virQEMUQAPISchemaTraverseContext
*ctxt,
char **queries,
@@ -329,6 +343,9 @@ virQEMUQAPISchemaTraverse(const char *baseName,
const char *metatype;
size_t i;
+if (virQEMUQAPISchemaTraverseContextValidateDepth(ctxt) < 0)
+return -2;
+
if (!(cur = virHashLookup(ctxt->schema, baseName)))
return -2;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: maintain user alias for video type 'none'
On Thu, Sep 05, 2019 at 11:17:38AM -0500, Jonathon Jongsma wrote:
> After parsing a video device with a model type of
> VIR_DOMAIN_VIDEO_TYPE_NONE, all device info is cleared (see
> virDomainDefPostParseVideo()) in order to avoid formatting any
> auto-generated values for the XML. Subsequently, however, an alias is
> generated for the video device (e.g. 'video0'), which results in an
> alias property being formatted in the XML output anyway. This creates
> confusion if the user has explicitly provided an alias for the video
> device since the alias will change.
>
> To avoid this, don't clear the user-defined alias for video devices of
> type "none".
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=1720612
s/See// - we had a discussion about using/not using "Resolves: " across
the commit messages; right now, the custom is to mention just the plain URL.
Btw. I had the very same question you posted to the BZ so thanks for asking.
It is a bit unfortunate, that we add a default when there's a
element present, because it makes things a bit complicated resulting
in introduction of a "none" device type, but it is what it is and I don't think
ovirt has really an alternative :(.
>
> Signed-off-by: Jonathon Jongsma
> ---
> src/conf/domain_conf.c | 7 +--
> tests/qemuxml2argvdata/video-none-device.xml | 1 +
> tests/qemuxml2xmloutdata/video-none-device.xml | 1 +
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6c429cd593..d11d1fb903 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5635,11 +5635,14 @@ virDomainDefPostParseVideo(virDomainDefPtr def,
> return 0;
>
> if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE) {
> +char *alias;
I'd put a blank line here to separate the declaration from everything else.
Reviewed-by: Erik Skultety
> /* we don't want to format any values we automatically fill in for
> - * videos into the XML, so clear them
> - */
> + * videos into the XML, so clear them, but retain any user-assigned
> + * alias */
> +VIR_STEAL_PTR(alias, def->videos[0]->info.alias);
> virDomainVideoDefClear(def->videos[0]);
> def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_NONE;
> +VIR_STEAL_PTR(def->videos[0]->info.alias, alias);
> } else {
> virDomainDeviceDef device = {
> .type = VIR_DOMAIN_DEVICE_VIDEO,
> diff --git a/tests/qemuxml2argvdata/video-none-device.xml
> b/tests/qemuxml2argvdata/video-none-device.xml
> index 4b591562b7..c1c60c1d9e 100644
> --- a/tests/qemuxml2argvdata/video-none-device.xml
> +++ b/tests/qemuxml2argvdata/video-none-device.xml
> @@ -31,6 +31,7 @@
>
>
>
> +
>
>
> function='0x0'/>
> diff --git a/tests/qemuxml2xmloutdata/video-none-device.xml
> b/tests/qemuxml2xmloutdata/video-none-device.xml
> index 6e76b394fe..82615f5125 100644
> --- a/tests/qemuxml2xmloutdata/video-none-device.xml
> +++ b/tests/qemuxml2xmloutdata/video-none-device.xml
> @@ -34,6 +34,7 @@
>
>
>
> +
>
>
> function='0x0'/>
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vircgroupv2: fix setting cpu.max period
On Thu, Sep 05, 2019 at 11:44:47AM +0200, Pavel Hrdina wrote:
> When we set cpu.max period we need to parse the cpu.max file first as
> it contains both quota and period values separated by space. When only
> a single number is written to that file it will set quota, in order to
"... quota. However, in order to..."
> change period we need to write both values.
>
> The code was prepared for that but mistakenly used new line to end the
> string with the first value.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1749227
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/vircgroupv2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
> index 2aca4e5d62..0663c67190 100644
> --- a/src/util/vircgroupv2.c
> +++ b/src/util/vircgroupv2.c
> @@ -1508,7 +1508,7 @@ virCgroupV2SetCpuCfsPeriod(virCgroupPtr group,
> _("Invalid 'cpu.max' data."));
> return -1;
> }
> -*tmp = '\n';
> +*tmp = '\0';
>
> if (virAsprintf(&value, "%s %llu", str, cfs_period) < 0)
> return -1;
Reviewed-by: Erik Skultety
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
