- Devel - libvirt List Archives

[libvirt] [PATCH v2] util: Block SIGPIPE until execve in child process
by Wang Yechao 15 Oct '19

15 Oct '19

Libvirtd has set SIGPIPE to ignored, and virFork resets all signal handlers to the defaults. But child process may write logs to stderr/stdout, that may generate SIGPIPE if journald has stopped. So block SIGPIPE in virFork, and unblock it before execve. Signed-off-by: Wang Yechao <wang.yechao255(a)zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2019-October/msg00720.html Changes in v2: - use pthread_sigmask to block SIGPIPE --- src/util/vircommand.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 79e1e87..bd3a582 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -321,6 +321,15 @@ virFork(void) virDispatchError(NULL); _exit(EXIT_CANCELED); } + + sigemptyset(&newmask); + sigaddset(&newmask, SIGPIPE); + if (pthread_sigmask(SIG_BLOCK, &newmask, NULL) != 0) { + virReportSystemError(errno, "%s", _("cannot block SIGPIPE")); + virDispatchError(NULL); + _exit(EXIT_CANCELED); + } + } return pid; } @@ -553,6 +562,7 @@ virExec(virCommandPtr cmd) struct sigaction waxon, waxoff; VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; + sigset_t set; if (cmd->args[0][0] != '/') { if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) { @@ -792,6 +802,13 @@ virExec(virCommandPtr cmd) /* Close logging again to ensure no FDs leak to child */ virLogReset(); + sigemptyset(&set); + sigaddset(&set, SIGPIPE); + if (pthread_sigmask(SIG_SETMASK, &set, NULL) != 0) { + virReportSystemError(errno, "%s", _("cannot unblock SIGPIPE")); + goto fork_error; + } + if (cmd->env) execve(binary, cmd->args, cmd->env); else -- 1.8.3.1

1 0

[libvirt] [PATCH] util: ignore SIGPIPE when writing to stderr/stdout
by Wang Yechao 15 Oct '19

15 Oct '19

libvirtd's stderr/stdout redirected to journald by default, so if the journald has stopped, libvirtd and libvirtd's child process will receive SIGPIPE signal when writing logs to stderr/stdout. journald stopped reasons: 1. manual command "systemctl stop systemd-journald.service" 2. oom killer kill it. ... Signed-off-by: Wang Yechao <wang.yechao255(a)zte.com.cn> --- src/util/virlog.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index 4c76fbc..127e121 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -39,6 +39,7 @@ # include <sys/un.h> #endif #include <fnmatch.h> +#include <signal.h> #include "virerror.h" #include "virlog.h" @@ -732,6 +733,9 @@ virLogOutputToFd(virLogSourcePtr source ATTRIBUTE_UNUSED, if (fd < 0) return; + if (fd == STDERR_FILENO || fd == STDOUT_FILENO) + signal(SIGPIPE, SIG_IGN); + if (virAsprintfQuiet(&msg, "%s: %s", timestamp, str) < 0) return; @@ -740,6 +744,10 @@ virLogOutputToFd(virLogSourcePtr source ATTRIBUTE_UNUSED, if (flags & VIR_LOG_STACK_TRACE) virLogStackTraceToFd(fd); + + if (fd == STDERR_FILENO || fd == STDOUT_FILENO) + signal(SIGPIPE, SIG_DFL); + } -- 1.8.3.1

4 6

[libvirt] Question about migration confirm phase
by Jim Fehlig 14 Oct '19

14 Oct '19

I've been investigating a lockd lock ordering bug in a migration error handling path in the libxl driver. In the perform phase, the src calls virDomainLockProcessPause to release the lock before sending the VM to dst. In this case the send fails for other reasons and an attempt is made to reacquire the lock with virDomainLockProcessResume. But that fails since the dst has not finished cleaning up the failed VM and releasing the lock it acquired when starting to receive the VM. My immediate reaction was "why not reacquire the lock in the confirm phase", but then I saw my older comment a few lines later in the perform phase code /* * Confirm phase will not be executed if perform fails. End the * job started in begin phase. */ Is that just a bug in the implementation, or is it intended to skip the confirm phase if perform fails? Regards, Jim

2 2

Re: [libvirt] Add support for vhost-user-scsi-pci/vhost-user-blk-pci
by Cole Robinson 14 Oct '19

14 Oct '19

On 10/14/19 3:12 AM, Li Feng wrote: > Hi Cole & Michal, > > I'm sorry for my late response, I just end my journey today. > Thank your response, your suggestion is very helpful to me. > > I have added Michal in this mail, Michal helps me review my initial patchset. > (https://www.spinics.net/linux/fedora/libvir/msg191339.html) > Whoops I missed that posting, I didn't realize you had sent patches! > All concern about this feature is the XML design. > My original XML design exposes more details of Qemu. > > <vhost-user-blk-pci type='unix'> > <source type='bind' path='/tmp/vhost-blk.sock'> > <reconnect enabled='yes' timeout='5' /> > </source> > <queue num='4'/> > </vhost-user-blk-pci> > > As Cole's suggestion, the better design with all vhost-user-scsi/blk > features would like this: > > vhost-user-blk: > > <disk type='vhostuser' device='disk'> > <source type='unix' path='/path/to/vhost-user-blk.sock' mode='client'> > <reconnect enabled='yes' timeout='5' /> > </source> > <target dev='vda' bus='virtio'/> > <queue num='4'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> > </disk> > > vhost-user-scsi: > > <disk type='vhostuser' device='disk'> > <source type='unix' path='/path/to/vhost-user-scsi.sock' mode='client'> > <reconnect enabled='yes' timeout='5' /> > </source> > <target dev='sda' bus='scsi'/> > <queue num='4'/> > </disk> > I think my SCSI idea is wrong, sorry. vhost-user-scsi is for passing a scsi host adapter to the VM, correct? If so, then it's not really a <disk>, and so using the existing vhost-scsi support in <hostdev> is probably better. <hostdev> could possible be used for vhost-user-blk as well Can you provide some examples of full qemu command lines using vhost-user-blk and vhost-user-scsi? Just linking to examples else where is fine, but I'm wondering if there's more context Internally we already have an abstraction for vhost-scsi: <hostdev mode='subsystem' type='scsi_host'> <source protocol='vhost' wwpn='XXX'/> </hostdev> The obvious extension would be <hostdev mode='subsystem' type='scsi_host'> <source protocol='vhostuser' type='unix' path='/path/to/vhost-user-scsi.sock' mode='client'/> </hostdev> Internally implementing this will be weird. The <source> parameters are only dictated by the hostdev type= field, but in this case they would be dictated by the <source protocol=> field, and we would want to reuse the internal chardev abstraction. vhost-user-blk could be implemented similarly, but with type='storage' which is the way we pass through block devices to LXC guests, but it isn't currently supported in the qemu driver. I dunno. Maybe Michal or danpb can provide guidance > Conclusion: > 1. Add new type(vhostuser) for disk label; > 2. Add queue sub-label for disk to support multiqueue(<queue > num='4'/>) or reusing the driver label > (<driver name='vhostuser' queues='4'), which one is better? > Qemu support multiqueue like this: > -device vhost-user-scsi-pci,id=scsi0,chardev=spdk_vhost_scsi0,num_queues=4 > -device vhost-user-blk-pci,chardev=spdk_vhost_blk0,num-queues=4 > num-queues is already supported by libvirt for both <disk> and <hostdev> with <driver queues=X/>, so whether we use <disk> or <hostdev> you won't need to add any new XML here. > Another question: > When qemu is connecting to a vhost-user-scsi controller[1], there may > exist multiple LUNs under one target, > then one disklabel(<disk/>) will represent multiple SCSI LUNs, > the 'dev' property(<target dev='sda' bus='scsi'/>) will be ignored, right? > In other words, for vhost-user-scsi disk, it more likes a controller, > maybe the controller label is suitable. > Yes you are right, and this was my understanding. But then its not really a <controller> in libvirt's sense because we can't attach emulated devices to it, so it's more a fit for the existing <hostdev> vhost-user support. Unfortunately it's not really a clean fit anywhere, there will have to be some kind of compromise. And I'm not sure if <disk> or <hostdev> is right for vhost-user-blk, but hopefully others have more clear opinions. Thanks, Cole > I look forward to hearing from you as soon as possible. > > [1]: https://spdk.io/doc/vhost.html > > Feng Li > > Cole Robinson <crobinso(a)redhat.com> 于2019年10月10日周四上午6:48写道： >> >> Sorry for the late reply, and thanks Jano for pointing out elsewhere >> that this didn't receive a response. >> >> On 8/12/19 5:56 AM, Li Feng wrote: >>> Hi Guys, >>> >>> And I want to add the vhost-user-scsi-pci/vhost-user-blk-pci support >>> for libvirt. >>> >>> The usage in qemu like this: >>> >>> Vhost-SCSI >>> -chardev socket,id=char0,path=/var/tmp/vhost.0 >>> -device vhost-user-scsi-pci,id=scsi0,chardev=char0 >>> Vhost-BLK >>> -chardev socket,id=char1,path=/var/tmp/vhost.1 >>> -device vhost-user-blk-pci,id=blk0,chardev=char1 >>> >> >> Indeed that matches what I see for the qemu commits too: >> >> https://git.qemu.org/?p=qemu.git;a=commit;h=00343e4b54b >> https://git.qemu.org/?p=qemu.git;a=commit;h=f12c1ebddf7 >> >>> What type should I add for libvirt. >>> Type1: >>> <hostdev mode='subsystem' type='vhost-user'> >>> <source protocol='vhost-user-scsi' path='/tmp/vhost-scsi.sock'></source> >>> <alias name="vhost-user-scsi-disk1"/> >>> </hostdev> >>> >>> >>> Type2: >>> >>> <disk type='network' device='disk'> >>> <driver name='qemu' type='raw' cache='none' io='native'/> >>> <source protocol='vhost-user' path='/tmp/vhost-scsi.sock'> >>> </source> >>> <target dev='sdb' bus='vhost-user-scsi'/> >>> <boot order='3'/> >>> <alias name='scsi0-0-0-1'/> >>> <address type='drive' controller='0' bus='0' target='0' unit='1'/> >>> </disk> >>> >>> >>> <disk type='network' device='disk'> >>> <driver name='qemu' type='raw' cache='none' io='native'/> >>> <source protocol='vhost-user' path='/tmp/vhost-blk.sock'> >>> </source> >>> <target dev='vda' bus='vhost-user-blk'/> >>> <boot order='1'/> >>> <alias name='virtio-disk0'/> >>> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' >>> function='0x0'/> >>> </disk> >>> >> >> I think wiring this into <disk> makes more sense. <hostdev> is really an >> abstraction for assigning a (typically) physical host device to the VM, >> so it handles things like hiding a PCI device from the host, and passing >> that exact device to the VM. >> >> In the vhost-user-scsi/blk case, the host device is just a special >> process running on the other side of a socket, and the device >> represented to the guest is a typical virtio device. So to me it makes >> more sense as a <disk> with a <source> that points at that socket. >> >> target bus=virtio vs bus=scsi is already used to distinguish between >> virtio-blk and virtio-scsi, so I think we can keep that bit as is, with >> the <address type=drive|pci> to match. We just need to differentiate >> between plain virtio and vhost-user >> >> network devices already have vhostuser support: >> >> <interface type='vhostuser'> >> <source type='unix' path='/tmp/vhost1.sock' mode='server|client'/> >> <model type='virtio'/> >> </interface> >> >> Internally that <source> is a virDomainChrSourceDefPtr which is our >> internal representation of a chardev. So I think something akin to this >> is the way to go. It will likely require updating a LOT of places in the >> code that check disk type= field, probably most places that care about >> whether type=NETWORK or type=!NETWORK will need to be mirrored for the >> new type. >> >> <disk type='vhostuser' device='disk'> >> <source type='unix' path='/path/to/vhost-user-blk.sock' mode='client'/> >> <target dev='vda' bus='virtio'/> >> </disk> >> >> <disk type='vhostuser' device='disk'> >> <source type='unix' path='/path/to/vhost-user-scsi.sock' mode='client'/> >> <target dev='sda' bus='scsi'/> >> </disk> >> >> - Cole > - Cole

1 0

[libvirt] [PATCH v2 0/3] unplug timeout changes for PPC64
by Daniel Henrique Barboza 14 Oct '19

14 Oct '19

This is a redesign of the previous patch series [1]. After the reviews of the first version, I ended up discarding the idea of a qemu user configuration for the unplug timeout value. Instead, this patch series now handles the problematic case of PPC64 guests with an exclusive PPC64 only timeout value of 10 seconds. Patch 3 now changes how the 'unplug timeout' message is reported, using VIR_ERR_OPERATION_TIMEOUT instead of VIR_ERR_OPERATION_FAILED and making it clearer that the timeout does not equal to unplug failed. [1] https://www.redhat.com/archives/libvir-list/2019-August/msg00698.html Daniel Henrique Barboza (3): qemu: use a bigger unplug timeout for PPC64 guests qemu: Remove qemu_hotplugpriv.h and qemuDomainRemoveDeviceWaitTime qemu_hotplug: make setvcpus timeout error message user-friendlier src/qemu/Makefile.inc.am | 1 - src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_driver.c | 11 +++++++++++ src/qemu/qemu_hotplug.c | 21 +++++++++------------ src/qemu/qemu_hotplugpriv.h | 32 -------------------------------- tests/qemuhotplugtest.c | 3 +-- 6 files changed, 24 insertions(+), 47 deletions(-) delete mode 100644 src/qemu/qemu_hotplugpriv.h -- 2.21.0

2 7

[libvirt] libvirt doc - missing entry in input devices section
by Ruth Netser 13 Oct '19

13 Oct '19

The option <input type='tablet' bus='usb'/> is missing from code box under https://libvirt.org/formatdomain.html#elementsInput section. (Exists for the other devices). -- Ruty Netser

2 2

Re: [libvirt] [PATCH v2 1/1] IDE: deprecate ide-drive
by Peter Krempa 11 Oct '19

11 Oct '19

On Thu, Oct 10, 2019 at 13:22:37 +0200, Philippe Mathieu-Daudé wrote: > On 10/10/19 12:43 AM, John Snow wrote: > > It's an old compatibility shim that just delegates to ide-cd or ide-hd. > > I'd like to refactor these some day, and getting rid of the super-object > > will make that easier. > > > > Either way, we don't need this. > > > > Libvirt-checked-by: Peter Krempa <pkrempa(a)redhat.com> > > Peter made a comment regarding Laszlo's Regression-tested-by tag: > > [...] nobody else is using > this convention (there are exactly 0 instances of > "Regression-tested-by" in the project git log as far as > I can see), and so in practice people reading the commits > won't really know what you meant by it. Everybody else > on the project uses "Tested-by" to mean either of the > two cases you describe above, without distinction... > > It probably applies to 'Libvirt-checked-by' too. I certainly didn't test it. So feel free to drop that line altogether.

3 5

Re: [libvirt] [PATCH 0/4] PCI multifunction partial assignment support
by Abdulla Bubshait 11 Oct '19

11 Oct '19

On Mon, 7 Oct 2019 18:11:32 -0300 Daniel Henrique Barboza <danielhb413 gmail com> wrote: > These series tries to solve the partial assignment of multifunction > hostdev PCI devices by introducing a new hostdev attribute called > 'assigned'. This is how it works: > > - it is a boolean value that will be efffective just for > multifunction hostdev PCI devices, since there's no other > occurrence for this kind of use in Libvirt. Trying to > declare assign='yes|no' in any other PCI hostdev device > will cause parse errors; I think this functionality should be available to all hostdev PCI devices and not just multifunction ones. You might have several devices that are part of the same IOMMU group that you don't mind giving up from the host, but don't want to supply all of them to the guest. This would be an alternative to using ACS patch in these situations. One example is having two nvme drives in a single IOMMU group, you will be able to pass a single one to the guest rather than being forced to pass both.

2 1

[libvirt] [PATCH v5 0/1] add test for driver.c:virConnectValidateURIPath()
by Daniel Henrique Barboza 11 Oct '19

11 Oct '19

changes from v4: - other patches got merged, this is the last survivor - enhanced the code by attending to all suggestions from Cole Robinson v4: https://www.redhat.com/archives/libvir-list/2019-September/msg01164.html Daniel Henrique Barboza (1): tests: add a test for driver.c:virConnectValidateURIPath() tests/Makefile.am | 7 ++- tests/virdriverconnvalidatetest.c | 90 +++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 tests/virdriverconnvalidatetest.c -- 2.21.0

2 2

[libvirt] [PATCH 0/3] security: Don't remember labels for TPM
by Michal Privoznik 11 Oct '19

11 Oct '19

As it turns out, /dev/tpm0 can't be opened more than once. This doesn't fit into our seclabel remembering approach and thus disable it for TPM devices. There's also another type of files which can't be opened more than once - /dev/vfio/N. Usually, this won't be a problem unless users try to attach/detach some devices from the same IOMMU group. This will require more treatment though because it's broken on more levels. 1) we remove /dev/vfio/N in private devtmpfs on device detach, even though there is another device still attached to domain from the same IOMMU group, 2) we remove the IOMMU group from CGroups, i.e. we effectively deny access to qemu 3) we restore seclabels (regardless of seclabel remembering) Therefore, I'm only addressing TPM issue here and will continue work on hostdevs. Michal Prívozník (3): security: Try to lock only paths with remember == true security_dac: Allow selective remember/recall for chardevs security: Don't remember labels for TPM src/security/security_dac.c | 91 ++++++++++++++++++++++----------- src/security/security_selinux.c | 16 +++--- 2 files changed, 71 insertions(+), 36 deletions(-) -- 2.21.0

2 7