On Tue, Dec 10, 2019 at 17:25:40 +0100, Pavel Mores wrote:
> Since blockcommit is asynchronous, libvirtd can be restarted while the
> operation runs. To ensure the information necessary to finish up the job
> is not lost, serialisation to and deserialisation from the status XML is
> added.
>
> To unittest this, the new element was only added to the active commit test,
> the non-active commit test doesn't have the new element so as to test its
> absence.
>
> Signed-off-by: Pavel Mores
> ---
> src/qemu/qemu_domain.c | 4
> tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 +
> 2 files changed, 5 insertions(+)
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 17:25:39 +0100, Pavel Mores wrote:
> When blockcommit finishes successfully, one of the
> qemuBlockJobProcessEventCompletedCommit() and
> qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
> This is where the delete flag (stored in qemuBlockJobCommitData since the
> previous commit) can actually be used to delete the committed snapshot
> images if requested.
>
> We use virFileRemove() instead of a simple unlink() to cover the case where
> the image to be removed is on an NFS volume.
>
> Signed-off-by: Pavel Mores
> ---
> src/qemu/qemu_blockjob.c | 46
> 1 file changed, 46 insertions(+)
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 17:25:38 +0100, Pavel Mores wrote:
> Propagate the delete flag from qemuDomainBlockCommit() (which was just
> ignoring it until now) to qemuBlockJobDiskNewCommit() where it can be
> stored in the qemuBlockJobCommitData structure which holds information
> necessary to finish the job asynchronously.
>
> In the actual qemuBlockJobDiskNewCommit() in this commit, we temporarily
> pass a literal 'false' to preserve the current behaviour until the whole
> implementation of the feature is in place.
>
> Signed-off-by: Pavel Mores
> ---
> src/qemu/qemu_blockjob.c | 4 +++-
> src/qemu/qemu_blockjob.h | 4 +++-
> src/qemu/qemu_driver.c | 2 +-
> 3 files changed, 7 insertions(+), 3 deletions(-)
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 19:45:01 -0300, Daniel Henrique Barboza wrote:
>
>
> On 12/10/19 5:00 AM, Peter Krempa wrote:
> > On Mon, Dec 09, 2019 at 20:15:05 -0300, Daniel Henrique Barboza wrote:
> > > (series based on master commit 97cafa610ecf5)
> > >
> > > This work was proposed by Cole in [1]. This is Cole's reasoning for
> > > it, copy/pasted from [1]:
> >
> > Nice work. Doing this was long overdue. My only suggestion is that after
> > this we should move all validation into a separate file. qemu_domain was
> > a code dumping place for a long time and since we now have a lot of
> > common code moving it out would be benficial for cleaning up an making
> > it more obvious.
>
>
> Got it. We can create this new file and move the validations from
> qemu_domain.c
> to the new file, before resuming moving more code out of qemu_command.c.
> That way
> we avoid moving the code twice.
Since you've got patches already it's okay to do it in two steps. It
will be much appreciated if you move it to a new file, but it's not
required for this series.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Dec 11, 2019 at 12:38:05AM +0800, Alex Williamson wrote:
> On Tue, 10 Dec 2019 02:44:44 -0500
> Yan Zhao wrote:
>
> > On Tue, Dec 10, 2019 at 05:16:08AM +0800, Alex Williamson wrote:
> > > On Mon, 9 Dec 2019 01:22:12 -0500
> > > Yan Zhao wrote:
> > >
> > > > On Fri, Dec 06, 2019 at 11:20:38PM +0800, Alex Williamson wrote:
> > > > > On Fri, 6 Dec 2019 01:04:07 -0500
> > > > > Yan Zhao wrote:
> > > > >
> > > > > > On Fri, Dec 06, 2019 at 07:55:30AM +0800, Alex Williamson wrote:
> > > > > > > On Wed, 4 Dec 2019 22:26:50 -0500
> > > > > > > Yan Zhao wrote:
> > > > > > >
> > > > > > > > Dynamic trap bar info region is a channel for QEMU and vendor
> > > > > > > > driver to
> > > > > > > > communicate dynamic trap info. It is of type
> > > > > > > > VFIO_REGION_TYPE_DYNAMIC_TRAP_BAR_INFO and subtype
> > > > > > > > VFIO_REGION_SUBTYPE_DYNAMIC_TRAP_BAR_INFO.
> > > > > > > >
> > > > > > > > This region has two fields: dt_fd and trap.
> > > > > > > > When QEMU detects a device regions of this type, it will create
> > > > > > > > an
> > > > > > > > eventfd and write its eventfd id to dt_fd field.
> > > > > > > > When vendor drivre signals this eventfd, QEMU reads trap field
> > > > > > > > of this
> > > > > > > > info region.
> > > > > > > > - If trap is true, QEMU would search the device's PCI BAR
> > > > > > > > regions and disable all the sparse mmaped subregions (if the
> > > > > > > > sparse
> > > > > > > > mmaped subregion is disablable).
> > > > > > > > - If trap is false, QEMU would re-enable those subregions.
> > > > > > > >
> > > > > > > > A typical usage is
> > > > > > > > 1. vendor driver first cuts its bar 0 into several sections,
> > > > > > > > all in a
> > > > > > > > sparse mmap array. So initally, all its bar 0 are passthroughed.
> > > > > > > > 2. vendor driver specifys part of bar 0 sections to be
> > > > > > > > disablable.
> > > > > > > > 3. on migration starts, vendor driver signals dt_fd and set
> > > > > > > > trap to true
> > > > > > > > to notify QEMU disabling the bar 0 sections of disablable flags
> > > > > > > > on.
> > > > > > > > 4. QEMU disables those bar 0 section and hence let vendor
> > > > > > > > driver be able
> > > > > > > > to trap access of bar 0 registers and make dirty page tracking
> > > > > > > > possible.
> > > > > > > > 5. on migration failure, vendor driver signals dt_fd to QEMU
> > > > > > > > again.
> > > > > > > > QEMU reads trap field of this info region which is false and
> > > > > > > > QEMU
> > > > > > > > re-passthrough the whole bar 0 region.
> > > > > > > >
> > > > > > > > Vendor driver specifies whether it supports
> > > > > > > > dynamic-trap-bar-info region
> > > > > > > > through cap VFIO_PCI_DEVICE_CAP_DYNAMIC_TRAP_BAR in
> > > > > > > > vfio_pci_mediate_ops->open().
> > > > > > > >
> > > > > > > > If vfio-pci detects this cap, it will create a default
> > > > > > > > dynamic_trap_bar_info region on behalf of vendor driver with
> > > > > > > > region len=0
> > > > > > > > and region->ops=null.
> > > > > > > > Vvendor driver should override this region's len, flags, rw,
> > > > > > > > mmap in its
> > > > > > > > vfio_pci_mediate_ops.
> > > > > > >
> > > > > > > TBH, I don't like this interface at all. Userspace doesn't pass
> > > > > > > data
> > > > > > > to the kernel via INFO ioctls. We have a SET_IRQS ioctl for
> > > > > > > configuring user signaling with eventfds. I think we only need to
> > > > > > > define an IRQ type that tells the user to re-evaluate the sparse
> > > > > > > mmap
> > > > > > > information for a region. The user would enumerate the device
> > > > > > > IRQs via
> > > > > > > GET_IRQ_INFO, find one of this type where the IRQ info would also
> > > > > > > indicate which region(s) should be re-evaluated on signaling.
> > > > > > > The user
> > > > > > > would enable that signaling via SET_IRQS and simply re-evaluate
> > > > > > > the
> > > > > > ok. I'll try to switch to this way. Thanks for this suggestion.
> > > > > >
> > > > > > > sparse mmap capability for the associated regions when signaled.
> > > > > > >
> > > > > >
> > > > > > Do you like the "disablable" flag of sparse mmap ?
> > > > > > I think it's a lightweight way for user to switch mmap state of a
> > > > > > whole region,
> > > > > > otherwise going through a complete flow of GET_REGION_INFO and
> > > > > > re-setup
> > > > > > region might be too heavy.
> > > > >
> > > > > No, I don't like the disable-able flag. At what frequency do we
> > > > > expect
> > > > > regions to change? It seems like we'd only change when switching into
> > > > > and out of the _SAVING state, which is rare. It seems easy for
> > > > > userspace, at least QEMU, to drop the entire mmap configuration and
> > > > >
> > > > ok. I'll try this way.
> > > >
> > > > > re-read it. Another concern here is how do we synchronize the event?
> > > > > Are we assuming that this event would occur
On Wed, Dec 11, 2019 at 12:58:24AM +0800, Alex Williamson wrote:
> On Mon, 9 Dec 2019 21:44:23 -0500
> Yan Zhao wrote:
>
> > > > > > Currently, yes, i40e has build dependency on vfio-pci.
> > > > > > It's like this, if i40e decides to support SRIOV and compiles in vf
> > > > > > related code who depends on vfio-pci, it will also have build
> > > > > > dependency
> > > > > > on vfio-pci. isn't it natural?
> > > > >
> > > > > No, this is not natural. There are certainly i40e VF use cases that
> > > > > have no interest in vfio and having dependencies between the two
> > > > > modules is unacceptable. I think you probably want to modularize the
> > > > > i40e vfio support code and then perhaps register a table in vfio-pci
> > > > > that the vfio-pci code can perform a module request when using a
> > > > > compatible device. Just and idea, there might be better options. I
> > > > > will not accept a solution that requires unloading the i40e driver in
> > > > > order to unload the vfio-pci driver. It's inconvenient with just one
> > > > > NIC driver, imagine how poorly that scales.
> > > > >
> > > > what about this way:
> > > > mediate driver registers a module notifier and every time when
> > > > vfio_pci is loaded, register to vfio_pci its mediate ops?
> > > > (Just like in below sample code)
> > > > This way vfio-pci is free to unload and this registering only gives
> > > > vfio-pci a name of what module to request.
> > > > After that,
> > > > in vfio_pci_open(), vfio-pci requests the mediate driver. (or puts
> > > > the mediate driver when mediate driver does not support mediating the
> > > > device)
> > > > in vfio_pci_release(), vfio-pci puts the mediate driver.
> > > >
> > > > static void register_mediate_ops(void)
> > > > {
> > > > int (*func)(struct vfio_pci_mediate_ops *ops) = NULL;
> > > >
> > > > func = symbol_get(vfio_pci_register_mediate_ops);
> > > >
> > > > if (func) {
> > > > func(&igd_dt_ops);
> > > > symbol_put(vfio_pci_register_mediate_ops);
> > > > }
> > > > }
> > > >
> > > > static int igd_module_notify(struct notifier_block *self,
> > > > unsigned long val, void *data)
> > > > {
> > > > struct module *mod = data;
> > > > int ret = 0;
> > > >
> > > > switch (val) {
> > > > case MODULE_STATE_LIVE:
> > > > if (!strcmp(mod->name, "vfio_pci"))
> > > > register_mediate_ops();
> > > > break;
> > > > case MODULE_STATE_GOING:
> > > > break;
> > > > default:
> > > > break;
> > > > }
> > > > return ret;
> > > > }
> > > >
> > > > static struct notifier_block igd_module_nb = {
> > > > .notifier_call = igd_module_notify,
> > > > .priority = 0,
> > > > };
> > > >
> > > >
> > > >
> > > > static int __init igd_dt_init(void)
> > > > {
> > > > ...
> > > > register_mediate_ops();
> > > > register_module_notifier(&igd_module_nb);
> > > > ...
> > > > return 0;
> > > > }
> > >
> > >
> > > No, this is bad. Please look at MODULE_ALIAS() and request_module() as
> > > used in the vfio-platform for loading reset driver modules. I think
> > > the correct approach is that vfio-pci should perform a request_module()
> > > based on the device being probed. Having the mediation provider
> > > listening for vfio-pci and registering itself regardless of whether we
> > > intend to use it assumes that we will want to use it and assumes that
> > > the mediation provider module is already loaded. We should be able to
> > > support demand loading of modules that may serve no other purpose than
> > > providing this mediation. Thanks,
> > hi Alex
> > Thanks for this message.
> > So is it good to create a separate module as mediation provider driver,
> > and alias its module name to "vfio-pci-mediate-vid-did".
> > Then when vfio-pci probes the device, it requests module of that name ?
>
> I think this would give us an option to have the mediator as a separate
> module, but not require it. Maybe rather than a request_module(),
> where if we follow the platform reset example we'd then expect the init
> code for the module to register into a list, we could do a
> symbol_request(). AIUI, this would give us a reference to the symbol
> if the module providing it is already loaded, and request a module
> (perhaps via an alias) if it's not already load. Thanks,
>
ok. got it!
Thank you :)
Yan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/6/19 9:50 AM, Daniel P. Berrangé wrote:
> As part of the goal to eliminate Perl from libvirt, this gets rid of the
> use of POD format for man pages. There's nothing especially bad about
> POD as a markup language compared to other lightweight markup languages
> like RST or Markdown. It hasn't found widespread usage outside of the
> Perl world though, and so switching from POD to RST brings a language
> which likely has more familiarity to contributors.
>
> This also nicely aligns with our use of RST of web pages, and indeed
> in this series things are setup so that all the man pages get published
> on the main libvirt.org website. Over time this will hopefuly draw
> search engines traffic to libvirt.org instead of random 3rd party
> websites hosting various out of date copies of the man pages.
>
> Reviewing the individual RST files is likely a very unpleasant task,
> especially the enourmous virsh man page. Most of the conversion work
> was automated with pod2rst, followed by lots of editting to cleanup
> its output. virsh had some further automated processing done to create
> headers for each command.
>
> It is probably more useful to review the rendered man page output
> and/or websites to see that it looks sane when read.
>
Reviewed-by: Cole Robinson
With some tweaks:
* There's a leftover pod2man in the spec file.
* This conflicts with the virsh --tls-destination changes so don't
forget to re-merge those
* virt-host-validate patch needs this diff
diff --git a/docs/Makefile.am b/docs/Makefile.am
index e1f8f7646d..4027c2e26c 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -236,7 +236,7 @@ manpages_rst += \
$(NULL)
endif ! WITH_LIBVIRTD
if WITH_HOST_VALIDATE
- manpages8_rst += manpages/virt-host-validate.rst
+ manpages1_rst += manpages/virt-host-validate.rst
else ! WITH_HOST_VALIDATE
manpages_rst += manpages/virt-host-validate.rst
endif ! WITH_HOST_VALIDATE
Non-blocking stuff:
The build process spits out noise like this now, but I didn't
investigate, maybe it was there before and I missed it:
I/O error : Attempt to load network entity
http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd
manpages/virkeycode-osx.html.in:2: warning: failed to load external
entity "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";
1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";
As for the format, there's some improvements and some worse things, most
are minor. The one place it's pretty ugly is virt-admin and virsh, with
the command format immediately after the command header. Old style looks
like:
quit, exit
quit this interactive terminal
Now it is:
quit, exit
quit
exit
quit this interactive terminal
For larger commands it's better, so it's mostly noticeable at the start
of the document with the short commands. Maybe drop the shell section
entirely for short style commands, or maybe there's some other option, I
didn't look into it much
The other bit is nested Example: sections with ~ underline. It puts the
Example: at the same indent as the top level command, which is ugly and
tough to read. Just grep the rst for '' and see how it manifests in
the output manpage. I think those few sections can be replaced by
boldified text with *Example:* and it looks better, at least with the
man page, but I didn't review the HTML.
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 5:26 AM, Daniel P. Berrangé wrote:
> This reverts commit 7be5fe66cd024b9ffba7c52cdbf5effedeac2c0d.
>
> This commit broke resctrl, because it missed the fact that the
> virResctrlInfoGetCache() has side-effects causing it to actually
> change the virResctrlInfo parameter, not merely get data from
> it.
>
> This code will need some refactoring before we can try separating
> it from virCapabilities again.
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 5:00 AM, Peter Krempa wrote:
On Mon, Dec 09, 2019 at 20:15:05 -0300, Daniel Henrique Barboza wrote:
(series based on master commit 97cafa610ecf5)
This work was proposed by Cole in [1]. This is Cole's reasoning for
it, copy/pasted from [1]:
Nice work. Doing this was long overdue. My only suggestion is that after
this we should move all validation into a separate file. qemu_domain was
a code dumping place for a long time and since we now have a lot of
common code moving it out would be benficial for cleaning up an making
it more obvious.
Got it. We can create this new file and move the validations from qemu_domain.c
to the new file, before resuming moving more code out of qemu_command.c. That
way
we avoid moving the code twice.
Thanks,
DHB
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 6:41 PM, Cole Robinson wrote:
On 12/9/19 6:15 PM, Daniel Henrique Barboza wrote:
(series based on master commit 97cafa610ecf5)
[...]
The hotplug code should already be calling into Validate code. When a
device is hotplugged via qemu_driver, we get:
qemu_driver.c
-> qemuDomainAttachDeviceFlags
-> qemuDomainAttachDeviceLiveAndConfig
-> virDomainDeviceDefParse
-> virDomainDeviceDefValidate
-> deviceValidateCallback
-> qemuDomainDeviceDefValidate
So if device validation is moved into the correct
qemuDomainDeviceDefValidateXXX function it will get called in the
hotplug path so they won't be lost.
Good to know. I assumed that the hotplug path was separated.
Perhaps this info can be put somewhere in the docs folder as reference. This
appears to be the kind of call hierarchy that doesn't change that often, so
it wouldn't be a burden to keep the doc updated.
- moving CPU Model validation is trickier than the rest because
there are code in DefPostParse() that makes CPU Model changes that
are then validated in qemu_command.c. Moving the validation to define
time doesn't cut in this case - the validation is considering
PostParse changes in the CPU Model and some of the will fail if
done by qemuDomainDefValidate time. I didn't think too much about
how to handle this case, but given that the approach would be
different from the other cases handled here, I left it out too.
Hmm I glanced at the qemuBuildCpuModelArgStr checks but it doesn't
strike me why those are an issue. Validate should always be triggered
after PostParse, so the ordering of those two shouldn't impact things
here. But I didn't attempt the change
Unfortunately I've erased the error I was seeing in this case. I recall
something to do with VIR_CPU_MODE_HOST_MODEL being set in
qemuDomainDefSetDefaultCPU(), but TBH it's easier to just move the code again
and see what happens.
I suspect a bug on your side. qemu_command.c has:
I'll attempt that in the next iterations of this work.
Thanks,
Daniel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/9/19 6:15 PM, Daniel Henrique Barboza wrote:
> (series based on master commit 97cafa610ecf5)
>
> This work was proposed by Cole in [1]. This is Cole's reasoning for
> it, copy/pasted from [1]:
>
> ---
> The benefits of moving to validate time is that XML is rejected by
> 'virsh define' rather than at 'virsh start' time. It also makes it easier
> to follow the cli building code, and makes it easier to verify qemu_command.c
> test suite code coverage for the important stuff like covering every CLI
> option. It's also a good intermediate step for sharing validation with
> domain capabilities building, like is done presently for rng models.
> ---
>
> Cole also mentioned that the machine features validation was a good
> place to start. I took it a step further and did it across the
> board on all qemu_command.c.
>
> This series didn't create any new validation, just moved them
> to domain define time. Any other outcome is unintended.
>
>
> Not all cases where covered in this work. For a first version
> I decided to do only the most trivial cases. These are the
> other cases I left out for another day:
>
Yes I think that's the right approach, handle the easy bits first and
deal the complicated bits in their own series. Some cases will likely
cause a lot of test suite churn, some cases may not even be worth it, we
will see!
> - all verifications contained in functions that are also called
> by qemu_hotplug.c. This means that the hotplug process will also
> use the validation code, and we can't just move it to qemu_domain.c.
> Besides, not all validation done in domain define time applies
> to hotplug, so it's not simply a matter of calling the same function
> from qemu_domain in qemu_hotplug. I have patches that moved the
> verification of all virtio options to qemu_domain.c, while also
> considering qemu_hotplug validation. In the end I decided to leave
> it away from this work for now because (1) it took 8 patches just
> for virtio case alone and (2) there are a lot of these cases in
> qemu_command.c and it would be too much to do it all at in this
> same series.
>
The hotplug code should already be calling into Validate code. When a
device is hotplugged via qemu_driver, we get:
qemu_driver.c
-> qemuDomainAttachDeviceFlags
-> qemuDomainAttachDeviceLiveAndConfig
-> virDomainDeviceDefParse
-> virDomainDeviceDefValidate
-> deviceValidateCallback
-> qemuDomainDeviceDefValidate
So if device validation is moved into the correct
qemuDomainDeviceDefValidateXXX function it will get called in the
hotplug path so they won't be lost.
> - moving CPU Model validation is trickier than the rest because
> there are code in DefPostParse() that makes CPU Model changes that
> are then validated in qemu_command.c. Moving the validation to define
> time doesn't cut in this case - the validation is considering
> PostParse changes in the CPU Model and some of the will fail if
> done by qemuDomainDefValidate time. I didn't think too much about
> how to handle this case, but given that the approach would be
> different from the other cases handled here, I left it out too.
>
Hmm I glanced at the qemuBuildCpuModelArgStr checks but it doesn't
strike me why those are an issue. Validate should always be triggered
after PostParse, so the ordering of those two shouldn't impact things
here. But I didn't attempt the change
> - SHMem: part of SHMem validation is being used by hotplug code.
> I could've moved the non-hotplug validation to define time,
> instead I decided it's better to to leave it to do it all at
> once in the future.
>
shmem is just another device, so it should be fine as long as its final
location is triggered by qemuDomainDeviceDefValidate
> - DBus-VMState: validation of this fella must be done by first
> querying if the hash vm->priv->dbusVMState has elements.
> qemuDomainDefValidate does not have 'vm->priv' in it's API,
> meaning that I would need to either change the API to include
> it (which means changing domainValidateCallback), or do the
> validation by another branch where I can get access to vm->priv.
>
Yeah this one is a little convoluted. The code is using the vm->priv
check to determine 'the user requested external slirp', which is really
a factor of interface type='user' + a qemu.conf setting, which we _can_
check at validate time against qemuCaps. But it's minor so I suggest
just ignoring it
> - vmcoreinfo: there are no challenges in moving vmcoreinfo
> validation to qemuDomainDefValidate. The problem were the
> unit tests. Moving this validation to domain define time
> breaks 925 tests on qemuxml2xmltest.c, including tests labelled
> as 'minimal' which should pass under any circurstances, as
> far as I understand. It is possible to just shoehorn
> QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but
> this would tamper the idea of having to deal with NONE or
> LATEST or a specific set of capabilities for certain tests.
> Another c
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> This is slightly more complicated because NVMe disk source is not
> a simple attribute to element. The format in which the
> PCI address and namespace ID are printed is the same as QEMU
> accepts them:
>
> nvme://:XX:XX.X/X
>
> Signed-off-by: Michal Privoznik
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> With NVMe disks, one can start a blockjob with a NVMe disk
> that is not visible in domain XML (at least right away). Usually,
> it's fairly easy to override this limitation of
> qemuDomainGetMemLockLimitBytes() - for instance for hostdevs we
> temporarily add the device to domain def, let the function
> calculate the limit and then remove the device. But it's not so
> easy with virStorageSourcePtr - in some cases they don't
> necessarily are attached to a disk. And even if they are it's
> done later in the process and frankly, I find it too complicated
> to be able to use the simple trick we use with hostdevs.
>
> Signed-off-by: Michal Privoznik
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> At the very beginning of the attach function the
> qemuDomainStorageSourceChainAccessAllow() is called which
> modifies CGroups, locks and seclabels for new disk and its
> backing chain. This must be followed by a counterpart which
> reverts back all the changes if something goes wrong. This boils
> down to calling qemuDomainStorageSourceChainAccessRevoke() which
> is done under 'error' label. But not all failure branches jump
> there. They just jump onto 'cleanup' label where no revoke is
> done. Such mistake is easy to do because 'cleanup' label does
> exist. Therefore, dissolve 'error' block in 'cleanup' and have
> everything jump onto 'cleanup' label.
>
> Signed-off-by: Michal Privoznik
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> Because this is a HMP we're dealing with, there is nothing like
> class of reply message, so we have to do some string comparison
> to guess if the command fails. Well, with NVMe disks whole new
> class of errors comes to play because qemu needs to initialize
> IOMMU and VFIO for them. You can see all the messages it may
> produce in qemu_vfio_init_pci().
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_monitor_text.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 9054682d60..6948a5bf90 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -75,6 +75,13 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
> goto cleanup;
> }
>
> +if (strstr(reply, "IOMMU") ||
> +strstr(reply, "VFIO")) {
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + reply);
> +goto cleanup;
> +}
> +
> ret = 0;
>
> cleanup:
>
For the code:
Reviewed-by: Cole Robinson
Does the blockdev infrastructure have magic that turns the Props into a
drive string? I don't see any -drive examples in the test output
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> This capability tracks if qemu is capable of:
>
> -drive file.driver=nvme
>
> The feature was added in QEMU's commit of v2.12.0-rc0~104^2~2.
>
> Signed-off-by: Michal Privoznik
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> This function is currently not called for any type of storage
> source that is not considered 'local' (as defined by
> virStorageSourceIsLocalStorage()). Well, NVMe disks are not
> 'local' from that point of view and therefore we will need to
> call this function more frequently.
>
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_apparmor.c | 25
> src/security/security_dac.c | 30 +++
> src/security/security_selinux.c | 51 +++-
> 3 files changed, 92 insertions(+), 14 deletions(-)
>
> diff --git a/src/security/security_apparmor.c
> b/src/security/security_apparmor.c
> index 21560b2330..7cea788931 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -779,9 +779,8 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
>virSecurityDomainImageLabelFlags flags
> G_GNUC_UNUSED)
> {
> virSecurityLabelDefPtr secdef;
> -
> -if (!src->path || !virStorageSourceIsLocalStorage(src))
> -return 0;
> +g_autofree char *vfioGroupDev = NULL;
> +const char *path;
>
> secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> if (!secdef || !secdef->relabel)
> @@ -790,15 +789,29 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (!secdef->imagelabel)
> return 0;
>
> +if (src->type == VIR_STORAGE_TYPE_NVME) {
> +const virStorageSourceNVMeDef *nvme = src->nvme;
> +
> +if (!(vfioGroupDev =
> virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +return -1;
> +
> +path = vfioGroupDev;
> +} else {
> +if (!src->path || !virStorageSourceIsLocalStorage(src))
> +return 0;
> +
> +path = src->path;
> +}
> +
> /* if the device doesn't exist, error out */
> -if (!virFileExists(src->path)) {
> +if (!virFileExists(path)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("\'%s\' does not exist"),
> - src->path);
> + path);
> return -1;
> }
>
> -return reload_profile(mgr, def, src->path, true);
> +return reload_profile(mgr, def, path, true);
> }
>
> static int
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index a9a1fad5d7..411aa1b159 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -911,6 +911,19 @@
> virSecurityDACSetImageLabelInternal(virSecurityManagerPtr mgr,
> return -1;
> }
>
> +/* This is not very clean. But so far we don't have NVMe
> + * storage pool backend so that its chownCallback would be
> + * called. And this place looks least offensive. */
> +if (src->type == VIR_STORAGE_TYPE_NVME) {
> +const virStorageSourceNVMeDef *nvme = src->nvme;
> +g_autofree char *vfioGroupDev = NULL;
> +
> +if (!(vfioGroupDev =
> virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +return -1;
> +
> +return virSecurityDACSetOwnership(mgr, NULL, vfioGroupDev, user,
> group, false);
> +}
> +
> /* We can't do restore on shared resources safely. Not even
> * with refcounting implemented in XATTRs because if there
> * was a domain running with the feature turned off the
> @@ -1017,6 +1030,23 @@
> virSecurityDACRestoreImageLabelSingle(virSecurityManagerPtr mgr,
> }
> }
>
> +/* This is not very clean. But so far we don't have NVMe
> + * storage pool backend so that its chownCallback would be
> + * called. And this place looks least offensive. */
> +if (src->type == VIR_STORAGE_TYPE_NVME) {
> +const virStorageSourceNVMeDef *nvme = src->nvme;
> +g_autofree char *vfioGroupDev = NULL;
> +
> +if (!(vfioGroupDev =
> virPCIDeviceAddressGetIOMMUGroupDev(&nvme->pciAddr)))
> +return -1;
> +
> +/* Ideally, we would check if there is not another PCI
> + * device within domain def that is in the same IOMMU
> + * group. But we're not doing that for hostdevs yet. */
> +
> +return virSecurityDACRestoreFileLabelInternal(mgr, NULL,
> vfioGroupDev, false);
> +}
> +
> return virSecurityDACRestoreFileLabelInternal(mgr, src, NULL, true);
> }
>
I think my suggested helpers will simplify selinux and apparmor a bit.
Not sure about DAC but I don't fully understand the chownCallback +
transaction stuff.
Looks mechanically sound AFAICT
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> If a domain has an NVMe disk configured, then we need to allow it
> on devices CGroup so that qemu can access it. There is one caveat
> though - if an NVMe disk is read only we need CGroup to allow
> write too. This is because when opening the device, qemu does
> couple of ioctl()-s which are considered as write.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_cgroup.c | 97 ++
> 1 file changed, 69 insertions(+), 28 deletions(-)
A lot going on here, a bit tough to digest, the extra helpers I
suggested should help here. But I didn't notice anything bad
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> If a domain has an NVMe disk configured, then we need to create
> /dev/vfio/* paths in domain's namespace so that qemu can open
> them.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c | 67 --
> 1 file changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7b515b9520..70c4ee8e65 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -13430,16 +13430,29 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg
> G_GNUC_UNUSED,
> {
> virStorageSourcePtr next;
> char *dst = NULL;
> +bool hasNVMe = false;
> int ret = -1;
>
> for (next = disk->src; virStorageSourceIsBacking(next); next =
> next->backingStore) {
> -if (!next->path || !virStorageSourceIsLocalStorage(next)) {
> -/* Not creating device. Just continue. */
> -continue;
> -}
> +if (next->type == VIR_STORAGE_TYPE_NVME) {
> +g_autofree char *nvmePath = NULL;
>
> -if (qemuDomainCreateDevice(next->path, data, false) < 0)
> -goto cleanup;
> +hasNVMe = true;
> +
> +if (!(nvmePath =
> virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
> +goto cleanup;
> +
> +if (qemuDomainCreateDevice(nvmePath, data, false) < 0)
> +goto cleanup;
> +} else {
> +if (!next->path || !virStorageSourceIsLocalStorage(next)) {
> +/* Not creating device. Just continue. */
> +continue;
> +}
> +
> +if (qemuDomainCreateDevice(next->path, data, false) < 0)
> +goto cleanup;
> +}
> }
>
I don't see anything wrong with this patch. In the interest of getting
this series into git, here is my:
Reviewed-by: Cole Robinson
But, I'm finding a lot of the 'next->type == VIR_STORAGE_TYPE_NVME'
sprinkled around unfortunate, here and in later patches. It seems like
there's two StorageSource cases here involving local filesystem paths:
1) Does src->path contain the storage location? Answered by
virStorageSourceIsLocalStorage
2) Does src have a local path that the VM needs to access? Until now
this was always the same as #1 && src->path, but not any more.
So maybe add helpers like:
bool
virStorageSourceHasLocalResource(virStorageSourcePtr src)
{
return next->path || next->type == VIR_STORAGE_TYPE_NVME;
}
const char *
virStorageSourceGetLocalResource(virStorageSourcePtr src)
{
if (!virStorageSourceHasLocalResource(src))
if (next->type == NVME)
return strdup(next->path);
}
I think virStorageSourceIsLocalStorage should be renamed too because now
it is ambiguous. Maybe something more explicit like SourceUsesPath, or
SourceIsLocalPath. Naming sucks obviously so suggestions welcome
With those funtions, the loop above becomes:
for (...) {
g_autofree char *path = NULL;
if (!HasLocalResource)
continue
if (!(path = GetLocalResource(...))
error
}
The hasNVMe bit can then be replaced with virStorageSourceChainHasNVMe
security driver usage will be simpler too I think. But maybe I'm missing
some complication
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> There are couple of places where a domain with a VFIO device gets
> special treatment: in CGroups when enabling/disabling access to
> /dev/vfio/vfio, and when creating/removing nodes in domain mount
> namespace. Well, a NVMe disk is a VFIO device too. Fortunately,
> we have this qemuDomainNeedsVFIO() function which is the only
> place that needs adjustment.
>
> Signed-off-by: Michal Privoznik
> ---
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> We have this beautiful function that does crystal ball
> divination. The function is named
> qemuDomainGetMemLockLimitBytes() and it calculates the upper
> limit of how much locked memory is given guest going to need. The
> function bases its guess on devices defined for a domain. For
> instance, if there is a VFIO hostdev defined then it adds 1GiB to
> the guessed maximum. Since NVMe disks are pretty much VFIO
> hostdevs (but not quite), we have to do the same sorcery.
>
> Signed-off-by: Michal Privoznik
> ACKed-by: Peter Krempa
> ---
> src/qemu/qemu_domain.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
For the patch as-is
Reviewed-by: Cole Robinson
But...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 59e98de2d2..7b515b9520 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11920,6 +11920,9 @@ getPPC64MemLockLimitBytes(virDomainDefPtr def)
> }
> }
>
> +if (virDomainDefHasNVMeDisk(def))
> +usesVFIO = true;
> +
We have qemuDomainNeedsVFIO which helpully captures these rules. Looks
the logic has already diverged for ppc here because it doesn't consider
IsMdev devices. Something for a future patch
- Cole
> memory = virDomainDefGetMemoryTotal(def);
>
> if (def->mem.max_memory)
> @@ -12018,6 +12021,7 @@ unsigned long long
> qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
> {
> unsigned long long memKB = 0;
> +bool usesVFIO = false;
> size_t i;
>
> /* prefer the hard limit */
> @@ -12058,11 +12062,17 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
> for (i = 0; i < def->nhostdevs; i++) {
> if (virHostdevIsVFIODevice(def->hostdevs[i]) ||
> virHostdevIsMdevDevice(def->hostdevs[i])) {
> -memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024;
> -goto done;
> +usesVFIO = true;
> +break;
> }
> }
>
> +if (virDomainDefHasNVMeDisk(def))
> +usesVFIO = true;
> +
> +if (usesVFIO)
> +memKB = virDomainDefGetMemoryTotal(def) + 1024 * 1024;
> +
> done:
> return memKB << 10;
> }
>
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/2/19 9:26 AM, Michal Privoznik wrote:
> The qemu driver has its own wrappers around virHostdev module (so
> that some arguments are filled in automatically). Extend these to
> include NVMe devices too.
>
> Signed-off-by: Michal Privoznik
> ACKed-by: Peter Krempa
> ---
> src/qemu/qemu_hostdev.c | 49 ++---
> src/qemu/qemu_hostdev.h | 10 +
> 2 files changed, 56 insertions(+), 3 deletions(-)
Reviewed-by: Cole Robinson
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 11:32:18AM -0500, Cole Robinson wrote:
> On 12/10/19 5:57 AM, Daniel P. Berrangé wrote:
> >
> >> Another issue: other drivers use the machine= setting too, libxl at
> >> least. This appears to drop filling in a default machine type for them,
> >> at least at XML parse time.
> >
> > I thought that was the case too, but I couln't find any functional use
> > of def->os.machine in any of the other drivers:
> >
> > $ git grep os.machine | grep -v -E '(qemu|conf)/'
> > libvirt-domain.c: * "os.machine" - the machine hardware name as a
> > string
> > libxl/libxl_conf.c:def->os.machine);
> > libxl/xen_common.c:def->os.machine = g_strdup(capsdata->machinetype);
> > vbox/vbox_common.c:VIR_DEBUG("def->os.machine %s",
> > def->os.machine);
> > vz/vz_sdk.c:if (def->os.machine != NULL || def->os.bootmenu != 0 ||
> >
> >
> > that libxl_conf.c usage is just an error message
> >
> > The xen_common.c usage is just when parsing XM/XL config files,
> > so output only.
> >
> > The vz_sdk.c usage reports error for any non-NULL machine
> >
> >
> > I guess there is a difference in that if the user does dumpxml for a
> > libxl guest we've no longer filled in the machine. I can fix that,
> > but it doesn't look like it'll have a functional effect.
>
> Check grep for 'xenfv' and 'xenpv', domaincapabilities does some machine
> value handling, and virt-manager will use the reported VM machine value
> and feed it back to domaincapabilities, so it could be used in a round
> about way.
Yes, I can see the dom capabilities reports them, and apps can of
coure put them in the XML they feed in, but AFAICT the libxl driver
will not do anything with the values provided when defining/starting
a guest
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
On Mon, 9 Dec 2019 21:44:23 -0500
Yan Zhao wrote:
> > > > > Currently, yes, i40e has build dependency on vfio-pci.
> > > > > It's like this, if i40e decides to support SRIOV and compiles in vf
> > > > > related code who depends on vfio-pci, it will also have build
> > > > > dependency
> > > > > on vfio-pci. isn't it natural?
> > > >
> > > > No, this is not natural. There are certainly i40e VF use cases that
> > > > have no interest in vfio and having dependencies between the two
> > > > modules is unacceptable. I think you probably want to modularize the
> > > > i40e vfio support code and then perhaps register a table in vfio-pci
> > > > that the vfio-pci code can perform a module request when using a
> > > > compatible device. Just and idea, there might be better options. I
> > > > will not accept a solution that requires unloading the i40e driver in
> > > > order to unload the vfio-pci driver. It's inconvenient with just one
> > > > NIC driver, imagine how poorly that scales.
> > > >
> > > what about this way:
> > > mediate driver registers a module notifier and every time when
> > > vfio_pci is loaded, register to vfio_pci its mediate ops?
> > > (Just like in below sample code)
> > > This way vfio-pci is free to unload and this registering only gives
> > > vfio-pci a name of what module to request.
> > > After that,
> > > in vfio_pci_open(), vfio-pci requests the mediate driver. (or puts
> > > the mediate driver when mediate driver does not support mediating the
> > > device)
> > > in vfio_pci_release(), vfio-pci puts the mediate driver.
> > >
> > > static void register_mediate_ops(void)
> > > {
> > > int (*func)(struct vfio_pci_mediate_ops *ops) = NULL;
> > >
> > > func = symbol_get(vfio_pci_register_mediate_ops);
> > >
> > > if (func) {
> > > func(&igd_dt_ops);
> > > symbol_put(vfio_pci_register_mediate_ops);
> > > }
> > > }
> > >
> > > static int igd_module_notify(struct notifier_block *self,
> > > unsigned long val, void *data)
> > > {
> > > struct module *mod = data;
> > > int ret = 0;
> > >
> > > switch (val) {
> > > case MODULE_STATE_LIVE:
> > > if (!strcmp(mod->name, "vfio_pci"))
> > > register_mediate_ops();
> > > break;
> > > case MODULE_STATE_GOING:
> > > break;
> > > default:
> > > break;
> > > }
> > > return ret;
> > > }
> > >
> > > static struct notifier_block igd_module_nb = {
> > > .notifier_call = igd_module_notify,
> > > .priority = 0,
> > > };
> > >
> > >
> > >
> > > static int __init igd_dt_init(void)
> > > {
> > > ...
> > > register_mediate_ops();
> > > register_module_notifier(&igd_module_nb);
> > > ...
> > > return 0;
> > > }
> >
> >
> > No, this is bad. Please look at MODULE_ALIAS() and request_module() as
> > used in the vfio-platform for loading reset driver modules. I think
> > the correct approach is that vfio-pci should perform a request_module()
> > based on the device being probed. Having the mediation provider
> > listening for vfio-pci and registering itself regardless of whether we
> > intend to use it assumes that we will want to use it and assumes that
> > the mediation provider module is already loaded. We should be able to
> > support demand loading of modules that may serve no other purpose than
> > providing this mediation. Thanks,
> hi Alex
> Thanks for this message.
> So is it good to create a separate module as mediation provider driver,
> and alias its module name to "vfio-pci-mediate-vid-did".
> Then when vfio-pci probes the device, it requests module of that name ?
I think this would give us an option to have the mediator as a separate
module, but not require it. Maybe rather than a request_module(),
where if we follow the platform reset example we'd then expect the init
code for the module to register into a list, we could do a
symbol_request(). AIUI, this would give us a reference to the symbol
if the module providing it is already loaded, and request a module
(perhaps via an alias) if it's not already load. Thanks,
Alex
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/5/19 12:11 PM, Arnaud Patard wrote:
> When emulating smartcard with host certificates, qemu needs to
> be able to read the certificates files. Add necessary code to
> add the smartcard certificates file path to the apparmor profile.
>
> Passthrough support has been tested with spicevmc and remote-viewer.
>
> v2:
> - Fix CodingStyle
> - Add support for 'host' case.
> - Add a comment to mention that the passthrough case doesn't need
> some configuration
> - Use one rule with '{,*}' instead of two rules.
>
> Signed-off-by: Arnaud Patard
> Index: libvirt/src/security/virt-aa-helper.c
> ===
> --- libvirt.orig/src/security/virt-aa-helper.c
> +++ libvirt/src/security/virt-aa-helper.c
> @@ -1271,6 +1271,39 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nsmartcards; i++) {
> +virDomainSmartcardDefPtr sc = ctl->def->smartcards[i];
> +virDomainSmartcardType sc_type = sc->type;
> +char *sc_db = (char *)VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
> +if (sc->data.cert.database)
> +sc_db = sc->data.cert.database;
> +switch (sc_type) {
> +/*
> + * Note: At time of writing, to get this working, qemu seccomp
> sandbox has
> + * to be disabled or the host must be running QEMU with commit
> + * 9a1565a03b79d80b236bc7cc2dbce52a2ef3a1b8.
> + * It's possibly due to libcacard:vcard_emul_new_event_thread(),
> which calls
> + * PR_CreateThread(), which calls {g,s}etpriority(). And
> resourcecontrol seccomp
> + * filter forbids it (cf src/qemu/qemu_command.c which seems to
> always use
> + * resourcecontrol=deny).
> + */
This doesn't seem like the type of thing to track in a permanent code
comment, nor a commit message, but as part of the email discussion.
Otherwise, for the code because I don't have a test setup:
Reviewed-by: Cole Robinson
If apparmor maintainers agree they can strip out of the comment so
doesn't require a repost either way IMO
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 10 Dec 2019 02:44:44 -0500
Yan Zhao wrote:
> On Tue, Dec 10, 2019 at 05:16:08AM +0800, Alex Williamson wrote:
> > On Mon, 9 Dec 2019 01:22:12 -0500
> > Yan Zhao wrote:
> >
> > > On Fri, Dec 06, 2019 at 11:20:38PM +0800, Alex Williamson wrote:
> > > > On Fri, 6 Dec 2019 01:04:07 -0500
> > > > Yan Zhao wrote:
> > > >
> > > > > On Fri, Dec 06, 2019 at 07:55:30AM +0800, Alex Williamson wrote:
> > > > > > On Wed, 4 Dec 2019 22:26:50 -0500
> > > > > > Yan Zhao wrote:
> > > > > >
> > > > > > > Dynamic trap bar info region is a channel for QEMU and vendor
> > > > > > > driver to
> > > > > > > communicate dynamic trap info. It is of type
> > > > > > > VFIO_REGION_TYPE_DYNAMIC_TRAP_BAR_INFO and subtype
> > > > > > > VFIO_REGION_SUBTYPE_DYNAMIC_TRAP_BAR_INFO.
> > > > > > >
> > > > > > > This region has two fields: dt_fd and trap.
> > > > > > > When QEMU detects a device regions of this type, it will create an
> > > > > > > eventfd and write its eventfd id to dt_fd field.
> > > > > > > When vendor drivre signals this eventfd, QEMU reads trap field of
> > > > > > > this
> > > > > > > info region.
> > > > > > > - If trap is true, QEMU would search the device's PCI BAR
> > > > > > > regions and disable all the sparse mmaped subregions (if the
> > > > > > > sparse
> > > > > > > mmaped subregion is disablable).
> > > > > > > - If trap is false, QEMU would re-enable those subregions.
> > > > > > >
> > > > > > > A typical usage is
> > > > > > > 1. vendor driver first cuts its bar 0 into several sections, all
> > > > > > > in a
> > > > > > > sparse mmap array. So initally, all its bar 0 are passthroughed.
> > > > > > > 2. vendor driver specifys part of bar 0 sections to be disablable.
> > > > > > > 3. on migration starts, vendor driver signals dt_fd and set trap
> > > > > > > to true
> > > > > > > to notify QEMU disabling the bar 0 sections of disablable flags
> > > > > > > on.
> > > > > > > 4. QEMU disables those bar 0 section and hence let vendor driver
> > > > > > > be able
> > > > > > > to trap access of bar 0 registers and make dirty page tracking
> > > > > > > possible.
> > > > > > > 5. on migration failure, vendor driver signals dt_fd to QEMU
> > > > > > > again.
> > > > > > > QEMU reads trap field of this info region which is false and QEMU
> > > > > > > re-passthrough the whole bar 0 region.
> > > > > > >
> > > > > > > Vendor driver specifies whether it supports dynamic-trap-bar-info
> > > > > > > region
> > > > > > > through cap VFIO_PCI_DEVICE_CAP_DYNAMIC_TRAP_BAR in
> > > > > > > vfio_pci_mediate_ops->open().
> > > > > > >
> > > > > > > If vfio-pci detects this cap, it will create a default
> > > > > > > dynamic_trap_bar_info region on behalf of vendor driver with
> > > > > > > region len=0
> > > > > > > and region->ops=null.
> > > > > > > Vvendor driver should override this region's len, flags, rw, mmap
> > > > > > > in its
> > > > > > > vfio_pci_mediate_ops.
> > > > > >
> > > > > > TBH, I don't like this interface at all. Userspace doesn't pass
> > > > > > data
> > > > > > to the kernel via INFO ioctls. We have a SET_IRQS ioctl for
> > > > > > configuring user signaling with eventfds. I think we only need to
> > > > > > define an IRQ type that tells the user to re-evaluate the sparse
> > > > > > mmap
> > > > > > information for a region. The user would enumerate the device IRQs
> > > > > > via
> > > > > > GET_IRQ_INFO, find one of this type where the IRQ info would also
> > > > > > indicate which region(s) should be re-evaluated on signaling. The
> > > > > > user
> > > > > > would enable that signaling via SET_IRQS and simply re-evaluate the
> > > > > >
> > > > > ok. I'll try to switch to this way. Thanks for this suggestion.
> > > > >
> > > > > > sparse mmap capability for the associated regions when signaled.
> > > > > >
> > > > >
> > > > > Do you like the "disablable" flag of sparse mmap ?
> > > > > I think it's a lightweight way for user to switch mmap state of a
> > > > > whole region,
> > > > > otherwise going through a complete flow of GET_REGION_INFO and
> > > > > re-setup
> > > > > region might be too heavy.
> > > >
> > > > No, I don't like the disable-able flag. At what frequency do we expect
> > > > regions to change? It seems like we'd only change when switching into
> > > > and out of the _SAVING state, which is rare. It seems easy for
> > > > userspace, at least QEMU, to drop the entire mmap configuration and
> > > ok. I'll try this way.
> > >
> > > > re-read it. Another concern here is how do we synchronize the event?
> > > > Are we assuming that this event would occur when a user switch to
> > > > _SAVING mode on the device? That operation is synchronous, the device
> > > > must be in saving mode after the write to device state completes, but
> > > > it seems like this might be trying to add an asynchronous dependency.
> > > > Will the write to device_state only complete once the user handles
On 12/10/19 5:57 AM, Daniel P. Berrangé wrote:
>
>> Another issue: other drivers use the machine= setting too, libxl at
>> least. This appears to drop filling in a default machine type for them,
>> at least at XML parse time.
>
> I thought that was the case too, but I couln't find any functional use
> of def->os.machine in any of the other drivers:
>
> $ git grep os.machine | grep -v -E '(qemu|conf)/'
> libvirt-domain.c: * "os.machine" - the machine hardware name as a string
> libxl/libxl_conf.c:def->os.machine);
> libxl/xen_common.c:def->os.machine = g_strdup(capsdata->machinetype);
> vbox/vbox_common.c:VIR_DEBUG("def->os.machine %s",
> def->os.machine);
> vz/vz_sdk.c:if (def->os.machine != NULL || def->os.bootmenu != 0 ||
>
>
> that libxl_conf.c usage is just an error message
>
> The xen_common.c usage is just when parsing XM/XL config files,
> so output only.
>
> The vz_sdk.c usage reports error for any non-NULL machine
>
>
> I guess there is a difference in that if the user does dumpxml for a
> libxl guest we've no longer filled in the machine. I can fix that,
> but it doesn't look like it'll have a functional effect.
Check grep for 'xenfv' and 'xenpv', domaincapabilities does some machine
value handling, and virt-manager will use the reported VM machine value
and feed it back to domaincapabilities, so it could be used in a round
about way.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This version incorporates Peter's feedback to v3.
Pavel Mores (4):
qemu: block: propagate the delete flag to where it can actually be
used
qemu: block: use the delete flag to delete snapshot images if
requested
qemu: block: store the delete flag in libvirtd's status XML
qemu: block: enable the snapshot image deletion feature
src/qemu/qemu_blockjob.c | 50 ++-
src/qemu/qemu_blockjob.h | 4 +-
src/qemu/qemu_domain.c| 4 ++
src/qemu/qemu_driver.c| 12 -
.../blockjob-blockdev-in.xml | 1 +
5 files changed, 67 insertions(+), 4 deletions(-)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
When blockcommit finishes successfully, one of the
qemuBlockJobProcessEventCompletedCommit() and
qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
This is where the delete flag (stored in qemuBlockJobCommitData since the
previous commit) can actually be used to delete the committed snapshot
images if requested.
We use virFileRemove() instead of a simple unlink() to cover the case where
the image to be removed is on an NFS volume.
Signed-off-by: Pavel Mores
---
src/qemu/qemu_blockjob.c | 46
1 file changed, 46 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 718e311213..498e2a716f 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -1004,6 +1004,44 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr
driver,
}
+/**
+ * qemuBlockJobDeleteImages:
+ * @driver: qemu driver object
+ * @vm: domain object
+ * @disk: disk object that the chain to be deleted is associated with
+ * @top: top snapshot of the chain to be deleted
+ *
+ * Helper for removing snapshot images. Intended for callers like
+ * qemuBlockJobProcessEventCompletedCommit() and
+ * qemuBlockJobProcessEventCompletedActiveCommit() as it relies on adjustments
+ * these functions perform on the 'backingStore' chain to function correctly.
+ *
+ * TODO look into removing backing store for non-local snapshots too
+ */
+static void
+qemuBlockJobDeleteImages(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr top)
+{
+virStorageSourcePtr p = top;
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+uid_t uid;
+gid_t gid;
+
+for (; p != NULL; p = p->backingStore) {
+if (virStorageSourceGetActualType(p) == VIR_STORAGE_TYPE_FILE) {
+
+qemuDomainGetImageIds(cfg, vm, p, disk->src, &uid, &gid);
+
+if (virFileRemove(p->path, uid, gid) < 0) {
+VIR_WARN("Unable to remove snapshot image file '%s' (%s)",
+ p->path, g_strerror(errno));
+}
+}
+}
+}
+
/**
* qemuBlockJobProcessEventCompletedCommit:
* @driver: qemu driver object
@@ -1070,6 +1108,10 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr
driver,
job->data.commit.topparent->backingStore = job->data.commit.base;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobDeleteImages(driver, vm, job->disk, job->data.commit.top);
+
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
@@ -1160,6 +1202,10 @@
qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver,
job->disk->src->readonly = job->data.commit.top->readonly;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobDeleteImages(driver, vm, job->disk, job->data.commit.top);
+
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
/* the mirror element does not serve functional purpose for the commit job
*/
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Since blockcommit is asynchronous, libvirtd can be restarted while the
operation runs. To ensure the information necessary to finish up the job
is not lost, serialisation to and deserialisation from the status XML is
added.
To unittest this, the new element was only added to the active commit test,
the non-active commit test doesn't have the new element so as to test its
absence.
Signed-off-by: Pavel Mores
---
src/qemu/qemu_domain.c | 4
tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 +
2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 767790bfc0..27926c7670 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2586,6 +2586,8 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void
*payload,
virBufferAsprintf(&childBuf, "\n",
job->data.commit.top->nodeformat);
if (job->data.commit.topparent)
virBufferAsprintf(&childBuf, "\n",
job->data.commit.topparent->nodeformat);
+if (job->data.commit.deleteCommittedImages)
+virBufferAddLit(&childBuf, "\n");
break;
case QEMU_BLOCKJOB_TYPE_CREATE:
@@ -3185,6 +3187,8 @@
qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job,
"string(./base/@node)",
&job->data.commit.base,
ctxt);
+if (virXPathNode("./deleteCommittedImages", ctxt))
+job->data.commit.deleteCommittedImages = true;
if (!job->data.commit.top ||
!job->data.commit.base)
goto broken;
diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
index 67ab099bd9..b5d62fd4ab 100644
--- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
+++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
@@ -242,6 +242,7 @@
+
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 7:43 AM, Daniel P. Berrangé wrote:
> The use of the parseOpaque parameter was mistakenly removed in
>
> commit 4a4132b4625778cf80acb9c92d06351b44468ac3
> Author: Daniel P. Berrangé
> Date: Tue Dec 3 10:49:49 2019 +
>
> conf: don't use passed in caps in post parse method
>
> causing the method to re-fetch qemuCaps that were already just
> fetched and put into parseOpaque.
>
> This is inefficient when parsing incoming XML, but for live
> XML this is more serious as it means we use the capabilities
> for the current QEMU binary on disk, rather than the running
> QEMU.
>
> That commit, however, did have a useful side effect of fixing
> a crasher bug in the qemu post parse callback introduced by
>
> commit 5e939cea896fb3373a6f68f86e325c657429ed3d
> Author: Jiri Denemark
> Date: Thu Sep 26 18:42:02 2019 +0200
>
> qemu: Store default CPU in domain XML
>
> The qemuDomainDefSetDefaultCPU() method in that patch did not
> allow for the possibility that qemuCaps would be NULL and thus
> resulted in a SEGV.
>
> This shows a risk in letting each check in the post parse
> callback look for qemuCaps == NULL. The safer option is to
> check once upfront and immediately stop (postpone) further
> validation.
>
> Honouring the cached caps for the live status XML, highlights
> a second flaw, which is that it shouldn't check the virt
> type or arch for running guests. The info needed to check this
> is not in the cache caps, only the list of flags are populated.
>
> Thus some of the post parse logic is made to only run for
> inactive guests. This showed a bug in one test data file which
> lacked an ID attribute for the live guest.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/qemu/qemu_domain.c| 52 +++
> .../qemustatusxml2xmldata/vcpus-multi-in.xml | 2 +-
> 2 files changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6f53e17b6a..2abe93c829 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4703,16 +4703,17 @@ static int
> qemuDomainDefPostParse(virDomainDefPtr def,
> unsigned int parseFlags,
> void *opaque,
> - void *parseOpaque G_GNUC_UNUSED)
> + void *parseOpaque)
> {
> virQEMUDriverPtr driver = opaque;
> g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> -g_autoptr(virQEMUCaps) qemuCaps = NULL;
> +/* Note that qemuCaps may be NULL when this function is called. This
> + * function shall not fail in that case. It will be re-run on VM startup
> + * with the capabilities populated. */
> +virQEMUCapsPtr qemuCaps = parseOpaque;
>
> -if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
> -def->emulator))) {
> +if (!qemuCaps)
> return 1;
> -}
>
ACK to this bit, but move the comment to be above the if (!qemuCaps) IMO
> if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -4721,18 +4722,31 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> return -1;
> }
>
> -if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Emulator '%s' does not support arch '%s'"),
> - def->emulator, virArchToString(def->os.arch));
> -return -1;
> -}
> +if (def->id == -1) {
> +if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Emulator '%s' does not support arch '%s'"),
> + def->emulator, virArchToString(def->os.arch));
> +return -1;
> +}
>
> -if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("Emulator '%s' does not support virt type '%s'"),
> - def->emulator,
> virDomainVirtTypeToString(def->virtType));
> -return -1;
> +if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Emulator '%s' does not support virt type
> '%s'"),
> + def->emulator,
> virDomainVirtTypeToString(def->virtType));
> +return -1;
> +}
> +if (!def->os.machine) {
> +const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
> +
> def->virtType);
> +def->os.machine = g_strdup(machine);
> +}
> +} else {
> +if (!def->os.machine) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing machine ty
On Tue, 2019-12-10 at 16:38 +0100, Fabiano Fidêncio wrote:
> Signed-off-by: Fabiano Fidêncio
> ---
> guests/host_vars/libvirt-centos-8/main.yml | 15 +++
> 1 file changed, 15 insertions(+)
The cover letter contained the following remark:
> The projects which were not added due to missing dependencies are:
> - libvirt-cim
> - libvirt-sandbox
> - libvirt-tck
I think it would make perfect sense to include that remark in the
commit message, so that it remains as part of the git history.
With that added,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> On 10 Dec 2019, at 16:08, marcandre.lur...@redhat.com wrote:
>
> From: Marc-André Lureau
>
> Support by usage name can be considered separately (with a 'usage'
> attribute?).
>
> Cc: Stefan Berger
> Signed-off-by: Marc-André Lureau
> ---
> docs/formatsecret.html.in | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in
> index 8d0630a7c3..8f5383cf64 100644
> --- a/docs/formatsecret.html.in
> +++ b/docs/formatsecret.html.in
> @@ -334,8 +334,8 @@ Secret value set
> of the vTPM.
> The element must contain
> a single name element that specifies a usage name
> - for the secret. The vTPM secret can then be used by UUID or by
> - this usage name via the element of
> + for the secret. The vTPM secret can then be used by UUID
> + via the element of
> a tpm when using an
> emulator.
> Since 5.6.0. The following is an example
> --
> 2.24.0.308.g228f53135a
Reminds me of our discussion :-)
Reviewed-by: Christophe de Dinechin
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-12-10 at 16:38 +0100, Fabiano Fidêncio wrote:
> In order to build our projects on a CentOS 8 machine / container, some
> changes will be required:
>
> - libssh2, libvirt dependency, is not longer present:
> -
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html-single/considerations_in_adopting_rhel_8/index#literal_libssh2_literal_is_not_available_in_rhel_8
>
> - librbd1-devel, libvirt dependency, has been replaced by librdb-devel:
> -
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html-single/considerations_in_adopting_rhel_8/index#package-replacements_changes-to-packages
>
> - libgovirt-devel and gtk-vnc2-devel, virt-viewer dependencies, are not
> present anymore:
> -
> https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html-single/considerations_in_adopting_rhel_8/index#removed-packages_changes-to-packages
>
> Signed-off-by: Fabiano Fidêncio
> ---
> guests/vars/mappings.yml | 4
> 1 file changed, 4 insertions(+)
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/9/19 11:55 PM, Cole Robinson wrote:
On 12/2/19 9:26 AM, Michal Privoznik wrote:
There is this class of PCI devices that act like disks: NVMe.
Therefore, they are both PCI devices and disks. While we already
have (and can assign a NVMe device to a domain
successfully) we don't have disk representation. There are three
problems with PCI assignment in case of a NVMe device:
1) domains with can't be migrated
2) NVMe device is assigned whole, there's no way to assign only a
namespace
3) Because hypervisors see they don't put block layer
on top of it - users don't get all the fancy features like
snapshots
NVMe namespaces are way of splitting one continuous NVDIMM memory
into smaller ones, effectively creating smaller NVMe-s (which can
then be partitioned, LVMed, etc.)
Because of all of this the following XML was chosen to model a
NVMe device:
Signed-off-by: Michal Privoznik
---
docs/formatdomain.html.in| 57 +++--
docs/schemas/domaincommon.rng| 32 ++
tests/qemuxml2argvdata/disk-nvme.xml | 63
3 files changed, 149 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/disk-nvme.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6df4a8b26e..fe871d933f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2944,6 +2944,13 @@
+
+
+
+
+
+
+
...
@@ -2957,7 +2964,8 @@
Valid values are "file", "block",
"dir" (since 0.7.5),
"network" (since 0.8.7), or
-"volume" (since 1.0.5)
+"volume" (since 1.0.5), or
+"nvme" (since 5.6.0)
6.0.0 or whatever version this will land in
and refer to the underlying source for the disk.
Since 0.0.3
@@ -3140,6 +3148,43 @@
Since 1.0.5
+nvme
+
+ To specify disk source for NVMe disk the source
+ element has the following attributes:
+
+type
+The type of address specified in address
+sub-element. Currently, only pci value is
+accepted.
+
+
+managed
+This attribute instructs libvirt to detach NVMe
+controller automatically on domain startup (yes)
+or expect the controller to be detached by system
+administrator (no).
+
+
+namespace
+The namespace ID which should be assigned to the domain.
+According to NVMe standard, namespace numbers start from 1,
+including.
+
+
+
+ The difference between
+ and is that the latter is plain
+ host device assignment with all its limitations (e.g. no live
+ migration), while the former makes hypervisor to run the NVMe
+ disk through hypervisor's block layer thus enabling all
+ features provided by the layer (e.g. snapshots, domain
+ migration, etc.). Moreover, since the NVMe disk is unbinded
+ from its PCI driver, the host kernel storage stack is not
+ involved (compared to passing say /dev/nvme0n1 via
+ and therefore lower
+ latencies can be achieved.
+
With "file", "block", and "volume", one or more optional
sub-elements seclabel, described
@@ -3302,11 +3347,17 @@
initiator IQN needed to access the source via mandatory
attribute name.
+ address
+ For disk of type nvme this element
+specifies the PCI address of the host NVMe
+controller.
+Since 5.6.0
Same
+
-For a "file" or "volume" disk type which represents a cdrom or floppy
-(the device attribute), it is possible to define
+For a "file" or "volume" disk type which represents a cdrom or
+floppy (the device attribute), it is possible to define
Stray change?
Oh right. I've realigned this area when adding the address description.
But this change does not belong here.
Also, tn the test XML you need to "s/qemu-system-i686/qemu-system-i386/"
or you'll hit a weird error. And VIR_TEST_REGENERATE_OUTPUT is also
busted, see my patches elsewhere on this list.
On 12/10/19 12:43 AM, Cole Robinson wrote:
On 12/2/19 9:26 AM, Michal Privoznik wrote:
To simplify implementation, some restrictions are added. For
instance, an NVMe disk can't go to any bus but virtio and has to
be type of 'disk' and can't have startupPolicy set.
Signed-off-by: Michal Privoznik
Reviewed-by: Cole Robinson
Arguably the nvme->namespace == 0 should be in a Validate callback, but
it's extremely minor
Fixed. Initially, I thought that namespace == 0 is invalid XML but I can
argue both ways. And since you have inclination to the Validate callback
I've moved the check there.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 5:56 AM, Peter Krempa wrote:
> On Tue, Dec 10, 2019 at 09:58:38 +, Daniel Berrange wrote:
>> On Mon, Dec 09, 2019 at 08:15:05PM -0300, Daniel Henrique Barboza wrote:
>>> (series based on master commit 97cafa610ecf5)
>>>
>>> This work was proposed by Cole in [1]. This is Cole's reasoning for
>>> it, copy/pasted from [1]:
>>>
>>> ---
>>> The benefits of moving to validate time is that XML is rejected by
>>> 'virsh define' rather than at 'virsh start' time. It also makes it easier
>>> to follow the cli building code, and makes it easier to verify
>>> qemu_command.c
>>> test suite code coverage for the important stuff like covering every CLI
>>> option. It's also a good intermediate step for sharing validation with
>>> domain capabilities building, like is done presently for rng models.
>>> ---
>>
>> I've not looked at the patches, but surely moving this validate from
>> start time, to be define time errors is going to cause functional
>> regressions in our ABI behaviour.
>>
>> My libvirtd daemons installs have many XML files defined which will
>> fail validation at various points in time, depending on what QEMU
>> builds I happen to have deployed. I only need them to pass the
>> validation when actually starting the VM.
>
> Validation is not done on the persistent or status XML files exactly for
> this reason.
>
> It's re-done when starting the VM so this should not be an issue.
>
Yup, that's what the VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag is all
about. It's used to skip validation at daemon startup, and various cases
like DomainDefCopy
Conceptually this moves closer to IMO the ideal model:
1) Serialize XML into C struct
2) Validate contents against hypervisor capabilities
3) Serialize C struct into command line string
Which has benefits: simplifies code in #1 and #3 which is the
interesting stuff, easier to test full code coverage of #1 and #3, gives
us the option to move validation into domaincapabilities code so we can
report to the user whether a feature will be rejected or not.
Also I suspect if we moved to using virtblocks for cli stuff, untangling
validation from the cli builder would be a step in that direction
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Let's always generate the unattended script file as, by doing this, we
can treat the files we have as templates, being able to change them
accordingly to whatever is needed by specific distros.
It's important to note that we *must* be careful and keep generating
those files using their "expected" filename, preferably using the same
name of the template, as some of the distros require that.
Signed-off-by: Fabiano Fidêncio
Reviewed-by: Andrea Bolognani
---
guests/lcitool | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/guests/lcitool b/guests/lcitool
index 8436ce7..379fecc 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -23,9 +23,11 @@ import json
import os
import platform
import random
+import shutil
import string
import subprocess
import sys
+import tempfile
import textwrap
import yaml
@@ -537,7 +539,18 @@ class Application:
raise Exception(
"Host {} doesn't support installation".format(host)
)
-initrd_inject = os.path.join(base, "configs", install_config)
+
+# Unattended install scripts are being generated on the fly, based
+# on the templates present in guests/configs/
+initrd_template = os.path.join(base, "configs", install_config)
+with open(initrd_template, 'r') as template:
+content = template.read()
+
+tempdir = tempfile.mkdtemp()
+initrd_inject = os.path.join(tempdir, install_config)
+
+with open(initrd_inject, "w") as inject:
+inject.write(content)
# preseed files must use a well-known name to be picked up by
# d-i; for kickstart files, we can use whatever name we please
@@ -587,6 +600,8 @@ class Application:
except Exception as ex:
raise Exception("Failed to install '{}': {}".format(host, ex))
+shutil.rmtree(tempdir, ignore_errors=True)
+
def _action_update(self, args):
self._execute_playbook("update", args.hosts, args.projects,
args.git_revision)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
In order to add CentOS 8 to jenkins-ci, we had to:
- Adjust lcitool to generate unattended scripts based on templates
instead of using the templates themselves, as a new parameter is
expected on kickstart files for RHEL / CentOS 8 (or newer);
- Enable the EPEL and PowerTools repos;
Then, the following projects have been added:
- libvirt
- Some adjustments had to be done as some dependencies got removed from
CentOS 8;
- libvirt-glib
- libvirt-dbus
- Some adjustments had to be done as flake8 doesn't pull all the needed
dependencies when installed on CentOS 8;
- libvirt-go
- libvirt-go-xml
- libvirt-ocaml
- libvirt-perl
- libvirt-python
- virt-viewer
- Some adjustments had to be done as libgovirt-devel and gtk-vnc2-devel
are not present anymore on CentOS 8;
- osinfo-db-tools
- oisnfo-db
- libosinfo
- virt-manager
The projects which were not added due to missing dependencies are:
- libvirt-cim
- libvirt-sandbox
- libvirt-tck
Fabiano Fidêncio (8):
lcitool: Generate the unattended script file
lcitool: Use install_url in the unattended install files
guests: Add CentOS 8
mappings: Adjustments for CentOS 8
guests: Install EPEL repo on all CentOS guests
guests: Enable PowerTools repo on CentOS 8 guests
guests: Add projects to CentOS 8
guests: Build projects on CentOS 8
guests/configs/kickstart.cfg | 6 +
guests/host_vars/libvirt-centos-8/docker.yml | 2 ++
guests/host_vars/libvirt-centos-8/install.yml | 2 ++
guests/host_vars/libvirt-centos-8/main.yml| 22
guests/inventory | 1 +
guests/lcitool| 26 ++-
guests/playbooks/build/jobs/defaults.yml | 2 ++
guests/playbooks/build/projects/libosinfo.yml | 1 +
.../playbooks/build/projects/libvirt-dbus.yml | 2 ++
guests/playbooks/build/projects/libvirt.yml | 1 +
.../build/projects/osinfo-db-tools.yml| 1 +
.../playbooks/build/projects/virt-manager.yml | 3 +++
guests/playbooks/update/tasks/base.yml| 9 ++-
guests/vars/mappings.yml | 4 +++
jenkins/jobs/defaults.yaml| 2 ++
jenkins/projects/libosinfo.yaml | 1 +
jenkins/projects/libvirt-dbus.yaml| 2 ++
jenkins/projects/libvirt.yaml | 1 +
jenkins/projects/osinfo-db-tools.yaml | 1 +
jenkins/projects/virt-manager.yaml| 3 +++
20 files changed, 90 insertions(+), 2 deletions(-)
create mode 100644 guests/host_vars/libvirt-centos-8/docker.yml
create mode 100644 guests/host_vars/libvirt-centos-8/install.yml
create mode 100644 guests/host_vars/libvirt-centos-8/main.yml
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Passing `url --url={{ install_url }}` to a kickstart file is, according
to Red Hat official documentation[0], mandatory for RHEL 8 and onwards
(which includes CentOS 8).
Using it for RHEL 7, CentOS 7, and Fedora does not cause any issue.
[0]:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/performing_an_advanced_rhel_installation/kickstart-commands-and-options-reference_installing-rhel-as-an-experienced-user
Signed-off-by: Fabiano Fidêncio
Reviewed-by: Andrea Bolognani
---
guests/configs/kickstart.cfg | 6 ++
guests/lcitool | 9 +
2 files changed, 15 insertions(+)
diff --git a/guests/configs/kickstart.cfg b/guests/configs/kickstart.cfg
index 713557a..76b7b65 100644
--- a/guests/configs/kickstart.cfg
+++ b/guests/configs/kickstart.cfg
@@ -1,3 +1,9 @@
+# Installation source
+#
+# The operating system will be installed from the following URL
+url --url={{ install_url }}
+
+
# Installer configuration
#
# Perform a text based installation followed by a reboot, and disable
diff --git a/guests/lcitool b/guests/lcitool
index 379fecc..803477e 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -542,9 +542,18 @@ class Application:
# Unattended install scripts are being generated on the fly, based
# on the templates present in guests/configs/
+unattended_options = [
+"install_url",
+]
+
initrd_template = os.path.join(base, "configs", install_config)
with open(initrd_template, 'r') as template:
content = template.read()
+for option in unattended_options:
+content = content.replace(
+"{{ " + option + " }}",
+facts[option]
+)
tempdir = tempfile.mkdtemp()
initrd_inject = os.path.join(tempdir, install_config)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
EPEL repository should be enabled on all supported CentOS guests as it
brings in packages which are part of the "base packages" for
libvirt-jenkins-ci.
Signed-off-by: Fabiano Fidêncio
Reviewed-by: Andrea Bolognani
---
guests/playbooks/update/tasks/base.yml | 1 -
1 file changed, 1 deletion(-)
diff --git a/guests/playbooks/update/tasks/base.yml
b/guests/playbooks/update/tasks/base.yml
index 5153917..965e442 100644
--- a/guests/playbooks/update/tasks/base.yml
+++ b/guests/playbooks/update/tasks/base.yml
@@ -15,7 +15,6 @@
state: latest
when:
- os_name == 'CentOS'
-- os_version == '7'
- name: Update installed packages
package:
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 10:03 AM, Pavel Hrdina wrote:
> On Tue, Dec 10, 2019 at 03:57:59PM +0100, Ján Tomko wrote:
>> On Tue, Dec 10, 2019 at 03:29:43PM +0100, Pavel Hrdina wrote:
>>> On Tue, Dec 10, 2019 at 03:14:40PM +0100, Christophe de Dinechin wrote:
> On 10 Dec 2019, at 15:11, Pavel Hrdina wrote:
>
> Clang complains about condition being always true:
>
> src/util/virkeyfile.c:113:23: error: result of comparison of constant 128
> with expression of type 'const char' is always true
> [-Werror,-Wtautological-constant-out-of-range-compare]
>while (!IS_EOF && IS_ASCII(CUR) && CUR != ']')
> ^
> src/util/virkeyfile.c:80:26: note: expanded from macro 'IS_ASCII'
> ~~~ ^ ~~~
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/virkeyfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
> index 816bfae96d..ee29bd7aa6 100644
> --- a/src/util/virkeyfile.c
> +++ b/src/util/virkeyfile.c
> @@ -77,7 +77,7 @@ struct _virKeyFileParserCtxt {
> #define IS_EOF (ctxt->cur >= ctxt->end)
> #define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
> #define IS_BLANK(c) (((c) == ' ') || ((c) == '\t'))
> -#define IS_ASCII(c) ((c) < 128)
> +#define IS_ASCII(c) (((unsigned char) c) < 128)
Probably want parentheses around c.
>>>
>>> Right, I'll fix it before pushing.
>>>
>>
>> with that change:
>> Reviewed-by: Ján Tomko
>
> Thanks, pushed now.
>
>> But the pre-existingcode seems odd - this is used to parse the INI-like
>> syntax
>> files for auth: https://libvirt.org/auth.html
>>
>> It does not really make sense to allow form feed or del in the group
>> entry name. I guess the only remotely reasonable character we'd block
>> by switching to g_ascii_isprint here is '\t'
>
> I was considering to "fix" the code as well but when looking into what
> GLib offers we can replace this whole file with GKeyFile [1].
>
> Pavel
>
> [1]
Ah cool. It would be nice to track a list somewhere of glib things that
can replace internal libvirt infrastructure. Maybe a subsection of the
bitesizedtasks wiki even though some of them will be not so bitesized.
What are the other glib bits people know about?
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-12-10 at 14:54 +, Daniel P. Berrangé wrote:
> On Tue, Dec 10, 2019 at 02:54:22PM +0100, Andrea Bolognani wrote:
> > The only reason why I'm even questioning whether this should be done
> > is capacity for the hypervisor host: the machine we're running all
> > builders on has
> >
> > CPUs: 8
> > Memory: 32 GiB
> > Storage: 450 GiB
> >
> > and each of the guests is configured to use
> >
> > CPUs: 2
> > Memory: 2 GiB
> > Storage: 20 GiB
> >
> > So while we're good, and actually have plenty of room to grow, on
> > the memory and storage front, we're already overcommitting our CPUs
> > pretty significantly, which I guess is at least part of the reason
> > why builds take so long.
>
> NB the memory that's free is not really free - it is being usefull
> as I/O cache for the VM disks. So more VMs will reduce I/O cache.
> Whether that will actually impact us I don't know though.
Oh yeah, I'm aware of that. But like you, I'm unsure about the exact
impact that makes.
We could arguably reduce the amount of memory assigned to guests to
1 GiB: building libvirt and friends is not exactly memory-intensive,
and we could potentially benefit from the additionaly I/O cache or
lessen the blow caused by adding more VMs.
> More importantly though, AFAICT, those are not 8 real CPUs.
>
> virsh nodeinfo reports 8 cores, but virsh capabilities
> reports it as a 1 socket, 4 core, 2 thread CPU.
>
> IOW we haven't really got 8 CPUs, more like equivalent of 5 CPUs.
> as HT only really gives a x1.3 boost in best case, and I suspect
> builds are not likely to be hitting the best case.
That's also true.
> > Can we afford to add 50% more load on the machine without making it
> > unusable? I don't know. But I think it would be worthwhile to at
> > least try and see how it handles an additional 25%, which is exactly
> > what this series does.
>
> Giving it a try is ok I guess.
Can I have an R-b then? O:-)
> I expect there's probably more we can do to optimize the setup
> too.
>
> For example, what actual features of qcow2 are we using ? We're
> not snapshotting VMs, we don't need grow-on-demand allocation.
> AFACT we're paying the performance cost of qcow2 (l1/l2 table
> lookups & metadata caching), for no reason. Switch the VMs to
> fully pre-allocated raw files may improve I/O performance.
> Raw LVM VGs would be even better but that will be painful
> to setup given the host install setup.
>
> I also wonder if we have the optimal aio setting for disks,
> as there's nothing in the XML.
>
> We could consider using cache=unsafe for VMs, though for
> that I think we'd want to separate off a separate disk
> for /home/jenkins so that if there was a host OS crash,
> we wouldn't have to rebuild the entire VMs - just throw
> away the data disk & recreate.
The home directory for the jenkins user contains some configuration
as well, so you'd have to run 'lcitool update' anyway after attaching
the new disk... At that point, it might be less work overall to just
rebuild the entire VM from scratch. We have only seen a couple of
unexpected host shutdowns so far, and all were caused by hardware
issues that resulted in a multi-day downtime anyway, so the overhead
of reinstalling the various guest OS' would not be the dealbreaker.
> Since we've got plenty of RAM, another obvious thing would be
> to turn on huge pages and use them for all guest RAM. This may
> well have a very significant performance boost from reducing
> CPU overhead which is our biggest bottleneck.
Yeah, all of the above sounds like it could help, but I'm not really
well versed on the performance tuning front so I wouldn't know where
to start and even how to properly measure the impact of each change.
We also have to keep in mind that, while the CentOS CI environment
is the main consumer of the repository, we want it to be possible for
any developer to deploy builders locally relatively painlessly, so
things like hugepages and LVM usage would definitely have to be
optional if we introduced them.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
From: Marc-André Lureau
Support by usage name can be considered separately (with a 'usage'
attribute?).
Cc: Stefan Berger
Signed-off-by: Marc-André Lureau
---
docs/formatsecret.html.in | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in
index 8d0630a7c3..8f5383cf64 100644
--- a/docs/formatsecret.html.in
+++ b/docs/formatsecret.html.in
@@ -334,8 +334,8 @@ Secret value set
of the vTPM.
The element must contain
a single name element that specifies a usage name
- for the secret. The vTPM secret can then be used by UUID or by
- this usage name via the element of
+ for the secret. The vTPM secret can then be used by UUID
+ via the element of
a tpm when using an
emulator.
Since 5.6.0. The following is an example
--
2.24.0.308.g228f53135a
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 03:57:59PM +0100, Ján Tomko wrote:
> On Tue, Dec 10, 2019 at 03:29:43PM +0100, Pavel Hrdina wrote:
> > On Tue, Dec 10, 2019 at 03:14:40PM +0100, Christophe de Dinechin wrote:
> > >
> > >
> > > > On 10 Dec 2019, at 15:11, Pavel Hrdina wrote:
> > > >
> > > > Clang complains about condition being always true:
> > > >
> > > > src/util/virkeyfile.c:113:23: error: result of comparison of constant
> > > > 128 with expression of type 'const char' is always true
> > > > [-Werror,-Wtautological-constant-out-of-range-compare]
> > > >while (!IS_EOF && IS_ASCII(CUR) && CUR != ']')
> > > > ^
> > > > src/util/virkeyfile.c:80:26: note: expanded from macro 'IS_ASCII'
> > > > ~~~ ^ ~~~
> > > >
> > > > Signed-off-by: Pavel Hrdina
> > > > ---
> > > > src/util/virkeyfile.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
> > > > index 816bfae96d..ee29bd7aa6 100644
> > > > --- a/src/util/virkeyfile.c
> > > > +++ b/src/util/virkeyfile.c
> > > > @@ -77,7 +77,7 @@ struct _virKeyFileParserCtxt {
> > > > #define IS_EOF (ctxt->cur >= ctxt->end)
> > > > #define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
> > > > #define IS_BLANK(c) (((c) == ' ') || ((c) == '\t'))
> > > > -#define IS_ASCII(c) ((c) < 128)
> > > > +#define IS_ASCII(c) (((unsigned char) c) < 128)
> > >
> > > Probably want parentheses around c.
> >
> > Right, I'll fix it before pushing.
> >
>
> with that change:
> Reviewed-by: Ján Tomko
Thanks, pushed now.
> But the pre-existingcode seems odd - this is used to parse the INI-like syntax
> files for auth: https://libvirt.org/auth.html
>
> It does not really make sense to allow form feed or del in the group
> entry name. I guess the only remotely reasonable character we'd block
> by switching to g_ascii_isprint here is '\t'
I was considering to "fix" the code as well but when looking into what
GLib offers we can replace this whole file with GKeyFile [1].
Pavel
[1]
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 03:29:43PM +0100, Pavel Hrdina wrote:
On Tue, Dec 10, 2019 at 03:14:40PM +0100, Christophe de Dinechin wrote:
> On 10 Dec 2019, at 15:11, Pavel Hrdina wrote:
>
> Clang complains about condition being always true:
>
> src/util/virkeyfile.c:113:23: error: result of comparison of constant 128
with expression of type 'const char' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
>while (!IS_EOF && IS_ASCII(CUR) && CUR != ']')
> ^
> src/util/virkeyfile.c:80:26: note: expanded from macro 'IS_ASCII'
> ~~~ ^ ~~~
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/virkeyfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
> index 816bfae96d..ee29bd7aa6 100644
> --- a/src/util/virkeyfile.c
> +++ b/src/util/virkeyfile.c
> @@ -77,7 +77,7 @@ struct _virKeyFileParserCtxt {
> #define IS_EOF (ctxt->cur >= ctxt->end)
> #define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
> #define IS_BLANK(c) (((c) == ' ') || ((c) == '\t'))
> -#define IS_ASCII(c) ((c) < 128)
> +#define IS_ASCII(c) (((unsigned char) c) < 128)
Probably want parentheses around c.
Right, I'll fix it before pushing.
with that change:
Reviewed-by: Ján Tomko
But the pre-existingcode seems odd - this is used to parse the INI-like syntax
files for auth: https://libvirt.org/auth.html
It does not really make sense to allow form feed or del in the group
entry name. I guess the only remotely reasonable character we'd block
by switching to g_ascii_isprint here is '\t'
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 02:54:22PM +0100, Andrea Bolognani wrote:
> This patch is intended to start a slightly larger discussion about
> our plans for the CentOS CI environment going forward.
>
> At the moment, we have active builders for
>
> CentOS 7
> Debian 9
> Debian 10
> Fedora 30
> Fedora 31
> Fedora Rawhide
> FreeBSD 11
> FreeBSD 12
>
> but we don't have builder for
>
> Debian sid
> FreeBSD -CURRENT
> Ubuntu 16.04
> Ubuntu 18.04
>
> despite them being fully supported in the libvirt-jenkins-ci
> repository.
>
> This makes sense for sid and -CURRENT, since the former covers the
> same "freshest Linux packages" angle that Rawhide already takes care
> of and the latter is often broken and not trivial to keep updated;
> both Ubuntu targets, however, should IMHO be part of the CentOS CI
> environment. Hence this series :)
>
> Moreover, we're in the process of adding
>
> CentOS 8
> openSUSE Leap 15.1
> openSUSE Tumbleweed
>
> as targets, of which the first two should also IMHO be added as they
> would provide useful additional coverage.
>
> The only reason why I'm even questioning whether this should be done
> is capacity for the hypervisor host: the machine we're running all
> builders on has
>
> CPUs: 8
> Memory: 32 GiB
> Storage: 450 GiB
>
> and each of the guests is configured to use
>
> CPUs: 2
> Memory: 2 GiB
> Storage: 20 GiB
>
> So while we're good, and actually have plenty of room to grow, on
> the memory and storage front, we're already overcommitting our CPUs
> pretty significantly, which I guess is at least part of the reason
> why builds take so long.
NB the memory that's free is not really free - it is being usefull
as I/O cache for the VM disks. So more VMs will reduce I/O cache.
Whether that will actually impact us I don't know though.
More importantly though, AFAICT, those are not 8 real CPUs.
virsh nodeinfo reports 8 cores, but virsh capabilities
reports it as a 1 socket, 4 core, 2 thread CPU.
IOW we haven't really got 8 CPUs, more like equivalent of 5 CPUs.
as HT only really gives a x1.3 boost in best case, and I suspect
builds are not likely to be hitting the best case.
> Can we afford to add 50% more load on the machine without making it
> unusable? I don't know. But I think it would be worthwhile to at
> least try and see how it handles an additional 25%, which is exactly
> what this series does.
Giving it a try is ok I guess.
I expect there's probably more we can do to optimize the setup
too.
For example, what actual features of qcow2 are we using ? We're
not snapshotting VMs, we don't need grow-on-demand allocation.
AFACT we're paying the performance cost of qcow2 (l1/l2 table
lookups & metadata caching), for no reason. Switch the VMs to
fully pre-allocated raw files may improve I/O performance.
Raw LVM VGs would be even better but that will be painful
to setup given the host install setup.
I also wonder if we have the optimal aio setting for disks,
as there's nothing in the XML.
We could consider using cache=unsafe for VMs, though for
that I think we'd want to separate off a separate disk
for /home/jenkins so that if there was a host OS crash,
we wouldn't have to rebuild the entire VMs - just throw
away the data disk & recreate.
Since we've got plenty of RAM, another obvious thing would be
to turn on huge pages and use them for all guest RAM. This may
well have a very significant performance boost from reducing
CPU overhead which is our biggest bottleneck.
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
This is a significant rework of v1 as
- patch 1 was dropped because an equivalent change was accidentally committed
in the meantime in 4c53267b70fc5c548b6530113c3f96870d8d7fc1
- patch 2 was dropped as it was just a mostly formal clean-up of patch 1
- patch 3 was redone using a different approach.
Patch 3 now takes the approach that was hinted at in the last paragraph of
v1's cover letter, more specifically it now fixes qemuProcessStop() rather
than its caller. Digging into git history I located the commit that
introduced qemuProcessStop()'s problematic behaviour and after consulting with
Michal, we agreed that fixing qemuProcessStop() seems safe enough, and
definitely a better solution conceptually.
Pavel Mores (1):
qemu: fix concurrency crash bug in snapshot revert
src/qemu/qemu_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This commit aims to fix
https://bugzilla.redhat.com/show_bug.cgi?id=1610207
The cause was apparently incorrect handling of jobs in snapshot revert code
which allowed a thread executing snapshot delete to begin job while
snapshot revert was still running on another thread. The snapshot delete
thread then waited on a condition variable in qemuMonitorSend() while the
revert thread finished, changing (and effectively corrupting) the
qemuMonitor structure under the delete thread which led to its crash.
The incorrect handling of jobs in revert code was due to the fact that
although qemuDomainRevertToSnapshot() correctly begins a job at the start,
the job was implicitly ended when qemuProcessStop() was called because the
job lives in the QEMU driver's private data (qemuDomainObjPrivate) that
was purged during qemuProcessStop().
This fix prevents qemuProcessStop() from clearing jobs as the idea of
qemuProcessStop() clearing jobs seems wrong in the first place. It was
(inadvertently) introduced in commit 888aa4b6b9db65e3db273341e79846, which
is effectively reverted by the second hunk of this commit. To preserve
the desired effects of the faulty commit, the first hunk is included as
suggested by Michal.
Signed-off-by: Pavel Mores
---
src/qemu/qemu_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6f53e17b6a..e4a1bccc18 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -403,6 +403,8 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj,
static void
qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
{
+qemuDomainObjResetJob(priv);
+qemuDomainObjResetAsyncJob(priv);
VIR_FREE(priv->job.current);
VIR_FREE(priv->job.completed);
virCondDestroy(&priv->job.cond);
@@ -2161,9 +2163,6 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr
priv)
virBitmapFree(priv->migrationCaps);
priv->migrationCaps = NULL;
-qemuDomainObjResetJob(priv);
-qemuDomainObjResetAsyncJob(priv);
-
virHashRemoveAll(priv->blockjobs);
virHashRemoveAll(priv->dbusVMStates);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 2:17 PM, Daniel P. Berrangé wrote:
Daniel P. Berrangé (6):
Fix typo breaking some migrate parameter handling
Add handling for VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS
Add support for VIR_MIGRATE_PARAM_TLS_DESTINATION constant
Add missing create_checkpoint method
Remove docs for has_metadata method which doesn't exist
Fix data type for VIR_CONNECT_IDENTITY_SASL_USER_NAME
Changes | 9 +
lib/Sys/Virt.xs | 25 +
lib/Sys/Virt/Domain.pm | 26 ++
lib/Sys/Virt/DomainCheckpoint.pm | 5 -
4 files changed, 56 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
This patch is intended to start a slightly larger discussion about
our plans for the CentOS CI environment going forward.
At the moment, we have active builders for
CentOS 7
Debian 9
Debian 10
Fedora 30
Fedora 31
Fedora Rawhide
FreeBSD 11
FreeBSD 12
but we don't have builder for
Debian sid
FreeBSD -CURRENT
Ubuntu 16.04
Ubuntu 18.04
despite them being fully supported in the libvirt-jenkins-ci
repository.
This makes sense for sid and -CURRENT, since the former covers the
same "freshest Linux packages" angle that Rawhide already takes care
of and the latter is often broken and not trivial to keep updated;
both Ubuntu targets, however, should IMHO be part of the CentOS CI
environment. Hence this series :)
Moreover, we're in the process of adding
CentOS 8
openSUSE Leap 15.1
openSUSE Tumbleweed
as targets, of which the first two should also IMHO be added as they
would provide useful additional coverage.
The only reason why I'm even questioning whether this should be done
is capacity for the hypervisor host: the machine we're running all
builders on has
CPUs: 8
Memory: 32 GiB
Storage: 450 GiB
and each of the guests is configured to use
CPUs: 2
Memory: 2 GiB
Storage: 20 GiB
So while we're good, and actually have plenty of room to grow, on
the memory and storage front, we're already overcommitting our CPUs
pretty significantly, which I guess is at least part of the reason
why builds take so long.
Can we afford to add 50% more load on the machine without making it
unusable? I don't know. But I think it would be worthwhile to at
least try and see how it handles an additional 25%, which is exactly
what this series does.
In my opinion, as long as the machine can keep up with demand and not
end up in a situation where it starts accumulating backlog jobs, it's
fine if builds take longer: developers who want to run a relatively
quick smoke test before posting patches can use Travis CI or 'make
ci-check@...' locally for the purpose, whereas the role of CentOS CI
in my eyes is to try and catch as many issues as possible after merge
so that they don't end up in a release. But I realize others might
see it differently, hence this lenghty cover letter :)
Andrea Bolognani (1):
jenkins: Start building on Ubuntu
jenkins/jobs/defaults.yaml| 2 ++
jenkins/projects/libvirt-dbus.yaml| 1 +
jenkins/projects/libvirt-sandbox.yaml | 2 ++
jenkins/projects/libvirt-tck.yaml | 2 ++
jenkins/projects/libvirt.yaml | 2 ++
jenkins/projects/virt-manager.yaml| 2 ++
6 files changed, 11 insertions(+)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Daniel P. Berrangé
---
Changes | 1 +
lib/Sys/Virt/DomainCheckpoint.pm | 5 -
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/Changes b/Changes
index f36d401..9d9a5c8 100644
--- a/Changes
+++ b/Changes
@@ -10,6 +10,7 @@ Revision history for perl module Sys::Virt
constant when invoking migrate
- Add VIR_MIGRATE_PARAM_TLS_DESTINATION constant
- Add missing create_checkpoint method on Sys::Virt::Domain
+ - Remove docs for has_metadata method which doesn't exit
5.10.0 2019-12-03
diff --git a/lib/Sys/Virt/DomainCheckpoint.pm b/lib/Sys/Virt/DomainCheckpoint.pm
index e8e875d..e1adae4 100644
--- a/lib/Sys/Virt/DomainCheckpoint.pm
+++ b/lib/Sys/Virt/DomainCheckpoint.pm
@@ -97,11 +97,6 @@ constants.
Return the parent of the checkpoint, if any
-=item $res = $domchkp->has_metadata()
-
-Returns a true value if this checkpoint has metadata associated with
-it.
-
=item my @checkpoints = $domchkp->list_all_children($flags)
Return a list of all domain checkpoints that are children of this
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 02:09:35PM +0100, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik
> ---
> Changes| 1 +
> lib/Sys/Virt.xs| 1 +
> lib/Sys/Virt/Domain.pm | 9 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/Changes b/Changes
> index 37529ac..cf5d8cc 100644
> --- a/Changes
> +++ b/Changes
> @@ -4,6 +4,7 @@ Revision history for perl module Sys::Virt
>
> - Add VIR_DOMAIN_JOB_SUCCESS and VIR_DOMAIN_JOB_STATS_KEEP_COMPLETED
> constants
> + - Add VIR_MIGRATE_PARAM_TLS_DESTINATION constant
>
> 5.10.0 2019-12-03
>
> diff --git a/lib/Sys/Virt.xs b/lib/Sys/Virt.xs
> index 98a12d7..a729a52 100644
> --- a/lib/Sys/Virt.xs
> +++ b/lib/Sys/Virt.xs
> @@ -9246,6 +9246,7 @@ BOOT:
>REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT,
> MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT);
>REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_BANDWIDTH_POSTCOPY,
> MIGRATE_PARAM_BANDWIDTH_POSTCOPY);
>REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS,
> MIGRATE_PARAM_PARALLEL_CONNECTIONS);
> + REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_TLS_DESTINATION,
> MIGRATE_PARAM_TLS_DESTINATION);
>
>REGISTER_CONSTANT(VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY,
> MIGRATE_MAX_SPEED_POSTCOPY);
This is incomplete, as the migrate methods need to be updated
to set the data type.
Don't bother about a v2 though as I'm co-incidentally fixing
this and many other perl bugs right now.
>
> diff --git a/lib/Sys/Virt/Domain.pm b/lib/Sys/Virt/Domain.pm
> index 2caec51..ae909de 100644
> --- a/lib/Sys/Virt/Domain.pm
> +++ b/lib/Sys/Virt/Domain.pm
> @@ -1141,6 +1141,15 @@ progress is not made
>
> The number of connections used during parallel migration.
>
> +=item C
> +
> +Override the destination host name used for TLS verification.
> +Normally the TLS certificate from the destination host must match
> +the host's name for TLS verification to succeed. When the
> +certificate does not match the destination hostname and the
> +expected cetificate's hostname is known, this parameter can be
> +used to pass this expected hostname when starting the migration.
I'll borrow your description for my patch though
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
On Tue, 10 Dec 2019 11:34:32 +0100
Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > +Markus
> >
> > On Tue, Dec 03, 2019 at 03:43:03PM +0100, Igor Mammedov wrote:
> >> On Tue, 3 Dec 2019 09:56:15 +0100
> >> Thomas Huth wrote:
> >>
> >> > On 02/12/2019 22.00, Eduardo Habkost wrote:
> >> > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote:
> >> > >> On Fri, 29 Nov 2019 18:46:12 +0100
> >> > >> Paolo Bonzini wrote:
> >> > >>
> >> > >>> On 29/11/19 13:16, Igor Mammedov wrote:
> >> > As for "-m", I'd make it just an alias that translates
> >> > -m/mem-path/mem-prealloc
> >> > >>>
> >> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0.
> >> > >>> CCing
> >> > >>> Thomas as mister deprecation. :)
> >> > >>
> >> > >> I'll add that to my series
> >> > >
> >> > > Considering that the plan is to eventually reimplement those
> >> > > options as syntactic sugar for memory backend options (hopefully
> >> > > in less than 2 QEMU releases), what's the point of deprecating
> >> > > them?
> >> >
> >> > Well, it depends on the "classification" [1] of the parameter...
> >> >
> >> > Let's ask: What's the main purpose of the option?
> >> >
> >> > Is it easier to use than the "full" option, and thus likely to be used
> >> > by a lot of people who run QEMU directly from the CLI? In that case it
> >> > should stay as "convenience option" and not be deprecated.
> >> >
> >> > Or is the option merely there to give the upper layers like libvirt or
> >> > some few users and their scripts some more grace period to adapt their
> >> > code, but we all agree that the options are rather ugly and should
> >> > finally go away? Then it's rather a "legacy option" and the deprecation
> >> > process is the right way to go. Our QEMU interface is still way
> >> > overcrowded, we should try to keep it as clean as possible.
> >>
> >> After switching to memdev for main RAM, users could use relatively
> >> short global options
> >> -global memory-backend.prealloc|share=on
> >> and
> >> -global memory-backend-file.mem-path=X|prealloc|share=on
> >>
> >> instead of us adding and maintaining slightly shorter
> >> -mem-shared/-mem-path/-mem-prealloc
> >
> > Global properties are a convenient way to expose knobs through
> > the command line with little effort, but we have no documentation
> > on which QOM properties are really supposed to be touched by
> > users using -global.
> >
> > Unless we fix the lack of documentation, I'd prefer to have
> > syntactic sugar translated to -global instead of recommending
> > direct usage of -global.
>
> Fair point.
>
> I'd take QOM property documentation over still more sugar.
>
> Sometimes, the practical way to make simple things simple is sugar. I
> can accept that. This doesn't look like such a case, though.
I can document concrete globals as replacement at the place
-mem-path/-mem-prealloc are documented during deprecation and
then in 2 releases we will just drop legacy syntax and keep only
globals over there.
(eventually it will spread various globals
over man page, which I don't like but we probably should start
somwhere and consolidate later if globals in man page become
normal practice.)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Signed-off-by: Michal Privoznik
---
Changes| 1 +
lib/Sys/Virt.xs| 1 +
lib/Sys/Virt/Domain.pm | 9 +
3 files changed, 11 insertions(+)
diff --git a/Changes b/Changes
index 37529ac..cf5d8cc 100644
--- a/Changes
+++ b/Changes
@@ -4,6 +4,7 @@ Revision history for perl module Sys::Virt
- Add VIR_DOMAIN_JOB_SUCCESS and VIR_DOMAIN_JOB_STATS_KEEP_COMPLETED
constants
+ - Add VIR_MIGRATE_PARAM_TLS_DESTINATION constant
5.10.0 2019-12-03
diff --git a/lib/Sys/Virt.xs b/lib/Sys/Virt.xs
index 98a12d7..a729a52 100644
--- a/lib/Sys/Virt.xs
+++ b/lib/Sys/Virt.xs
@@ -9246,6 +9246,7 @@ BOOT:
REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT,
MIGRATE_PARAM_AUTO_CONVERGE_INCREMENT);
REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_BANDWIDTH_POSTCOPY,
MIGRATE_PARAM_BANDWIDTH_POSTCOPY);
REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS,
MIGRATE_PARAM_PARALLEL_CONNECTIONS);
+ REGISTER_CONSTANT_STR(VIR_MIGRATE_PARAM_TLS_DESTINATION,
MIGRATE_PARAM_TLS_DESTINATION);
REGISTER_CONSTANT(VIR_DOMAIN_MIGRATE_MAX_SPEED_POSTCOPY,
MIGRATE_MAX_SPEED_POSTCOPY);
diff --git a/lib/Sys/Virt/Domain.pm b/lib/Sys/Virt/Domain.pm
index 2caec51..ae909de 100644
--- a/lib/Sys/Virt/Domain.pm
+++ b/lib/Sys/Virt/Domain.pm
@@ -1141,6 +1141,15 @@ progress is not made
The number of connections used during parallel migration.
+=item C
+
+Override the destination host name used for TLS verification.
+Normally the TLS certificate from the destination host must match
+the host's name for TLS verification to succeed. When the
+certificate does not match the destination hostname and the
+expected cetificate's hostname is known, this parameter can be
+used to pass this expected hostname when starting the migration.
+
=back
=item $ddom = $dom->migrate(destcon, flags=0, dname=undef, uri=undef,
bandwidth=0)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Dec 08, 2019 at 06:36:22PM -0500, Cole Robinson wrote:
> On 11/20/19 9:48 AM, Pavel Hrdina wrote:
> > Changes in v2:
> > - fixed missing parentheses in patch 01
> > - added patch 02 to cover c_isascii
> >
>
> With the two issues I pointed out fixed:
>
> Reviewed-by: Cole Robinson
Thanks, fixed and pushed.
Pavel
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
The use of the parseOpaque parameter was mistakenly removed in
commit 4a4132b4625778cf80acb9c92d06351b44468ac3
Author: Daniel P. Berrangé
Date: Tue Dec 3 10:49:49 2019 +
conf: don't use passed in caps in post parse method
causing the method to re-fetch qemuCaps that were already just
fetched and put into parseOpaque.
This is inefficient when parsing incoming XML, but for live
XML this is more serious as it means we use the capabilities
for the current QEMU binary on disk, rather than the running
QEMU.
That commit, however, did have a useful side effect of fixing
a crasher bug in the qemu post parse callback introduced by
commit 5e939cea896fb3373a6f68f86e325c657429ed3d
Author: Jiri Denemark
Date: Thu Sep 26 18:42:02 2019 +0200
qemu: Store default CPU in domain XML
The qemuDomainDefSetDefaultCPU() method in that patch did not
allow for the possibility that qemuCaps would be NULL and thus
resulted in a SEGV.
This shows a risk in letting each check in the post parse
callback look for qemuCaps == NULL. The safer option is to
check once upfront and immediately stop (postpone) further
validation.
Honouring the cached caps for the live status XML, highlights
a second flaw, which is that it shouldn't check the virt
type or arch for running guests. The info needed to check this
is not in the cache caps, only the list of flags are populated.
Thus some of the post parse logic is made to only run for
inactive guests. This showed a bug in one test data file which
lacked an ID attribute for the live guest.
Signed-off-by: Daniel P. Berrangé
---
src/qemu/qemu_domain.c| 52 +++
.../qemustatusxml2xmldata/vcpus-multi-in.xml | 2 +-
2 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6f53e17b6a..2abe93c829 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4703,16 +4703,17 @@ static int
qemuDomainDefPostParse(virDomainDefPtr def,
unsigned int parseFlags,
void *opaque,
- void *parseOpaque G_GNUC_UNUSED)
+ void *parseOpaque)
{
virQEMUDriverPtr driver = opaque;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-g_autoptr(virQEMUCaps) qemuCaps = NULL;
+/* Note that qemuCaps may be NULL when this function is called. This
+ * function shall not fail in that case. It will be re-run on VM startup
+ * with the capabilities populated. */
+virQEMUCapsPtr qemuCaps = parseOpaque;
-if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
-def->emulator))) {
+if (!qemuCaps)
return 1;
-}
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4721,18 +4722,31 @@ qemuDomainDefPostParse(virDomainDefPtr def,
return -1;
}
-if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Emulator '%s' does not support arch '%s'"),
- def->emulator, virArchToString(def->os.arch));
-return -1;
-}
+if (def->id == -1) {
+if (!virQEMUCapsIsArchSupported(qemuCaps, def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Emulator '%s' does not support arch '%s'"),
+ def->emulator, virArchToString(def->os.arch));
+return -1;
+}
-if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Emulator '%s' does not support virt type '%s'"),
- def->emulator,
virDomainVirtTypeToString(def->virtType));
-return -1;
+if (!virQEMUCapsIsVirtTypeSupported(qemuCaps, def->virtType)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Emulator '%s' does not support virt type '%s'"),
+ def->emulator,
virDomainVirtTypeToString(def->virtType));
+return -1;
+}
+if (!def->os.machine) {
+const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
+
def->virtType);
+def->os.machine = g_strdup(machine);
+}
+} else {
+if (!def->os.machine) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing machine type"));
+return -1;
+}
}
if (def->os.bootloader || def->os.bootloaderArgs) {
@@ -4741,12 +4755,6 @@ qemuDomainDefPostParse(virDomainDefPtr def,
return -1;
}
-if (!def->os.machine) {
-const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
-
On Tue, Dec 10, 2019 at 12:41 PM Henning Schild
wrote:
> Am Tue, 10 Dec 2019 12:01:24 +0100
> schrieb Christian Ehrhardt :
>
> > On Tue, Dec 10, 2019 at 10:46 AM Henning Schild
> > wrote:
> >
> > > Hi all,
> > >
> > > the systemd shutdown scripts work sequentially with a 300s timeout
> > > (seen on Debian). If a VM does not have ACPI support, or the ACPI
> > > support failed for some reason, you are looking at a 300s timeout
> > > per instance for a host shutdown/reboot.
> > > i.e. 10 instances without working ACPI = 3000s to shut down
> > >
> > > I think the systemd scripting should be parallel instead of
> > > sequentially. So if you have many VMs without working ACPI you just
> > > have to wait 300s in total for the host to shut down.
> > >
> >
> > Hi Henning,
> > this is configurable in /etc/default/libvirt-guests
> > For example Ubuntu (otherwise using the same bits) changes that to run
> > PARALLEL_SHUTDOWN=10
> > SHUTDOWN_TIMEOUT=120
>
> Sweet. I went for the PARALLEL_SHUTDOWN=10 and left the 300. Maybe the
> default PARALLEL_SHUTDOWN value should not be 0 ?
>
> > I never got bugs about that config being too aggressive.
> > The change is old and as easy as:
> >
> https://git.launchpad.net/ubuntu/+source/libvirt/tree/debian/patches/ubuntu/parallel-shutdown.patch?h=ubuntu/focal-devel
> > Maybe you just want to open a bug with Debian to change the default
> > config there as well?
>
> No it is a bug in libvirt having the "wrong" defaults. And a bug in
> ubuntu not fixing it upstream ;).
>
No it isn't as it was discussed and nacked (years ago)
> Thanks,
> Henning
>
> > Steps to reproduce:
> > > - star a VM that does not support ACPI
> > > - reboot the host and wait 300s for the VM to be shut down
> > > - now start it multiple times
> > > - wait multiples of 300s for the shutdown
> > >
> > > Expected behaviour:
> > > - no matter how many instances do not support ACPI, make it 300s
> > > max because we shut them down in parallel
> > >
> > >
> > > regards,
> > > Henning
> > >
> > >
> > > --
> > > libvir-list mailing list
> > > libvir-list@redhat.com
> > > https://www.redhat.com/mailman/listinfo/libvir-list
> > >
> > >
> >
>
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
We had disabled this a year ago with 72afaed959f8; however, a few
months ago we started enabling EPEL on CentOS 7 with d63d6a59cc11,
and from that source we can install a recent Go compiler.
Signed-off-by: Andrea Bolognani
---
guests/host_vars/libvirt-centos-7/main.yml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/guests/host_vars/libvirt-centos-7/main.yml
b/guests/host_vars/libvirt-centos-7/main.yml
index 94e29af..e0579ad 100644
--- a/guests/host_vars/libvirt-centos-7/main.yml
+++ b/guests/host_vars/libvirt-centos-7/main.yml
@@ -5,6 +5,8 @@ projects:
- libvirt-cim
- libvirt-dbus
- libvirt-glib
+ - libvirt-go
+ - libvirt-go-xml
- libvirt-ocaml
- libvirt-perl
- libvirt-python
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Am Tue, 10 Dec 2019 12:01:24 +0100
schrieb Christian Ehrhardt :
> On Tue, Dec 10, 2019 at 10:46 AM Henning Schild
> wrote:
>
> > Hi all,
> >
> > the systemd shutdown scripts work sequentially with a 300s timeout
> > (seen on Debian). If a VM does not have ACPI support, or the ACPI
> > support failed for some reason, you are looking at a 300s timeout
> > per instance for a host shutdown/reboot.
> > i.e. 10 instances without working ACPI = 3000s to shut down
> >
> > I think the systemd scripting should be parallel instead of
> > sequentially. So if you have many VMs without working ACPI you just
> > have to wait 300s in total for the host to shut down.
> >
>
> Hi Henning,
> this is configurable in /etc/default/libvirt-guests
> For example Ubuntu (otherwise using the same bits) changes that to run
> PARALLEL_SHUTDOWN=10
> SHUTDOWN_TIMEOUT=120
Sweet. I went for the PARALLEL_SHUTDOWN=10 and left the 300. Maybe the
default PARALLEL_SHUTDOWN value should not be 0 ?
> I never got bugs about that config being too aggressive.
> The change is old and as easy as:
> https://git.launchpad.net/ubuntu/+source/libvirt/tree/debian/patches/ubuntu/parallel-shutdown.patch?h=ubuntu/focal-devel
> Maybe you just want to open a bug with Debian to change the default
> config there as well?
No it is a bug in libvirt having the "wrong" defaults. And a bug in
ubuntu not fixing it upstream ;).
Thanks,
Henning
> Steps to reproduce:
> > - star a VM that does not support ACPI
> > - reboot the host and wait 300s for the VM to be shut down
> > - now start it multiple times
> > - wait multiples of 300s for the shutdown
> >
> > Expected behaviour:
> > - no matter how many instances do not support ACPI, make it 300s
> > max because we shut them down in parallel
> >
> >
> > regards,
> > Henning
> >
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
> >
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/12/2019 12.06, Markus Armbruster wrote:
> Thomas Huth writes:
>
>> It's been deprecated since QEMU v3.1, so it's time to finally
>> remove it. The "id" parameter can simply be used instead.
>>
>> Signed-off-by: Thomas Huth
> [...]
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 335295be50..ff280ccd16 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -488,18 +488,16 @@
>> #
>> # @id: identifier for monitor commands
>> #
>> -# @name: identifier for monitor commands, ignored if @id is present
>> -#
>> # @opts: device type specific properties (legacy)
>> #
>> # Since: 1.2
>> #
>> # 'vlan': dropped in 3.0
>> +# 'name': dropped in 5.0
>> ##
>
> Uh, when did we start to add "dropped in" to our doc comments?
>
> We should either do this systematically, or not at all. If the former,
> we have quite a few "dropped in" to add belatedly.
IIRC this has been suggested by Eric, see e.g.:
https://patchwork.kernel.org/patch/10227335/#21501161
Anyway, it should not matter much for this patch here, since the line
gets removed again in the 2nd patch anyway.
Thomas
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Thomas Huth writes:
> It's been deprecated since QEMU v3.1, so it's time to finally
> remove it. The "id" parameter can simply be used instead.
>
> Signed-off-by: Thomas Huth
[...]
> diff --git a/qapi/net.json b/qapi/net.json
> index 335295be50..ff280ccd16 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -488,18 +488,16 @@
> #
> # @id: identifier for monitor commands
> #
> -# @name: identifier for monitor commands, ignored if @id is present
> -#
> # @opts: device type specific properties (legacy)
> #
> # Since: 1.2
> #
> # 'vlan': dropped in 3.0
> +# 'name': dropped in 5.0
> ##
Uh, when did we start to add "dropped in" to our doc comments?
We should either do this systematically, or not at all. If the former,
we have quite a few "dropped in" to add belatedly.
I vote for "not at all".
> { 'struct': 'NetLegacy',
>'data': {
> '*id': 'str',
> -'*name': 'str',
> 'opts': 'NetLegacyOptions' } }
>
> ##
[...]
History:
$ git-log -S"dropped in" -- qapi qapi-schema.json
commit ffaee83bcb28913b8b854aeab78b1a1f2115712d
Author: Markus Armbruster
Date: Tue Jul 9 17:20:53 2019 +0200
qapi: Move query-target from misc.json to machine.json
Move query-target and its return type TargetInfo from misc.json to
machine.json, where they are covered by MAINTAINERS section "Machine
core". Also move its implementation from arch_init.c to
hw/core/machine-qmp-cmds, where it is likewise covered.
All users of SysEmuTarget are now in machine.json. Move it there from
common.json.
Signed-off-by: Markus Armbruster
Message-Id: <20190709152053.16670-3-arm...@redhat.com>
commit 416756cc049006ab8a05fe39e5f2e6af25cad9d2
Author: Thomas Huth
Date: Tue Aug 21 13:27:48 2018 +0200
Record history of ppcemb target in common.json
We recently removed the long deprecated "ppcemb" target. This adds a
comment in common.json about the SysEmuTarget type, recording when it was
removed.
Suggested-by: Eric Blake
Signed-off-by: David Gibson
commit 4088b5536436090207dcf6d15e47908f74b2d8f2
Author: Thomas Huth
Date: Tue May 15 18:26:20 2018 +0200
qapi/net.json: Fix the version number of the "vlan" removal
"vlan" will be dropped in 2.13, not in 2.12. And while we're at it,
use the better wording "dropped in" instead of "removed with" (also
for the "dump" removal).
Reported-by: Stefan Hajnoczi
Reported-by: Eric Blake
Signed-off-by: Thomas Huth
Reviewed-by: Eric Blake
Signed-off-by: Michael Tokarev
commit 608cfed66a6adeac136b0c09cd62d081062256f3
Author: Markus Armbruster
Date: Thu Aug 24 21:14:00 2017 +0200
qapi-schema: Collect UI stuff in qapi/ui.json
UI stuff is remote desktop stuff (Spice, VNC) and input stuff (mouse,
keyboard).
Cc: Gerd Hoffmann
Signed-off-by: Markus Armbruster
Message-Id: <1503602048-12268-9-git-send-email-arm...@redhat.com>
Reviewed-by: Marc-André Lureau
Reviewed-by: Gerd Hoffmann
commit 912092b8e47f31c3db25e088af8460d9e752da29
Author: Gerd Hoffmann
Date: Thu Jul 27 12:47:20 2017 +0200
ui: drop altgr and altgr_r QKeyCodes
The right alt key (alt_r aka KEY_RIGHTALT) is used for AltGr.
The altgr and altgr_r keys simply don't exist. Drop them.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Eric Blake
Message-id: 20170727104720.30061-1-kra...@redhat.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Some comments were only present in the Ansible part and not in
the Jenkins part and vice versa.
Signed-off-by: Andrea Bolognani
---
Pushed as trivial.
guests/playbooks/build/projects/libvirt-tck.yml | 2 ++
jenkins/projects/libosinfo.yaml | 2 --
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/guests/playbooks/build/projects/libvirt-tck.yml
b/guests/playbooks/build/projects/libvirt-tck.yml
index ef88cd3..b8d848c 100644
--- a/guests/playbooks/build/projects/libvirt-tck.yml
+++ b/guests/playbooks/build/projects/libvirt-tck.yml
@@ -1,5 +1,7 @@
---
- set_fact:
+# CentOS 7 doesn't include perl-generators, which is necessary to
+# build libvirt-tck
name: libvirt-tck
machines:
- libvirt-debian-9
diff --git a/jenkins/projects/libosinfo.yaml b/jenkins/projects/libosinfo.yaml
index cde10ec..819c5ac 100644
--- a/jenkins/projects/libosinfo.yaml
+++ b/jenkins/projects/libosinfo.yaml
@@ -1,8 +1,6 @@
---
- project:
name: libosinfo
-# libosinfo depends on meson 0.49.0, which is not available on
-# CentOS 7, Debian 9, Ubuntu 18;
machines: '{all_machines}'
title: libosinfo
archive_format: xz
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 10:46 AM Henning Schild
wrote:
> Hi all,
>
> the systemd shutdown scripts work sequentially with a 300s timeout
> (seen on Debian). If a VM does not have ACPI support, or the ACPI
> support failed for some reason, you are looking at a 300s timeout per
> instance for a host shutdown/reboot.
> i.e. 10 instances without working ACPI = 3000s to shut down
>
> I think the systemd scripting should be parallel instead of
> sequentially. So if you have many VMs without working ACPI you just
> have to wait 300s in total for the host to shut down.
>
Hi Henning,
this is configurable in /etc/default/libvirt-guests
For example Ubuntu (otherwise using the same bits) changes that to run
PARALLEL_SHUTDOWN=10
SHUTDOWN_TIMEOUT=120
I never got bugs about that config being too aggressive.
The change is old and as easy as:
https://git.launchpad.net/ubuntu/+source/libvirt/tree/debian/patches/ubuntu/parallel-shutdown.patch?h=ubuntu/focal-devel
Maybe you just want to open a bug with Debian to change the default config
there as well?
Steps to reproduce:
> - star a VM that does not support ACPI
> - reboot the host and wait 300s for the VM to be shut down
> - now start it multiple times
> - wait multiples of 300s for the shutdown
>
> Expected behaviour:
> - no matter how many instances do not support ACPI, make it 300s max
>because we shut them down in parallel
>
>
> regards,
> Henning
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 09:58:38 +, Daniel Berrange wrote:
> On Mon, Dec 09, 2019 at 08:15:05PM -0300, Daniel Henrique Barboza wrote:
> > (series based on master commit 97cafa610ecf5)
> >
> > This work was proposed by Cole in [1]. This is Cole's reasoning for
> > it, copy/pasted from [1]:
> >
> > ---
> > The benefits of moving to validate time is that XML is rejected by
> > 'virsh define' rather than at 'virsh start' time. It also makes it easier
> > to follow the cli building code, and makes it easier to verify
> > qemu_command.c
> > test suite code coverage for the important stuff like covering every CLI
> > option. It's also a good intermediate step for sharing validation with
> > domain capabilities building, like is done presently for rng models.
> > ---
>
> I've not looked at the patches, but surely moving this validate from
> start time, to be define time errors is going to cause functional
> regressions in our ABI behaviour.
>
> My libvirtd daemons installs have many XML files defined which will
> fail validation at various points in time, depending on what QEMU
> builds I happen to have deployed. I only need them to pass the
> validation when actually starting the VM.
Validation is not done on the persistent or status XML files exactly for
this reason.
It's re-done when starting the VM so this should not be an issue.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 10 Dec 2019 10:36:36 +
Daniel P. Berrangé wrote:
> On Tue, Dec 10, 2019 at 11:24:44AM +0100, Cornelia Huck wrote:
> > On Tue, 10 Dec 2019 10:09:34 +
> > Daniel P. Berrangé wrote:
> >
> > > On Mon, Dec 09, 2019 at 02:23:38PM -0600, Jonathon Jongsma wrote:
> > > > mdevctl also supports assigning arbitrary sysfs attributes to a device.
> > > > These attributes have an explicit ordering and are written to sysfs in
> > > > the specified order when a device is started. This might be the only
> > > > thing that doesn't fit into the current xml format.
> >
> > Not sure how much the 'explicit ordering' is actually required by the
> > devices currently supporting this. It's probably a good idea to keep
> > this, though, as future device types might end up having such a
> > requirement.
> >
> > > Well we need to define a schema, but there will need to be some kind
> > > of validation added because. AFAICT, mdevctl does no validation, so a
> > > plain passthrough of this allows arbitrary writing of files anywhere
> > > on the host given a suitable malicious attribute name.
> >
> > Uh, we really should do something about that in mdevctl as well. Writes
> > outside the sysfs hierarchy should not be allowed.
>
> I'm pretty worried about overall safety/reliability of the mdevctrl
> tool in general. Given that it is written in shell, it is really hard
> to ensure that it isn't vulnerable to any shell quoting / meta character
> flaws, whether from malicious or accidental data input.
I'm not sure I'm trusting myself too much to get that right, either...
review obviously welcome, but this is shell, as you say.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 07:22:37AM -0300, Daniel Henrique Barboza wrote:
> Hi,
>
> I failed to make a reservation about what I've done in patch in the
> cover letter. In the commit msg I mentioned about the XML files
> considering SPICE support as default to ppc64, aarch64 and s390 archs and
> fixing all the xmls.
>
> It is worth disclaiming that what I can assert to be true is the lack of
> SPICE support for ppc64. I can't put my money on the lack of SPICE for
> the other 2 archs - what I did in the patch was to assume that the emulator
> capabilities for aarch64 and s390, that didn't report SPICE support, was
> accurate. I think it's an educated guess, but a guess nevertheless.
SPICE is present on arch64.
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
On Tue, Dec 10, 2019 at 11:24:44AM +0100, Cornelia Huck wrote:
> On Tue, 10 Dec 2019 10:09:34 +
> Daniel P. Berrangé wrote:
>
> > On Mon, Dec 09, 2019 at 02:23:38PM -0600, Jonathon Jongsma wrote:
> > > mdevctl also supports assigning arbitrary sysfs attributes to a device.
> > > These attributes have an explicit ordering and are written to sysfs in
> > > the specified order when a device is started. This might be the only
> > > thing that doesn't fit into the current xml format.
>
> Not sure how much the 'explicit ordering' is actually required by the
> devices currently supporting this. It's probably a good idea to keep
> this, though, as future device types might end up having such a
> requirement.
>
> > Well we need to define a schema, but there will need to be some kind
> > of validation added because. AFAICT, mdevctl does no validation, so a
> > plain passthrough of this allows arbitrary writing of files anywhere
> > on the host given a suitable malicious attribute name.
>
> Uh, we really should do something about that in mdevctl as well. Writes
> outside the sysfs hierarchy should not be allowed.
I'm pretty worried about overall safety/reliability of the mdevctrl
tool in general. Given that it is written in shell, it is really hard
to ensure that it isn't vulnerable to any shell quoting / meta character
flaws, whether from malicious or accidental data input.
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
Eduardo Habkost writes:
> +Markus
>
> On Tue, Dec 03, 2019 at 03:43:03PM +0100, Igor Mammedov wrote:
>> On Tue, 3 Dec 2019 09:56:15 +0100
>> Thomas Huth wrote:
>>
>> > On 02/12/2019 22.00, Eduardo Habkost wrote:
>> > > On Mon, Dec 02, 2019 at 08:39:48AM +0100, Igor Mammedov wrote:
>> > >> On Fri, 29 Nov 2019 18:46:12 +0100
>> > >> Paolo Bonzini wrote:
>> > >>
>> > >>> On 29/11/19 13:16, Igor Mammedov wrote:
>> > As for "-m", I'd make it just an alias that translates
>> > -m/mem-path/mem-prealloc
>> > >>>
>> > >>> I think we should just deprecate -mem-path/-mem-prealloc in 5.0. CCing
>> > >>> Thomas as mister deprecation. :)
>> > >>
>> > >> I'll add that to my series
>> > >
>> > > Considering that the plan is to eventually reimplement those
>> > > options as syntactic sugar for memory backend options (hopefully
>> > > in less than 2 QEMU releases), what's the point of deprecating
>> > > them?
>> >
>> > Well, it depends on the "classification" [1] of the parameter...
>> >
>> > Let's ask: What's the main purpose of the option?
>> >
>> > Is it easier to use than the "full" option, and thus likely to be used
>> > by a lot of people who run QEMU directly from the CLI? In that case it
>> > should stay as "convenience option" and not be deprecated.
>> >
>> > Or is the option merely there to give the upper layers like libvirt or
>> > some few users and their scripts some more grace period to adapt their
>> > code, but we all agree that the options are rather ugly and should
>> > finally go away? Then it's rather a "legacy option" and the deprecation
>> > process is the right way to go. Our QEMU interface is still way
>> > overcrowded, we should try to keep it as clean as possible.
>>
>> After switching to memdev for main RAM, users could use relatively
>> short global options
>> -global memory-backend.prealloc|share=on
>> and
>> -global memory-backend-file.mem-path=X|prealloc|share=on
>>
>> instead of us adding and maintaining slightly shorter
>> -mem-shared/-mem-path/-mem-prealloc
>
> Global properties are a convenient way to expose knobs through
> the command line with little effort, but we have no documentation
> on which QOM properties are really supposed to be touched by
> users using -global.
>
> Unless we fix the lack of documentation, I'd prefer to have
> syntactic sugar translated to -global instead of recommending
> direct usage of -global.
Fair point.
I'd take QOM property documentation over still more sugar.
Sometimes, the practical way to make simple things simple is sugar. I
can accept that. This doesn't look like such a case, though.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 03:27:06PM +0800, Wang Huaqiang wrote:
> This patch introduced a bug and broke the 'resctrl' feature.
>
> It introduced a 'divide by zero' error if you defined any 'resctrl'
>
> allocation group through .
>
>
> Reason is 'caps->resctrl' is fully initialized through two steps,
> 'virResctrlInfoNew'
>
> invokes 'virResctrlGetInfo' completes the first step, later,
> 'virResctrlInfoGetCache'
>
> accomplishes the filling of
> 'caps->resctrl->levels->types->control.granularity'.
Urgh, that is really horribly misleading API. An object getter
method should not be making changes to the object.
virResctrlInfoGetCache needs splitting into two methods - one
setter which updates the control granularity, and then make
this getter into something with no side-effects.
> The simplest way to fix the bug is drawback this patch, but still have the
> undesirable overhead.
I've posted the simple revert for now, since refactoring the code is
a bigger job.
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
