Re: [libvirt] [PATCH 06/25] qemu: blockjob: Register new and running blockjobs in the global table
Hi guys,
Em sex, 12 de jul de 2019 às 13:06, Peter Krempa escreveu:
>
> Add the job structure to the table when instantiating a new job and
> remove it when it terminates/fails.
>
> Signed-off-by: Peter Krempa
> ---
> src/qemu/qemu_blockjob.c | 29 ++---
> src/qemu/qemu_blockjob.h | 6 --
> src/qemu/qemu_driver.c| 16
> src/qemu/qemu_migration.c | 4 ++--
> src/qemu/qemu_process.c | 6 +++---
> 5 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index c102417e43..8cbfc556b3 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -94,8 +94,16 @@ qemuBlockJobDataNew(qemuBlockJobType type,
>
> static int
> qemuBlockJobRegister(qemuBlockJobDataPtr job,
> + virDomainObjPtr vm,
> virDomainDiskDefPtr disk)
> {
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +if (virHashAddEntry(priv->blockjobs, job->name, virObjectRef(job)) < 0) {
> +virObjectUnref(job);
> +return -1;
> +}
> +
> if (disk) {
> job->disk = disk;
> QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job);
> @@ -106,8 +114,10 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job,
>
>
> static void
> -qemuBlockJobUnregister(qemuBlockJobDataPtr job)
> +qemuBlockJobUnregister(qemuBlockJobDataPtr job,
> + virDomainObjPtr vm)
> {
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> qemuDomainDiskPrivatePtr diskPriv;
>
> if (job->disk) {
> @@ -120,6 +130,9 @@ qemuBlockJobUnregister(qemuBlockJobDataPtr job)
>
> job->disk = NULL;
> }
> +
> +/* this may remove the last reference of 'job' */
> +virHashRemoveEntry(priv->blockjobs, job->name);
> }
>
>
> @@ -132,7 +145,8 @@ qemuBlockJobUnregister(qemuBlockJobDataPtr job)
> * Returns 0 on success and -1 on failure.
> */
> qemuBlockJobDataPtr
> -qemuBlockJobDiskNew(virDomainDiskDefPtr disk,
> +qemuBlockJobDiskNew(virDomainObjPtr vm,
> +virDomainDiskDefPtr disk,
> qemuBlockJobType type,
> const char *jobname)
> {
> @@ -141,7 +155,7 @@ qemuBlockJobDiskNew(virDomainDiskDefPtr disk,
> if (!(job = qemuBlockJobDataNew(type, jobname)))
> return NULL;
>
> -if (qemuBlockJobRegister(job, disk) < 0)
> +if (qemuBlockJobRegister(job, vm, disk) < 0)
> return NULL;
>
> VIR_RETURN_PTR(job);
> @@ -189,13 +203,14 @@ qemuBlockJobStarted(qemuBlockJobDataPtr job)
> * to @job if it was started.
> */
> void
> -qemuBlockJobStartupFinalize(qemuBlockJobDataPtr job)
> +qemuBlockJobStartupFinalize(virDomainObjPtr vm,
> +qemuBlockJobDataPtr job)
> {
> if (!job)
> return;
>
> if (job->state == QEMU_BLOCKJOB_STATE_NEW)
> -qemuBlockJobUnregister(job);
> +qemuBlockJobUnregister(job, vm);
>
> virObjectUnref(job);
> }
> @@ -314,7 +329,7 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr
> driver,
> virStorageSourceBackingStoreClear(disk->src);
> ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, NULL, true));
> ignore_value(qemuBlockNodeNamesDetect(driver, vm, asyncJob));
> -qemuBlockJobUnregister(job);
> +qemuBlockJobUnregister(job, vm);
> }
>
>
> @@ -369,7 +384,7 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver,
> }
> disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
> -qemuBlockJobUnregister(job);
> +qemuBlockJobUnregister(job, vm);
> break;
>
> case VIR_DOMAIN_BLOCK_JOB_LAST:
> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> index da529090ad..b7aaa86f4d 100644
> --- a/src/qemu/qemu_blockjob.h
> +++ b/src/qemu/qemu_blockjob.h
> @@ -80,7 +80,8 @@ struct _qemuBlockJobData {
>
>
> qemuBlockJobDataPtr
> -qemuBlockJobDiskNew(virDomainDiskDefPtr disk,
> +qemuBlockJobDiskNew(virDomainObjPtr vm,
> +virDomainDiskDefPtr disk,
> qemuBlockJobType type,
> const char *jobname)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
I think this patch is missing an attribute shift... Compilation is failing:
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index d07ab75c8b..b0d17963fd 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -106,7 +106,7 @@ qemuBlockJobDiskNew(virDomainObjPtr vm,
virDomainDiskDefPtr disk,
qemuBlockJobType type,
const char *jobname)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
qemuBlockJobDataPtr
qemuBlockJobDiskGetJob(virDomainDiskDefPtr disk)
> @@ -98,7 +99,8 @@ qemuBlockJobIsRunning(qemuBlockJobDataPtr job)
>
[libvirt] [PATCH] util: change the return value of virCgroupRemove if failed
virCgroupRemove return -1 when removing cgroup failed.
But there are retry code to remove cgroup in QemuProcessStop:
retry:
if ((ret = qemuRemoveCgroup(vm)) < 0) {
if (ret == -EBUSY && (retries++ < 5)) {
usleep(200*1000);
goto retry;
}
VIR_WARN("Failed to remove cgroup for %s",
vm->def->name);
}
The return value of qemuRemoveCgroup will never be equal to "-EBUSY",
so change the return value of virCgroupRemove if failed.
Signed-off-by: Wang Yechao
---
src/util/vircgroup.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 268e401..260ed2d 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2399,11 +2399,13 @@ int
virCgroupRemove(virCgroupPtr group)
{
size_t i;
+int ret = 0;
for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
-if (group->backends[i] &&
-group->backends[i]->remove(group) < 0) {
-return -1;
+if (group->backends[i])
+ret = group->backends[i]->remove(group);
+if (ret < 0)
+return ret;
}
}
--
1.8.3.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] remote: increase daemon shutdown timer to 2 minutes
On 7/18/19 8:53 AM, Daniel P. Berrangé wrote: > Shutting down the daemon after 30 seconds of being idle is a little bit > too aggressive. Especially when using 'virsh' in single-shot mode, as > opposed to interactive shell mode, it would not be unusual to have > more than 30 seconds between commands. This will lead to the daemon > shutting down and starting up between a series of commands. > > Increasing the shutdown timer to 2 minutes will make it less likely that > the daemon will shutdown while the user is in the middle of a series of > commands. I often use virsh in single-shot mode, and often get distracted, so 5 minutes is more to my taste :-). > > Signed-off-by: Daniel P. Berrangé > --- > src/remote/libvirtd.service.in | 2 +- > src/rpc/virnetsocket.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Jim Fehlig Regards, Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On Thu, 18 Jul 2019 17:08:23 -0400 Laine Stump wrote: > On 7/18/19 2:58 PM, Daniel Henrique Barboza wrote: > > > > On 7/18/19 2:18 PM, Laine Stump wrote: > > > >> But to back up a bit - what is it about managed='yes' that makes you > >> want to do it that way instead of managed='no'? Do you really ever > >> need the devices to be binded to the host driver? Or are you just > >> using managed='yes' because there's not a standard/concenient place > >> to configure devices to be permanently binded to vfio-pci immediately > >> when the host boots? driverctl can do this fwiw. > > > > It's a case of user convenience for devices that has mixed usage, at > > least in my opinion. > > > > For example, I can say from personal experience dealing with devices > > that will never be used directly by the host, such as NVIDIA GPUs that are > > used only as hostdevs of guests, that this code I'm developing is > > pointless. In that setup the GPUs are binded to vfio-pci right after the > > host boots using a /etc/rc.d script (or something equivalent). Not sure > > if this is the standard way of binding a device to vfio-pci, but it works > > for that environment. > > > Yeah, the problem is that there really isn't a standard "this is *the > one correct way* to configure this" place for this config, so everybody > just does what works for them, making it difficult to provide a > "recommended solution" in the libvirt documentation that you (i.e. "I" > :-)) have a high level of confidence in. I think driverctl is trying to be that solution, but as soon as you say for example "NVIDIA GPUs only work this way", there are immediately a dozen users explaining a fifteen different ways that they bind their NVIDIA GPU only to vfio-pci while the VM is running and return it back when it stops. There are cases where users try to fight the kernel to get devices bound to vfio-pci exclusively and before everything else and it never changes, cases where we can let the kernel do its thing and grab it for vfio-pci later, and cases where we bounce devices between drivers depending on the current use case. > > The problem is with devices that the user expects to use both in guests > > and in the host. In that case, the user will need either to handle the > > nodedev > > detach/reattach manually or to use managed=true and let Libvirt re-attach > > the devices every time the guest is destroyed. Even if the device is > > going to > > be used in the same or another guest right after (meaning that we > > re-attached > > the device back simply to detach it again right after), using managed=true > > is convenient because the user doesn't need to think about the state of > > the device. > > > Yeah, I agree that there are uses for managed='yes' and it's a good > thing to have. It's just that I think most of the time it's being used > when it isn't needed (and thus shouldn't be used). We can't guess what the user is trying to accomplish, managed=true is a more user friendly default. Unfortunatley nm trying to dhcp any new NIC that appears is also a more user friendly default and the combination of those is less than desirable. > >> I think we should spend more time making it easier to have devices > >> "pre-binded" to vfio-pci at boot time, so that we could discourage > >> use of managed='yes'. (not "instead of" what you're doing, but "in > >> addition to" it). driverctl, and soon hopefully mdevctl. > > I think managed=true use can be called a 'bad user habit' in that > > sense. I can > > think of some ways to alleviate it: > > > > - an user setting in an conf file that changes how managed=true works. > > Instead > > of detach/re-attach the device, Libvirt will only detach the device, > > leaving the > > device bind to vfio-pci even after guest destroy > > > > > - same idea, but with a (yet another) XML attribute "re-attach=false" > > in the > > hostdev definition. I like this idea better because you can set customized > > behavior for each device/guest instead of changing the managed mechanic to > > everyone > > > I posted a patch to support that (with a new managed mode called > "detach", which would automatically bind the device to vfio-pci at geust > startup, and leave it binded to vfio-pci when the guest released it) a > couple years ago, and it was rejected upstream (after a lot of discussion): > > > https://www.redhat.com/archives/libvir-list/2016-March/msg00593.html > > > I believe the main reason was that it was "giving the consumer yet > another option that they probably wouldn't understand, and would make > the wrong choice on", or something like that... > > > I still like the idea, but it wasn't worth spending more time on the debate If we have driverctl to bind a device to a user preferred driver at start, doesn't managed=true become a no-op? (or is that what this patch was trying to do?) > > - for boot time (daemon start time), one way I can think of is an XML > > file with the
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On 7/18/19 2:58 PM, Daniel Henrique Barboza wrote: On 7/18/19 2:18 PM, Laine Stump wrote: But to back up a bit - what is it about managed='yes' that makes you want to do it that way instead of managed='no'? Do you really ever need the devices to be binded to the host driver? Or are you just using managed='yes' because there's not a standard/concenient place to configure devices to be permanently binded to vfio-pci immediately when the host boots? It's a case of user convenience for devices that has mixed usage, at least in my opinion. For example, I can say from personal experience dealing with devices that will never be used directly by the host, such as NVIDIA GPUs that are used only as hostdevs of guests, that this code I'm developing is pointless. In that setup the GPUs are binded to vfio-pci right after the host boots using a /etc/rc.d script (or something equivalent). Not sure if this is the standard way of binding a device to vfio-pci, but it works for that environment. Yeah, the problem is that there really isn't a standard "this is *the one correct way* to configure this" place for this config, so everybody just does what works for them, making it difficult to provide a "recommended solution" in the libvirt documentation that you (i.e. "I" :-)) have a high level of confidence in. The problem is with devices that the user expects to use both in guests and in the host. In that case, the user will need either to handle the nodedev detach/reattach manually or to use managed=true and let Libvirt re-attach the devices every time the guest is destroyed. Even if the device is going to be used in the same or another guest right after (meaning that we re-attached the device back simply to detach it again right after), using managed=true is convenient because the user doesn't need to think about the state of the device. Yeah, I agree that there are uses for managed='yes' and it's a good thing to have. It's just that I think most of the time it's being used when it isn't needed (and thus shouldn't be used). I think we should spend more time making it easier to have devices "pre-binded" to vfio-pci at boot time, so that we could discourage use of managed='yes'. (not "instead of" what you're doing, but "in addition to" it). I think managed=true use can be called a 'bad user habit' in that sense. I can think of some ways to alleviate it: - an user setting in an conf file that changes how managed=true works. Instead of detach/re-attach the device, Libvirt will only detach the device, leaving the device bind to vfio-pci even after guest destroy - same idea, but with a (yet another) XML attribute "re-attach=false" in the hostdev definition. I like this idea better because you can set customized behavior for each device/guest instead of changing the managed mechanic to everyone I posted a patch to support that (with a new managed mode called "detach", which would automatically bind the device to vfio-pci at geust startup, and leave it binded to vfio-pci when the guest released it) a couple years ago, and it was rejected upstream (after a lot of discussion): https://www.redhat.com/archives/libvir-list/2016-March/msg00593.html I believe the main reason was that it was "giving the consumer yet another option that they probably wouldn't understand, and would make the wrong choice on", or something like that... I still like the idea, but it wasn't worth spending more time on the debate - for boot time (daemon start time), one way I can think of is an XML file with the hostdev devices that are supposed to be pre-binded to vfio-pci by libvirtd. Then the user can run the guests using managed=false in those devices knowing that those devices are already taken care of. I don't know how this idea interacts with the new "remember_owner" feature that Michal implemented recently though . Back when I posted my patches, we discussed adding persistent config to the nodedevice driver that would take care of binding certain devices to vfio-pci. Unfortunately that misses one usage class - the case when a device should *never* be bound to the host driver at all; those need to be handled by something in the host boot process much earlier than libvirtd. - we can make a 're-definition' of what managed=false means for PCI hostdevs. Instead of failing to execute the guest if the device isn't bind to vfio-pci, managed=false means that Libvirt will detach the device from the host if necessary, but it will not re-attach it back. If such a redefinition is a massive break to API and user expect behavior (I suspect it is ...) we can create a "managed=mixed" with this mechanic Yeah, changing the meaning of managed='no' would be a non-starter. (I guess your "managed='mixed'" is pretty much what my 2016 patch did, only with a different name). -- libvir-list mailing list libvir-list@redhat.com
[libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices
There are two places in virhostdev that executes a re-attach operation
in all pci devices of a virPCIDeviceListPtr array:
virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The
difference is that the code inside virHostdevPreparePCIDevices uses
virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices
virHostdevReattachPCIDevice() is used. Both are called in the
same manner inside a loop.
To make the code easier to follow and to standardize it further,
a new virHostdevReattachAllPCIDevices() helper function is created,
which is now called in both places. virHostdevReattachPCIDevice()
was chosen as re-attach function because it is an improved version
of the raw virPCIDeviceReattach(), including a timer to wait for
device cleanup in case of pci_stub_kvm.
Signed-off-by: Daniel Henrique Barboza
---
src/util/virhostdev.c | 114 +++---
1 file changed, 52 insertions(+), 62 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 7cb0beb545..53aacb59b4 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,56 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
+static void
+virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
+virPCIDevicePtr actual)
+{
+/* Wait for device cleanup if it is qemu/kvm */
+if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
+int retries = 100;
+while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
+ && retries) {
+usleep(100*1000);
+retries--;
+}
+}
+
+VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
+if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+ virGetLastErrorMessage());
+virResetLastError();
+}
+}
+
+static void
+virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
+virHostdevManagerPtr mgr)
+{
+size_t i;
+
+for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+virPCIDevicePtr actual;
+
+/* We need to look up the actual device because that's what
+ * virHostdevReattachPCIDevice() expects as its argument */
+if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
+continue;
+
+if (virPCIDeviceGetManaged(actual))
+virHostdevReattachPCIDevice(mgr, actual);
+else
+VIR_DEBUG("Not reattaching unmanaged PCI device %s",
+ virPCIDeviceGetName(actual));
+}
+}
+
static int
virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs,
virHostdevManagerPtr mgr)
@@ -899,26 +949,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
}
reattachdevs:
-for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-virPCIDevicePtr actual;
-
-/* We need to look up the actual device because that's what
- * virPCIDeviceReattach() expects as its argument */
-if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
-continue;
-
-if (virPCIDeviceGetManaged(actual)) {
-VIR_DEBUG("Reattaching managed PCI device %s",
- virPCIDeviceGetName(pci));
-ignore_value(virPCIDeviceReattach(actual,
- mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs));
-} else {
-VIR_DEBUG("Not reattaching unmanaged PCI device %s",
- virPCIDeviceGetName(pci));
-}
-}
+virHostdevReattachAllPCIDevices(pcidevs, mgr);
cleanup:
virObjectUnlock(mgr->activePCIHostdevs);
@@ -927,33 +958,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
return ret;
}
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
-virPCIDevicePtr actual)
-{
-/* Wait for device cleanup if it is qemu/kvm */
-if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
-int retries = 100;
-while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
- && retries) {
-usleep(100*1000);
-retries--;
-}
-}
-
-VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
-if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
-VIR_ERROR(_("Failed to
[libvirt] [PATCH v1 1/3] virhostdev: introduce virHostdevResetAllPCIDevices
This code that executes virPCIDeviceReset in all virPCIDevicePtr
objects of a given virPCIDeviceListPtr list is replicated twice
in the code. Putting it in a helper function helps with
readability.
Signed-off-by: Daniel Henrique Barboza
---
src/util/virhostdev.c | 54 +++
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a3647a6cf4..7cb0beb545 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
+static int
+virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs,
+ virHostdevManagerPtr mgr)
+{
+int ret = 0;
+size_t i;
+
+for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+
+/* We can avoid looking up the actual device here, because performing
+ * a PCI reset on a device doesn't require any information other than
+ * the address, which 'pci' already contains */
+VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
+if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+VIR_ERROR(_("Failed to reset PCI device: %s"),
+ virGetLastErrorMessage());
+virResetLastError();
+ret = -1;
+}
+}
+
+return ret;
+}
+
int
virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
const char *drv_name,
@@ -765,17 +791,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
/* Step 3: Now that all the PCI hostdevs have been detached, we
* can safely reset them */
-for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
-/* We can avoid looking up the actual device here, because performing
- * a PCI reset on a device doesn't require any information other than
- * the address, which 'pci' already contains */
-VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
-if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0)
-goto reattachdevs;
-}
+if (virHostdevResetAllPCIDevices(pcidevs, mgr) < 0)
+goto reattachdevs;
/* Step 4: For SRIOV network devices, Now that we have detached the
* the network device, set the new netdev config */
@@ -1046,20 +1063,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
}
/* Step 4: perform a PCI Reset on all devices */
-for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
-/* We can avoid looking up the actual device here, because performing
- * a PCI reset on a device doesn't require any information other than
- * the address, which 'pci' already contains */
-VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
-if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
-VIR_ERROR(_("Failed to reset PCI device: %s"),
- virGetLastErrorMessage());
-virResetLastError();
-}
-}
+virHostdevResetAllPCIDevices(pcidevs, mgr);
/* Step 5: Reattach managed devices to their host drivers; unmanaged
* devices don't need to be processed further */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 0/3] misc virhostdevs cleanups
These are cleanups that I made together with an attempt to enable parcial PCI Multifunction assignment with managed=true. That work will be scrapped after discussions with Laine in [1], but these cleanups kind of make sense on their own, so here they are. [1] https://www.redhat.com/archives/libvir-list/2019-July/msg01175.html Daniel Henrique Barboza (3): virhostdev: introduce virHostdevResetAllPCIDevices virhostdev: introduce virHostdevReattachAllPCIDevices virhostdev: remove virHostdevReattachPCIDevice src/util/virhostdev.c | 151 +- src/util/virpci.c | 29 +++- 2 files changed, 86 insertions(+), 94 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice
We have 3 pieces of code that do slightly the same thing, but
it varies depending on where it is called:
- virPCIDeviceReattach(). This is where the actual re-attach
work happens;
- virHostdevReattachPCIDevice(). This is a static function from
virhostdev.c that calls virPCIDeviceReattach() after waiting
for device cleanup for the KVM PCI stub driver;
- virHostdevPCINodeDeviceReAttach(). This function also
calls virPCIDeviceReattach(), but setting some device properties
beforehand.
All these extra operations that are done before virPCIDeviceReattach()
can be moved inside the function itself, allowing for the
same re-attach behavior everywhere.
This patch consolidates all these pre-conditions inside the
body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice()
can be removed and the callers can use virPCIDeviceReattach()
directly instead.
Signed-off-by: Daniel Henrique Barboza
---
src/util/virhostdev.c | 43 +--
src/util/virpci.c | 29 ++---
2 files changed, 35 insertions(+), 37 deletions(-)
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 53aacb59b4..23be037a39 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,33 +613,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
}
}
-/*
- * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
- * are locked
- */
-static void
-virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
-virPCIDevicePtr actual)
-{
-/* Wait for device cleanup if it is qemu/kvm */
-if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
-int retries = 100;
-while (virPCIDeviceWaitForCleanup(actual, "kvm_assigned_device")
- && retries) {
-usleep(100*1000);
-retries--;
-}
-}
-
-VIR_DEBUG("Reattaching PCI device %s", virPCIDeviceGetName(actual));
-if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
- mgr->inactivePCIHostdevs) < 0) {
-VIR_ERROR(_("Failed to re-attach PCI device: %s"),
- virGetLastErrorMessage());
-virResetLastError();
-}
-}
-
static void
virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
virHostdevManagerPtr mgr)
@@ -655,11 +628,17 @@ virHostdevReattachAllPCIDevices(virPCIDeviceListPtr
pcidevs,
if (!(actual = virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
continue;
-if (virPCIDeviceGetManaged(actual))
-virHostdevReattachPCIDevice(mgr, actual);
-else
+if (virPCIDeviceGetManaged(actual)) {
+if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
+ mgr->inactivePCIHostdevs) < 0) {
+VIR_ERROR(_("Failed to re-attach PCI device: %s"),
+ virGetLastErrorMessage());
+virResetLastError();
+}
+} else {
VIR_DEBUG("Not reattaching unmanaged PCI device %s",
virPCIDeviceGetName(actual));
+}
}
}
@@ -2050,10 +2029,6 @@ virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr,
if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), ))
goto cleanup;
-virPCIDeviceSetUnbindFromStub(pci, true);
-virPCIDeviceSetRemoveSlot(pci, true);
-virPCIDeviceSetReprobe(pci, true);
-
if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs,
mgr->inactivePCIHostdevs) < 0)
goto cleanup;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 75e8daadd5..4594643d3c 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
return 0;
}
+/*
+ * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
+ * are locked
+ */
int
virPCIDeviceReattach(virPCIDevicePtr dev,
virPCIDeviceListPtr activeDevs,
virPCIDeviceListPtr inactiveDevs)
{
+int ret = -1;
+
if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Not reattaching active device %s"), dev->name);
-return -1;
+goto exit;
+}
+
+virPCIDeviceSetUnbindFromStub(dev, true);
+virPCIDeviceSetRemoveSlot(dev, true);
+virPCIDeviceSetReprobe(dev, true);
+
+/* Wait for device cleanup if it is qemu/kvm */
+if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
+int retries = 100;
+while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
+ && retries) {
+usleep(100*1000);
+retries--;
+}
}
if (virPCIDeviceUnbindFromStub(dev) < 0)
-return -1;
+goto exit;
/* Steal the dev from list inactiveDevs */
if (inactiveDevs) {
@@ -1529,7 +1549,10
[libvirt] [PATCH] news: mention new bochs display device
Signed-off-by: Jonathon Jongsma --- docs/news.xml | 9 + 1 file changed, 9 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 07d4575a65..1134309ec2 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -54,6 +54,15 @@ + + + qemu: Introduce a new video model of type 'bochs' + + + Introduce a new video model type that supports the + bochs-display device that was added in qemu version 3.0. + + -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 00/20] Add support for vTPM state encryption
On 7/12/19 12:23 PM, Stefan Berger wrote: This series of patches addresses the RFE in BZ 172830: https://bugzilla.redhat.com/show_bug.cgi?id=1728030 This series of patches adds support for vTPM state encryption by passing the read-end of a pipe's file descriptor to 'swtpm_setup' and 'swtpm' where they can read a passphrase from and derive a key from that passphrase. The TPM's domain XML looks to enable state encryption looks like this: Hi Daniel, I adapted this now to what you suggested. Can you have a look ? Stefan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On 7/18/19 2:18 PM, Laine Stump wrote: On 7/18/19 11:56 AM, Daniel Henrique Barboza wrote: On 7/18/19 12:29 PM, Laine Stump wrote: On 7/18/19 10:29 AM, Daniel Henrique Barboza wrote: Hi, I have a PoC that enables partial coldplug assignment of multifunction PCI devices with managed mode. At this moment, Libvirt can't handle this scenario - the code will detach only the hostdevs from the XML, when in fact the whole IOMMU needs to be detached. This can be verified by the fact that Libvirt handles the unmanaged scenario well, as long as the user detaches the whole IOMMU beforehand. I have played with 2 approaches. The one I am planning to contribute back is a change inside virHostdevGetPCIHostDeviceList(), that adds the extra PCI devices for detach/re-attach in case a PCI Multifunction device in managed mode is presented in the XML. If you're thinking of doing that automatically, then I should warn you that we had discussed that a long time ago, and decided that it was a bad idea to do it because it was likely someone would, e.g. try to assign an audio device to their guest that happened to be one function on a multifunction device that also contained a disk controller (or some other device) that the host needed for proper operation. Let's say that I have a Multi PCI card with 4 functions, and I want a guest to use only the function 0 of that card. At this moment, I'm only able to do that if I manually execute nodedev-detach on all 4 functions beforehand and use function 0 as a hostdev with managed=false. What I've implemented is a way of doing the detach/re-attach of the whole IOMMU for the user, if the hostdev is set with managed=true (and perhaps I should also consider verifying the 'multifunction=yes' attribute as well, for more clarity). I am not trying to assign all the IOMMU devices to the guest - not sure if that's what you were talking about up there, but I'm happy to emphasize that's not the case. No, we're talking about the same thing. We specifically talked about the possibility of doing exactly this several years ago, and decided against it. Now, yes, if the user is unaware of the consequences of detaching all devices of the IOMMU from the host, bad things can happen. If that's what you're saying, fair enough. I can make an argument about how we can't shield the user from his/her own 'unawareness' forever, but in the end it's better to be on the safe side. We really shouldn't do anything with any host device if it's not explicitly given in the config. It may be that in *your* particular case, you understand that the functions you don't want to assign to the guest are not otherwise used, and it's not dangerous to suddenly detach them from their host driver. But you can't assume that will always be the case. If you *really* can't accept just assigning all the devices in that IOMMU group to the guest (thus making them all explicitly listed in the config, and obvious to the administrator that they won't be available on the host) and simply not using them, then you either need to separately detach those particular functions from the host, or come up with a way of having the domain config explicitly list them as "detached from the host but not actually attached to the guest". I can live with that - it will automate the detach/re-attach process, which is my goal here, and it force the user to know exactly what is going to be detached from the host, minimizing errors. If no one is against adding an extra parameter 'unassigned=true' to the hostdev in these cases, I can make this happen. I don't have any idealogical opinion against that (maybe there's a better name for the attribute, but I can't think of it). I gavve the attribute the first name I could think of when I wrote that. As long as it make sense I have no reservations about changing it later on. But to back up a bit - what is it about managed='yes' that makes you want to do it that way instead of managed='no'? Do you really ever need the devices to be binded to the host driver? Or are you just using managed='yes' because there's not a standard/concenient place to configure devices to be permanently binded to vfio-pci immediately when the host boots? It's a case of user convenience for devices that has mixed usage, at least in my opinion. For example, I can say from personal experience dealing with devices that will never be used directly by the host, such as NVIDIA GPUs that are used only as hostdevs of guests, that this code I'm developing is pointless. In that setup the GPUs are binded to vfio-pci right after the host boots using a /etc/rc.d script (or something equivalent). Not sure if this is the standard way of binding a device to vfio-pci, but it works for that environment. Thus, for that scenario, we'll never use managed=true because it doesn't make sense. The problem is with devices that the user expects to use both in guests and in
Re: [libvirt] [PATCH v3] qapi: add dirty-bitmaps to query-named-block-nodes result
On 7/18/19 6:20 AM, no-re...@patchew.org wrote: > PASS 17 test-bdrv-drain /bdrv-drain/graph-change/drain_all > = > ==10263==ERROR: AddressSanitizer: heap-use-after-free on address > 0x6122c1f0 at pc 0x555fd5bd7cb6 bp 0x7f3e853b8680 sp 0x7f3e853b8678 > WRITE of size 1 at 0x6122c1f0 thread T5 > ==10262==WARNING: ASan doesn't fully support makecontext/swapcontext > functions and may produce false positives in some cases! > #0 0x555fd5bd7cb5 in aio_notify /tmp/qemu-test/src/util/async.c:351:9 > #1 0x555fd5bd98eb in qemu_bh_schedule > /tmp/qemu-test/src/util/async.c:167:9 > #2 0x555fd5bdcaf0 in aio_co_schedule /tmp/qemu-test/src/util/async.c:464:5 I'm fairly certain that this isn't related to this patch. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On 7/18/19 11:56 AM, Daniel Henrique Barboza wrote: On 7/18/19 12:29 PM, Laine Stump wrote: On 7/18/19 10:29 AM, Daniel Henrique Barboza wrote: Hi, I have a PoC that enables partial coldplug assignment of multifunction PCI devices with managed mode. At this moment, Libvirt can't handle this scenario - the code will detach only the hostdevs from the XML, when in fact the whole IOMMU needs to be detached. This can be verified by the fact that Libvirt handles the unmanaged scenario well, as long as the user detaches the whole IOMMU beforehand. I have played with 2 approaches. The one I am planning to contribute back is a change inside virHostdevGetPCIHostDeviceList(), that adds the extra PCI devices for detach/re-attach in case a PCI Multifunction device in managed mode is presented in the XML. If you're thinking of doing that automatically, then I should warn you that we had discussed that a long time ago, and decided that it was a bad idea to do it because it was likely someone would, e.g. try to assign an audio device to their guest that happened to be one function on a multifunction device that also contained a disk controller (or some other device) that the host needed for proper operation. Let's say that I have a Multi PCI card with 4 functions, and I want a guest to use only the function 0 of that card. At this moment, I'm only able to do that if I manually execute nodedev-detach on all 4 functions beforehand and use function 0 as a hostdev with managed=false. What I've implemented is a way of doing the detach/re-attach of the whole IOMMU for the user, if the hostdev is set with managed=true (and perhaps I should also consider verifying the 'multifunction=yes' attribute as well, for more clarity). I am not trying to assign all the IOMMU devices to the guest - not sure if that's what you were talking about up there, but I'm happy to emphasize that's not the case. No, we're talking about the same thing. We specifically talked about the possibility of doing exactly this several years ago, and decided against it. Now, yes, if the user is unaware of the consequences of detaching all devices of the IOMMU from the host, bad things can happen. If that's what you're saying, fair enough. I can make an argument about how we can't shield the user from his/her own 'unawareness' forever, but in the end it's better to be on the safe side. We really shouldn't do anything with any host device if it's not explicitly given in the config. It may be that in *your* particular case, you understand that the functions you don't want to assign to the guest are not otherwise used, and it's not dangerous to suddenly detach them from their host driver. But you can't assume that will always be the case. If you *really* can't accept just assigning all the devices in that IOMMU group to the guest (thus making them all explicitly listed in the config, and obvious to the administrator that they won't be available on the host) and simply not using them, then you either need to separately detach those particular functions from the host, or come up with a way of having the domain config explicitly list them as "detached from the host but not actually attached to the guest". I can live with that - it will automate the detach/re-attach process, which is my goal here, and it force the user to know exactly what is going to be detached from the host, minimizing errors. If no one is against adding an extra parameter 'unassigned=true' to the hostdev in these cases, I can make this happen. I don't have any idealogical opinion against that (maybe there's a better name for the attribute, but I can't think of it). But to back up a bit - what is it about managed='yes' that makes you want to do it that way instead of managed='no'? Do you really ever need the devices to be binded to the host driver? Or are you just using managed='yes' because there's not a standard/concenient place to configure devices to be permanently binded to vfio-pci immediately when the host boots? Truthfully, a great majority of the most troubling bugs with device assignment are due to use of managed='yes', since it exercises the kernel's device driver binding/unbinding code so much, and reveals strange races in the (usually kernel) code, but in almost all cases the devices being assigned to guests are *never* used directly by the host anyway, so there is no point in repeatedly rebinding the host driver to the device - it just sits there unused [1] until the next time it is needed by a guest, and at that time it gets rebinded to vfio-pci, rinse, repeat. I think we should spend more time making it easier to have devices "pre-binded" to vfio-pci at boot time, so that we could discourage use of managed='yes'. (not "instead of" what you're doing, but "in addition to" it). [1] (in the case of network device VFs, often it isn't "unused", but instead is *improperly* used on the
[libvirt] [PATCH 4/8] qemu: Use QEMU_BLOCKJOB_STATE_PIVOTING/ABORTING in qemuDomainBlockJobAbort
Set the correct job states after the operation is requested in qemu.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 4 +++-
src/qemu/qemu_driver.c | 8 +---
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index fe0114bf26..0f08cf7967 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -256,7 +256,9 @@ bool
qemuBlockJobIsRunning(qemuBlockJobDataPtr job)
{
return job->state == QEMU_BLOCKJOB_STATE_RUNNING ||
- job->state == QEMU_BLOCKJOB_STATE_READY;
+ job->state == QEMU_BLOCKJOB_STATE_READY ||
+ job->state == QEMU_BLOCKJOB_STATE_ABORTING ||
+ job->state == QEMU_BLOCKJOB_STATE_PIVOTING;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fa9e3c2bfc..39b5ea5e7e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17028,6 +17028,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
if (disk && disk->mirror)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
+job->state = QEMU_BLOCKJOB_STATE_PIVOTING;
cleanup:
return ret;
@@ -17182,10 +17183,10 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
goto endjob;
}
-if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
-disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
+if (job->state == QEMU_BLOCKJOB_STATE_ABORTING ||
+job->state == QEMU_BLOCKJOB_STATE_PIVOTING) {
virReportError(VIR_ERR_OPERATION_INVALID,
- _("another job on disk '%s' is still being ended"),
+ _("block job on disk '%s' is still being ended"),
disk->dst);
goto endjob;
}
@@ -17209,6 +17210,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
if (disk->mirror)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
+job->state = QEMU_BLOCKJOB_STATE_ABORTING;
}
ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps));
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 6/8] qemu: driver: Blockdevize qemuDomainBlockJobAbort/Pivot
Use job-complete/job-abort instead of the blockjob-* variants for
blockdev.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a69a0e084..12ae31b9a7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16981,6 +16981,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
{
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
+bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
switch ((qemuBlockJobType) job->type) {
case QEMU_BLOCKJOB_TYPE_NONE:
@@ -17016,7 +17017,10 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
* that pivot failed, we need to reflect that failure into the
* overall return value. */
qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorDrivePivot(priv->mon, job->name);
+if (blockdev)
+ret = qemuMonitorJobComplete(priv->mon, job->name);
+else
+ret = qemuMonitorDrivePivot(priv->mon, job->name);
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
ret = -1;
goto cleanup;
@@ -17157,6 +17161,8 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
qemuBlockJobDataPtr job = NULL;
virDomainObjPtr vm;
+qemuDomainObjPrivatePtr priv = NULL;
+bool blockdev = false;
int ret = -1;
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
@@ -17183,6 +17189,9 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
goto endjob;
}
+priv = vm->privateData;
+blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
+
if (job->state == QEMU_BLOCKJOB_STATE_ABORTING ||
job->state == QEMU_BLOCKJOB_STATE_PIVOTING) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -17199,7 +17208,10 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
goto endjob;
} else {
qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), job->name);
+if (blockdev)
+ret = qemuMonitorJobCancel(priv->mon, job->name, false);
+else
+ret = qemuMonitorBlockJobCancel(priv->mon, job->name);
if (qemuDomainObjExitMonitor(driver, vm) < 0) {
ret = -1;
goto endjob;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/8] qemu: driver: Report error if pivoting fails in qemuDomainBlockJobAbort
As the error message is now available and we know whether the job failed
we can report an error straight away rather than having the user check
the event.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 39b5ea5e7e..7a69a0e084 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17230,6 +17230,21 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
}
qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
}
+
+if (pivot &&
+job->state == QEMU_BLOCKJOB_STATE_FAILED) {
+if (job->errmsg)
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("block job '%s' failed while pivoting"),
+ job->name);
+else
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("block job '%s' failed while pivoting: %s"),
+ job->name, NULLSTR(job->errmsg));
+
+ret = -1;
+goto endjob;
+}
}
endjob:
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 7/8] qemu: Remove stale comment outlining how to extend qemuDomainBlockPivot
With -blockdev: - we track the job and check it after restart - have the ability to ask qemu to persist it to collect result - have the ability to report errors. This solves all points the comment outlined so remove it. Also all jobs handle the disk state modification along with the event so there's nothing special the comment says. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 10 -- 1 file changed, 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 12ae31b9a7..cd0e1d5cf8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17006,16 +17006,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; } -/* Attempt the pivot. Record the attempt now, to prevent duplicate - * attempts; but the actual disk change will be made when emitting - * the event. - * XXX On libvirtd restarts, if we missed the qemu event, we need - * to double check what state qemu is in. - * XXX We should be using qemu's rerror flag to make sure the job - * remains alive until we know its final state. - * XXX If the abort command is synchronous but the qemu event says - * that pivot failed, we need to reflect that failure into the - * overall return value. */ qemuDomainObjEnterMonitor(driver, vm); if (blockdev) ret = qemuMonitorJobComplete(priv->mon, job->name); -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/8] qemu: Make checks in qemuDomainBlockPivot depend on data of the job
Do decisions based on the configuration of the job rather than the data
stored with the disk.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 739cbc5ed3..fa9e3c2bfc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16982,17 +16982,26 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
-if (!disk->mirror) {
+switch ((qemuBlockJobType) job->type) {
+case QEMU_BLOCKJOB_TYPE_NONE:
+case QEMU_BLOCKJOB_TYPE_PULL:
+case QEMU_BLOCKJOB_TYPE_COMMIT:
+case QEMU_BLOCKJOB_TYPE_INTERNAL:
+case QEMU_BLOCKJOB_TYPE_LAST:
virReportError(VIR_ERR_OPERATION_INVALID,
- _("pivot of disk '%s' requires an active copy job"),
- disk->dst);
+ _("job type '%s' does not support pivot"),
+ NULLSTR(qemuBlockjobTypeToString(job->type)));
goto cleanup;
+
+case QEMU_BLOCKJOB_TYPE_COPY:
+case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
+break;
}
-if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
+if (job->state != QEMU_BLOCKJOB_STATE_READY) {
virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
- _("disk '%s' not ready for pivot yet"),
- disk->dst);
+ _("block job '%s' not ready for pivot yet"),
+ job->name);
goto cleanup;
}
@@ -17017,7 +17026,8 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
if (ret < 0)
goto cleanup;
-disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
+if (disk && disk->mirror)
+disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT;
cleanup:
return ret;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/8] qemu: driver: blockdevize qemuDomainGetBlockJobInfo
Use the stored job name rather than passing in the disk alias when
refering to the job which allows the same code to work also when
-blockdev will be used.
Note that this API does not require the change to use 'query-job' as it
will ever only work with blockjobs bound to disks due to the arguments
which allow only refering to a disk. For the disk-less jobs we'll need
to add a separate API later.
The change to qemuMonitorGetBlockJobInfo is required as the API was
striping the 'drive-' prefix when returning the data which is not
desired any more.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 9 +++--
src/qemu/qemu_monitor.c | 2 +-
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 329c166255..739cbc5ed3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17292,6 +17292,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
virDomainDiskDefPtr disk;
int ret = -1;
qemuMonitorBlockJobInfo rawInfo;
+VIR_AUTOUNREF(qemuBlockJobDataPtr) job = NULL;
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES, -1);
@@ -17314,9 +17315,13 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
goto endjob;
}
+if (!(job = qemuBlockJobDiskGetJob(disk))) {
+ret = 0;
+goto endjob;
+}
+
qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm),
- disk->info.alias, );
+ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm), job->name,
);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
if (ret <= 0)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 5ad66d1dca..a880da3ab6 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3463,7 +3463,7 @@ qemuMonitorGetBlockJobInfo(qemuMonitorPtr mon,
VIR_DEBUG("alias=%s, info=%p", alias, info);
-if (!(all = qemuMonitorGetAllBlockJobInfo(mon, false)))
+if (!(all = qemuMonitorGetAllBlockJobInfo(mon, true)))
return -1;
if ((data = virHashLookup(all, alias))) {
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 8/8] qemu: driver: Remove semi-stale comment about asynchronous job abort
Now that we track the job separately we watch only for the abort of the
one single block job so the comment is no longer accurate. Also
describing asynchronous operation is not really necessary.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cd0e1d5cf8..22166c0745 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17217,12 +17217,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
driver->caps));
-/*
- * With the ABORT_ASYNC flag we don't need to do anything, the event will
- * come from qemu and will update the XML as appropriate, but without the
- * ABORT_ASYNC flag, we must block to guarantee synchronous operation. We
- * do the waiting while still holding the VM job, to prevent newly
- * scheduled block jobs from confusing us. */
if (!async) {
qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
while (qemuBlockJobIsRunning(job)) {
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/8] qemu: blockjob: Add block job states for abort and pivot operations
When initiating a pivot or abort of a block job we need to track which
one was initiated. Currently it was done via data stashed in
virDomainDiskDef. Add possibility to track this also together with the
job itself.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_blockjob.c | 9 -
src/qemu/qemu_blockjob.h | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 292610d089..fe0114bf26 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -53,7 +53,9 @@ VIR_ENUM_IMPL(qemuBlockjobState,
"ready",
"new",
"running",
- "concluded");
+ "concluded",
+ "aborting",
+ "pivoting");
VIR_ENUM_IMPL(qemuBlockjob,
QEMU_BLOCKJOB_TYPE_LAST,
@@ -599,6 +601,8 @@
qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job,
case QEMU_BLOCKJOB_STATE_NEW:
case QEMU_BLOCKJOB_STATE_RUNNING:
case QEMU_BLOCKJOB_STATE_CONCLUDED:
+case QEMU_BLOCKJOB_STATE_ABORTING:
+case QEMU_BLOCKJOB_STATE_PIVOTING:
case QEMU_BLOCKJOB_STATE_LAST:
default:
break;
@@ -724,6 +728,9 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
case QEMU_BLOCKJOB_STATE_NEW:
case QEMU_BLOCKJOB_STATE_RUNNING:
case QEMU_BLOCKJOB_STATE_LAST:
+/* these are never processed as 'newstate' */
+case QEMU_BLOCKJOB_STATE_ABORTING:
+case QEMU_BLOCKJOB_STATE_PIVOTING:
default:
job->newstate = -1;
}
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index d07ab75c8b..1f353116c8 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -40,6 +40,8 @@ typedef enum {
QEMU_BLOCKJOB_STATE_RUNNING,
QEMU_BLOCKJOB_STATE_CONCLUDED, /* job has finished, but it's unknown
whether it has failed or not */
+QEMU_BLOCKJOB_STATE_ABORTING,
+QEMU_BLOCKJOB_STATE_PIVOTING,
QEMU_BLOCKJOB_STATE_LAST
} qemuBlockjobState;
verify((int)QEMU_BLOCKJOB_STATE_NEW == VIR_DOMAIN_BLOCK_JOB_LAST);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/8] qemu: blockdevize qemuDomainGetBlockJobInfo and qemuDomainBlockJobAbort (blockdev-add saga)
In the effort to use blockdev we need a few tweaks for the support APIs. Peter Krempa (8): qemu: driver: blockdevize qemuDomainGetBlockJobInfo qemu: Make checks in qemuDomainBlockPivot depend on data of the job qemu: blockjob: Add block job states for abort and pivot operations qemu: Use QEMU_BLOCKJOB_STATE_PIVOTING/ABORTING in qemuDomainBlockJobAbort qemu: driver: Report error if pivoting fails in qemuDomainBlockJobAbort qemu: driver: Blockdevize qemuDomainBlockJobAbort/Pivot qemu: Remove stale comment outlining how to extend qemuDomainBlockPivot qemu: driver: Remove semi-stale comment about asynchronous job abort src/qemu/qemu_blockjob.c | 13 +- src/qemu/qemu_blockjob.h | 2 + src/qemu/qemu_driver.c | 88 ++-- src/qemu/qemu_monitor.c | 2 +- 4 files changed, 72 insertions(+), 33 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] qemu: Fix handling of block job data in event handlers (blockdev-add saga fixes)
Some of the refactors which allowed carrying more data with blockjobs created a race condition when a too-quick job would actually delete the data. This is a problem for legacy block jobs only. Peter Krempa (3): qemu: process: Don't use qemuBlockJobStartupFinalize in qemuProcessHandleBlockJob qemu: driver: Don't use qemuBlockJobStartupFinalize in processBlockJobEvent qemu: driver: Add debug message when we conjure block job data object src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_process.c | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: driver: Add debug message when we conjure block job data object
Report in logs when we don't find existing block job data and create it
just to handle the job.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0438dd2878..482f915b67 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4702,6 +4702,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
}
if (!(job = qemuBlockJobDiskGetJob(disk))) {
+VIR_DEBUG("creating new block job object for '%s'", diskAlias);
if (!(job = qemuBlockJobDiskNew(vm, disk, type, diskAlias)))
goto endjob;
job->state = QEMU_BLOCKJOB_STATE_RUNNING;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] qemu: driver: Don't use qemuBlockJobStartupFinalize in processBlockJobEvent
While this function does start a block job in case when we'd not be able
to get our internal data for it, the handler sets the job state to
QEMU_BLOCKJOB_STATE_RUNNING anyways, thus qemuBlockJobStartupFinalize
would just unref the job.
Since the other usage of qemuBlockJobStartupFinalize in the other part
of the event handler was a bug replace this one anyways even if it would
not cause problems.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 329c166255..0438dd2878 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4686,7 +4686,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
int status)
{
virDomainDiskDefPtr disk;
-qemuBlockJobDataPtr job = NULL;
+VIR_AUTOUNREF(qemuBlockJobDataPtr) job = NULL;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
return;
@@ -4712,7 +4712,6 @@ processBlockJobEvent(virQEMUDriverPtr driver,
qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
endjob:
-qemuBlockJobStartupFinalize(vm, job);
qemuDomainObjEndJob(driver, vm);
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] qemu: process: Don't use qemuBlockJobStartupFinalize in qemuProcessHandleBlockJob
The block job event handler qemuProcessHandleBlockJob looks at the block job data to see whether the job requires synchronous handling. Since the block job event may arrive before we continue the job handling (if the job has no data to copy) we could hit the state when the job is still set as QEMU_BLOCKJOB_STATE_NEW (as we move it to the QEMU_BLOCKJOB_STATE_RUNNING state only after returning from monitor). If the event handler uses qemuBlockJobStartupFinalize it would unregister and free the job. Thankfully this is not a big problem for legacy blockjobs as we don't need much data for them but since we'd re-instantiate the job data structure we'd report wrong job type for active commit as qemu reports it as a regular commit job. Fix it by not using qemuBlockJobStartupFinalize function in qemuProcessHandleBlockJob as it is not starting the job anyways. https://bugzilla.redhat.com/show_bug.cgi?id=1721375 Signed-off-by: Peter Krempa --- src/qemu/qemu_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f3ffd76259..75205bc121 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -936,7 +936,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, virQEMUDriverPtr driver = opaque; struct qemuProcessEvent *processEvent = NULL; virDomainDiskDefPtr disk; -qemuBlockJobDataPtr job = NULL; +VIR_AUTOUNREF(qemuBlockJobDataPtr) job = NULL; char *data = NULL; virObjectLock(vm); @@ -983,7 +983,6 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } cleanup: -qemuBlockJobStartupFinalize(vm, job); qemuProcessEventFree(processEvent); virObjectUnlock(vm); return 0; -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On 7/18/19 12:29 PM, Laine Stump wrote: On 7/18/19 10:29 AM, Daniel Henrique Barboza wrote: Hi, I have a PoC that enables partial coldplug assignment of multifunction PCI devices with managed mode. At this moment, Libvirt can't handle this scenario - the code will detach only the hostdevs from the XML, when in fact the whole IOMMU needs to be detached. This can be verified by the fact that Libvirt handles the unmanaged scenario well, as long as the user detaches the whole IOMMU beforehand. I have played with 2 approaches. The one I am planning to contribute back is a change inside virHostdevGetPCIHostDeviceList(), that adds the extra PCI devices for detach/re-attach in case a PCI Multifunction device in managed mode is presented in the XML. If you're thinking of doing that automatically, then I should warn you that we had discussed that a long time ago, and decided that it was a bad idea to do it because it was likely someone would, e.g. try to assign an audio device to their guest that happened to be one function on a multifunction device that also contained a disk controller (or some other device) that the host needed for proper operation. Let's say that I have a Multi PCI card with 4 functions, and I want a guest to use only the function 0 of that card. At this moment, I'm only able to do that if I manually execute nodedev-detach on all 4 functions beforehand and use function 0 as a hostdev with managed=false. What I've implemented is a way of doing the detach/re-attach of the whole IOMMU for the user, if the hostdev is set with managed=true (and perhaps I should also consider verifying the 'multifunction=yes' attribute as well, for more clarity). I am not trying to assign all the IOMMU devices to the guest - not sure if that's what you were talking about up there, but I'm happy to emphasize that's not the case. Now, yes, if the user is unaware of the consequences of detaching all devices of the IOMMU from the host, bad things can happen. If that's what you're saying, fair enough. I can make an argument about how we can't shield the user from his/her own 'unawareness' forever, but in the end it's better to be on the safe side. It may be that in *your* particular case, you understand that the functions you don't want to assign to the guest are not otherwise used, and it's not dangerous to suddenly detach them from their host driver. But you can't assume that will always be the case. If you *really* can't accept just assigning all the devices in that IOMMU group to the guest (thus making them all explicitly listed in the config, and obvious to the administrator that they won't be available on the host) and simply not using them, then you either need to separately detach those particular functions from the host, or come up with a way of having the domain config explicitly list them as "detached from the host but not actually attached to the guest". I can live with that - it will automate the detach/re-attach process, which is my goal here, and it force the user to know exactly what is going to be detached from the host, minimizing errors. If no one is against adding an extra parameter 'unassigned=true' to the hostdev in these cases, I can make this happen. Thanks, DHB Now, there's a catch. Inside both virHostdevPreparePCIDevices() and virHostdevReAttachPCIDevices() there are code to save/restore the network configuration for SR-IOV devices. These functions iterates in the hostdevs list, instead of the pcidevs list I'm changing. The final result, given that the current conditions used for SR-IOV matches the conditions for multifunction PCI devices as well, is that not all virtual functions will get their network configuration saved/restored. If you're not going to use a device (which is implied by the fact that it's not in the hostdevs list) then nothing about its network config will change, so there is no reason to save/restore it. For example, a guest that uses 3 of 4 functions of a PCI MultiFunction card, let's say functions 0,1 and 3. The code will handle the detach of all the IOMMU, including the function 2 that isn't declared in the XML. Again, the above sentence implies that you're wanting to make this completely automatic, which we previously decided was something we didn't want to do. However, since function 2 isn't a hostdev, its network config will not be restored after the VM shutdown. You're talking about something that will never occur - on every SRIOV network card I've ever seen each VF is in its own IOMMU group, and can be assigned to a guest independent of what's done with any other VF. I've never seen a case (except maybe once with a newly released motherboard that had broken IOMMU firmware(?)) where a VF was in the same IOMMU group as any other device. Now comes the question: how much effort should be spent into making the network config of all the functions be restored? Is this a blocker
[libvirt] [PATCH] network: Link with libxml2
Since fb9f6ce6253 we are including a libxml header file in the network driver but never link with it. This hasn't caused an immediate problem because in the end the network driver links with libvirt.la. But apparently, it's causing a build issue on old Ubuntu. Signed-off-by: Michal Privoznik --- Pushed under trivial & build breaker rules. src/network/Makefile.inc.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/Makefile.inc.am b/src/network/Makefile.inc.am index 52270049d5..23cf39b6f4 100644 --- a/src/network/Makefile.inc.am +++ b/src/network/Makefile.inc.am @@ -47,7 +47,7 @@ libvirt_driver_network_impl_la_CFLAGS = \ $(AM_CFLAGS) \ $(NULL) libvirt_driver_network_impl_la_SOURCES = $(NETWORK_DRIVER_SOURCES) -libvirt_driver_network_impl_la_LIBADD = $(DBUS_LIBS) +libvirt_driver_network_impl_la_LIBADD = $(DBUS_LIBS) $(LIBXML_LIBS) libexec_PROGRAMS += libvirt_leaseshelper libvirt_leaseshelper_SOURCES = $(NETWORK_LEASES_HELPER_SOURCES) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
On 7/18/19 10:29 AM, Daniel Henrique Barboza wrote: Hi, I have a PoC that enables partial coldplug assignment of multifunction PCI devices with managed mode. At this moment, Libvirt can't handle this scenario - the code will detach only the hostdevs from the XML, when in fact the whole IOMMU needs to be detached. This can be verified by the fact that Libvirt handles the unmanaged scenario well, as long as the user detaches the whole IOMMU beforehand. I have played with 2 approaches. The one I am planning to contribute back is a change inside virHostdevGetPCIHostDeviceList(), that adds the extra PCI devices for detach/re-attach in case a PCI Multifunction device in managed mode is presented in the XML. If you're thinking of doing that automatically, then I should warn you that we had discussed that a long time ago, and decided that it was a bad idea to do it because it was likely someone would, e.g. try to assign an audio device to their guest that happened to be one function on a multifunction device that also contained a disk controller (or some other device) that the host needed for proper operation. It may be that in *your* particular case, you understand that the functions you don't want to assign to the guest are not otherwise used, and it's not dangerous to suddenly detach them from their host driver. But you can't assume that will always be the case. If you *really* can't accept just assigning all the devices in that IOMMU group to the guest (thus making them all explicitly listed in the config, and obvious to the administrator that they won't be available on the host) and simply not using them, then you either need to separately detach those particular functions from the host, or come up with a way of having the domain config explicitly list them as "detached from the host but not actually attached to the guest". Now, there's a catch. Inside both virHostdevPreparePCIDevices() and virHostdevReAttachPCIDevices() there are code to save/restore the network configuration for SR-IOV devices. These functions iterates in the hostdevs list, instead of the pcidevs list I'm changing. The final result, given that the current conditions used for SR-IOV matches the conditions for multifunction PCI devices as well, is that not all virtual functions will get their network configuration saved/restored. If you're not going to use a device (which is implied by the fact that it's not in the hostdevs list) then nothing about its network config will change, so there is no reason to save/restore it. For example, a guest that uses 3 of 4 functions of a PCI MultiFunction card, let's say functions 0,1 and 3. The code will handle the detach of all the IOMMU, including the function 2 that isn't declared in the XML. Again, the above sentence implies that you're wanting to make this completely automatic, which we previously decided was something we didn't want to do. However, since function 2 isn't a hostdev, its network config will not be restored after the VM shutdown. You're talking about something that will never occur - on every SRIOV network card I've ever seen each VF is in its own IOMMU group, and can be assigned to a guest independent of what's done with any other VF. I've never seen a case (except maybe once with a newly released motherboard that had broken IOMMU firmware(?)) where a VF was in the same IOMMU group as any other device. Now comes the question: how much effort should be spent into making the network config of all the functions be restored? Is this a blocker for the whole code to be accepted or, given it is proper documented somewhere, it can be done later on? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] logging: pass binary name not logfile name when enabling logging
On Thu, Jul 18, 2019 at 05:14:13PM +0200, Ján Tomko wrote: > On Thu, Jul 18, 2019 at 03:53:32PM +0100, Daniel P. Berrangé wrote: > > Ping > > > > On Mon, Jul 08, 2019 at 03:13:09PM +0100, Daniel P. Berrangé wrote: > > > Instead of having each caller pass in the desired logfile name, pass in > > > the binary name instead. The logging code can then just derive a logfile > > > name by appending ".log". > > > > > > Signed-off-by: Daniel P. Berrangé > > > --- > > > src/locking/lock_daemon.c | 2 +- > > > src/logging/log_daemon.c | 2 +- > > > src/remote/remote_daemon.c | 2 +- > > > src/util/virlog.c | 20 ++-- > > > 4 files changed, 13 insertions(+), 13 deletions(-) > > > > > IIUC this was: > Acked-by: Michal Privoznik > as a part of the daemon split series > > https://www.redhat.com/archives/libvir-list/2019-July/msg00799.html Oh, whoops, I forgot that I had it as part of that large series too. > > Reviewed-by: Ján Tomko 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] logging: pass binary name not logfile name when enabling logging
On Thu, Jul 18, 2019 at 03:53:32PM +0100, Daniel P. Berrangé wrote: Ping On Mon, Jul 08, 2019 at 03:13:09PM +0100, Daniel P. Berrangé wrote: Instead of having each caller pass in the desired logfile name, pass in the binary name instead. The logging code can then just derive a logfile name by appending ".log". Signed-off-by: Daniel P. Berrangé --- src/locking/lock_daemon.c | 2 +- src/logging/log_daemon.c | 2 +- src/remote/remote_daemon.c | 2 +- src/util/virlog.c | 20 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) IIUC this was: Acked-by: Michal Privoznik as a part of the daemon split series https://www.redhat.com/archives/libvir-list/2019-July/msg00799.html 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/5] network: xmlns dnsmasq option passthrough
On 7/18/19 4:59 AM, Daniel P. Berrangé wrote: On Wed, Jul 17, 2019 at 05:38:51PM -0400, Cole Robinson wrote: On 7/17/19 12:49 PM, Laine Stump wrote: On 7/14/19 8:03 PM, Cole Robinson wrote: There's several unresolved RFEs for the bridge driver that are essentially requests to add XML wrappers for underlying dnsmasq options. This series adds a dnsmasq xmlns namespace for XML that allows passing option strings directly to the generated dnsmasq config file. It will allow motivated users to work around libvirt until those types of RFEs are properly implemented. This all looks like a reasonable facsimile of the qemu:commandline stuff, and it worked in my tests (including not creating any problems with all the existing networks I have on my test system). My one reservation would be that it will tend to discourage adding *supported* methods of configuring things that people will instead use this backdoor to implement. But on the other hand, it makes it possible to do new things without needing to wait for an officially supported method (which could take a very long time, or simply never happen, as we've seen). (Actually I for some reason thought we already *had* support for the specific example you used - CNAME records. I guess I had lumped it in with SRV and TXT records in my memory. It really should be straightforward to do, and should still be done, but that shouldn't stop this patch series from going in). This is for the entire series: Reviewed-by: Laine Stump Thanks, I've pushed this now. Regarding the bigger point, I think it's worth considering whether we should aim to expose every requested dnsmasq option as official XML or not. Definitely not; there's a lot of things that maybe only one person would ever do, or it's just too difficult and pointless to try and abstract the config in a way that retains full functionality while being implementation agnostic (that's what stopped the dhcp options stuff in its tracks - the original patches had a problem with escaped characters or something, but once you started down the rabbit hole there was just no end). But if something is commonly used then it's better to have a straightforward way to do it so that everybody does it the same way, and we don't end up with all sorts of obscure questions because someone follows some loosely connected bad advice they found on a random page on the internet and their setup becomes completely broken. We effectively have one network driver; there's an impl for vbox and esx but they seem very basic. It doesn't look like we are going to have another backend impl any time soon. I wouldn't rule it out. I can't remember where it was, but a few months ago I had a discussion with some folks precisely about replacing dnsmasq with new daemon(s). Primarily the purpose would getting a more secure solution modern solution written in a memory safe language. Unless the requested option has some specific reason to be represented in libvirt XML, like if another part of libvirt may need/want to programmatically act on that data for some reason, maybe it's okay to say that dnsmasq passthrough is the solution. Some examples might be * dhcp-option: https://bugzilla.redhat.com/show_bug.cgi?id=666556 * auth-zone: https://bugzilla.redhat.com/show_bug.cgi?id=1690943 * This bug has a lot of mentioned options: https://bugzilla.redhat.com/show_bug.cgi?id=824573 Most of the stuff across these bugs is not really dnsmasq specific. It would be applicable to any DNS/DHCP service, so its just a matter of expressing it sensibly in the XML, so you're not tied to dnsmasq specific syntax. Yeah, in the past we may have been trying too hard to make the XML backend-agnostic. And even for domain XML, it's never been the case that the exact same XML would work unmodified for both Xen and qemu, for example. It definitely is good to work *towards* that though; makes it less of a task in the case that a change in backend does happen (for example, what Dan mentioned above). -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] logging: pass binary name not logfile name when enabling logging
Ping
On Mon, Jul 08, 2019 at 03:13:09PM +0100, Daniel P. Berrangé wrote:
> Instead of having each caller pass in the desired logfile name, pass in
> the binary name instead. The logging code can then just derive a logfile
> name by appending ".log".
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/locking/lock_daemon.c | 2 +-
> src/logging/log_daemon.c | 2 +-
> src/remote/remote_daemon.c | 2 +-
> src/util/virlog.c | 20 ++--
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index bc2fb4a7fb..7cdcd61722 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -532,7 +532,7 @@ virLockDaemonSetupLogging(virLockDaemonConfigPtr config,
> /* Define the default output. This is only applied if there was no
> setting
> * from either the config or the environment.
> */
> -if (virLogSetDefaultOutput("virtlockd.log", godaemon, privileged) < 0)
> +if (virLogSetDefaultOutput("virtlockd", godaemon, privileged) < 0)
> return -1;
>
> if (virLogGetNbOutputs() == 0)
> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> index 014596b280..c8de7aa687 100644
> --- a/src/logging/log_daemon.c
> +++ b/src/logging/log_daemon.c
> @@ -467,7 +467,7 @@ virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> /* Define the default output. This is only applied if there was no
> setting
> * from either the config or the environment.
> */
> -if (virLogSetDefaultOutput("virtlogd.log", godaemon, privileged) < 0)
> +if (virLogSetDefaultOutput("virtlogd", godaemon, privileged) < 0)
> return -1;
>
> if (virLogGetNbOutputs() == 0)
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index fdc9e4333a..ffdd3b84ad 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -610,7 +610,7 @@ daemonSetupLogging(struct daemonConfig *config,
> /* Define the default output. This is only applied if there was no
> setting
> * from either the config or the environment.
> */
> -if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0)
> +if (virLogSetDefaultOutput("libvirtd", godaemon, privileged) < 0)
> return -1;
>
> if (virLogGetNbOutputs() == 0)
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 248ce19902..da433878df 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -175,7 +175,7 @@ virLogSetDefaultOutputToJournald(void)
>
>
> static int
> -virLogSetDefaultOutputToFile(const char *filename, bool privileged)
> +virLogSetDefaultOutputToFile(const char *binary, bool privileged)
> {
> int ret = -1;
> char *logdir = NULL;
> @@ -183,8 +183,8 @@ virLogSetDefaultOutputToFile(const char *filename, bool
> privileged)
>
> if (privileged) {
> if (virAsprintf(,
> -"%d:file:%s/log/libvirt/%s", virLogDefaultPriority,
> -LOCALSTATEDIR, filename) < 0)
> +"%d:file:%s/log/libvirt/%s.log",
> virLogDefaultPriority,
> +LOCALSTATEDIR, binary) < 0)
> goto cleanup;
> } else {
> if (!(logdir = virGetUserCacheDirectory()))
> @@ -197,8 +197,8 @@ virLogSetDefaultOutputToFile(const char *filename, bool
> privileged)
> }
> umask(old_umask);
>
> -if (virAsprintf(, "%d:file:%s/%s",
> -virLogDefaultPriority, logdir, filename) < 0)
> +if (virAsprintf(, "%d:file:%s/%s.log",
> +virLogDefaultPriority, logdir, binary) < 0)
> goto cleanup;
> }
>
> @@ -211,19 +211,19 @@ virLogSetDefaultOutputToFile(const char *filename, bool
> privileged)
>
> /*
> * virLogSetDefaultOutput:
> - * @filename: the file that the output should be redirected to (only needed
> - *when @godaemon equals true
> + * @binary: the binary for which logging is performed. The log file name
> + * will be derived from the binary name, with ".log" appended.
> * @godaemon: whether we're running daemonized
> * @privileged: whether we're running with root privileges or not (session)
> *
> * Decides on what the default output (journald, file, stderr) should be
> - * according to @filename, @godaemon, @privileged. This function should be
> run
> + * according to @binary, @godaemon, @privileged. This function should be run
> * exactly once at daemon startup, so no locks are used.
> *
> * Returns 0 on success, -1 in case of a failure.
> */
> int
> -virLogSetDefaultOutput(const char *filename, bool godaemon, bool privileged)
> +virLogSetDefaultOutput(const char *binary, bool godaemon, bool privileged)
> {
> bool have_journald = access("/run/systemd/journal/socket", W_OK) >= 0;
>
> @@ -237,7 +237,7 @@ virLogSetDefaultOutput(const char *filename, bool
> godaemon, bool
[libvirt] [PATCH] remote: increase daemon shutdown timer to 2 minutes
Shutting down the daemon after 30 seconds of being idle is a little bit
too aggressive. Especially when using 'virsh' in single-shot mode, as
opposed to interactive shell mode, it would not be unusual to have
more than 30 seconds between commands. This will lead to the daemon
shutting down and starting up between a series of commands.
Increasing the shutdown timer to 2 minutes will make it less likely that
the daemon will shutdown while the user is in the middle of a series of
commands.
Signed-off-by: Daniel P. Berrangé
---
src/remote/libvirtd.service.in | 2 +-
src/rpc/virnetsocket.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in
index 2e51429e7a..3ddf0e229b 100644
--- a/src/remote/libvirtd.service.in
+++ b/src/remote/libvirtd.service.in
@@ -25,7 +25,7 @@ EnvironmentFile=-/etc/sysconfig/libvirtd
# VMs can be performed. We don't want it to stick around if
# unused though, so we set a timeout. The socket activation
# then ensures it gets started again if anything needs it
-ExecStart=@sbindir@/libvirtd --timeout 30 $LIBVIRTD_ARGS
+ExecStart=@sbindir@/libvirtd --timeout 120 $LIBVIRTD_ARGS
ExecReload=/bin/kill -HUP $MAINPID
KillMode=process
Restart=on-failure
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 34a9947eb3..3282bc0817 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -137,7 +137,7 @@ static int virNetSocketForkDaemon(const char *binary)
{
int ret;
virCommandPtr cmd = virCommandNewArgList(binary,
- "--timeout=30",
+ "--timeout=120",
NULL);
virCommandAddEnvPassCommon(cmd);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] util: Fix memleak after (legacy) blockjob (blockdev-add saga spin-off)
On 7/18/19 4:42 PM, Peter Krempa wrote: While verifying that legacy block jobs are not broken I've found a memleak. Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata src/util/virstoragefile.c | 36 +++- 1 file changed, 19 insertions(+), 17 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 0/2] util: Fix memleak after (legacy) blockjob (blockdev-add saga spin-off)
On Thu, Jul 18, 2019 at 04:42:00PM +0200, Peter Krempa wrote: While verifying that legacy block jobs are not broken I've found a memleak. Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata src/util/virstoragefile.c | 36 +++- 1 file changed, 19 insertions(+), 17 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
[libvirt] [PATCH 0/2] util: Fix memleak after (legacy) blockjob (blockdev-add saga spin-off)
While verifying that legacy block jobs are not broken I've found a memleak. Peter Krempa (2): util: storage: Clean up label use in virStorageFileGetMetadataInternal util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata src/util/virstoragefile.c | 36 +++- 1 file changed, 19 insertions(+), 17 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] util: storage: Clean up label use in virStorageFileGetMetadataInternal
The function does not do any cleanup, so replace the 'cleanup' label
with return of -1 and the 'done' label with return of 0.
Signed-off-by: Peter Krempa
---
src/util/virstoragefile.c | 27 +++
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 269d0050fd..4e2e7540f1 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -973,7 +973,6 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
int *backingFormat)
{
int dummy;
-int ret = -1;
size_t i;
if (!backingFormat)
@@ -989,7 +988,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
meta->format >= VIR_STORAGE_FILE_LAST) {
virReportSystemError(EINVAL, _("unknown storage file meta->format %d"),
meta->format);
-goto cleanup;
+return -1;
}
if (fileTypeInfo[meta->format].cryptInfo != NULL) {
@@ -999,7 +998,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta,
int expt_fmt = fileTypeInfo[meta->format].cryptInfo[i].format;
if (!meta->encryption) {
if (VIR_ALLOC(meta->encryption) < 0)
-goto cleanup;
+return -1;
meta->encryption->format = expt_fmt;
} else {
@@ -1008,7 +1007,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr
meta,
_("encryption format %d doesn't match "
"expected format %d"),
meta->encryption->format, expt_fmt);
-goto cleanup;
+return -1;
}
}
meta->encryption->payload_offset =
@@ -1021,12 +1020,12 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr
meta,
* code into this method, for non-magic files
*/
if (!fileTypeInfo[meta->format].magic)
-goto done;
+return 0;
/* Optionally extract capacity from file */
if (fileTypeInfo[meta->format].sizeOffset != -1) {
if ((fileTypeInfo[meta->format].sizeOffset + 8) > len)
-goto done;
+return 0;
if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN)
meta->capacity = virReadBufInt64LE(buf +
@@ -1037,7 +1036,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr
meta,
/* Avoid unlikely, but theoretically possible overflow */
if (meta->capacity > (ULLONG_MAX /
fileTypeInfo[meta->format].sizeMultiplier))
-goto done;
+return 0;
meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier;
}
@@ -1047,25 +1046,21 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr
meta,
backingFormat,
buf, len);
if (store == BACKING_STORE_INVALID)
-goto done;
+return 0;
if (store == BACKING_STORE_ERROR)
-goto cleanup;
+return -1;
}
if (fileTypeInfo[meta->format].getFeatures != NULL &&
fileTypeInfo[meta->format].getFeatures(>features, meta->format,
buf, len) < 0)
-goto cleanup;
+return -1;
if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features &&
VIR_STRDUP(meta->compat, "1.1") < 0)
-goto cleanup;
-
- done:
-ret = 0;
+return -1;
- cleanup:
-return ret;
+return 0;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] util: storage: Don't leak metadata on repeated calls of virStorageFileGetMetadata
When querying storage metadata after a block job we re-run virStorageFileGetMetadata on the top level storage file. This means that the workers (virStorageFileGetMetadataInternal) must not overwrite any pointers without freeing them. This was not considered for src->compat and src->features. Fix it and add a comment mentioning that. Signed-off-by: Peter Krempa --- src/util/virstoragefile.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4e2e7540f1..a6de6a1e45 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -965,7 +965,11 @@ virStorageFileGetEncryptionPayloadOffset(const struct FileEncryptionInfo *info, * assuming it has the given FORMAT, populate information into META * with information about the file and its backing store. Return format * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be - * pre-populated in META */ + * pre-populated in META. + * + * Note that this function may be called repeatedly on @meta, so it must + * clean up any existing allocated memory which would be overwritten. + */ int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, @@ -1052,10 +1056,13 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, return -1; } +virBitmapFree(meta->features); +meta->features = NULL; if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(>features, meta->format, buf, len) < 0) return -1; +VIR_FREE(meta->compat); if (meta->format == VIR_STORAGE_FILE_QCOW2 && meta->features && VIR_STRDUP(meta->compat, "1.1") < 0) return -1; -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [RFC] handling hostdev save/load net config for non SR-IOV devices
Hi, I have a PoC that enables partial coldplug assignment of multifunction PCI devices with managed mode. At this moment, Libvirt can't handle this scenario - the code will detach only the hostdevs from the XML, when in fact the whole IOMMU needs to be detached. This can be verified by the fact that Libvirt handles the unmanaged scenario well, as long as the user detaches the whole IOMMU beforehand. I have played with 2 approaches. The one I am planning to contribute back is a change inside virHostdevGetPCIHostDeviceList(), that adds the extra PCI devices for detach/re-attach in case a PCI Multifunction device in managed mode is presented in the XML. Now, there's a catch. Inside both virHostdevPreparePCIDevices() and virHostdevReAttachPCIDevices() there are code to save/restore the network configuration for SR-IOV devices. These functions iterates in the hostdevs list, instead of the pcidevs list I'm changing. The final result, given that the current conditions used for SR-IOV matches the conditions for multifunction PCI devices as well, is that not all virtual functions will get their network configuration saved/restored. For example, a guest that uses 3 of 4 functions of a PCI MultiFunction card, let's say functions 0,1 and 3. The code will handle the detach of all the IOMMU, including the function 2 that isn't declared in the XML. However, since function 2 isn't a hostdev, its network config will not be restored after the VM shutdown. Now comes the question: how much effort should be spent into making the network config of all the functions be restored? Is this a blocker for the whole code to be accepted or, given it is proper documented somewhere, it can be done later on? Thanks, DHB -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] tools: Introduce virshNodedevCapabilityNameCompleter
On Tue, Jun 18, 2019 at 03:46:16PM +0200, Michal Privoznik wrote: > This is a very simple completer for completing --cap argument of > nodedev-list command. > > Signed-off-by: Michal Privoznik > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh-completer: Separate comma list construction into a function
On Thu, Jul 18, 2019 at 11:36:27AM +0200, Martin Kletzander wrote:
> On Mon, Jul 15, 2019 at 04:27:04PM +0200, Michal Privoznik wrote:
> > On 7/15/19 1:07 PM, Erik Skultety wrote:
> > > On Wed, Jun 19, 2019 at 01:50:14PM +0200, Michal Privoznik wrote:
> > > > On 6/19/19 12:59 PM, Erik Skultety wrote:
> > > > > On Tue, Jun 18, 2019 at 03:46:15PM +0200, Michal Privoznik wrote:
> > > > > > There are more arguments than 'shutdown --mode' that accept a
> > > > > > list of strings separated by commas. 'nodedev-list --cap' is one
> > > > > > of them. To avoid duplicating code, let's separate interesting
> > > > > > bits of virshDomainShutdownModeCompleter() into a function that
> > > > > > can then be reused.
> > > > > >
> > > > > > Signed-off-by: Michal Privoznik
> > > > > > ---
> > > > > >tools/virsh-completer.c | 120
> > > > > > ++--
> > > > > >1 file changed, 77 insertions(+), 43 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
> > > > > > index 7d5cf8cb90..ef2f39320e 100644
> > > > > > --- a/tools/virsh-completer.c
> > > > > > +++ b/tools/virsh-completer.c
> > > > > > @@ -69,6 +69,79 @@
> > > > > > */
> > > > > >
> > > > > >
> > > > > > +/**
> > > > > > + * virshCommaStringListComplete:
> > > > > > + * @input: user input so far
> > > > > > + * @options: ALL options available for argument
> > > > > > + *
> > > > > > + * Some arguments to our commands accept the following form:
> > > > > > + *
> > > > > > + * virsh command --arg str1,str2,str3
> > > > > > + *
> > > > > > + * This does not play nicely with our completer funtions, because
> > > > > > + * they have to return strings prepended with user's input. For
> > > > > > + * instance:
> > > > > > + *
> > > > > > + * str1,str2,str3,strA
> > > > > > + * str1,str2,str3,strB
> > > > > > + * str1,str2,str3,strC
> > > > >
> > > > > ^This sounds rather sub-optimal. I wouldn't even insist on making the
> > > > > suggestions contextual like it is now, IOW not suggesting options
> > > > > which have
> > > > > already been specified and would rather return the same list of
> > > > > possible
> > > > > options than a string with the user input prepended.
> > > >
> > > > So IIUC, for 'shutdown --mode ' you want to see:
> > > >
> > > >"acpi", "agent", "initctl", "signal", "paravirt"
> > > >
> > > > and for 'shutdown --mode acpi,agent,' you want to see the same
> > > > list again (optionally with already specified strings removed)? Yep,
> > > > that
> > > > would be great but I don't think that is how readline works. At least, I
> > > > don't know how to achieve that. Do you perhaps have an idea?
> > >
> > > It very well may be the case that it doesn't work the way we'd like to
> > > and I
> > > don't understand how it actually works, but why does readline even matter
> > > here?
> > > Readline calls our completers which generate the output presented to the
> > > user,
> > > so we should be in charge what is returned, so why do we need to prepend
> > > the
> > > user input then? In fact, I found there's a function called
> > > vshCompleterFilter
> > > which removes the whole output list if the items are not prepended with
> > > the
> > > original user input, why is that? When I commented out the bit dropping
> > > items
> > > from the list and stopped pre-pending the user input, I achieved what I
> > > suggested in my original response to this series, a context-based list
> > > without
> > > unnecessary prefixes.
> >
> > This very likely did not work and only gave impression it is working.
> > I've just tried what you suggest here and find it not working. The
> > reason is that if we return only one option to complete it replaces the
> > whole argument with that string. Or, if we return more strings then the
> > argument is replaced with their longest shared prefix. For instance, if
> > our completer would return only {"testA", "testB", NULL}, then the
> > following input:
> >
> > virsh # start t
> >
> > would be overwritten to:
> >
> > virsh # start test
> > testA testB
> >
> > This is expected and in fact desired. But things get tricky when we
> > start dealing with out argument lists:
> >
> > virsh # shutdown --mode
> >
> > gets you:
> >
> > virsh # shutdown --mode a
> > acpi agent
> >
> > So far so good. But then you introduce comma:
> >
> > virsh # shutdown --mode agent,a
> >
> > Now, there is only one possible completion = "acpi". So readline saves
> > you some typing and turns that into:
> >
> > virsh # shutdown --mode acpi
> >
> > Problem is that readline does not handle comma as a separator. Okay, we
> > can fix that. It's easy to put comma at the end of @break_characters in
> > vshReadlineInit(). But that breaks our option lookup because then @text
> > == "a" in vshReadlineParse(). On one hand we want @text == "a" because
> > that means that readline split user's input at the comma, on the other
> > hand we can't now
Re: [libvirt] [PATCH] util: command: Ignore bitmap errors when enumerating file descriptors to close
On 7/18/19 3:12 PM, Peter Krempa wrote: virCommandMassCloseGetFDsLinux fails when running libvird on valgrind with the following message: libvirt: error : internal error: unable to set FD as open: 1024 This is because valgrind opens few file descriptors beyond the limit: 65701125 lr-x--. 1 root root 64 Jul 18 14:48 1024 -> /home/pipo/build/libvirt/gcc/src/.libs/libvirtd 65701126 lrwx--. 1 root root 64 Jul 18 14:48 1025 -> '/tmp/valgrind_proc_3849_cmdline_186612e3 (deleted)' 65701127 lrwx--. 1 root root 64 Jul 18 14:48 1026 -> '/tmp/valgrind_proc_3849_auxv_186612e3 (deleted)' 65701128 lrwx--. 1 root root 64 Jul 18 14:48 1027 -> /dev/pts/11 65701129 lr-x--. 1 root root 64 Jul 18 14:48 1028 -> 'pipe:[65689522]' 65701130 l-wx--. 1 root root 64 Jul 18 14:48 1029 -> 'pipe:[65689522]' 65701131 lr-x--. 1 root root 64 Jul 18 14:48 1030 -> /tmp/vgdb-pipe-from-vgdb-to-3849-by-root-on-angien Ignore bitmap errors in this case since we'd leak those FD's anyways in the previous scenario. Signed-off-by: Peter Krempa --- src/util/vircommand.c | 7 +-- 1 file changed, 1 insertion(+), 6 deletions(-) Reviewed-by: Michal Privoznik Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] libvirt_private.syms: Properly expose virPCI* function from virpci.h
There are couple of functions that are meant to be exposed but are missing syms file adjustment. Signed-off-by: Michal Privoznik --- Pushed under trivial rule. src/libvirt_private.syms | 5 + 1 file changed, 5 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9640e91eaa..1d9eb17a86 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2614,6 +2614,7 @@ virObjectUnref; # util/virpci.h virPCIDeviceAddressAsString; virPCIDeviceAddressEqual; +virPCIDeviceAddressFree; virPCIDeviceAddressGetIOMMUGroupAddresses; virPCIDeviceAddressGetIOMMUGroupNum; virPCIDeviceAddressGetSysfsFile; @@ -2625,6 +2626,7 @@ virPCIDeviceCopy; virPCIDeviceDetach; virPCIDeviceFileIterate; virPCIDeviceFree; +virPCIDeviceGetAddress; virPCIDeviceGetConfigPath; virPCIDeviceGetDriverPathAndName; virPCIDeviceGetIOMMUGroupDev; @@ -2664,11 +2666,14 @@ virPCIDeviceSetUsedBy; virPCIDeviceUnbind; virPCIDeviceWaitForCleanup; virPCIEDeviceInfoFree; +virPCIELinkSpeedTypeFromString; +virPCIELinkSpeedTypeToString; virPCIGetDeviceAddressFromSysfsLink; virPCIGetHeaderType; virPCIGetMdevTypes; virPCIGetNetName; virPCIGetPhysicalFunction; +virPCIGetSysfsFile; virPCIGetVirtualFunctionIndex; virPCIGetVirtualFunctionInfo; virPCIGetVirtualFunctions; -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dockerfiles PATCH 2/3] refresh: Add libosinfo-related projects
On Thu, Jul 18, 2019 at 03:05:16PM +0200, Andrea Bolognani wrote: > On Thu, 2019-07-18 at 13:35 +0100, Daniel P. Berrangé wrote: > > On Thu, Jul 18, 2019 at 02:20:34PM +0200, Andrea Bolognani wrote: > > > self.projects = [ > > > +"libosinfo", > > > "libvirt", > > > +"osinfo-db", > > > +"osinfo-db-tools", > > > ] > > > > We get away with this single container for libosinfo we don't have > > any dependancies between components. It doesn't work in general > > though. > > > > ie the build deps for libvirt-perl skip 'libvirt' because they > > assume we already chain built libvirt in the CI. This doesn't > > happen except in Jenkins CI though. > > Yeah, that's a good point. I probably shouldn't have picked the perl bindings on reflection, because they can't be built without the libvirt from git. Python / go bindings would be better as they aim to keep buildable with historic versions. > > We've never created a solution for fully populating deps such > > that the system is self-contained and doesn't need to chain > > build other projects first. > > I guess we would need basically an alternative mode where you tell > lcitool to make the resulting guest or container image entirely > self-contained... And in order to do that we'd also need to teach > it about relationships between projects, which is something that up > until now we have managed to live without. We don't need to teach it about relationships between projects in an explicit sense, in fact it could be helpful if we don't teach that. Rather we just need two lists of package. One minimal set for the chain build scenario, and one extra set for the self-contanied build scenario. eg --- chain_build_packages: - python2 - python2-devel - python2-lxml - python2-nose - python3 - python3-devel - python3-lxml - python3-nose isolated_build_packages: - libvirt Doing it with package lists would give us flexibility. For example, for python where we want to test against many libvirt versions, we have the opiton to list many versions using my virt-ark repo for example --- chain_build_packages: - python2 - python2-devel - python2-lxml - python2-nose - python3 - python3-devel - python3-lxml - python3-nose isolated_build_packages: - libvirt-ark-2_0_0 - libvirt-ark-3_0_0 - libvirt-ark-4_0_0 > Even once we have that in place, though, the two modes can't quite > live side by side in the same container, so a project like for > example virt-manager will not be able to use the libvirt container > images but would have to prepare its own (using self-contained > mode)... > > So perhaps we should not even consider merging this even though we > could still get away with it in this specific case, and instead have > the libosinfo project set up their own set of tailored container > images from day one? Having containers per project looks reasonable, especially as we can now more easily automate their creation so it isn't such an admin burden as before. Probably makes sense to still keep it all under the libvirt org, since we're still reusing the libvirt jenkins toolchain. We'd just need a project specific name prefix on each 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] util: command: Ignore bitmap errors when enumerating file descriptors to close
virCommandMassCloseGetFDsLinux fails when running libvird on valgrind
with the following message:
libvirt: error : internal error: unable to set FD as open: 1024
This is because valgrind opens few file descriptors beyond the limit:
65701125 lr-x--. 1 root root 64 Jul 18 14:48 1024 ->
/home/pipo/build/libvirt/gcc/src/.libs/libvirtd
65701126 lrwx--. 1 root root 64 Jul 18 14:48 1025 ->
'/tmp/valgrind_proc_3849_cmdline_186612e3 (deleted)'
65701127 lrwx--. 1 root root 64 Jul 18 14:48 1026 ->
'/tmp/valgrind_proc_3849_auxv_186612e3 (deleted)'
65701128 lrwx--. 1 root root 64 Jul 18 14:48 1027 -> /dev/pts/11
65701129 lr-x--. 1 root root 64 Jul 18 14:48 1028 -> 'pipe:[65689522]'
65701130 l-wx--. 1 root root 64 Jul 18 14:48 1029 -> 'pipe:[65689522]'
65701131 lr-x--. 1 root root 64 Jul 18 14:48 1030 ->
/tmp/vgdb-pipe-from-vgdb-to-3849-by-root-on-angien
Ignore bitmap errors in this case since we'd leak those FD's anyways in
the previous scenario.
Signed-off-by: Peter Krempa
---
src/util/vircommand.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 5141611ca4..4501c96bbf 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -446,12 +446,7 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd
ATTRIBUTE_UNUSED,
goto cleanup;
}
-if (virBitmapSetBit(fds, fd) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to set FD as open: %d"),
- fd);
-goto cleanup;
-}
+ignore_value(virBitmapSetBit(fds, fd));
}
if (rc < 0)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] network: wire up dnsmasq option xmlns
On Sun, Jul 14, 2019 at 08:04:00PM -0400, Cole Robinson wrote:
This maps to XML like:
...
To dnsmasq config options
...
foo=bar
cname=*.foo.example.com,master.example.com
Signed-off-by: Cole Robinson
---
docs/schemas/network.rng | 11 ++
src/network/bridge_driver.c | 130 +-
src/network/bridge_driver.h | 12 ++
tests/Makefile.am | 14 +-
.../networkxml2confdata/dnsmasq-options.conf | 18 +++
tests/networkxml2confdata/dnsmasq-options.xml | 15 ++
tests/networkxml2conftest.c | 8 +-
tests/networkxml2xmlin/dnsmasq-options.xml| 15 ++
tests/networkxml2xmlout/dnsmasq-options.xml | 17 +++
tests/networkxml2xmltest.c| 11 +-
10 files changed, 239 insertions(+), 12 deletions(-)
create mode 100644 tests/networkxml2confdata/dnsmasq-options.conf
create mode 100644 tests/networkxml2confdata/dnsmasq-options.xml
create mode 100644 tests/networkxml2xmlin/dnsmasq-options.xml
create mode 100644 tests/networkxml2xmlout/dnsmasq-options.xml
+static int
+networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
+void **data)
+{
+networkDnsmasqXmlNsDefPtr nsdata = NULL;
+int ret = -1;
+
+if (xmlXPathRegisterNs(ctxt, BAD_CAST "dnsmasq",
+ BAD_CAST DNSMASQ_NAMESPACE_HREF) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to register xml namespace '%s'"),
+ DNSMASQ_NAMESPACE_HREF);
+return -1;
+}
This breaks the build on Debian in Jenkins and Ubuntu 18 on travis:
https://ci.centos.org/view/libvirt/job/libvirt-build/systems=libvirt-debian-9/663/console
https://travis-ci.org/libvirt/libvirt/jobs/560399203
Jano
+
+if (VIR_ALLOC(nsdata) < 0)
+return -1;
+
+if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
+goto cleanup;
+
+if (nsdata->noptions > 0)
+VIR_STEAL_PTR(*data, nsdata);
+
+ret = 0;
+
+ cleanup:
+networkDnsmasqDefNamespaceFree(nsdata);
+return ret;
+}
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/5] conf: Add virNetworkXMLNamespace
On Sun, Jul 14, 2019 at 08:03:59PM -0400, Cole Robinson wrote:
Just the plumbing, no real implementation yet
Signed-off-by: Cole Robinson
---
src/conf/network_conf.c | 22 --
src/conf/network_conf.h | 21 -
src/network/bridge_driver.c | 2 +-
3 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index b7ce569d4a..b167b57e85 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -41,9 +41,24 @@
#include "virmacmap.h"
#include "virenum.h"
+typedef int (*virNetworkDefNamespaceParse)(xmlXPathContextPtr, void **);
+typedef void (*virNetworkDefNamespaceFree)(void *);
+typedef int (*virNetworkDefNamespaceXMLFormat)(virBufferPtr, void *);
+typedef const char *(*virNetworkDefNamespaceHref)(void);
+
+typedef struct _virNetworkXMLNamespace virNetworkXMLNamespace;
+typedef virNetworkXMLNamespace *virNetworkXMLNamespacePtr;
+struct _virNetworkXMLNamespace {
+virNetworkDefNamespaceParse parse;
+virNetworkDefNamespaceFree free;
+virNetworkDefNamespaceXMLFormat format;
+virNetworkDefNamespaceHref href;
+};
These declarations are identical to virDomainXMLNamespace
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dockerfiles PATCH 2/3] refresh: Add libosinfo-related projects
On Thu, 2019-07-18 at 13:35 +0100, Daniel P. Berrangé wrote: > On Thu, Jul 18, 2019 at 02:20:34PM +0200, Andrea Bolognani wrote: > > self.projects = [ > > +"libosinfo", > > "libvirt", > > +"osinfo-db", > > +"osinfo-db-tools", > > ] > > We get away with this single container for libosinfo we don't have > any dependancies between components. It doesn't work in general > though. > > ie the build deps for libvirt-perl skip 'libvirt' because they > assume we already chain built libvirt in the CI. This doesn't > happen except in Jenkins CI though. Yeah, that's a good point. > We've never created a solution for fully populating deps such > that the system is self-contained and doesn't need to chain > build other projects first. I guess we would need basically an alternative mode where you tell lcitool to make the resulting guest or container image entirely self-contained... And in order to do that we'd also need to teach it about relationships between projects, which is something that up until now we have managed to live without. Even once we have that in place, though, the two modes can't quite live side by side in the same container, so a project like for example virt-manager will not be able to use the libvirt container images but would have to prepare its own (using self-contained mode)... So perhaps we should not even consider merging this even though we could still get away with it in this specific case, and instead have the libosinfo project set up their own set of tailored container images from day one? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virSecurityManagerMetadataLock: Skip over duplicate paths
On 7/18/19 8:30 AM, Michal Privoznik wrote:
On 7/18/19 11:28 AM, Daniel P. Berrangé wrote:
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
If there are two paths on the list that are the same we need to
lock it only once. Because when we try to lock it the second time
then open() fails. And if it didn't, locking it the second time
would fail for sure. After all, it is sufficient to lock all
paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
diff --git a/src/security/security_manager.c
b/src/security/security_manager.c
index ade2c96141..7c905f0785 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1294,16 +1294,35 @@
virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
* paths A B and there's another that is trying to lock them
* in reversed order a deadlock might occur. But if we sort
* the paths alphabetically then both processes will try lock
- * paths in the same order and thus no deadlock can occur. */
+ * paths in the same order and thus no deadlock can occur.
+ * Lastly, it makes searching for duplicate paths below
+ * simpler. */
qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) {
const char *p = paths[i];
struct stat sb;
+ size_t j;
int retries = 10 * 1000;
int fd;
- if (!p || stat(p, ) < 0)
+ if (!p)
+ continue;
+
+ /* If there's a duplicate path on the list, skip it over.
+ * Not only we would fail open()-ing it the second time,
+ * we would deadlock with ourselves trying to lock it the
+ * second time. After all, we've locked it when iterating
+ * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible
to hit the same problem when we have cold plugged one device and
then later try to hotplug another device using the same resource,
or do the SRIOV assignment grouping requirements make the hotplug
impossible ?
Okay, so digging deeper I think I know what the problem is. In
general, it is of course possible to open a file multiple times, but
that is not true for "/dev/vfio/N" which can be opened exactly one
time. Indeed, looking into vfio_group_fops_open() in
kernel.git/drivers/vfio/vfio.c if a group is already opened, then
-EBUSY is returned (what we've seen in the error message). In fact,
git log blames v3.11-rc1~46^2~1 which explicitly disallowed multiple
open()-s.
So, in theory, our code works, because we open() the files we are
chown()-ing and close them immediatelly after. So the problem in which
we try to lock /dev/vfio/N multiple times won't happen. However, since
qemu already has the device opened, we will fail opening it.
I can see two options here:
1) ignore this specific error in virSecurityManagerMetadataLock().
This will of course mean that the caller
(virSecurityDACTransactionRun()) will chown() the /dev/vfio/N file
without a lock, but then again, we have namespaces and there can't be
any other domain using the path. And also, we don't really chown() if
the file already has the correct owner - which is going to be 99.99%
cases unless there would be different seclabels for two PCI devices
(do we even allow for ?).
This option seems better to me because it already establishes an
exception rule for
files that can be opened only once. For now we found out about
/dev/vfio/N files, but
there might be more in the future, and then it is just a matter of
adding more files
to this exception rule.
Thanks,
DHB
2) Make virSecurityDACSetHostdevLabel() and
virSecurityDACRestoreHostdevLabel() be NO-OP if a domain already/still
has a device from the same IOMMU group like the one we're
attaching/detaching. If these are NO-OPs then no relabel is done and
thus the whole code I'm modifying in 1) is not even called.
I have a preference for 1) because the code will be cleaner IMO.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dockerfiles PATCH 2/3] refresh: Add libosinfo-related projects
On Thu, Jul 18, 2019 at 02:20:34PM +0200, Andrea Bolognani wrote: > The libosinfo project would like to start using these container > images in their own CI pipeline, so they need the corresponding > build dependencies to be included. > > Signed-off-by: Andrea Bolognani > --- > refresh | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/refresh b/refresh > index 81df77e..2491613 100755 > --- a/refresh > +++ b/refresh > @@ -59,13 +59,18 @@ class Dockerfile: > self.cross_arch = None > > self.projects = [ > +"libosinfo", > "libvirt", > +"osinfo-db", > +"osinfo-db-tools", > ] We get away with this single container for libosinfo we don't have any dependancies between components. It doesn't work in general though. ie the build deps for libvirt-perl skip 'libvirt' because they assume we already chain built libvirt in the CI. This doesn't happen except in Jenkins CI though. We've never created a solution for fully populating deps such that the system is self-contained and doesn't need to chain build other projects first. 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] [dockerfiles PATCH 0/3] Add libosinfo-related projects
Andrea, On Thu, 2019-07-18 at 14:20 +0200, Andrea Bolognani wrote: > See patch 2/3 for more information. > > Andrea Bolognani (3): > refresh: Store projects in a more convenient format > refresh: Add libosinfo-related projects > Refresh after adding libosinfo-related projects > > buildenv-centos-7.Dockerfile | 11 + > buildenv-debian-10-cross-aarch64.Dockerfile | 15 > buildenv-debian-10-cross-armv6l.Dockerfile| 15 > buildenv-debian-10-cross-armv7l.Dockerfile| 15 > buildenv-debian-10-cross-i686.Dockerfile | 15 > buildenv-debian-10-cross-mips.Dockerfile | 15 > buildenv-debian-10-cross-mips64el.Dockerfile | 15 > buildenv-debian-10-cross-mipsel.Dockerfile| 15 > buildenv-debian-10-cross-ppc64le.Dockerfile | 15 > buildenv-debian-10-cross-s390x.Dockerfile | 15 > buildenv-debian-10.Dockerfile | 15 > buildenv-debian-9-cross-aarch64.Dockerfile| 15 > buildenv-debian-9-cross-armv6l.Dockerfile | 15 > buildenv-debian-9-cross-armv7l.Dockerfile | 15 > buildenv-debian-9-cross-mips.Dockerfile | 15 > buildenv-debian-9-cross-mips64el.Dockerfile | 15 > buildenv-debian-9-cross-mipsel.Dockerfile | 15 > buildenv-debian-9-cross-ppc64le.Dockerfile| 15 > buildenv-debian-9-cross-s390x.Dockerfile | 15 > buildenv-debian-9.Dockerfile | 15 > buildenv-debian-sid-cross-aarch64.Dockerfile | 15 > buildenv-debian-sid-cross-armv6l.Dockerfile | 15 > buildenv-debian-sid-cross-armv7l.Dockerfile | 15 > buildenv-debian-sid-cross-i686.Dockerfile | 15 > buildenv-debian-sid-cross-mips.Dockerfile | 15 > buildenv-debian-sid-cross-mips64el.Dockerfile | 15 > buildenv-debian-sid-cross-mipsel.Dockerfile | 15 > buildenv-debian-sid-cross-ppc64le.Dockerfile | 15 > buildenv-debian-sid-cross-s390x.Dockerfile| 15 > buildenv-debian-sid.Dockerfile| 15 > buildenv-fedora-29.Dockerfile | 15 > buildenv-fedora-30.Dockerfile | 15 > buildenv-fedora-rawhide.Dockerfile| 24 > +++ > buildenv-ubuntu-16.Dockerfile | 15 > buildenv-ubuntu-18.Dockerfile | 15 > refresh | 17 + > 36 files changed, 543 insertions(+), 4 deletions(-) > The patches look sane to me. There's one minor comment in the second patch in the series, tho. Reviewed-by: Fabiano Fidêncio I'd still wait for Daniel's input before pushing the changes. :-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dockerfiles PATCH 2/3] refresh: Add libosinfo-related projects
On Thu, 2019-07-18 at 14:20 +0200, Andrea Bolognani wrote: > The libosinfo project would like to start using these container > images in their own CI pipeline, so they need the corresponding > build dependencies to be included. > > Signed-off-by: Andrea Bolognani > --- > refresh | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/refresh b/refresh > index 81df77e..2491613 100755 > --- a/refresh > +++ b/refresh > @@ -59,13 +59,18 @@ class Dockerfile: > self.cross_arch = None > > self.projects = [ > +"libosinfo", > "libvirt", > +"osinfo-db", > +"osinfo-db-tools", > ] > > # Fedora Rawhide is special in that we use it to perform > MinGW > # builds, so we need to add the corresponding projects > if self.os == "fedora-rawhide": > self.projects += [ > +"osinfo-db-tools+mingw*", Please, move osinfo-db-tools+mingw* after libvirt+mingw* entry. By doing this, we'll avoid triggering your OCD. 0:-) > +"libosinfo+mingw*", > "libvirt+mingw*", > ] > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dockerfiles PATCH 3/3] Refresh after adding libosinfo-related projects
Signed-off-by: Andrea Bolognani --- buildenv-centos-7.Dockerfile | 11 + buildenv-debian-10-cross-aarch64.Dockerfile | 15 buildenv-debian-10-cross-armv6l.Dockerfile| 15 buildenv-debian-10-cross-armv7l.Dockerfile| 15 buildenv-debian-10-cross-i686.Dockerfile | 15 buildenv-debian-10-cross-mips.Dockerfile | 15 buildenv-debian-10-cross-mips64el.Dockerfile | 15 buildenv-debian-10-cross-mipsel.Dockerfile| 15 buildenv-debian-10-cross-ppc64le.Dockerfile | 15 buildenv-debian-10-cross-s390x.Dockerfile | 15 buildenv-debian-10.Dockerfile | 15 buildenv-debian-9-cross-aarch64.Dockerfile| 15 buildenv-debian-9-cross-armv6l.Dockerfile | 15 buildenv-debian-9-cross-armv7l.Dockerfile | 15 buildenv-debian-9-cross-mips.Dockerfile | 15 buildenv-debian-9-cross-mips64el.Dockerfile | 15 buildenv-debian-9-cross-mipsel.Dockerfile | 15 buildenv-debian-9-cross-ppc64le.Dockerfile| 15 buildenv-debian-9-cross-s390x.Dockerfile | 15 buildenv-debian-9.Dockerfile | 15 buildenv-debian-sid-cross-aarch64.Dockerfile | 15 buildenv-debian-sid-cross-armv6l.Dockerfile | 15 buildenv-debian-sid-cross-armv7l.Dockerfile | 15 buildenv-debian-sid-cross-i686.Dockerfile | 15 buildenv-debian-sid-cross-mips.Dockerfile | 15 buildenv-debian-sid-cross-mips64el.Dockerfile | 15 buildenv-debian-sid-cross-mipsel.Dockerfile | 15 buildenv-debian-sid-cross-ppc64le.Dockerfile | 15 buildenv-debian-sid-cross-s390x.Dockerfile| 15 buildenv-debian-sid.Dockerfile| 15 buildenv-fedora-29.Dockerfile | 15 buildenv-fedora-30.Dockerfile | 15 buildenv-fedora-rawhide.Dockerfile| 24 +++ buildenv-ubuntu-16.Dockerfile | 15 buildenv-ubuntu-18.Dockerfile | 15 35 files changed, 530 insertions(+) diff --git a/buildenv-centos-7.Dockerfile b/buildenv-centos-7.Dockerfile index 298bbc7..629012c 100644 --- a/buildenv-centos-7.Dockerfile +++ b/buildenv-centos-7.Dockerfile @@ -10,6 +10,7 @@ RUN yum update -y && \ bash \ bash-completion \ ca-certificates \ +check-devel \ chrony \ cyrus-sasl-devel \ dbus-devel \ @@ -22,14 +23,21 @@ RUN yum update -y && \ gettext \ gettext-devel \ git \ +glib2-devel \ glibc-common \ glibc-devel \ glusterfs-api-devel \ gnutls-devel \ +gobject-introspection-devel \ +gtk-doc \ +hwdata \ +intltool \ iproute \ iscsi-initiator-utils \ +json-glib-devel \ kmod \ libacl-devel \ +libarchive-devel \ libattr-devel \ libblkid-devel \ libcap-ng-devel \ @@ -40,6 +48,7 @@ RUN yum update -y && \ libpciaccess-devel \ librbd1-devel \ libselinux-devel \ +libsoup-devel \ libssh-devel \ libssh2-devel \ libtirpc-devel \ @@ -49,6 +58,7 @@ RUN yum update -y && \ libxml2 \ libxml2-devel \ libxslt \ +libxslt-devel \ lsof \ lvm2 \ make \ @@ -74,6 +84,7 @@ RUN yum update -y && \ strace \ sudo \ systemtap-sdt-devel \ +vala \ vim \ xfsprogs-devel \ yajl-devel && \ diff --git a/buildenv-debian-10-cross-aarch64.Dockerfile b/buildenv-debian-10-cross-aarch64.Dockerfile index 9498453..602f545 100644 --- a/buildenv-debian-10-cross-aarch64.Dockerfile +++ b/buildenv-debian-10-cross-aarch64.Dockerfile @@ -12,6 +12,7 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ bash-completion \ ca-certificates \ ccache \ +check \ chrony \ dnsmasq-base \ dwarves \ @@ -20,6 +21,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ gdb \ gettext \ git \ +gtk-doc-tools \ +hwdata \ +intltool \ iproute2 \ kmod \ libc-dev-bin \ @@ -38,12 +42,17 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ perl \ pkgconf \ policykit-1 \ +python3 \ +python3-lxml \ +python3-pytest \ +python3-requests \ qemu-utils \ radvd \ screen \ scrub \ strace \ sudo \
[libvirt] [dockerfiles PATCH 1/3] refresh: Store projects in a more convenient format
Signed-off-by: Andrea Bolognani --- refresh | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/refresh b/refresh index 1db0fc8..81df77e 100755 --- a/refresh +++ b/refresh @@ -58,12 +58,16 @@ class Dockerfile: self.os = stem self.cross_arch = None +self.projects = [ +"libvirt", +] + # Fedora Rawhide is special in that we use it to perform MinGW # builds, so we need to add the corresponding projects if self.os == "fedora-rawhide": -self.projects = "libvirt,libvirt+mingw*" -else: -self.projects = "libvirt" +self.projects += [ +"libvirt+mingw*", +] def refresh(self, lcitool): @@ -81,7 +85,7 @@ class Dockerfile: args += [ "libvirt-" + self.os, -self.projects, +",".join(self.projects) ] rc = subprocess.run(args, capture_output=True) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dockerfiles PATCH 2/3] refresh: Add libosinfo-related projects
The libosinfo project would like to start using these container images in their own CI pipeline, so they need the corresponding build dependencies to be included. Signed-off-by: Andrea Bolognani --- refresh | 5 + 1 file changed, 5 insertions(+) diff --git a/refresh b/refresh index 81df77e..2491613 100755 --- a/refresh +++ b/refresh @@ -59,13 +59,18 @@ class Dockerfile: self.cross_arch = None self.projects = [ +"libosinfo", "libvirt", +"osinfo-db", +"osinfo-db-tools", ] # Fedora Rawhide is special in that we use it to perform MinGW # builds, so we need to add the corresponding projects if self.os == "fedora-rawhide": self.projects += [ +"osinfo-db-tools+mingw*", +"libosinfo+mingw*", "libvirt+mingw*", ] -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [dockerfiles PATCH 0/3] Add libosinfo-related projects
See patch 2/3 for more information. Andrea Bolognani (3): refresh: Store projects in a more convenient format refresh: Add libosinfo-related projects Refresh after adding libosinfo-related projects buildenv-centos-7.Dockerfile | 11 + buildenv-debian-10-cross-aarch64.Dockerfile | 15 buildenv-debian-10-cross-armv6l.Dockerfile| 15 buildenv-debian-10-cross-armv7l.Dockerfile| 15 buildenv-debian-10-cross-i686.Dockerfile | 15 buildenv-debian-10-cross-mips.Dockerfile | 15 buildenv-debian-10-cross-mips64el.Dockerfile | 15 buildenv-debian-10-cross-mipsel.Dockerfile| 15 buildenv-debian-10-cross-ppc64le.Dockerfile | 15 buildenv-debian-10-cross-s390x.Dockerfile | 15 buildenv-debian-10.Dockerfile | 15 buildenv-debian-9-cross-aarch64.Dockerfile| 15 buildenv-debian-9-cross-armv6l.Dockerfile | 15 buildenv-debian-9-cross-armv7l.Dockerfile | 15 buildenv-debian-9-cross-mips.Dockerfile | 15 buildenv-debian-9-cross-mips64el.Dockerfile | 15 buildenv-debian-9-cross-mipsel.Dockerfile | 15 buildenv-debian-9-cross-ppc64le.Dockerfile| 15 buildenv-debian-9-cross-s390x.Dockerfile | 15 buildenv-debian-9.Dockerfile | 15 buildenv-debian-sid-cross-aarch64.Dockerfile | 15 buildenv-debian-sid-cross-armv6l.Dockerfile | 15 buildenv-debian-sid-cross-armv7l.Dockerfile | 15 buildenv-debian-sid-cross-i686.Dockerfile | 15 buildenv-debian-sid-cross-mips.Dockerfile | 15 buildenv-debian-sid-cross-mips64el.Dockerfile | 15 buildenv-debian-sid-cross-mipsel.Dockerfile | 15 buildenv-debian-sid-cross-ppc64le.Dockerfile | 15 buildenv-debian-sid-cross-s390x.Dockerfile| 15 buildenv-debian-sid.Dockerfile| 15 buildenv-fedora-29.Dockerfile | 15 buildenv-fedora-30.Dockerfile | 15 buildenv-fedora-rawhide.Dockerfile| 24 +++ buildenv-ubuntu-16.Dockerfile | 15 buildenv-ubuntu-18.Dockerfile | 15 refresh | 17 + 36 files changed, 543 insertions(+), 4 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dockerfiles PATCH] Add git-publish configuration file
On Fri, 2019-05-03 at 14:51 +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > .gitpublish | 4 > 1 file changed, 4 insertions(+) > create mode 100644 .gitpublish Now pushed under the trivial rule. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] tools: Introduce virshNodedevCapabilityNameCompleter
On Tue, Jun 18, 2019 at 03:46:14PM +0200, Michal Privoznik wrote: Yet again, couple of patches I have on my local branch for the feature I'm working on and which can go separately. Michal Prívozník (2): virsh-completer: Separate comma list construction into a function tools: Introduce virshNodedevCapabilityNameCompleter tools/virsh-completer.c | 147 tools/virsh-completer.h | 4 ++ tools/virsh-nodedev.c | 1 + 3 files changed, 109 insertions(+), 43 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 2/2] virSecurityManagerMetadataLock: Skip over duplicate paths
On 7/18/19 11:28 AM, Daniel P. Berrangé wrote:
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
If there are two paths on the list that are the same we need to
lock it only once. Because when we try to lock it the second time
then open() fails. And if it didn't, locking it the second time
would fail for sure. After all, it is sufficient to lock all
paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index ade2c96141..7c905f0785 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr
mgr ATTRIBUTE_UNUSED,
* paths A B and there's another that is trying to lock them
* in reversed order a deadlock might occur. But if we sort
* the paths alphabetically then both processes will try lock
- * paths in the same order and thus no deadlock can occur. */
+ * paths in the same order and thus no deadlock can occur.
+ * Lastly, it makes searching for duplicate paths below
+ * simpler. */
qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) {
const char *p = paths[i];
struct stat sb;
+size_t j;
int retries = 10 * 1000;
int fd;
-if (!p || stat(p, ) < 0)
+if (!p)
+continue;
+
+/* If there's a duplicate path on the list, skip it over.
+ * Not only we would fail open()-ing it the second time,
+ * we would deadlock with ourselves trying to lock it the
+ * second time. After all, we've locked it when iterating
+ * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible
to hit the same problem when we have cold plugged one device and
then later try to hotplug another device using the same resource,
or do the SRIOV assignment grouping requirements make the hotplug
impossible ?
Okay, so digging deeper I think I know what the problem is. In general,
it is of course possible to open a file multiple times, but that is not
true for "/dev/vfio/N" which can be opened exactly one time. Indeed,
looking into vfio_group_fops_open() in kernel.git/drivers/vfio/vfio.c if
a group is already opened, then -EBUSY is returned (what we've seen in
the error message). In fact, git log blames v3.11-rc1~46^2~1 which
explicitly disallowed multiple open()-s.
So, in theory, our code works, because we open() the files we are
chown()-ing and close them immediatelly after. So the problem in which
we try to lock /dev/vfio/N multiple times won't happen. However, since
qemu already has the device opened, we will fail opening it.
I can see two options here:
1) ignore this specific error in virSecurityManagerMetadataLock(). This
will of course mean that the caller (virSecurityDACTransactionRun())
will chown() the /dev/vfio/N file without a lock, but then again, we
have namespaces and there can't be any other domain using the path. And
also, we don't really chown() if the file already has the correct owner
- which is going to be 99.99% cases unless there would be different
seclabels for two PCI devices (do we even allow for
?).
2) Make virSecurityDACSetHostdevLabel() and
virSecurityDACRestoreHostdevLabel() be NO-OP if a domain already/still
has a device from the same IOMMU group like the one we're
attaching/detaching. If these are NO-OPs then no relabel is done and
thus the whole code I'm modifying in 1) is not even called.
I have a preference for 1) because the code will be cleaner IMO.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] virSecurityManagerMetadataLock: Skip over duplicate paths
Thanks for the quick patches. They fixed the problem I was having with a domain using a PCI Multifunction card, 4 hostdevs that belongs to IOMMU group 1. The problem triggered when QEMU tried to open the path /dev/vfio/1 for the second time. For both patches: Tested-by: Daniel Henrique Barboza On 7/18/19 6:14 AM, Michal Privoznik wrote: See 2/2 for explanation. Michal Prívozník (2): virSecurityManagerMetadataLock: Expand the comment on deadlocks virSecurityManagerMetadataLock: Skip over duplicate paths src/security/security_manager.c | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test_driver: implement virDomainInjectNMI
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 24
1 file changed, 24 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2e33a9dd55..90e1ede7c4 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6716,6 +6716,29 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED,
}
+static int
+testDomainInjectNMI(virDomainPtr domain,
+unsigned int flags)
+{
+virDomainObjPtr vm = NULL;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (!(vm = testDomObjFromDomain(domain)))
+return -1;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto cleanup;
+
+/* do nothing */
+ret = 0;
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static int
testDomainSendKey(virDomainPtr domain,
unsigned int codeset,
@@ -7792,6 +7815,7 @@ static virHypervisorDriver testHypervisorDriver = {
.nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */
.domainRename = testDomainRename, /* 4.1.0 */
.domainScreenshot = testDomainScreenshot, /* 1.0.5 */
+.domainInjectNMI = testDomainInjectNMI, /* 5.6.0 */
.domainSendKey = testDomainSendKey, /* 5.5.0 */
.domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
.domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qapi: add dirty-bitmaps to query-named-block-nodes result
Patchew URL: https://patchew.org/QEMU/20190717173937.18747-1-js...@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
PASS 32 test-opts-visitor /visitor/opts/range/beyond
PASS 33 test-opts-visitor /visitor/opts/dict/unvisited
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
tests/test-coroutine -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl
--test-name="test-coroutine"
==10068==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
==10068==WARNING: ASan is ignoring requested __asan_handle_no_return: stack
top: 0x7fff27ea5000; bottom 0x7f02672f8000; size: 0x00fcc0bad000 (1085565227008)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
PASS 1 test-coroutine /basic/no-dangling-access
---
PASS 1 fdc-test /x86_64/fdc/cmos
PASS 2 fdc-test /x86_64/fdc/no_media_on_start
PASS 3 fdc-test /x86_64/fdc/read_without_media
==10065==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 4 fdc-test /x86_64/fdc/media_change
PASS 5 fdc-test /x86_64/fdc/sense_interrupt
PASS 6 fdc-test /x86_64/fdc/relative_seek
---
PASS 11 test-aio /aio/event/wait
PASS 12 test-aio /aio/event/flush
PASS 13 test-aio /aio/event/wait/no-flush-cb
==10091==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 14 test-aio /aio/timer/schedule
PASS 15 test-aio /aio/coroutine/queue-chaining
PASS 16 test-aio /aio-gsource/flush
---
PASS 28 test-aio /aio-gsource/timer/schedule
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
tests/test-aio-multithread -m=quick -k --tap < /dev/null |
./scripts/tap-driver.pl --test-name="test-aio-multithread"
PASS 1 test-aio-multithread /aio/multi/lifecycle
==10097==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 2 test-aio-multithread /aio/multi/schedule
PASS 12 fdc-test /x86_64/fdc/read_no_dma_19
PASS 3 test-aio-multithread /aio/multi/mutex/contended
PASS 13 fdc-test /x86_64/fdc/fuzz-registers
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img
tests/ide-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl
--test-name="ide-test"
==10125==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 1 ide-test /x86_64/ide/identify
==10131==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 4 test-aio-multithread /aio/multi/mutex/handoff
PASS 2 ide-test /x86_64/ide/flush
==10142==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 5 test-aio-multithread /aio/multi/mutex/mcs
PASS 3 ide-test /x86_64/ide/bmdma/simple_rw
PASS 6 test-aio-multithread /aio/multi/mutex/pthread
==10154==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
tests/test-throttle -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl
--test-name="test-throttle"
PASS 1 test-throttle /throttle/leak_bucket
==10161==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 2 test-throttle /throttle/compute_wait
PASS 3 test-throttle /throttle/init
PASS 4 test-throttle /throttle/destroy
---
PASS 15 test-throttle /throttle/config/iops_size
PASS 4 ide-test /x86_64/ide/bmdma/trim
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
tests/test-thread-pool -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl
--test-name="test-thread-pool"
==10169==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
==10166==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 1 test-thread-pool /thread-pool/submit
PASS 2 test-thread-pool /thread-pool/submit-aio
PASS 3 test-thread-pool /thread-pool/submit-co
PASS 4 test-thread-pool /thread-pool/submit-many
PASS 5 ide-test /x86_64/ide/bmdma/short_prdt
==10177==WARNING: ASan doesn't fully support makecontext/swapcontext functions
and may produce false positives in some cases!
PASS 6 ide-test /x86_64/ide/bmdma/one_sector_short_prdt
==10183==WARNING: ASan doesn't fully support makecontext/swapcontext
Re: [libvirt] [PATCH 1/1] Revert "Revert "qemu: Temporary disable owner remembering""
On 7/18/19 5:48 AM, Michal Privoznik wrote:
On 7/18/19 8:46 AM, Michal Privoznik wrote:
On 7/17/19 7:20 PM, Daniel Henrique Barboza wrote:
After this commit, QEMU domains with PCI hostdevs running with
managed=true started to fail to launch with the following error:
error : virProcessRunInFork:1170 : internal error: child reported
(status=125): unable to open /dev/vfio/1: Device or resource busy
One way to avoid this issue is to disable this new feature
in qemu.conf, setting remember_owner=0. However, given that
this feature is enabled by default and it is breaking domains that
were running before it, it is best to revert the change until
it is fixed for this scenario as well.
Well, ideally, we want the feature to be turned on by default, just
like namespaces for instance. I've temporarily disabled the feature
back in the day because we were close to release and it turned out
the feature was not ready. But now there is still plenty of time to
fix it.
Anyway, I'll investigate. Meanwhile, can you share your
config or even better the full domain definition please?
Okay, so I think I know what is going on. You have two -s in
your domain and both of them belong to the same IOMMU group, right?
Yes. I forgot to mention that in this reversal patch, sorry. The error
is reproduced
with a VM using a PCI Multifunction card - alll 4 functions in the IOMMU
group 1.
I'll test your already proposed fix in a few moments. Just need a hit of
coffee first :)
Thanks,
DHB
If that is the case, then the same path appears twice in the list of
paths passed to virSecurityManagerMetadataLock(). For instance, in my
case:
Thread 2.1 "libvirtd" hit Breakpoint 3, virSecurityManagerMetadataLock
(mgr=0x7f365c026660, paths=0x7f36a001c1e0, npaths=6) at
security/security_manager.c:1283
1283 {
virSecurityManagerMetadataLock 1 # p *paths@npaths
$6 = {0x7f36a0027720 "/var/lib/libvirt/images/fedora.qcow2",
0x7f36a001a660 "/var/lib/libvirt/images/fd.img",
0x7f36a001c470
"/dev/disk/by-path/ip-10.37.132.232:3260-iscsi-iqn.2017-03.com.mprivozn:server-lun-1",
0x7f36a00262b0 "/dev/vfio/9",
0x7f36a0025e20 "/dev/vfio/9",
0x7f36a001d880
"/var/lib/libvirt/qemu/channel/target/domain-1-fedora/org.qemu.guest_agent.0"}
The "/dev/vfio/9" path is there twice because I have this graphics
card that has two functions and I'm passing them both into the domain.
The problem here is that when virSecurityManagerMetadataLock() gets to
first /dev/vfio/9 path, it opens it and locks it. Then it wants to
process the next path (which is the same path), but it fails
open()-ing the path, because it's locked. Honestly, I don't know if
that is expected behaviour, but even it if wasn't then we would fail
to lock the path second time. I'm proposing a fix in no time.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] test_driver: implement virDomainGetCPUStats
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 131 +
1 file changed, 131 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index fcb80c9e47..2907c043cb 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3258,6 +3258,136 @@ static int testDomainSetMetadata(virDomainPtr dom,
return ret;
}
+
+static int
+testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
+int nparams)
+{
+if (nparams == 0) /* return supported number of params */
+return 3;
+
+if (virTypedParameterAssign([0], VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG, 77102913900) < 0)
+return -1;
+
+if (nparams > 1 &&
+virTypedParameterAssign([1],
+VIR_DOMAIN_CPU_STATS_USERTIME,
+VIR_TYPED_PARAM_ULLONG, 4565000) < 0)
+return -1;
+
+if (nparams > 2 &&
+virTypedParameterAssign([2],
+VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
+VIR_TYPED_PARAM_ULLONG, 1139000) < 0)
+return -1;
+
+if (nparams > 3)
+nparams = 3;
+
+return nparams;
+}
+
+
+static int
+testDomainGetPercpuStats(virTypedParameterPtr params,
+ unsigned int nparams,
+ int start_cpu,
+ unsigned int ncpus,
+ int total_cpus)
+{
+size_t i;
+int need_cpus;
+int param_idx;
+int ret = -1;
+
+/* return the number of supported params */
+if (nparams == 0 && ncpus != 0)
+return 2;
+
+/* return total number of cpus */
+if (ncpus == 0) {
+ret = total_cpus;
+goto cleanup;
+}
+
+if (start_cpu >= total_cpus) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("start_cpu %d larger than maximum of %d"),
+ start_cpu, total_cpus - 1);
+goto cleanup;
+}
+
+/* return percpu cputime in index 0 */
+param_idx = 0;
+
+/* number of cpus to compute */
+need_cpus = MIN(total_cpus, start_cpu + ncpus);
+
+for (i = 0; i < need_cpus; i++) {
+if (i < start_cpu)
+continue;
+int idx = (i - start_cpu) * nparams + param_idx;
+if (virTypedParameterAssign([idx],
+VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+202542145062 + 10 * i) < 0)
+goto cleanup;
+}
+
+/* return percpu vcputime in index 1 */
+param_idx = 1;
+
+if (param_idx < nparams) {
+for (i = start_cpu; i < need_cpus; i++) {
+int idx = (i - start_cpu) * nparams + param_idx;
+if (virTypedParameterAssign([idx],
+VIR_DOMAIN_CPU_STATS_VCPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+26254023765 + 10 * i) < 0)
+goto cleanup;
+}
+param_idx++;
+}
+
+ret = param_idx;
+ cleanup:
+return ret;
+}
+
+
+static int
+testDomainGetCPUStats(virDomainPtr dom,
+ virTypedParameterPtr params,
+ unsigned int nparams,
+ int start_cpu,
+ unsigned int ncpus,
+ unsigned int flags)
+{
+virDomainObjPtr vm = NULL;
+testDriverPtr privconn = dom->conn->privateData;
+int ret = -1;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+if (!(vm = testDomObjFromDomain(dom)))
+return -1;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto cleanup;
+
+if (start_cpu == -1)
+ret = testDomainGetDomainTotalCpuStats(params, nparams);
+else
+ret = testDomainGetPercpuStats(params, nparams, start_cpu, ncpus,
+ privconn->nodeInfo.cores);
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+
static int
testDomainSendProcessSignal(virDomainPtr dom,
long long pid_value,
@@ -7794,6 +7924,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainSendKey = testDomainSendKey, /* 5.5.0 */
.domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
.domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
+.domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */
.domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */
.connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */
.domainManagedSave = testDomainManagedSave, /* 1.1.4 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virsh-completer: Separate comma list construction into a function
On Mon, Jul 15, 2019 at 04:27:04PM +0200, Michal Privoznik wrote:
On 7/15/19 1:07 PM, Erik Skultety wrote:
On Wed, Jun 19, 2019 at 01:50:14PM +0200, Michal Privoznik wrote:
On 6/19/19 12:59 PM, Erik Skultety wrote:
On Tue, Jun 18, 2019 at 03:46:15PM +0200, Michal Privoznik wrote:
There are more arguments than 'shutdown --mode' that accept a
list of strings separated by commas. 'nodedev-list --cap' is one
of them. To avoid duplicating code, let's separate interesting
bits of virshDomainShutdownModeCompleter() into a function that
can then be reused.
Signed-off-by: Michal Privoznik
---
tools/virsh-completer.c | 120 ++--
1 file changed, 77 insertions(+), 43 deletions(-)
diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index 7d5cf8cb90..ef2f39320e 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -69,6 +69,79 @@
*/
+/**
+ * virshCommaStringListComplete:
+ * @input: user input so far
+ * @options: ALL options available for argument
+ *
+ * Some arguments to our commands accept the following form:
+ *
+ * virsh command --arg str1,str2,str3
+ *
+ * This does not play nicely with our completer funtions, because
+ * they have to return strings prepended with user's input. For
+ * instance:
+ *
+ * str1,str2,str3,strA
+ * str1,str2,str3,strB
+ * str1,str2,str3,strC
^This sounds rather sub-optimal. I wouldn't even insist on making the
suggestions contextual like it is now, IOW not suggesting options which have
already been specified and would rather return the same list of possible
options than a string with the user input prepended.
So IIUC, for 'shutdown --mode ' you want to see:
"acpi", "agent", "initctl", "signal", "paravirt"
and for 'shutdown --mode acpi,agent,' you want to see the same
list again (optionally with already specified strings removed)? Yep, that
would be great but I don't think that is how readline works. At least, I
don't know how to achieve that. Do you perhaps have an idea?
It very well may be the case that it doesn't work the way we'd like to and I
don't understand how it actually works, but why does readline even matter here?
Readline calls our completers which generate the output presented to the user,
so we should be in charge what is returned, so why do we need to prepend the
user input then? In fact, I found there's a function called vshCompleterFilter
which removes the whole output list if the items are not prepended with the
original user input, why is that? When I commented out the bit dropping items
from the list and stopped pre-pending the user input, I achieved what I
suggested in my original response to this series, a context-based list without
unnecessary prefixes.
This very likely did not work and only gave impression it is working.
I've just tried what you suggest here and find it not working. The
reason is that if we return only one option to complete it replaces the
whole argument with that string. Or, if we return more strings then the
argument is replaced with their longest shared prefix. For instance, if
our completer would return only {"testA", "testB", NULL}, then the
following input:
virsh # start t
would be overwritten to:
virsh # start test
testA testB
This is expected and in fact desired. But things get tricky when we
start dealing with out argument lists:
virsh # shutdown --mode
gets you:
virsh # shutdown --mode a
acpi agent
So far so good. But then you introduce comma:
virsh # shutdown --mode agent,a
Now, there is only one possible completion = "acpi". So readline saves
you some typing and turns that into:
virsh # shutdown --mode acpi
Problem is that readline does not handle comma as a separator. Okay, we
can fix that. It's easy to put comma at the end of @break_characters in
vshReadlineInit(). But that breaks our option lookup because then @text
== "a" in vshReadlineParse(). On one hand we want @text == "a" because
that means that readline split user's input at the comma, on the other
hand we can't now properly identify which --option is user trying to
autocomplete because neither --option has "a" as its value (--mode has
"agent,a").
I also tried a few other random completions to see
whether I didn't break something by stripping some code from
vshCompleterFilter and it looks like it worked, so the question is, what was
the reason for that function in the first place, since I haven't experienced
the effects described by commit d4e63aff5d0 which introduced it?
The reason for existence of vshCompleterFilter() is to filter out
non-relevant options. For instance, in aforementioned shutdown mode
completer - we want, I want completers to be as simple as possible.
Therefore, the shutdown mode completer returns all five strings,
regardless of user's input. Then the filter function prunes out those
strings (=options) which do not share prefix with user's input. For
instance, if user's input is "a" then "initctl", "signal" and
Re: [libvirt] [PATCH] util: Fix uninitalized variable to avoid garbage value.
On 7/18/19 5:44 AM, Julio Faracco wrote: This commit is similar with 596aa144. It fixes an uninitialized variable to avoid garbage value. This case, it uses time 't' 0 if an error occurs with virTimeMillisNowRaw. Signed-off-by: Julio Faracco --- src/util/virtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ACKed and pushed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] virSecurityManagerMetadataLock: Skip over duplicate paths
On Thu, Jul 18, 2019 at 11:14:49AM +0200, Michal Privoznik wrote:
> If there are two paths on the list that are the same we need to
> lock it only once. Because when we try to lock it the second time
> then open() fails. And if it didn't, locking it the second time
> would fail for sure. After all, it is sufficient to lock all
> paths just once satisfy the caller.
>
> Reported-by: Daniel Henrique Barboza
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_manager.c | 23 +--
> 1 file changed, 21 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index ade2c96141..7c905f0785 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr
> mgr ATTRIBUTE_UNUSED,
> * paths A B and there's another that is trying to lock them
> * in reversed order a deadlock might occur. But if we sort
> * the paths alphabetically then both processes will try lock
> - * paths in the same order and thus no deadlock can occur. */
> + * paths in the same order and thus no deadlock can occur.
> + * Lastly, it makes searching for duplicate paths below
> + * simpler. */
> qsort(paths, npaths, sizeof(*paths), cmpstringp);
>
> for (i = 0; i < npaths; i++) {
> const char *p = paths[i];
> struct stat sb;
> +size_t j;
> int retries = 10 * 1000;
> int fd;
>
> -if (!p || stat(p, ) < 0)
> +if (!p)
> +continue;
> +
> +/* If there's a duplicate path on the list, skip it over.
> + * Not only we would fail open()-ing it the second time,
> + * we would deadlock with ourselves trying to lock it the
> + * second time. After all, we've locked it when iterating
> + * over it the first time. */
Presumably this deals with the problem at cold boot. Is it possible
to hit the same problem when we have cold plugged one device and
then later try to hotplug another device using the same resource,
or do the SRIOV assignment grouping requirements make the hotplug
impossible ?
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/2] virSecurityManagerMetadataLock: Expand the comment on deadlocks
On Thu, Jul 18, 2019 at 11:14:48AM +0200, Michal Privoznik wrote: > Document why we need to sort paths while it's still fresh in my > memory. > > Signed-off-by: Michal Privoznik > --- > src/security/security_manager.c | 7 ++- > 1 file changed, 6 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
[libvirt] [PATCH 1/2] virSecurityManagerMetadataLock: Expand the comment on deadlocks
Document why we need to sort paths while it's still fresh in my
memory.
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index c205c3bf17..ade2c96141 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1289,7 +1289,12 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
if (VIR_ALLOC_N(fds, npaths) < 0)
return NULL;
-/* Sort paths to lock in order to avoid deadlocks. */
+/* Sort paths to lock in order to avoid deadlocks with other
+ * processes. For instance, if one process wants to lock
+ * paths A B and there's another that is trying to lock them
+ * in reversed order a deadlock might occur. But if we sort
+ * the paths alphabetically then both processes will try lock
+ * paths in the same order and thus no deadlock can occur. */
qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) {
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] virSecurityManagerMetadataLock: Skip over duplicate paths
If there are two paths on the list that are the same we need to
lock it only once. Because when we try to lock it the second time
then open() fails. And if it didn't, locking it the second time
would fail for sure. After all, it is sufficient to lock all
paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index ade2c96141..7c905f0785 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1294,16 +1294,35 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr
mgr ATTRIBUTE_UNUSED,
* paths A B and there's another that is trying to lock them
* in reversed order a deadlock might occur. But if we sort
* the paths alphabetically then both processes will try lock
- * paths in the same order and thus no deadlock can occur. */
+ * paths in the same order and thus no deadlock can occur.
+ * Lastly, it makes searching for duplicate paths below
+ * simpler. */
qsort(paths, npaths, sizeof(*paths), cmpstringp);
for (i = 0; i < npaths; i++) {
const char *p = paths[i];
struct stat sb;
+size_t j;
int retries = 10 * 1000;
int fd;
-if (!p || stat(p, ) < 0)
+if (!p)
+continue;
+
+/* If there's a duplicate path on the list, skip it over.
+ * Not only we would fail open()-ing it the second time,
+ * we would deadlock with ourselves trying to lock it the
+ * second time. After all, we've locked it when iterating
+ * over it the first time. */
+for (j = 0; j < i; j++) {
+if (STREQ_NULLABLE(p, paths[j]))
+break;
+}
+
+if (i != j)
+continue;
+
+if (stat(p, ) < 0)
continue;
if (S_ISDIR(sb.st_mode)) {
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] virSecurityManagerMetadataLock: Skip over duplicate paths
See 2/2 for explanation. Michal Prívozník (2): virSecurityManagerMetadataLock: Expand the comment on deadlocks virSecurityManagerMetadataLock: Skip over duplicate paths src/security/security_manager.c | 28 ++-- 1 file changed, 26 insertions(+), 2 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/5] network: xmlns dnsmasq option passthrough
On Wed, Jul 17, 2019 at 05:38:51PM -0400, Cole Robinson wrote: > On 7/17/19 12:49 PM, Laine Stump wrote: > > On 7/14/19 8:03 PM, Cole Robinson wrote: > >> There's several unresolved RFEs for the bridge > >> driver that are essentially requests to add XML wrappers > >> for underlying dnsmasq options. > >> > >> This series adds a dnsmasq xmlns namespace for > >> XML that allows passing option strings directly to the > >> generated dnsmasq config file. It will allow motivated > >> users to work around libvirt until those types of RFEs > >> are properly implemented. > > > > > > This all looks like a reasonable facsimile of the qemu:commandline > > stuff, and it worked in my tests (including not creating any problems > > with all the existing networks I have on my test system). > > > > > > My one reservation would be that it will tend to discourage adding > > *supported* methods of configuring things that people will instead use > > this backdoor to implement. But on the other hand, it makes it possible > > to do new things without needing to wait for an officially supported > > method (which could take a very long time, or simply never happen, as > > we've seen). > > > > > > (Actually I for some reason thought we already *had* support for the > > specific example you used - CNAME records. I guess I had lumped it in > > with SRV and TXT records in my memory. It really should be > > straightforward to do, and should still be done, but that shouldn't stop > > this patch series from going in). > > > > > > This is for the entire series: > > > > > > Reviewed-by: Laine Stump > > Thanks, I've pushed this now. > > Regarding the bigger point, I think it's worth considering whether we > should aim to expose every requested dnsmasq option as official XML or > not. We effectively have one network driver; there's an impl for vbox > and esx but they seem very basic. It doesn't look like we are going to > have another backend impl any time soon. I wouldn't rule it out. I can't remember where it was, but a few months ago I had a discussion with some folks precisely about replacing dnsmasq with new daemon(s). Primarily the purpose would getting a more secure solution modern solution written in a memory safe language. > Unless the requested option has some specific reason to be represented > in libvirt XML, like if another part of libvirt may need/want to > programmatically act on that data for some reason, maybe it's okay to > say that dnsmasq passthrough is the solution. Some examples might be > > * dhcp-option: https://bugzilla.redhat.com/show_bug.cgi?id=666556 > * auth-zone: https://bugzilla.redhat.com/show_bug.cgi?id=1690943 > * This bug has a lot of mentioned options: > https://bugzilla.redhat.com/show_bug.cgi?id=824573 Most of the stuff across these bugs is not really dnsmasq specific. It would be applicable to any DNS/DHCP service, so its just a matter of expressing it sensibly in the XML, so you're not tied to dnsmasq specific syntax. 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/1] Revert "Revert "qemu: Temporary disable owner remembering""
On 7/18/19 8:46 AM, Michal Privoznik wrote:
On 7/17/19 7:20 PM, Daniel Henrique Barboza wrote:
After this commit, QEMU domains with PCI hostdevs running with
managed=true started to fail to launch with the following error:
error : virProcessRunInFork:1170 : internal error: child reported
(status=125): unable to open /dev/vfio/1: Device or resource busy
One way to avoid this issue is to disable this new feature
in qemu.conf, setting remember_owner=0. However, given that
this feature is enabled by default and it is breaking domains that
were running before it, it is best to revert the change until
it is fixed for this scenario as well.
Well, ideally, we want the feature to be turned on by default, just like
namespaces for instance. I've temporarily disabled the feature back in
the day because we were close to release and it turned out the feature
was not ready. But now there is still plenty of time to fix it.
Anyway, I'll investigate. Meanwhile, can you share your
config or even better the full domain definition please?
Okay, so I think I know what is going on. You have two -s in
your domain and both of them belong to the same IOMMU group, right?
If that is the case, then the same path appears twice in the list of
paths passed to virSecurityManagerMetadataLock(). For instance, in my case:
Thread 2.1 "libvirtd" hit Breakpoint 3, virSecurityManagerMetadataLock
(mgr=0x7f365c026660, paths=0x7f36a001c1e0, npaths=6) at
security/security_manager.c:1283
1283{
virSecurityManagerMetadataLock 1 # p *paths@npaths
$6 = {0x7f36a0027720 "/var/lib/libvirt/images/fedora.qcow2",
0x7f36a001a660 "/var/lib/libvirt/images/fd.img",
0x7f36a001c470
"/dev/disk/by-path/ip-10.37.132.232:3260-iscsi-iqn.2017-03.com.mprivozn:server-lun-1",
0x7f36a00262b0 "/dev/vfio/9",
0x7f36a0025e20 "/dev/vfio/9",
0x7f36a001d880
"/var/lib/libvirt/qemu/channel/target/domain-1-fedora/org.qemu.guest_agent.0"}
The "/dev/vfio/9" path is there twice because I have this graphics card
that has two functions and I'm passing them both into the domain. The
problem here is that when virSecurityManagerMetadataLock() gets to first
/dev/vfio/9 path, it opens it and locks it. Then it wants to process the
next path (which is the same path), but it fails open()-ing the path,
because it's locked. Honestly, I don't know if that is expected
behaviour, but even it if wasn't then we would fail to lock the path
second time. I'm proposing a fix in no time.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH v2 03/20] quayadmin: Tweak quiet logic
On Wed, 2019-07-17 at 18:14 +0100, Daniel P. Berrangé wrote: > On Wed, Jul 17, 2019 at 03:49:12PM +0200, Andrea Bolognani wrote: > > If we've been asked not to produce any output, we can bail > > early: doing so means we don't need to increase indentation > > for subsequent code, and in some cases we can even avoid > > fetching the JSON data from the response object. > > Unless I'm mis-reading the last point doesn't seem to affect > this patch - we're still fetching JSON, which is good I think, > as it means we check the response is well formed, and not an > error of some kind We skip calling res.json() when all we need the JSON for is some data to be displayed and we're in quiet mode. All the usual checks on the return code still happens. I assume res.json() needs to do some processing when it's called, and now we can skip that. Either way that's a secondary concern, it was mostly about the indentation :) Thanks for the review, and for getting the script started in the first place! The entire series has been pushed now. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/1] Revert "Revert "qemu: Temporary disable owner remembering""
On 7/17/19 7:20 PM, Daniel Henrique Barboza wrote: After this commit, QEMU domains with PCI hostdevs running with managed=true started to fail to launch with the following error: error : virProcessRunInFork:1170 : internal error: child reported (status=125): unable to open /dev/vfio/1: Device or resource busy One way to avoid this issue is to disable this new feature in qemu.conf, setting remember_owner=0. However, given that this feature is enabled by default and it is breaking domains that were running before it, it is best to revert the change until it is fixed for this scenario as well. Well, ideally, we want the feature to be turned on by default, just like namespaces for instance. I've temporarily disabled the feature back in the day because we were close to release and it turned out the feature was not ready. But now there is still plenty of time to fix it. Anyway, I'll investigate. Meanwhile, can you share your config or even better the full domain definition please? Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
