Re: [libvirt] question about syntax of storage volume element
On 9/28/18 12:54 AM, Jim Fehlig wrote:
> I've attempted to use virt-manager to create a new VM that uses a volume
> from an rbd-based network pool, but have not been able to progress past
> step 4/5 where VM storage is selected. It appears virt-manager has
> problems properly detecting the volume as network-based storage, but
> before investigating those further I have a question about the syntax of
> the element of a storage volume. The storage management page
> [0] of the website describing rbd volumes claims that the
> subelement contains an 'rbd:rbd/' prefix in the volume path. But the
> page describing pool and volume format [1] syntax does not contain any
> info wrt specifying network URLs in the subelement.
The [1] pool/vol page is meant to be a bit more "generic" with respect
to the general format of the .. field because some
fields can have different formats/expectations.
The [0] storage page should represent more details for each storage
backend's expectations over the format to be used. Although my
recollection is that it's modified/patched far less frequently than the
[1] page.
When in doubt though, I usually search "tests/*/*.{xml|args} and then of
course looking at the source code. If I must, I'll look at the RNG
files. In this case storagevol.rng says @path could be "anyURI" or
"absFilePath", nothing specific for RBD.
My git history search shows [0] seems to have been originally written
and intended as is seen now (commit 74951eade).
In the long run it doesn't matter if the has the "rbd:" prefix
because virStorageSourceParseRBDColonString will strip it before
duplicating and then later qemuBuildNetworkDriveStr will slap it back
onto the command line.
How's that for a round-about way of saying - it doesn't seem to matter.
>
> What is the expectation wrt the subelement of the
> element within rbd volume config? In general, should the
> subelement encode the scheme (e.g. rbd://) of a network-based volume?
> And if so, should it be formatted in the traditional 'rbd://' vs
> 'rbd:rbd/' syntax?
I don't think "rbd:rbd/" is the prefix, just the "rbd:". It just so
happens the example shows the RBD pool "source" name as "rbd" and the
"volume" as "myvol". If you look at the that's more indicative of
path to the volume. The example when paired with the pool of source name
"rbdpool" probably should have shown "rbd:rbdpool/myvol" and the key
"rbdpool/myvol" (to be less confusing).
As for "expected" format - well being consistent would be "nice" -
probably depends on when the code was originally added though. Adding
new or consistent formats ends up being a coding and documenting
exercise and we never seem to be able to "lose" old formats and aren't
supposed to "rewrite" someone's files - so we're stuck.
John
>
> Regards,
> Jim
>
> [0] https://libvirt.org/storage.html#StorageBackendRBD
> [1] https://libvirt.org/formatstorage.html#StorageVolTarget
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [tck PATCH] Fix runtime "undefined global" error in 100-disk-encryption.t
On 10/01/2018 04:07 PM, Laine Stump wrote:
> Commit 3836a38c added a $secret-undefine call at the end of
> 100-disk-encryption.t because the presence of the secret was
> reportedly causing an error when the test was run a 2nd
> time. Unfortunately the definition of "my $secret" was inside a SKIP:
> { ... } block, but the $secret->undefine was added just outside that
> block, so the test failed when it was run.
>
> Signed-off-by: Laine Stump
> ---
>
> NB: this test is disabled unconditionally at the start of the SKIP
> block anyway, so I'm not sure how Jim encountered the error leading to
> the original patch.
>
> scripts/qemu/100-disk-encryption.t | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qemu/100-disk-encryption.t
> b/scripts/qemu/100-disk-encryption.t
> index 1a36650..12386ae 100644
> --- a/scripts/qemu/100-disk-encryption.t
> +++ b/scripts/qemu/100-disk-encryption.t
> @@ -117,6 +117,6 @@ diag "Undefining the inactive domain config";
> $dom->undefine;
>
> ok_error(sub { $conn->get_domain_by_name("tck") }, "NO_DOMAIN error raised
> from missing domain", 42);
> -}
>
> $secret->undefine;
> +}
Reviewed-by: Yash Mankad
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [tck PATCH] Fix runtime "undefined global" error in 100-disk-encryption.t
Commit 3836a38c added a $secret-undefine call at the end of
100-disk-encryption.t because the presence of the secret was
reportedly causing an error when the test was run a 2nd
time. Unfortunately the definition of "my $secret" was inside a SKIP:
{ ... } block, but the $secret->undefine was added just outside that
block, so the test failed when it was run.
Signed-off-by: Laine Stump
---
NB: this test is disabled unconditionally at the start of the SKIP
block anyway, so I'm not sure how Jim encountered the error leading to
the original patch.
scripts/qemu/100-disk-encryption.t | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qemu/100-disk-encryption.t
b/scripts/qemu/100-disk-encryption.t
index 1a36650..12386ae 100644
--- a/scripts/qemu/100-disk-encryption.t
+++ b/scripts/qemu/100-disk-encryption.t
@@ -117,6 +117,6 @@ diag "Undefining the inactive domain config";
$dom->undefine;
ok_error(sub { $conn->get_domain_by_name("tck") }, "NO_DOMAIN error raised
from missing domain", 42);
-}
$secret->undefine;
+}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
On 9/28/18 5:36 AM, Wang Yechao wrote:
> After calling qemuAgentClose(), it is still possible for
> the QEMU Agent I/O event callback to get invoked. This
> will trigger an agent error because mon->fd has been set
> to -1 at this point. Then vm->privateData->agentError is
> always 'true' except that restart libvirtd or restart
> qemu-guest-agent process in guest.
>
> Silently ignore the case where mon->fd is -1, likewise for
> mon->watch being zero.
>
> Signed-off-by: Wang Yechao
> ---
> v1 patch:
> https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
>
> Changes in v2:
> - do not set agentError, let agent state as disconnected instead of error.
> ---
> src/qemu/qemu_agent.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
Since the v1 review referenced commit 89563efc which it seems you
applied or liberally copied here, I think it would be a "good idea" to
reference that commit in your commit message. Imitation is a form of
flattery and so as is attribution.
John
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 97ad0e7..d842b0e 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon)
> VIR_EVENT_HANDLE_HANGUP |
> VIR_EVENT_HANDLE_ERROR;
>
> +if (!mon->watch)
> +return;
> +
> if (mon->lastError.code == VIR_ERR_OK) {
> events |= VIR_EVENT_HANDLE_READABLE;
>
> @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
> VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd,
> events);
> #endif
>
> +if (mon->fd == -1 || mon->watch == 0) {
> +virObjectUnlock(mon);
> +virObjectUnref(mon);
> +return;
> +}
> +
> if (mon->fd != fd || mon->watch != watch) {
> if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> eof = true;
> @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon)
> virObjectLock(mon);
>
> if (mon->fd >= 0) {
> -if (mon->watch)
> +if (mon->watch) {
> virEventRemoveHandle(mon->watch);
> +mon->watch = 0;
> +}
> VIR_FORCE_CLOSE(mon->fd);
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] question about syntax of storage volume element
On 09/28/2018 12:54 AM, Jim Fehlig wrote: I've attempted to use virt-manager to create a new VM that uses a volume from an rbd-based network pool, but have not been able to progress past step 4/5 where VM storage is selected. It appears virt-manager has problems properly detecting the volume as network-based storage, but before investigating those further I have a question about the syntax of the element of a storage volume. Yeah virt-manager is known to be lacking WRT rbd. I did some work a few years back but didn't finish it. At least it doesn't know how to correctly use a volume with any auth data in the XML. I need to get another rbd setup to test with and fix it all The storage management page [0] of the website describing rbd volumes claims that the subelement contains an 'rbd:rbd/' prefix in the volume path. But the page describing pool and volume format [1] syntax does not contain any info wrt specifying network URLs in the subelement. What is the expectation wrt the subelement of the element within rbd volume config? In general, should the subelement encode the scheme (e.g. rbd://) of a network-based volume? And if so, should it be formatted in the traditional 'rbd://' vs 'rbd:rbd/' syntax? I noticed this too. sheepdog is weird as well. Gluster has full URIs in the volume though I certainly think full URIs is the way it should be... even if nothing natively handles the URI format we output, I think a field should be fully descriptive/unique, like how file volumes in directory pools show absolute paths. I guess the question though is if we can change it at this point, it's been like that for years, apps may be depending on the weird format in someway - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: reintroduce tests for libxl's legacy nested setting
On 10/1/18 2:35 AM, Erik Skultety wrote: On Mon, Oct 01, 2018 at 08:04:57AM +0200, Erik Skultety wrote: On Thu, Sep 27, 2018 at 08:25:53AM -0600, Jim Fehlig wrote: On 9/27/18 3:29 AM, Erik Skultety wrote: On Wed, Sep 26, 2018 at 11:31:19AM -0600, Jim Fehlig wrote: The preferred location for setting the nested CPU flag changed in Xen 4.10 and is advertised via the LIBXL_HAVE_BUILDINFO_NESTED_HVM define. Commit 95d19cd0 changed libxl to use the new preferred location but unconditionally changed the tests, causing 'make check' failures against Xen < 4.10 that do not contain the new location. Commit e94415d5 fixed the failures by only running the tests when LIBXL_HAVE_BUILDINFO_NESTED_HVM is defined. Since libvirt supports several versions of Xen that use the old nested location, it is prudent to test the flag is set correctly. This patch reintroduces the tests for the legacy location of the nested setting. Signed-off-by: Jim Fehlig --- We could probably get by with one test for the old nested location, in which case I'd drop vnuma-hvm-legacy-nest. Any opinions on that? I verified with a few different platforms. I don't have a better idea on what to do about the legacy tests, we either add more (even identical) test files or we figure out some black magic to do the same thing (not preferred). Anyway, to answer your question, even though it might be enough, I'd like to stay consistent and keep both, so that if one day someone is looking at the source they don't wonder why only one of them is being run in the legacy mode. I hope that makes sense. Yep, no problem. Should I push now or after release? Ah, sorry, we definitely want this in the release, so safe for freeze. I went ahead and pushed the patch myself, since DV plans on doing the release at some point during the day which might already by too late for you because of a different timezone. Yep, I would have missed the release. Thanks for pushing it! Regards Jim -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Release of libvirt-4.8.0
It is out, as planned, tagged in git, the signed tarball and rpms are at the usual place: ftp://libvirt.org/libvirt/ I also make a python binding release which include event testing updates, you can find those at ftp://libvirt.org/libvirt/python/ This is a relatively large release considering the number of patches going in please note the Removed Feature section below: New features: - Xen: Support PM Suspend and Wakeup The libxl driver now supports the virDomainPMSuspendForDuration and virDomainPMWakeup APIs. Removed features: - Xen: Drop support for Xen 4.4 and 4.5 Xen 4.4 and 4.5 are no longer supported by the Xen community. Drop support for these older versions and require Xen >= 4.6. - nwfilter: Disallow binding creation in session mode Ensure that a filter binding creation is not attempted in session mode and generates a proper error message. Improvements: - qemu: Retrieve guest hostname through QEMU Guest Agent command QEMU is now able to retrieve the guest hostname using a new QEMU-GA command called 'guest-get-host-name'. Virsh users can execute 'domhostname' for QEMU driver for domains configured to use the Guest Agent. - virsh: Implement vsh-table in virsh and virsh-admin The new API fixes problems with table-alignment, making the tables more readable and deals with unicode. Bug fixes: - storage: Allow inputvol to be encrypted When creating a storage volume based on another volume, the base input volume is allowed to be encrypted. - virsh: Require explicit --domain for domxml-to-native The --domain option for domxml-to-native virsh command has always been documented as required, but commit v4.3.0-127-gd86531daf2 accidentally made it optional. - lxc_monitor: Avoid AB / BA lock race A deadlock situation could occur when autostarting a LXC domain 'guest' due to two threads attempting to take opposing locks while holding opposing locks (AB BA problem). Thanks everybody for your help with this release. Enjoy ! Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veill...@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v4] news: Update for 4.8.0 release
On Mon, Oct 01, 2018 at 04:40:12PM +0200, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCH v4] news: Update for 4.8.0 release
Signed-off-by: Fabiano Fidêncio --- I went through the commits since last release and added a few notes which seem to be relevant for the news. In case you notice there's something missing, please, either send me the text to add it (and I'll re-sping this patch) and add the text before merging this one. Changes since v1: - Actually run make-check in order to be sure the changes are fine. Changes since v2: - Fixed and reworded some parts according to John's and Erik's suggestions. - Added, according to Erik's suggestion: - util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds - virsh: Implement vsh-table in virsh and virsh-admin Changes since v3: - Dropped: - util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds (by Peter's suggestion) - utils: Introduce resource monitor capability interface (by John's suggestion as this work is still a on-going work) - libxl: Add support to set shadow memory for any guest type (after a face-to-face discussion with Peter and Erik) -- docs/news.xml | 47 +++ 1 file changed, 47 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3ed6ff8aeb..166e641811 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -55,6 +55,15 @@ Drop support for these older versions and require Xen >= 4.6. + + + nwfilter: Disallow binding creation in session mode + + + Ensure that a filter binding creation is not attempted in session + mode and generates a proper error message. + + @@ -68,8 +77,46 @@ Guest Agent. + + + virsh: Implement vsh-table in virsh and virsh-admin + + + The new API fixes problems with table-alignment, making the tables + more readable and deals with unicode. + + + + + storage: Allow inputvol to be encrypted + + + When creating a storage volume based on another volume, the base + input volume is allowed to be encrypted. + + + + + virsh: Require explicit --domain for domxml-to-native + + + The --domain option for domxml-to-native virsh command has always + been documented as required, but commit v4.3.0-127-gd86531daf2 + accidentally made it optional. + + + + + lxc_monitor: Avoid AB / BA lock race + + + A deadlock situation could occur when autostarting a LXC domain + 'guest' due to two threads attempting to take opposing locks while + holding opposing locks (AB BA problem). + + -- 2.19.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: also label listen UNIX sockets
On Mon, Oct 01, 2018 at 01:14:34PM +0200, Ján Tomko wrote:
> On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
> > On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
> > > We switched to opening mode='bind' sockets ourselves:
> > > commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
> > > qemu: support passing pre-opened UNIX socket listen FD
> > > in v4.5.0-rc1~251
> > >
> > > Then fixed qemuBuildChrChardevStr to change libvirtd's label
> > > while creating the socket:
> > > commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
> > > qemu: ensure FDs passed to QEMU for chardevs have correct SELinux
> > > labels
> > > v4.5.0-rc1~52
> > >
> > > Also add labeling of these sockets to the DAC driver.
> > > Instead of trying to figure out which one was created by libvirt,
> > > label it if it exists.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1633389
> > >
> > > Signed-off-by: Ján Tomko
> > > ---
> > > src/security/security_dac.c | 7 ++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> > > index 62442745dd..da4a6c72fe 100644
> > > --- a/src/security/security_dac.c
> > > +++ b/src/security/security_dac.c
> > > @@ -1308,7 +1308,12 @@
> > > virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
> > > break;
> > >
> > > case VIR_DOMAIN_CHR_TYPE_UNIX:
> > > -if (!dev_source->data.nix.listen) {
> > > +if (!dev_source->data.nix.listen ||
> > > +(dev_source->data.nix.path &&
> > > + virFileExists(dev_source->data.nix.path))) {
> > > +/* Also label mode='bind' sockets if they exist,
> > > + * e.g. because they were created by libvirt
> > > + * and passed via FD */
> > > if (virSecurityDACSetOwnership(mgr, NULL,
> > > dev_source->data.nix.path,
> > > user, group) < 0)
> >
> > So we're labeling path even if nix.listen == false, shouldn't we check for
> > the
> > file's existence too? Or have we already done it and I missed this fact?
>
> For mode='connect', the path not existing is fatal, so we can safely
> call virSecurityDACSetOwnership and let it fail.
>
> For mode='bind', the path not existing is valid for QEMUs which do not
> support FD passing for chardevs. If QEMU supports FD passing, the
> path should exist because libvirt created it earlier. I could not think
> of a nicer idea on how to pass down the logic that decides whether
> libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is
> an odd place to be creating sockets), so I opted to use the file's
> existence as a witness of libvirt pre-creating it. Which is what I tried
> to say by this commit message snippet:
> > > Instead of trying to figure out which one was created by libvirt,
> > > label it if it exists.
>
> Shall I reword the message? E.g.:
> Instead of duplicating the logic which decides whether libvirt should
> pre-create the socket, assume an existing path means it was created by
> libvirt.
s/means/meaning that
...otherwise sounds meaningful...
>
>
> > Basically what I'm aiming at is that nix.path will always be set at this
> > point,
>
> Yes, the dev_source->data.nix.path condition might be superfluous. I was
> being overly cautious.
>
> As evidenced by:
> commit 614193fac67445a7e92bf620ffef726ed1bd6f07
>conf: Fix check for chardev source path
> An empty path should be caught by our validation.
>
> > either explicitly by the user (most cases) or it would have been generated
> > by
> > us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
> > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.
>
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN |
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)
>
> > I'm just simply wondering whether the
> > condition cannot be further simplified to:
> >
> > if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
>
> This introduces a change in behavior for mode='connect' - the DAC driver
> will no longer report an error for a non-existent path.
> I have not checked whether it's checked anywhere else (possibly the
> SELinux driver), but it is a legitimate error and I don't see the need
> to skip it.
>
> Moreover, this hides the reason for using virFileExists in the first
> place - it's only needed for type='listen' sockets.
Fair enough, let's got with your proposed change then (wait for after the
release though):
Reviewed-by: Erik Skultety
>
> Hope that answers your questions.
> Looking forward to hearing from you.
>
> Jano
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v3] news: Update for 4.8.0 release
On Mon, 2018-10-01 at 15:22 +0200, Peter Krempa wrote: > On Mon, Oct 01, 2018 at 15:04:37 +0200, Fabiano Fidêncio wrote: > > Signed-off-by: Fabiano Fidêncio > > --- > > I went through the commits since last release and added a few notes > > which seem to be relevant for the news. > > > > In case you notice there's something missing, please, either send > > me the > > text to add it (and I'll re-sping this patch) and add the text > > before > > merging this one. > > > > Changes since v1: > > - Actually run make-check in order to be sure the changes are fine. > > > > Changes since v2: > > - Fixed and reworded some parts according to John's and Erik's > > suggestions. > > - Added, according to Erik's suggestion: > > - util: Introduce VIR_AUTOCLOSE macro to automatically close > > files' fds > > - virsh: Implement vsh-table in virsh and virsh-admin > > --- > > docs/news.xml | 77 > > +++ > > 1 file changed, 77 insertions(+) > > > > diff --git a/docs/news.xml b/docs/news.xml > > index 3ed6ff8aeb..36692ba456 100644 > > --- a/docs/news.xml > > +++ b/docs/news.xml > > @@ -44,6 +44,18 @@ > >and virDomainPMWakeup APIs. > > > > > > + > > + > > + utils: Introduce monitor capability interface > > Missing word 'resource' > > > + > > + > > + The resource monitor has been introduced and it creates > > the interface > > + for getting the host capability of the resource monitor > > from the system > > + resource control. > > + The resource monitor takes the role of RDT monitoring > > groups and could > > + be used to monitor the resource consumption information. > > Not quite sure that this helps anybody in understanding the new > feature. May I ask for some help here to have it in a way that it would actually help someone to understand the new feature? > > > + > > + > > > > > > > > [...] > > > @@ -68,8 +89,64 @@ > >Guest Agent. > > > > > > + > > + > > + storage: Allow to use any format as input volume for > > encryption > > + > > + > > + Libvirt has supported 'raw' input volumes for encryption > > since 4.5.0. > > + Now, it's not going to limit the usage to 'raw' only > > anymore. > > + > > + > > + > > + > > + libxl: Add support to set shadow memory for any guest > > type > > + > > + > > + PVH guests now can take advantage of using shadow > > memory. > > I'm not quite sure the users will know what this means. And here as well? > > > + > > + > > + > > + > > + util: Introduce VIR_AUTOCLOSE macro to automatically > > close files' fds > > + > > + > > + The Macro automatically force closes the fds by calling > > + virForceCloseHelper when the fd goes out of > > scope and is > > + used to eliminate VIR_FORCE_CLOSE in > > cleanup sections. > > + > > This is internal stuff not really worth mentioning in the news. Okay, I'll drop this one. > > > + -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v3] news: Update for 4.8.0 release
On Mon, Oct 01, 2018 at 03:04:37PM +0200, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > I went through the commits since last release and added a few notes > which seem to be relevant for the news. > > In case you notice there's something missing, please, either send me the > text to add it (and I'll re-sping this patch) and add the text before > merging this one. > > Changes since v1: > - Actually run make-check in order to be sure the changes are fine. > > Changes since v2: > - Fixed and reworded some parts according to John's and Erik's > suggestions. > - Added, according to Erik's suggestion: > - util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds > - virsh: Implement vsh-table in virsh and virsh-admin > --- > docs/news.xml | 77 +++ > 1 file changed, 77 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index 3ed6ff8aeb..36692ba456 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -44,6 +44,18 @@ >and virDomainPMWakeup APIs. > > > + > + > + utils: Introduce monitor capability interface > + > + > + The resource monitor has been introduced and it creates the > interface > + for getting the host capability of the resource monitor from the > system > + resource control. > + The resource monitor takes the role of RDT monitoring groups and > could > + be used to monitor the resource consumption information. > + > + > > > > @@ -55,6 +67,15 @@ >Drop support for these older versions and require Xen >= 4.6. > > > + > + > + nwfilter: Disallow binding creation in session mode > + > + > + Ensure that a filter binding creation is not attempted in session > + mode and generates a proper error message. > + > + > > > > @@ -68,8 +89,64 @@ >Guest Agent. > > > + > + > + storage: Allow to use any format as input volume for encryption > + > + > + Libvirt has supported 'raw' input volumes for encryption since > 4.5.0. > + Now, it's not going to limit the usage to 'raw' only anymore. > + So I followed John's comments and looked at the stuff more thoroughly. First of all, you probably want to reference commit b975afc7 instead of ^this one, since that has the actual bugzilla on it + the other 2 patches in that series did some necessary preparations. After reading the bugzilla and going through the code, it truly is a bugfix rather than an improvement and my wording would simply be: When creating a storage volume based on another volume, the base input volume is allowed to be encrypted. I don't have any further comments, since you already got some from Peter. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v3] news: Update for 4.8.0 release
On Mon, Oct 01, 2018 at 15:04:37 +0200, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > I went through the commits since last release and added a few notes > which seem to be relevant for the news. > > In case you notice there's something missing, please, either send me the > text to add it (and I'll re-sping this patch) and add the text before > merging this one. > > Changes since v1: > - Actually run make-check in order to be sure the changes are fine. > > Changes since v2: > - Fixed and reworded some parts according to John's and Erik's > suggestions. > - Added, according to Erik's suggestion: > - util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds > - virsh: Implement vsh-table in virsh and virsh-admin > --- > docs/news.xml | 77 +++ > 1 file changed, 77 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index 3ed6ff8aeb..36692ba456 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -44,6 +44,18 @@ >and virDomainPMWakeup APIs. > > > + > + > + utils: Introduce monitor capability interface Missing word 'resource' > + > + > + The resource monitor has been introduced and it creates the > interface > + for getting the host capability of the resource monitor from the > system > + resource control. > + The resource monitor takes the role of RDT monitoring groups and > could > + be used to monitor the resource consumption information. Not quite sure that this helps anybody in understanding the new feature. > + > + > > > [...] > @@ -68,8 +89,64 @@ >Guest Agent. > > > + > + > + storage: Allow to use any format as input volume for encryption > + > + > + Libvirt has supported 'raw' input volumes for encryption since > 4.5.0. > + Now, it's not going to limit the usage to 'raw' only anymore. > + > + > + > + > + libxl: Add support to set shadow memory for any guest type > + > + > + PVH guests now can take advantage of using shadow memory. I'm not quite sure the users will know what this means. > + > + > + > + > + util: Introduce VIR_AUTOCLOSE macro to automatically close files' > fds > + > + > + The Macro automatically force closes the fds by calling > + virForceCloseHelper when the fd goes out of scope and > is > + used to eliminate VIR_FORCE_CLOSE in cleanup sections. > + This is internal stuff not really worth mentioning in the news. > + -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa wrote: > On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote: >> On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa >> wrote: >> > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan >> >> wrote: >> >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> >> Move the domainPostParseDataAlloc/Free calls to >> >> >> virDomainDeviceDefParse. As an consequence >> >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> >> removed. >> >> >> >> >> >> Signed-off-by: Marc Hartmayer >> >> >> Reviewed-by: Boris Fiuczynski >> >> >> --- >> >> >> src/conf/domain_conf.c | 37 +++-- >> >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> >> >> > >> >> > I'm not against this per se; however, I should not that the code was >> >> > specifically extracted in commit e168bc8a. >> >> >> >> There are the following three functions: >> >> >> >> virDomainDeviceDefParse >> >> virDomainDeviceDefPostParse >> >> virDomainDeviceDefPostParseOne >> >> >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> >> the allocation of private data across the callbacks. This is absolutely >> >> fine. >> >> >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse instead of having an extra wrapper function >> >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> >> the QEMU caps for the virDomainDeviceDefValidate call in >> >> virDomainDeviceDefParse as well. >> > >> > For the above it's not necessary to open-code what >> > virDomainDeviceDefPostParseOne >> > in a rather massive function. You can pass the opaque in if you want. >> >> I don’t get it. Where can I pass the opaque? > > virDomainDeviceDefParse is a big spaghettified function and > adding other code to it is unwanted. If you need to unify the allocation > of private data, move the validation function call to the helper (with > appropriate rename) rather than blatantly pasting the code back. Ahhh okay. Any ideas for the name? Thanks. Side note: What has always surprised me is that the "virDomainDeviceDefPostParse" function is called right out of the context of the function "virDomainDeviceDefParse". Shouldn’t it be called directly _after_ virDomainDeviceDefParse? -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCH v3] news: Update for 4.8.0 release
Signed-off-by: Fabiano Fidêncio --- I went through the commits since last release and added a few notes which seem to be relevant for the news. In case you notice there's something missing, please, either send me the text to add it (and I'll re-sping this patch) and add the text before merging this one. Changes since v1: - Actually run make-check in order to be sure the changes are fine. Changes since v2: - Fixed and reworded some parts according to John's and Erik's suggestions. - Added, according to Erik's suggestion: - util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds - virsh: Implement vsh-table in virsh and virsh-admin --- docs/news.xml | 77 +++ 1 file changed, 77 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3ed6ff8aeb..36692ba456 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,18 @@ and virDomainPMWakeup APIs. + + + utils: Introduce monitor capability interface + + + The resource monitor has been introduced and it creates the interface + for getting the host capability of the resource monitor from the system + resource control. + The resource monitor takes the role of RDT monitoring groups and could + be used to monitor the resource consumption information. + + @@ -55,6 +67,15 @@ Drop support for these older versions and require Xen >= 4.6. + + + nwfilter: Disallow binding creation in session mode + + + Ensure that a filter binding creation is not attempted in session + mode and generates a proper error message. + + @@ -68,8 +89,64 @@ Guest Agent. + + + storage: Allow to use any format as input volume for encryption + + + Libvirt has supported 'raw' input volumes for encryption since 4.5.0. + Now, it's not going to limit the usage to 'raw' only anymore. + + + + + libxl: Add support to set shadow memory for any guest type + + + PVH guests now can take advantage of using shadow memory. + + + + + util: Introduce VIR_AUTOCLOSE macro to automatically close files' fds + + + The Macro automatically force closes the fds by calling + virForceCloseHelper when the fd goes out of scope and is + used to eliminate VIR_FORCE_CLOSE in cleanup sections. + + + + + virsh: Implement vsh-table in virsh and virsh-admin + + + The new API fixes problems with table-alignment, making the tables + more readable and deals with unicode. + + + + + virsh: Require explicit --domain for domxml-to-native + + + The --domain option for domxml-to-native virsh command has always + been documented as required, but commit v4.3.0-127-gd86531daf2 + accidentally made it optional. + + + + + lxc_monitor: Avoid AB / BA lock race + + + A deadlock situation could occur when autostarting a LXC domain + 'guest' due to two threads attempting to take opposing locks while + holding opposing locks (AB BA problem). + + -- 2.19.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 14:58:11 +0200, Marc Hartmayer wrote: > On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa > wrote: [...] > >> Yep, that’s why I said in the cover letter “With this patch series the > >> behavior is still not (completely) fixed, but the performance has been > >> significantly improved.”. > > > > I don't see what should be fixed here, but mostly as I did not read the > > series. > > The problem is that we currently don’t take into account that the QEMU > binary can change during a task (e.g. define a domain). Therefore, it’s > possible that two calls of virQEMUCapsCacheLookup return different QEMU > capabilities. I don't think that is too big of a problem in the light that the binary can change between the final call to virQEMUCapsCacheLookup which is used by the code generating the commandline and actual exec of the qemu binary once the commandline is generated. Solving that won't be easy at all. Since virDomainDefValidate is called with the final capabilities instance when starting the qemu process the possibility that it would change and the problems it might create are negligible when compared to actually execing something else. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa wrote: […snip…] >> I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t >> valid anymore) as early as possible in the overall “task/job” process. > > Well, technically priv->qemuCaps is invalid and unneeded when the VM is > not running and valid and immutable if it is running. > > If the VM is not running, there is no qemu process associated to it so > it does not make sense to store random qemuCaps along with it, since the > qemu binary might change at the point when we attempt to start it. Yep, that makes sense. > > I was not a fan of needing qemuCaps in the post parse infrastructure of > qemu, but at this point we can't do much about it especially as we want > to keep the config stable after defined. > >> > This patch does not seem to make sense with the justification provided >> > here as the post-parse callbacks fill in the proper caps here and this >> > part clearly uses a new domain configuration, so the default behaviour >> > should be used. >> > >> > Changing this would also need that we change the normal parser path to >> > achieve the same results which is impossible as you don't have access to >> > priv->qemuCaps prior to creating the virDomainObj object. >> >> Yep, that’s why I said in the cover letter “With this patch series the >> behavior is still not (completely) fixed, but the performance has been >> significantly improved.”. > > I don't see what should be fixed here, but mostly as I did not read the > series. The problem is that we currently don’t take into account that the QEMU binary can change during a task (e.g. define a domain). Therefore, it’s possible that two calls of virQEMUCapsCacheLookup return different QEMU capabilities. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote: > On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa > wrote: > > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: > >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan > >> wrote: > >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: > >> >> Move the domainPostParseDataAlloc/Free calls to > >> >> virDomainDeviceDefParse. As an consequence > >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be > >> >> removed. > >> >> > >> >> Signed-off-by: Marc Hartmayer > >> >> Reviewed-by: Boris Fiuczynski > >> >> --- > >> >> src/conf/domain_conf.c | 37 +++-- > >> >> 1 file changed, 11 insertions(+), 26 deletions(-) > >> >> > >> > > >> > I'm not against this per se; however, I should not that the code was > >> > specifically extracted in commit e168bc8a. > >> > >> There are the following three functions: > >> > >> virDomainDeviceDefParse > >> virDomainDeviceDefPostParse > >> virDomainDeviceDefPostParseOne > >> > >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid > >> the allocation of private data across the callbacks. This is absolutely > >> fine. > >> > >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to > >> virDomainDeviceDefParse instead of having an extra wrapper function > >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse > >> the QEMU caps for the virDomainDeviceDefValidate call in > >> virDomainDeviceDefParse as well. > > > > For the above it's not necessary to open-code what > > virDomainDeviceDefPostParseOne > > in a rather massive function. You can pass the opaque in if you want. > > I don’t get it. Where can I pass the opaque? virDomainDeviceDefParse is a big spaghettified function and adding other code to it is unwanted. If you need to unify the allocation of private data, move the validation function call to the helper (with appropriate rename) rather than blatantly pasting the code back. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
On Thu, Sep 20, 2018 at 19:44:50 +0200, Marc Hartmayer wrote:
> This allows the QEMU driver to use the originally used QEMU
> capabilities for copying the domain. It avoids the usage of a possible
> changed QEMU capability as virQEMUCapsCacheLookup might return a
> different QEMU capability than used before (for example when during
> the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy
> will invalidate the originally used QEMU capability).
>
> For other drivers @parseOpqaue will still be NULL, because
> xmlopt->privateData.getParseOpaque is currently only implemented for
> the QEMU driver.
>
> Signed-off-by: Marc Hartmayer
> Reviewed-by: Stefan Zimmermann
> Reviewed-by: Boris Fiuczynski
> ---
> src/conf/domain_conf.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1ee43950ae2d..a3f2fcb0a001 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps,
> virDomainObjPtr domain)
> {
> int ret = -1;
> +void *parseOpaque = NULL;
>
> if (!domain->persistent)
> return 0;
> @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps,
> if (domain->newDef)
> return 0;
>
> -if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL,
> false)))
> +if (xmlopt->privateData.getParseOpaque)
> +parseOpaque = xmlopt->privateData.getParseOpaque(domain);
I'd prefer if the caller passes this in explicitly since virDomainDefCopy
uses it explicitly.
Additionally the caller will be aware where the pointer is taken. The
callback is really used only in the place where we are restoring the
existing domain object from the status XML and thus we really need the
specific caps they were used. Since the code is common the callback
needs to be used.
Ideally priv->qemuCaps will be cleared when the VM is NOT running so at
the point above you'd get NULL anyways in most cases.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa wrote: > On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: >> On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan >> wrote: >> > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> >> Move the domainPostParseDataAlloc/Free calls to >> >> virDomainDeviceDefParse. As an consequence >> >> virDomainDeviceDefPostParseOne is superfluous and can therefore be >> >> removed. >> >> >> >> Signed-off-by: Marc Hartmayer >> >> Reviewed-by: Boris Fiuczynski >> >> --- >> >> src/conf/domain_conf.c | 37 +++-- >> >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> >> > >> > I'm not against this per se; however, I should not that the code was >> > specifically extracted in commit e168bc8a. >> >> There are the following three functions: >> >> virDomainDeviceDefParse >> virDomainDeviceDefPostParse >> virDomainDeviceDefPostParseOne >> >> Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid >> the allocation of private data across the callbacks. This is absolutely >> fine. >> >> What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to >> virDomainDeviceDefParse instead of having an extra wrapper function >> (virDomainDeviceDefPostParse/One) for that. With this change I can reuse >> the QEMU caps for the virDomainDeviceDefValidate call in >> virDomainDeviceDefParse as well. > > For the above it's not necessary to open-code what > virDomainDeviceDefPostParseOne > in a rather massive function. You can pass the opaque in if you want. I don’t get it. Where can I pass the opaque? At the end, we must use the same qemuCaps in virDomainDeviceDefValidate that we already used for virDomainDeviceDefPostParse(One). > > Please don't remove the wrapper. > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v4 2/4] block/rbd: Attempt to parse legacy filenames
On Sat, Sep 22, 2018 at 08:18:26AM +0200, Markus Armbruster wrote:
> Jeff Cody writes:
>
> > When we converted rbd to get rid of the older key/value-centric
> > encoding format, we broke compatibility with image files with backing
> > file strings encoded in the old format.
> >
> > This leaves a bit of an ugly conundrum, and a hacky solution.
> >
> > If the initial attempt to parse the "proper" options fails, it assumes
> > that we may have an older key/value encoded filename. Fall back to
> > attempting to parse the filename, and extract the required options from
> > it. If that fails, pass along the original error message.
> >
> > We do not support mixed modern usage alongside legacy keyvalue pair
> > usage.
> >
> > A deprecation warning has been added, although care should be taken
> > when actually deprecating since the impact is not limited to
> > commandline or qapi usage, but also opening existing images.
> >
> > Reviewed-by: Eric Blake
> > Signed-off-by: Jeff Cody
> > ---
> > block/rbd.c | 53 +++--
> > 1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index b199450f9f..5090e4f662 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -678,6 +678,33 @@ static int qemu_rbd_convert_options(QDict *options,
> > BlockdevOptionsRbd **opts,
> > return 0;
> > }
> >
> > +static int qemu_rbd_attempt_legacy_options(QDict *options,
> > + BlockdevOptionsRbd **opts,
> > + char **keypairs)
> > +{
> > +char *filename;
> > +int r;
> > +
> > +filename = g_strdup(qdict_get_try_str(options, "filename"));
> > +if (!filename) {
> > +return -EINVAL;
> > +}
> > +qdict_del(options, "filename");
> > +
> > +qemu_rbd_parse_filename(filename, options, NULL);
> > +
> > +/* keypairs freed by caller */
> > +*keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
> > +if (*keypairs) {
> > +qdict_del(options, "=keyvalue-pairs");
> > +}
> > +
> > +r = qemu_rbd_convert_options(options, opts, NULL);
> > +
> > +g_free(filename);
> > +return r;
> > +}
> > +
> > static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
> > Error **errp)
> > {
> > @@ -700,8 +727,30 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> > *options, int flags,
> >
> > r = qemu_rbd_convert_options(options, , _err);
> > if (local_err) {
> > -error_propagate(errp, local_err);
> > -goto out;
> > +/* If keypairs are present, that means some options are present in
> > + * the modern option format. Don't attempt to parse legacy option
> > + * formats, as we won't support mixed usage. */
> > +if (keypairs) {
> > +error_propagate(errp, local_err);
> > +goto out;
> > +}
> > +
> > +/* If the initial attempt to convert and process the options
> > failed,
> > + * we may be attempting to open an image file that has the rbd
> > options
> > + * specified in the older format consisting of all key/value pairs
> > + * encoded in the filename. Go ahead and attempt to parse the
> > + * filename, and see if we can pull out the required options. */
> > +r = qemu_rbd_attempt_legacy_options(options, , );
> > +if (r < 0) {
> > +error_propagate(errp, local_err);
> > +goto out;
>
> This reports the error from qemu_rbd_convert_options(), as you commit
> message explains. Would explaining it in a comment help future readers?
>
> > +}
> > +/* Take care whenever deciding to actually deprecate; once this
> > ability
> > + * is removed, we will not be able to open any images with
> > legacy-styled
> > + * backing image strings. */
> > +error_report("RBD options encoded in the filename as keyvalue
> > pairs "
> > + "is deprecated. Future versions may cease to parse "
> > + "these options in the future.");
>
> "Future versions may ... in the future": you're serious about this
> happening only in the future, aren't you? ;)
Eric noticed this as well :)
>
> Quote error_report()'s contract: "The resulting message should be a
> single phrase, with no newline or trailing punctuation."
>
> Let's scratch everything from the first period on.
>
Since the two requested changes are comments only and minor, and a PR has
already been sent, I went ahead and updated the patch and will send a v2
PR with these patches. I left the r-b for this patch untouched.
> > }
> >
> > /* Remove the processed options from the QDict (the visitor processes
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] news: Update for 4.8.0 release
On 10/01/2018 01:58 PM, Erik Skultety wrote: > On Mon, Oct 01, 2018 at 12:57:18PM +0200, Fabiano Fidêncio wrote: >> Signed-off-by: Fabiano Fidêncio >> --- >> I went through the commits since last release and added a few notes >> which seem to be relevant for the news. >> >> In case you notice there's something missing, please, either send me the >> text to add it (and I'll re-sping this patch) and add the text before >> merging this one. >> >> Changes since v1: >> - Actually run make-check in order to be sure the changes are fine. >> --- >> docs/news.xml | 69 +++ >> 1 file changed, 69 insertions(+) >> > Now, this is just a matter of preference, you did a good job, but I'd probably > also mention the following: > 09d35afd2c util: file: introduce VIR_AUTOCLOSE macro to close fd of the file > automatic Is it something that should interest our users? In my view it's just a matter of our internal implementation. We haven't mention VIR_AUTOFREE() back in the day. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] Faster libvirtd restart with nwfilter rules
ping On 24.09.2018 10:41, Nikolay Shirokovskiy wrote: > Hi, all. > > > On fat hosts which are capable to run hundreds of VMs restarting libvirtd > makes it's services unavailable for a long time if VMs use network filters. > In > my tests each of 100 VMs has no-promisc [1] and no-mac-spoofing filters and > executing virsh list right after daemon restart takes appoximately 140s if no > firewalld is running (that is ebtables/iptables/ip6tables commands are used > to > configure kernel tables). > > > The problem is daemon does not even start to read from client connections > because state drivers are not initialized. Initialization is blocked in state > > drivers autostart which grabs VMs locks. And VMs locks are hold by VMs > reconnection code. Each VM reloads network tables on reconnection and this > > reloading is serialized on updateMutex in gentech nwfilter driver. > Workarounding autostart won't help much because even if state drivers will > initialize listing VM won't be possible because listing VMs takes each VM > lock > one by one too. However managing VM that passed reconnection phase will be > > possible which takes same 140s in worst case. > > > Note that this issue is only applicable if we use filters configuration > that > don't need ip learning. In the latter case situation is different because > reconnection code spawns new thread that apply network rules only after ip is > > learned from traffic and this thread does not grab VM lock. As result VMs are > > managable but reloading filters in background takes appoximately those same > 140s. I guess managing network filters during this period can have issues > too. > Anyway this situation does not look good so fixing the described issue by > > spawning threads even without ip learning does not look nice to me. > > > What speed up is possible on conservative approach? First we can remove for > > test purpuses firewall ruleLock, gentech dirver updateMutex and filter object > > mutex which do not serve function in restart scenario. This gives 36s restart > > time. The speed up is archived because heavy fork/preexec steps are now run > > concurrently. > > Next we can try to reduce fork/preexec time. To estimate its contibution alone > let's bring back the above locks. It turns out the most time takes fork itself > and closing 8k (on my system) file descriptors in preexec. Using vfork gives > 2x boost and so does dropping mass close. (I check this mass close > contribution > because I not quite understand the purpose of this step - libvirt typically > set > close-on-exec flag on it's descriptors). So this two optimizations alone can > result in restart time of 30s. > > Unfortunately combining the above two approaches does not give boost multiple > of them along. The reason is due to concurrency and high number of VMs (100) > preexec boost does not have significant role and using vfork dininishes > concurrency as it freezes all parent threads before execve. So dropping locks > and closes gives 33s restart time and adding vfork to this gives 25s restart > time. > > Another approach is to use --atomic-file option for ebtables > (iptables/ip6tables unfortunately does not have one). The idea is to save > table > to file/edit file/commit table to kernel. I hoped this could give performance > boost because we don't need to load/store kernel network table for a single > rule update. In order to isolate approaches I also dropped all ip/ip6 updates > which can not be done this way. In this approach we can not drop ruleLock in > firewall because no other VM threads should change tables between save/commit. > This approach gives restart time 25s. But this approach is broken anyway as we > can not be sure another application doesn't change newtork table between > save/commit in which case these changes will be lost. > > After all I think we need to move in a different direction. We can add API to > all binaries and firewalld to execute many commands in one run. We
Re: [libvirt] [PATCH v2] news: Update for 4.8.0 release
On 10/1/18 6:57 AM, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > I went through the commits since last release and added a few notes > which seem to be relevant for the news. > > In case you notice there's something missing, please, either send me the > text to add it (and I'll re-sping this patch) and add the text before > merging this one. > > Changes since v1: > - Actually run make-check in order to be sure the changes are fine. > --- > docs/news.xml | 69 +++ > 1 file changed, 69 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index 3ed6ff8aeb..62148d28af 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -44,6 +44,26 @@ >and virDomainPMWakeup APIs. > > > + > + > + utils: Introduce monitor capability interface > + > + > + The resource monitor has been introduces and it creates the > interface *introduced But I agree w/ Erik, this is the same as the following. > + for getting the host capability of the resource monitor from the > system > + resource control. > + The resource monitor takes the role of RDT monitoring groups and > could > + be used to monitor the resource consumption information. > + > + > + > + > + conf: Introduce RDT monitor host capability > + > + > + Introduce cache monitor (CMT) and memory bandwidth monitor (MBM). > + > + This is all for the Resource Control util subsystem. Essentially a way to monitor the Cache and Memory Bandwidth and provide capabilities to know what type of data points are available through the domain capabilities. > > > > @@ -55,6 +75,15 @@ >Drop support for these older versions and require Xen >= 4.6. > > > + > + > + nwfilter: Disallow binding creation in session mode > + > + > + Ensure that a filter binding creation is not attempted in session > + mode and generate the proper error message. > + > + FWIW: IDC - but I guess a long time ago I got conditioned to not supply news.xml articles for every bz fixed. I'd be more inclined to just drop this one especially since nwfilter binding is so new, but I'm also fine with noting it. > > > > @@ -68,8 +97,48 @@ >Guest Agent. > > > + > + > + storage: Allow to use any format as input volume for encryption > + > + > + Since v4.5.0 libvirt has support to use a 'raw' input volume for > + encryption. From now on, let's not limit this to 'raw' only. > + > + This is all about LUKS encryption... Prior to 4.5 encrypted volumes was supported though - so I'd be careful to note a version. The 4.5 change was to "fix" the code in order to handle the multi-step processing that qemu-img now needs to encrypt a LUKS volume. At best this is a we're now allowing the input volume to be encrypted. > + > + > + libxl: Add support to set shadow memory for any guest type > + > + > + PVH guests now can take advantage of using shadow memory. > + > + > > > + > + > + virsh: Require explicit --domain for domxml-to-native > + > + > + The domxml-to-native virsh command accepts either --xml or --domain > + option followed by a file or domain name respectively, The --domain > + option is documented as required, which means an argument with no > + option is treated as --xml. Commit v4.3.0-127-gd86531daf2 broke > this > + by making --domain optional and thus an argument with no option was > + treated as --domain. > + > + > + > + > + lxc_monitor: Avoid AB / BA lock race > + > + > + A dealock situation could occur when autostarting a LXC domain > 'guest' deadlock John > + due to two threads attempting to take opposing locks while holding > + opposing locks (AB BA problem). > + > + > > > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] news: Update for 4.8.0 release
On Mon, Oct 01, 2018 at 12:57:18PM +0200, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > I went through the commits since last release and added a few notes > which seem to be relevant for the news. > > In case you notice there's something missing, please, either send me the > text to add it (and I'll re-sping this patch) and add the text before > merging this one. > > Changes since v1: > - Actually run make-check in order to be sure the changes are fine. > --- > docs/news.xml | 69 +++ > 1 file changed, 69 insertions(+) > > diff --git a/docs/news.xml b/docs/news.xml > index 3ed6ff8aeb..62148d28af 100644 > --- a/docs/news.xml > +++ b/docs/news.xml > @@ -44,6 +44,26 @@ >and virDomainPMWakeup APIs. > > > + > + > + utils: Introduce monitor capability interface > + > + > + The resource monitor has been introduces and it creates the > interface > + for getting the host capability of the resource monitor from the > system > + resource control. > + The resource monitor takes the role of RDT monitoring groups and > could > + be used to monitor the resource consumption information. > + I think ^this is related to the entry below, so that would IMHO suffice. > + > + > + > + conf: Introduce RDT monitor host capability > + > + > + Introduce cache monitor (CMT) and memory bandwidth monitor (MBM). > + > + > > > > @@ -55,6 +75,15 @@ >Drop support for these older versions and require Xen >= 4.6. > > > + > + > + nwfilter: Disallow binding creation in session mode > + > + > + Ensure that a filter binding creation is not attempted in session > + mode and generate the proper error message. s/the proper/a proper > + > + > > > > @@ -68,8 +97,48 @@ >Guest Agent. > > > + > + > + storage: Allow to use any format as input volume for encryption > + > + > + Since v4.5.0 libvirt has support to use a 'raw' input volume for > + encryption. From now on, let's not limit this to 'raw' only. I'd say: Libvirt has supported 'raw' input volumes for encryption since 4.5.0. Now, it's not going to limit the usage to 'raw' only anymore. > + > + > + > + > + libxl: Add support to set shadow memory for any guest type > + > + > + PVH guests now can take advantage of using shadow memory. > + > + > > > + > + > + virsh: Require explicit --domain for domxml-to-native > + > + > + The domxml-to-native virsh command accepts either --xml or --domain > + option followed by a file or domain name respectively, The --domain > + option is documented as required, which means an argument with no > + option is treated as --xml. Commit v4.3.0-127-gd86531daf2 broke > this > + by making --domain optional and thus an argument with no option was > + treated as --domain. Too verbose, how about: The --domain option for domxml-to-native virsh command has always been documented as required, but commit v4.3.0-127-gd86531daf2 accidentally made it optional. > + > + > + > + > + lxc_monitor: Avoid AB / BA lock race > + > + > + A dealock situation could occur when autostarting a LXC domain > 'guest' > + due to two threads attempting to take opposing locks while holding > + opposing locks (AB BA problem). > + > + > > > Now, this is just a matter of preference, you did a good job, but I'd probably also mention the following: 09d35afd2c util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatic ally 0396cf5336 virsh: Implement vsh-table to iface-list (basically extending it to the whole virsh) your call...with the grammar adjustments mentioned above: Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] uml: Fix weird logic inside umlConnectOpen() function.
On 9/28/18 12:13 AM, Julio Faracco wrote:
> The pointer related to uml_driver needs to be checked before its usage
> inside the function. Some attributes of the driver are being accessed
> while the pointer is NULL considering the current logic.
>
I see it's pushed already... Perhaps something in the future to consider
- when finding stuff like this, look for the commit that broke the
original logic, then indicate in the commit message what you found.
For this commit 0420a0324 changed to check @uml_driver->privileged
before !uml_driver was checked.
> Signed-off-by: Julio Faracco
> ---
> src/uml/uml_driver.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index fcd468243e..d1c71d8521 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -1193,6 +1193,13 @@ static virDrvOpenStatus umlConnectOpen(virConnectPtr
> conn,
> {
> virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
>
> +/* URI was good, but driver isn't active */
^^ This comment is wrong now ;-)
No big deal...
In any case the logic at least is the same as most other drivers now.
John
> +if (uml_driver == NULL) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("uml state driver is not active"));
> +return VIR_DRV_OPEN_ERROR;
> +}
> +
> /* Check path and tell them correct path if they made a mistake */
> if (uml_driver->privileged) {
> if (STRNEQ(conn->uri->path, "/system") &&
> @@ -1211,13 +1218,6 @@ static virDrvOpenStatus umlConnectOpen(virConnectPtr
> conn,
> }
> }
>
> -/* URI was good, but driver isn't active */
> -if (uml_driver == NULL) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("uml state driver is not active"));
> -return VIR_DRV_OPEN_ERROR;
> -}
> -
> if (virConnectOpenEnsureACL(conn) < 0)
> return VIR_DRV_OPEN_ERROR;
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: also label listen UNIX sockets
On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
We switched to opening mode='bind' sockets ourselves:
commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
qemu: support passing pre-opened UNIX socket listen FD
in v4.5.0-rc1~251
Then fixed qemuBuildChrChardevStr to change libvirtd's label
while creating the socket:
commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
v4.5.0-rc1~52
Also add labeling of these sockets to the DAC driver.
Instead of trying to figure out which one was created by libvirt,
label it if it exists.
https://bugzilla.redhat.com/show_bug.cgi?id=1633389
Signed-off-by: Ján Tomko
---
src/security/security_dac.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 62442745dd..da4a6c72fe 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
-if (!dev_source->data.nix.listen) {
+if (!dev_source->data.nix.listen ||
+(dev_source->data.nix.path &&
+ virFileExists(dev_source->data.nix.path))) {
+/* Also label mode='bind' sockets if they exist,
+ * e.g. because they were created by libvirt
+ * and passed via FD */
if (virSecurityDACSetOwnership(mgr, NULL,
dev_source->data.nix.path,
user, group) < 0)
So we're labeling path even if nix.listen == false, shouldn't we check for the
file's existence too? Or have we already done it and I missed this fact?
For mode='connect', the path not existing is fatal, so we can safely
call virSecurityDACSetOwnership and let it fail.
For mode='bind', the path not existing is valid for QEMUs which do not
support FD passing for chardevs. If QEMU supports FD passing, the
path should exist because libvirt created it earlier. I could not think
of a nicer idea on how to pass down the logic that decides whether
libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is
an odd place to be creating sockets), so I opted to use the file's
existence as a witness of libvirt pre-creating it. Which is what I tried
to say by this commit message snippet:
Instead of trying to figure out which one was created by libvirt,
label it if it exists.
Shall I reword the message? E.g.:
Instead of duplicating the logic which decides whether libvirt should
pre-create the socket, assume an existing path means it was created by
libvirt.
Basically what I'm aiming at is that nix.path will always be set at this point,
Yes, the dev_source->data.nix.path condition might be superfluous. I was
being overly cautious.
As evidenced by:
commit 614193fac67445a7e92bf620ffef726ed1bd6f07
conf: Fix check for chardev source path
An empty path should be caught by our validation.
either explicitly by the user (most cases) or it would have been generated by
us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN |
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)
I'm just simply wondering whether the
condition cannot be further simplified to:
if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
This introduces a change in behavior for mode='connect' - the DAC driver
will no longer report an error for a non-existent path.
I have not checked whether it's checked anywhere else (possibly the
SELinux driver), but it is a legitimate error and I don't see the need
to skip it.
Moreover, this hides the reason for using virFileExists in the first
place - it's only needed for type='listen' sockets.
Hope that answers your questions.
Looking forward to hearing from you.
Jano
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] news: Update for 4.8.0 release
Signed-off-by: Fabiano Fidêncio --- I went through the commits since last release and added a few notes which seem to be relevant for the news. In case you notice there's something missing, please, either send me the text to add it (and I'll re-sping this patch) and add the text before merging this one. Changes since v1: - Actually run make-check in order to be sure the changes are fine. --- docs/news.xml | 69 +++ 1 file changed, 69 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3ed6ff8aeb..62148d28af 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,26 @@ and virDomainPMWakeup APIs. + + + utils: Introduce monitor capability interface + + + The resource monitor has been introduces and it creates the interface + for getting the host capability of the resource monitor from the system + resource control. + The resource monitor takes the role of RDT monitoring groups and could + be used to monitor the resource consumption information. + + + + + conf: Introduce RDT monitor host capability + + + Introduce cache monitor (CMT) and memory bandwidth monitor (MBM). + + @@ -55,6 +75,15 @@ Drop support for these older versions and require Xen >= 4.6. + + + nwfilter: Disallow binding creation in session mode + + + Ensure that a filter binding creation is not attempted in session + mode and generate the proper error message. + + @@ -68,8 +97,48 @@ Guest Agent. + + + storage: Allow to use any format as input volume for encryption + + + Since v4.5.0 libvirt has support to use a 'raw' input volume for + encryption. From now on, let's not limit this to 'raw' only. + + + + + libxl: Add support to set shadow memory for any guest type + + + PVH guests now can take advantage of using shadow memory. + + + + + virsh: Require explicit --domain for domxml-to-native + + + The domxml-to-native virsh command accepts either --xml or --domain + option followed by a file or domain name respectively, The --domain + option is documented as required, which means an argument with no + option is treated as --xml. Commit v4.3.0-127-gd86531daf2 broke this + by making --domain optional and thus an argument with no option was + treated as --domain. + + + + + lxc_monitor: Avoid AB / BA lock race + + + A dealock situation could occur when autostarting a LXC domain 'guest' + due to two threads attempting to take opposing locks while holding + opposing locks (AB BA problem). + + -- 2.19.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: also label listen UNIX sockets
On Mon, Oct 01, 2018 at 10:34:38AM +0200, Michal Privoznik wrote: On 09/27/2018 05:02 PM, Ján Tomko wrote: We switched to opening mode='bind' sockets ourselves: commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 qemu: support passing pre-opened UNIX socket listen FD in v4.5.0-rc1~251 Then fixed qemuBuildChrChardevStr to change libvirtd's label while creating the socket: commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels v4.5.0-rc1~52 Also add labeling of these sockets to the DAC driver. Instead of trying to figure out which one was created by libvirt, label it if it exists. https://bugzilla.redhat.com/show_bug.cgi?id=1633389 Signed-off-by: Ján Tomko --- src/security/security_dac.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) How come SELinux is not affected? We shouldn't rely on default policy doing the right thing. As mentioned in the commit message, the SELinux label is set before the socket creation since the commit mentioned in the commit message. Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate
On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> For the usage of the parameter @parseOpqaue within
>> virDomainDefValidate it must be ensured that the parameter
>> @parseOpaque is not NULL. But since there are code paths where
>> virDomainDefValidate is called with @parseOpaque ==
>> NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue ==
>> NULL leads to this).
>>
>> To prevent this, domainPostParseDataAlloc is called within
>> virDomainDefValidate to ensure that @parseOpaque is always set. The
>> usage of domainPostParseDataAlloc within virDomainDefValidate is
>> allowed since virDomainDefValidate is only called after
>> virDomainDefPostParse.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 21 +
>> 1 file changed, 21 insertions(+)
>>
>
> Thus is similar to commit e168bc8a did for virDomainDefPostParse...
Yes. That was actually my “inspiration”.
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index ae7f3ed95faf..4f1c569b5733 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def,
>> virDomainXMLOptionPtr xmlopt,
>> void *parseOpaque)
>> {
>> +int ret = -1;
>> +bool localParseOpaque = false;
>> +
>> struct virDomainDefPostParseDeviceIteratorData data = {
>> .caps = caps,
>> .xmlopt = xmlopt,
>> @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def,
>> if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)
>> return 0;
>>
>> +if (!data.parseOpaque &&
>> +xmlopt->config.domainPostParseDataAlloc) {
>> +ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags,
>> + xmlopt->config.priv,
>> + );
>
> So ret = 1 if virQEMUCapsCacheLookup failed in
> qemuDomainPostParseDataAlloc; otherwise, ret = 0; Just looks strange
> below with the ret == 1 check.
Yes, do you have something better in mind? Maybe we can add an error
label. While fixing it we can also adapt virDomainDefPostParse (were
it’s, in my opinion, even more confusing since @ret is used multiple
times).
>
>> +
>> +if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
>
> This though has/uses specific @parseFlag options and sets
> def->postParseFailed when it may not be true if for some reason of
> course the post parse processing never got the parseOpaque value.
> Wouldn't it perhaps be problematic in the patch3 scenario where you'd be
> passing the priv->qemuCaps when @postParseFailed?
See my answer for patch 3.
>
> John
>
>> +goto cleanup;
>> +localParseOpaque = true;
>> +}
>> +
>> /* call the domain config callback */
>> if (xmlopt->config.domainValidateCallback &&
>> xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv, data.parseOpaque) < 0)
>> @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def,
>> if (virDomainDefValidateInternal(def) < 0)
>> return -1;
>>
>> + cleanup:
>> +if (localParseOpaque && xmlopt->config.domainPostParseDataFree)
>> +xmlopt->config.domainPostParseDataFree(data.parseOpaque);
>> +
>> +if (ret == 1)
>> +return -1;
>> +
>> return 0;
>> }
>>
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote: > On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan > wrote: > > On 9/20/18 1:44 PM, Marc Hartmayer wrote: > >> Move the domainPostParseDataAlloc/Free calls to > >> virDomainDeviceDefParse. As an consequence > >> virDomainDeviceDefPostParseOne is superfluous and can therefore be > >> removed. > >> > >> Signed-off-by: Marc Hartmayer > >> Reviewed-by: Boris Fiuczynski > >> --- > >> src/conf/domain_conf.c | 37 +++-- > >> 1 file changed, 11 insertions(+), 26 deletions(-) > >> > > > > I'm not against this per se; however, I should not that the code was > > specifically extracted in commit e168bc8a. > > There are the following three functions: > > virDomainDeviceDefParse > virDomainDeviceDefPostParse > virDomainDeviceDefPostParseOne > > Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid > the allocation of private data across the callbacks. This is absolutely > fine. > > What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to > virDomainDeviceDefParse instead of having an extra wrapper function > (virDomainDeviceDefPostParse/One) for that. With this change I can reuse > the QEMU caps for the virDomainDeviceDefValidate call in > virDomainDeviceDefParse as well. For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want. Please don't remove the wrapper. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 10:31:36 +0200, Marc Hartmayer wrote:
> On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa
> wrote:
> > On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
> >>
> >>
> >> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> >> > ...although priv->qemuCaps will be NULL in almost every case when the
> >> > post parse callback has failed. That may change in the future.
> >> >
> >> > Signed-off-by: Marc Hartmayer
> >> > Reviewed-by: Boris Fiuczynski
> >> > ---
> >> > src/qemu/qemu_process.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >> > index 44c63c42d618..6c5a6472d8cd 100644
> >> > --- a/src/qemu/qemu_process.c
> >> > +++ b/src/qemu/qemu_process.c
> >> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
> >>
> >> The comment just above here could use a tweak for grammar ;-):
> >>
> >> /* in case when the post parse callback failed we need to re-run it
> >> on the
> >> * old config prior we start the VM */
> >>
> >> > if (vm->def->postParseFailed) {
> >> > VIR_DEBUG("re-running the post parse callback");
> >> >
> >> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> >> > NULL) < 0)
> >> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> >> > priv->qemuCaps) < 0)
> >>
> >> Searching through history of this line finds Peter's original commit in
> >> this area - 7726d158, which seems to indicate a very specific reason for
> >> providing a NULL capabilities value here.
> >>
> >> I think from this patch on is something Peter has worked on a lot, so I
> >> would prefer to defer to Peter on them since I'm sure he understands all
> >> the various PostParse and Validate special conditions and various flags
> >> usage better than I do.
> >
> > Thanks for notifying me.
>
> First, thanks for your answer!
>
> >
> > In general, when parsing a "new" domain config as it's the case here,
> > NULL should be passed in.
>
> Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps)
> explicitly beforehand? (for example if the 'postParseFailed' flag is set
> and we’re starting the domain).
As stated below, in the normal code-flow at the point when the XML is
parsed there is no virDomainObj and thus no priv.
Even when you create a virDomainObj prior to that, the priv->qemuCaps is
based on the binary which in turn is based on defaults added by the post
parse callback as the binary is a qemu-driver-specific.
> > The qemu driver registers the 'domainPostParseDataAlloc' callback to
> > qemuDomainPostParseDataAlloc. This callback is executed after the
> > 'basic' post parse callback which fills in the emulator binary.
> >
> > Passing an explicit qemuCaps is meant only for special cases e.g.
> > migration where we have a specific set of capabilities which need to be
> > used rather than a new one.
>
> I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t
> valid anymore) as early as possible in the overall “task/job” process.
Well, technically priv->qemuCaps is invalid and unneeded when the VM is
not running and valid and immutable if it is running.
If the VM is not running, there is no qemu process associated to it so
it does not make sense to store random qemuCaps along with it, since the
qemu binary might change at the point when we attempt to start it.
I was not a fan of needing qemuCaps in the post parse infrastructure of
qemu, but at this point we can't do much about it especially as we want
to keep the config stable after defined.
> > This patch does not seem to make sense with the justification provided
> > here as the post-parse callbacks fill in the proper caps here and this
> > part clearly uses a new domain configuration, so the default behaviour
> > should be used.
> >
> > Changing this would also need that we change the normal parser path to
> > achieve the same results which is impossible as you don't have access to
> > priv->qemuCaps prior to creating the virDomainObj object.
>
> Yep, that’s why I said in the cover letter “With this patch series the
> behavior is still not (completely) fixed, but the performance has been
> significantly improved.”.
I don't see what should be fixed here, but mostly as I did not read the
series.
> IMHO, it’s a design flaw that the virDomainObjList class is responsible
> for the creation of the virDomainObj… The responsibilities of the
> classes are mixed and this enforces the lock order virDomainObjList ->
> virDomainObj (what actually is a performance bottleneck). IMHO, we
> should create the virDomainObj earlier and add it to the
> virDomainObjList when it’s useful.
I think that is orthogonal from the post parse infrastructure. Post
parse applies on virDomainDef object, while the domain list is of
virDomainObj type. That is thre reason to have private copy of the caps
in the post parse code flow. You don't have to associate a config with a
Re: [libvirt] [libvirt PATCH] news: Update for 4.8.0 release
On Mon, 2018-10-01 at 11:48 +0200, Fabiano Fidêncio wrote: > Signed-off-by: Fabiano Fidêncio > --- > I went through the commits since last release and added a few notes > which seem to be relevant for the news. > > In case you notice there's something missing, please, either send me > the > text to add it (and I'll re-sping this patch) and add the text before > merging this one. Self-nack! Let me submit a v2. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/11] conf: Extend virDomainDefValidate(Callback) for parseOpaque
On Sat, Sep 29, 2018 at 05:34 PM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> Add @parseOpaque argument to virDomainDefValidate and
>> virDomainDefValidateCallback, but don't use it for now since it's not
>> ensured that it's always a non-NULL value.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 11 +++
>> src/conf/domain_conf.h | 6 --
>> src/qemu/qemu_domain.c | 3 ++-
>> src/qemu/qemu_process.c | 2 +-
>> src/vz/vz_driver.c | 3 ++-
>> 5 files changed, 16 insertions(+), 9 deletions(-)
>>
>
> I like this idea especially since the Validate paths are the ones where
> qemuCaps seem to be most useful.
>
> Couple of nits below
>
> John
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index e61f04ea2271..ae7f3ed95faf 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6271,6 +6271,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
>> * @caps: driver capabilities object
>> * @parseFlags: virDomainDefParseFlags
>> * @xmlopt: XML parser option object
>> + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's
>> qemuCaps)
>> *
>> * This validation function is designed to take checks of globally invalid
>> * configurations that the parser needs to accept so that VMs don't vanish
>> upon
>> @@ -6284,12 +6285,14 @@ int
>> virDomainDefValidate(virDomainDefPtr def,
>> virCapsPtr caps,
>> unsigned int parseFlags,
>> - virDomainXMLOptionPtr xmlopt)
>> + virDomainXMLOptionPtr xmlopt,
>> + void *parseOpaque)
>> {
>> struct virDomainDefPostParseDeviceIteratorData data = {
>> .caps = caps,
>> .xmlopt = xmlopt,
>> .parseFlags = parseFlags,
>> +.parseOpaque = parseOpaque,
>> };
>>
>> /* validate configuration only in certain places */
>> @@ -6298,7 +6301,7 @@ virDomainDefValidate(virDomainDefPtr def,
>>
>> /* call the domain config callback */
>> if (xmlopt->config.domainValidateCallback &&
>> -xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv) < 0)
>> +xmlopt->config.domainValidateCallback(def, caps,
>> xmlopt->config.priv, data.parseOpaque) < 0)
>> return -1;
>>
>> /* iterate the devices */
>> @@ -21063,7 +21066,7 @@ virDomainObjParseXML(xmlDocPtr xml,
>> goto error;
>>
>> /* valdiate configuration */
>
> May as well fix the typo above *validate
Will change.
>
>> -if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0)
>> +if (virDomainDefValidate(obj->def, caps, flags, xmlopt, parseOpaque) <
>> 0)
>> goto error;
>>
>> return obj;
>> @@ -21154,7 +21157,7 @@ virDomainDefParseNode(xmlDocPtr xml,
>> goto cleanup;
>>
>> /* valdiate configuration */
>
> Consistency is the key ;-)
Yep :D
>
>> -if (virDomainDefValidate(def, caps, flags, xmlopt) < 0)
>> +if (virDomainDefValidate(def, caps, flags, xmlopt, parseOpaque) < 0)
>> goto cleanup;
>>
[…snip…]
>> static int
>> vzDomainDefValidate(const virDomainDef *def,
>> virCapsPtr caps ATTRIBUTE_UNUSED,
>> -void *opaque)
>> +void *opaque,
>> +void *parserOpaque ATTRIBUTE_UNUSED)
>
> nit: @parseOpaque
Okay.
>
>> {
>> if (vzCheckUnsupportedControllers(def, opaque) < 0)
>> return -1;
>>
>
Thanks for the review!
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> Move the domainPostParseDataAlloc/Free calls to
>> virDomainDeviceDefParse. As an consequence
>> virDomainDeviceDefPostParseOne is superfluous and can therefore be
>> removed.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 37 +++--
>> 1 file changed, 11 insertions(+), 26 deletions(-)
>>
>
> I'm not against this per se; however, I should not that the code was
> specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse
virDomainDeviceDefPostParse
virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid
the allocation of private data across the callbacks. This is absolutely
fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to
virDomainDeviceDefParse instead of having an extra wrapper function
(virDomainDeviceDefPostParse/One) for that. With this change I can reuse
the QEMU caps for the virDomainDeviceDefValidate call in
virDomainDeviceDefParse as well.
For safety I added Peter to CC.
>
> John
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3c307d325a89..e61f04ea2271 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>> return 0;
>> }
>>
>> -static int
>> -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev,
>> - const virDomainDef *def,
>> - virCapsPtr caps,
>> - unsigned int flags,
>> - virDomainXMLOptionPtr xmlopt)
>> -{
>> -void *parseOpaque = NULL;
>> -int ret;
>> -
>> -if (xmlopt->config.domainPostParseDataAlloc) {
>> -if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
>> -xmlopt->config.priv,
>> -) < 0)
>> -return -1;
>> -}
>> -
>> -ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt,
>> parseOpaque);
>> -
>> -if (parseOpaque && xmlopt->config.domainPostParseDataFree)
>> -xmlopt->config.domainPostParseDataFree(parseOpaque);
>> -
>> -return ret;
>> -}
>> -
>>
>> struct virDomainDefPostParseDeviceIteratorData {
>> virCapsPtr caps;
>> @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>> {
>> xmlDocPtr xml;
>> xmlNodePtr node;
>> +void *parseOpaque = NULL;
>> xmlXPathContextPtr ctxt = NULL;
>> virDomainDeviceDefPtr dev = NULL;
>> char *netprefix;
>> @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr,
>> break;
>> }
>>
>> +if (xmlopt->config.domainPostParseDataAlloc) {
>> +if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags,
>> +xmlopt->config.priv,
>> +) < 0)
>> +goto error;
>> +}
>> +
>> /* callback to fill driver specific device aspects */
>> -if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0)
>> +if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt,
>> parseOpaque) < 0)
>> goto error;
>>
>> /* validate the configuration */
>> @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr,
>> goto error;
>>
>> cleanup:
>> +if (parseOpaque && xmlopt->config.domainPostParseDataFree)
>> +xmlopt->config.domainPostParseDataFree(parseOpaque);
>> xmlFreeDoc(xml);
>> xmlXPathFreeContext(ctxt);
>> return dev;
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [libvirt PATCH] news: Update for 4.8.0 release
Signed-off-by: Fabiano Fidêncio --- I went through the commits since last release and added a few notes which seem to be relevant for the news. In case you notice there's something missing, please, either send me the text to add it (and I'll re-sping this patch) and add the text before merging this one. --- docs/news.xml | 88 +++ 1 file changed, 88 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 3ed6ff8aeb..29b12cb04d 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -44,6 +44,50 @@ and virDomainPMWakeup APIs. + +utils: Introduce monitor capability interface + + +The resource monitor has been introduced and it creates the interface +for getting the host capability of the resource monitor from the system +resource control. +The resource monitor takes the role of RDT monitoring groups and could +be used to monitor the resource consumption information. + + +conf: Introduce RDT monitor host capability + + +Introduce cache monitor (CMT) and memory bandwidth monitor (MBM). +For the former (CMT), the host capability is shown like: + host + ... +cache + bank id='0' level='3' type='both' size='15' unit='MiB' cpus='0-5' +control granularity='768' min='1536' unit='KiB' type='both' maxAllocs='4'/ + /bank + monitor level='3' 'reuseThreshold'='270336' maxMonitors='176' +feature name='llc_occupancy'/ + /monitor +/cache +... + /host + +For the latter (MBM), the capability is shown like this: + host +... +memory_bandwidth + node id='1' cpus='6-11' +control granularity='10' min ='10' maxAllocs='4'/ + /node + monitor maxMonitors='176' +feature name='mbm_total_bytes'/ +feature name='mbm_local_bytes'/ + /monitor +/memory_bandwidth +... + /host + @@ -55,6 +99,15 @@ Drop support for these older versions and require Xen >= 4.6. + + + nwfilter: Disallow binding creation in session mode + + + Ensure that a filter binding creation is not attempted in session + mode and generate the proper error message. + + @@ -68,8 +121,43 @@ Guest Agent. + +storage: Allow to use any format as input volume for encryption + + +Since v4.5.0 libvirt has support to use a 'raw' input volume for +encryption. From now on, let's not limit this to 'raw' only. + + +libxl: Add support to set shadow memory for any guest type, not only +HVM + + +PVH guests now can take advantage of using shadow memory. + + + + virsh: Require explicit --domain for domxml-to-native + + + The domxml-to-native virsh command accepts either --xml or --domain + option followed by a file or domain name respectively, The --domain + option is documented as required, which means an argument with no + option is treated as --xml. Commit v4.3.0-127-gd86531daf2 broke this + by making --domain optional and thus an argument with no option was + treated as --domain. + + + +lxc_monitor: Avoid AB / BA lock race + + +A dealock situation could occur when autostarting a LXC domain 'guest' +due to two threads attempting to take opposing locks while holding +opposing locks (AB BA problem). + -- 2.19.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] Various Coverity based concerns
On 09/28/2018 05:28 PM, John Ferlan wrote:
> I'm sure it'll be felt one or two could just be false positives,
> but I have 35-40 of true false positives and it seems at least
> these go above just noise on the channel.
>
> Perhaps the most difficult one to immediately see was the libxl
> refcnt patch. That involves a little bit of theory and has been
> in my noise pile for a while until I noted that the @args is
> being added in a loop to a callback function that just Unref's
> it when done. So if there was more than 1 IP Address, then all
> sorts of fun things could happen. Without any change, the Alloc
> is matched by the Unref, but with the change we add a Ref to
> match each Unref in the I/O loop and we just ensure the Unref
> is done for the path that put @args into the I/O callback.
>
> I also think the nwfilter patch was "interesting" insomuch as
> it has my "favorite" 'if (int-value) {' condition. IOW, if
> not zero, then do something. What became "interesting" is that
> the virNWFilterIPAddrMapDelIPAddr could return -1 if the
> virHashLookup on @req->binding->portdevname returned NULL,
> so when "shrinking" the code to only call the instantiation
> for/when there was an IP Address found resolves a couple of
> issues in the code.
>
> John Ferlan (7):
> lxc: Only check @nparams in lxcDomainBlockStatsFlags
> libxl: Fix possible object refcnt issue
> tests: Inline a sysconf call for linuxCPUStatsToBuf
> util: Data overrun may lead to divide by zero
> tests: Alter logic in testCompareXMLToDomConfig
> tests: Use STRNEQ_NULLABLE
> nwfilter: Alter virNWFilterSnoopReqLeaseDel logic
>
> src/libxl/libxl_migration.c | 4 ++--
> src/lxc/lxc_driver.c | 2 +-
> src/nwfilter/nwfilter_dhcpsnoop.c | 9 -
> src/util/virutil.c| 11 +--
> tests/commandtest.c | 4 ++--
> tests/libxlxml2domconfigtest.c| 11 +--
> tests/virhostcputest.c| 12 ++--
> 7 files changed, 29 insertions(+), 24 deletions(-)
>
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/11] conf: Add function description for virDomainDefPostParse
On Sat, Sep 29, 2018 at 04:08 AM +0200, John Ferlan wrote: > On 9/20/18 1:44 PM, Marc Hartmayer wrote: >> Signed-off-by: Marc Hartmayer >> Reviewed-by: Boris Fiuczynski >> --- >> src/conf/domain_conf.c | 13 + >> 1 file changed, 13 insertions(+) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index a3f2fcb0a001..3c307d325a89 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -5215,6 +5215,19 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def, >> } >> >> >> +/** >> + * virDomainDefPostParse: >> + * @def: domain definition >> + * @caps: driver capabilities object >> + * @parseFlags: virDomainDefParseFlags >> + * @xmlopt: XML parser option object >> + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's >> qemuCaps) >> + * >> + * This function does various common and hypervisor specific auto >> + * generation, verification, and validation. > > I think this is a good start. I'd really like to see what the > "expectations" are for various @parseFlags. By far one of the more > confusing things. Also while we're at it - @parseOpaque details and/or > assumptions w/r/t whether it should be provided or not. I really hope this function will be divided into separate functions in some time… Currently, it does way _too_ many things. > > I also note a few callers use the comment "/* callback to fill driver > specific domain aspects */", so that maybe good to pull in... Okay. > > John > >> + * >> + * Returns 0 on success, -1 on error >> + */ >> int >> virDomainDefPostParse(virDomainDefPtr def, >>virCapsPtr caps, >> > -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: agent: Avoid agentError when closing the QEMU agent
On 09/28/2018 11:36 AM, Wang Yechao wrote:
> After calling qemuAgentClose(), it is still possible for
> the QEMU Agent I/O event callback to get invoked. This
> will trigger an agent error because mon->fd has been set
> to -1 at this point. Then vm->privateData->agentError is
> always 'true' except that restart libvirtd or restart
> qemu-guest-agent process in guest.
>
> Silently ignore the case where mon->fd is -1, likewise for
> mon->watch being zero.
>
> Signed-off-by: Wang Yechao
> ---
> v1 patch:
> https://www.redhat.com/archives/libvir-list/2018-September/msg01382.html
>
> Changes in v2:
> - do not set agentError, let agent state as disconnected instead of error.
> ---
> src/qemu/qemu_agent.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 97ad0e7..d842b0e 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -530,6 +530,9 @@ static void qemuAgentUpdateWatch(qemuAgentPtr mon)
> VIR_EVENT_HANDLE_HANGUP |
> VIR_EVENT_HANDLE_ERROR;
>
> +if (!mon->watch)
> +return;
> +
> if (mon->lastError.code == VIR_ERR_OK) {
> events |= VIR_EVENT_HANDLE_READABLE;
>
> @@ -555,6 +558,12 @@ qemuAgentIO(int watch, int fd, int events, void *opaque)
> VIR_DEBUG("Agent %p I/O on watch %d fd %d events %d", mon, watch, fd,
> events);
> #endif
>
> +if (mon->fd == -1 || mon->watch == 0) {
> +virObjectUnlock(mon);
> +virObjectUnref(mon);
> +return;
> +}
> +
Is this safe to do? What if there's a thread waiting for a message from
the agent? Shouldn't @eof variable be set in this case so that
appropriate code is run?
> if (mon->fd != fd || mon->watch != watch) {
> if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR))
> eof = true;
> @@ -788,8 +797,10 @@ void qemuAgentClose(qemuAgentPtr mon)
> virObjectLock(mon);
>
> if (mon->fd >= 0) {
> -if (mon->watch)
> +if (mon->watch) {
> virEventRemoveHandle(mon->watch);
> +mon->watch = 0;
> +}
> VIR_FORCE_CLOSE(mon->fd);
> }
>
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
On Sat, Sep 29, 2018 at 04:06 AM +0200, John Ferlan wrote:
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> This allows the QEMU driver to use the originally used QEMU
>> capabilities for copying the domain. It avoids the usage of a possible
>> changed QEMU capability as virQEMUCapsCacheLookup might return a
>> different QEMU capability than used before (for example when during
>> the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy
>> will invalidate the originally used QEMU capability).
>>
>> For other drivers @parseOpqaue will still be NULL, because
>> xmlopt->privateData.getParseOpaque is currently only implemented for
>> the QEMU driver.
>>
>> Signed-off-by: Marc Hartmayer
>> Reviewed-by: Stefan Zimmermann
>> Reviewed-by: Boris Fiuczynski
>> ---
>> src/conf/domain_conf.c | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>
> This one I'm not so sure about. One would think the domain def that's
> being copied would have already gone through PostParse, et. al. The
> SetDefTransient is where we're copying the recently Parsed and Validated
> @def, so why would it need the qemuCaps especially since @flags would be
> PARSE_INACTIVE | PARSE_SKIP_VALIDATE in *DefCopy.
As far as I can see virDomainDefParseNode doesn’t skip the
virDomainDefPostParse part (there are the caps needed). Can you please
double check this?
An interesting scenario/test would be to see what happens if the QEMU
binary (and therefore the capabilities) has changed before the
virDomainObjSetDefTransient call.
>
> John
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1ee43950ae2d..a3f2fcb0a001 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps,
>> virDomainObjPtr domain)
>> {
>> int ret = -1;
>> +void *parseOpaque = NULL;
>>
>> if (!domain->persistent)
>> return 0;
>> @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps,
>> if (domain->newDef)
>> return 0;
>>
>> -if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt,
>> NULL, false)))
>> +if (xmlopt->privateData.getParseOpaque)
>> +parseOpaque = xmlopt->privateData.getParseOpaque(domain);
>> +
>> +if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt,
>> parseOpaque, false)))
>> goto out;
>>
>> ret = 0;
>>
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC] lxc: Include support to lxc version 3.0 and higher.
On 10/01/2018 04:18 AM, Julio Faracco wrote:
> This patch introduce the new settings for LXC 3.0 and higher. The older
> versions keep the compatibility to deprecated settings for LXC, but
> after release 3.0, the compatibility was removed. This commit adds the
> support to the refactored settings.
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_native.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index cbdec723dd..c0f70ebb7d 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -200,7 +200,9 @@ lxcSetRootfs(virDomainDefPtr def,
> int type = VIR_DOMAIN_FS_TYPE_MOUNT;
> VIR_AUTOFREE(char *) value = NULL;
>
> -if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
> +if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0 &&
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0)
The intention looks good to me, but the formatting doesn't. Either:
/* comment */
if (func() <= 0 &&
func() <= 0)
return -1;
or:
if (func() <= 0) {
/* comment */
if (func() <= 0)
return -1;
}
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: reintroduce tests for libxl's legacy nested setting
On Mon, Oct 01, 2018 at 08:04:57AM +0200, Erik Skultety wrote: > On Thu, Sep 27, 2018 at 08:25:53AM -0600, Jim Fehlig wrote: > > On 9/27/18 3:29 AM, Erik Skultety wrote: > > > On Wed, Sep 26, 2018 at 11:31:19AM -0600, Jim Fehlig wrote: > > > > The preferred location for setting the nested CPU flag changed in > > > > Xen 4.10 and is advertised via the LIBXL_HAVE_BUILDINFO_NESTED_HVM > > > > define. Commit 95d19cd0 changed libxl to use the new preferred > > > > location but unconditionally changed the tests, causing 'make check' > > > > failures against Xen < 4.10 that do not contain the new location. > > > > > > > > Commit e94415d5 fixed the failures by only running the tests when > > > > LIBXL_HAVE_BUILDINFO_NESTED_HVM is defined. Since libvirt supports > > > > several versions of Xen that use the old nested location, it is > > > > prudent to test the flag is set correctly. This patch reintroduces > > > > the tests for the legacy location of the nested setting. > > > > > > > > Signed-off-by: Jim Fehlig > > > > --- > > > > > > > > We could probably get by with one test for the old nested location, > > > > in which case I'd drop vnuma-hvm-legacy-nest. Any opinions on that? > > > > > > I verified with a few different platforms. I don't have a better idea on > > > what > > > to do about the legacy tests, we either add more (even identical) test > > > files > > > or we figure out some black magic to do the same thing (not preferred). > > > Anyway, to answer your question, even though it might be enough, I'd like > > > to > > > stay consistent and keep both, so that if one day someone is looking at > > > the > > > source they don't wonder why only one of them is being run in the legacy > > > mode. > > > I hope that makes sense. > > > > Yep, no problem. Should I push now or after release? > > Ah, sorry, we definitely want this in the release, so safe for freeze. I went ahead and pushed the patch myself, since DV plans on doing the release at some point during the day which might already by too late for you because of a different timezone. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: also label listen UNIX sockets
On 09/27/2018 05:02 PM, Ján Tomko wrote: > We switched to opening mode='bind' sockets ourselves: > commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 > qemu: support passing pre-opened UNIX socket listen FD > in v4.5.0-rc1~251 > > Then fixed qemuBuildChrChardevStr to change libvirtd's label > while creating the socket: > commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 > qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels > v4.5.0-rc1~52 > > Also add labeling of these sockets to the DAC driver. > Instead of trying to figure out which one was created by libvirt, > label it if it exists. > > https://bugzilla.redhat.com/show_bug.cgi?id=1633389 > > Signed-off-by: Ján Tomko > --- > src/security/security_dac.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) How come SELinux is not affected? We shouldn't rely on default policy doing the right thing. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa wrote:
> On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
>>
>>
>> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
>> > ...although priv->qemuCaps will be NULL in almost every case when the
>> > post parse callback has failed. That may change in the future.
>> >
>> > Signed-off-by: Marc Hartmayer
>> > Reviewed-by: Boris Fiuczynski
>> > ---
>> > src/qemu/qemu_process.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> > index 44c63c42d618..6c5a6472d8cd 100644
>> > --- a/src/qemu/qemu_process.c
>> > +++ b/src/qemu/qemu_process.c
>> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
>>
>> The comment just above here could use a tweak for grammar ;-):
>>
>> /* in case when the post parse callback failed we need to re-run it
>> on the
>> * old config prior we start the VM */
>>
>> > if (vm->def->postParseFailed) {
>> > VIR_DEBUG("re-running the post parse callback");
>> >
>> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL)
>> > < 0)
>> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
>> > priv->qemuCaps) < 0)
>>
>> Searching through history of this line finds Peter's original commit in
>> this area - 7726d158, which seems to indicate a very specific reason for
>> providing a NULL capabilities value here.
>>
>> I think from this patch on is something Peter has worked on a lot, so I
>> would prefer to defer to Peter on them since I'm sure he understands all
>> the various PostParse and Validate special conditions and various flags
>> usage better than I do.
>
> Thanks for notifying me.
First, thanks for your answer!
>
> In general, when parsing a "new" domain config as it's the case here,
> NULL should be passed in.
Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps)
explicitly beforehand? (for example if the 'postParseFailed' flag is set
and we’re starting the domain).
>
> The qemu driver registers the 'domainPostParseDataAlloc' callback to
> qemuDomainPostParseDataAlloc. This callback is executed after the
> 'basic' post parse callback which fills in the emulator binary.
>
> Passing an explicit qemuCaps is meant only for special cases e.g.
> migration where we have a specific set of capabilities which need to be
> used rather than a new one.
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t
valid anymore) as early as possible in the overall “task/job” process.
>
> This patch does not seem to make sense with the justification provided
> here as the post-parse callbacks fill in the proper caps here and this
> part clearly uses a new domain configuration, so the default behaviour
> should be used.
>
> Changing this would also need that we change the normal parser path to
> achieve the same results which is impossible as you don't have access to
> priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the
behavior is still not (completely) fixed, but the performance has been
significantly improved.”.
IMHO, it’s a design flaw that the virDomainObjList class is responsible
for the creation of the virDomainObj… The responsibilities of the
classes are mixed and this enforces the lock order virDomainObjList ->
virDomainObj (what actually is a performance bottleneck). IMHO, we
should create the virDomainObj earlier and add it to the
virDomainObjList when it’s useful.
What’s your opinion about that?
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] security: dac: also label listen UNIX sockets
On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
> We switched to opening mode='bind' sockets ourselves:
> commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
> qemu: support passing pre-opened UNIX socket listen FD
> in v4.5.0-rc1~251
>
> Then fixed qemuBuildChrChardevStr to change libvirtd's label
> while creating the socket:
> commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
> qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
> v4.5.0-rc1~52
>
> Also add labeling of these sockets to the DAC driver.
> Instead of trying to figure out which one was created by libvirt,
> label it if it exists.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1633389
>
> Signed-off-by: Ján Tomko
> ---
> src/security/security_dac.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 62442745dd..da4a6c72fe 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr
> mgr,
> break;
>
> case VIR_DOMAIN_CHR_TYPE_UNIX:
> -if (!dev_source->data.nix.listen) {
> +if (!dev_source->data.nix.listen ||
> +(dev_source->data.nix.path &&
> + virFileExists(dev_source->data.nix.path))) {
> +/* Also label mode='bind' sockets if they exist,
> + * e.g. because they were created by libvirt
> + * and passed via FD */
> if (virSecurityDACSetOwnership(mgr, NULL,
> dev_source->data.nix.path,
> user, group) < 0)
So we're labeling path even if nix.listen == false, shouldn't we check for the
file's existence too? Or have we already done it and I missed this fact?
Basically what I'm aiming at is that nix.path will always be set at this point,
either explicitly by the user (most cases) or it would have been generated by
us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. I'm just simply wondering whether the
condition cannot be further simplified to:
if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
goto done;
ret = 0;
break;
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virFileIsSharedFSType: Check for fuse.glusterfs too
https://bugzilla.redhat.com/show_bug.cgi?id=1632711
GlusterFS is typically safe when it comes to migration. It's a
network FS after all. However, it can be mounted via FUSE driver
they provide. If that is the case we fail to identify it and
think migration is not safe and require VIR_MIGRATE_UNSAFE flag.
Signed-off-by: Michal Privoznik
---
src/util/virfile.c | 77 --
1 file changed, 75 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f8ae07fe4a..ccffa063a6 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3458,6 +3458,75 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# ifndef HUGETLBFS_MAGIC
# define HUGETLBFS_MAGIC 0x958458f6
# endif
+# ifndef FUSE_SUPER_MAGIC
+# define FUSE_SUPER_MAGIC 0x65735546
+# endif
+
+# define PROC_MOUNTS "/proc/mounts"
+
+static int
+virFileIsShareFixFUSE(const char *path,
+ long *f_type)
+{
+char *dirpath = NULL;
+const char **mounts = NULL;
+size_t nmounts = 0;
+size_t i;
+char *p;
+FILE *f = NULL;
+struct mntent mb;
+char mntbuf[1024];
+bool found = false;
+int ret = -1;
+
+if (VIR_STRDUP(dirpath, path) < 0)
+return -1;
+
+if (!(f = setmntent(PROC_MOUNTS, "r"))) {
+virReportSystemError(errno,
+ _("Unable to open %s"),
+ PROC_MOUNTS);
+goto cleanup;
+}
+
+while (getmntent_r(f, , mntbuf, sizeof(mntbuf))) {
+if (STRNEQ("fuse.glusterfs", mb.mnt_type))
+continue;
+
+if (VIR_APPEND_ELEMENT_COPY(mounts, nmounts, mb.mnt_dir) < 0)
+goto cleanup;
+}
+
+do {
+if ((p = strrchr(dirpath, '/')) == NULL) {
+virReportSystemError(EINVAL,
+ _("Invalid relative path '%s'"), path);
+goto cleanup;
+}
+
+if (p == dirpath)
+*(p+1) = '\0';
+else
+*p = '\0';
+
+for (i = 0; i < nmounts; i++) {
+if (STREQ(dirpath, mounts[i])) {
+found = true;
+VIR_DEBUG("Found gluster FUSE mountpoint=%s for path=%s. "
+ "Fixing shared FS type", mounts[i], path);
+*f_type = GFS2_MAGIC;
+}
+}
+} while (!found && p != dirpath);
+
+ret = 0;
+ cleanup:
+endmntent(f);
+VIR_FREE(mounts);
+VIR_FREE(dirpath);
+return ret;
+}
+
int
virFileIsSharedFSType(const char *path,
@@ -3503,6 +3572,12 @@ virFileIsSharedFSType(const char *path,
return -1;
}
+if (sb.f_type == FUSE_SUPER_MAGIC) {
+VIR_DEBUG("Found FUSE mount for path=%s. Trying to fix it", path);
+if (virFileIsShareFixFUSE(path, (long *) _type) < 0)
+return -1;
+}
+
VIR_DEBUG("Check if path %s with FS magic %lld is shared",
path, (long long int)sb.f_type);
@@ -3594,8 +3669,6 @@ virFileGetDefaultHugepageSize(unsigned long long *size)
return 0;
}
-# define PROC_MOUNTS "/proc/mounts"
-
int
virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs,
size_t *ret_nfs)
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
>
>
> On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> > ...although priv->qemuCaps will be NULL in almost every case when the
> > post parse callback has failed. That may change in the future.
> >
> > Signed-off-by: Marc Hartmayer
> > Reviewed-by: Boris Fiuczynski
> > ---
> > src/qemu/qemu_process.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index 44c63c42d618..6c5a6472d8cd 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
>
> The comment just above here could use a tweak for grammar ;-):
>
> /* in case when the post parse callback failed we need to re-run it
> on the
> * old config prior we start the VM */
>
> > if (vm->def->postParseFailed) {
> > VIR_DEBUG("re-running the post parse callback");
> >
> > -if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL)
> > < 0)
> > +if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt,
> > priv->qemuCaps) < 0)
>
> Searching through history of this line finds Peter's original commit in
> this area - 7726d158, which seems to indicate a very specific reason for
> providing a NULL capabilities value here.
>
> I think from this patch on is something Peter has worked on a lot, so I
> would prefer to defer to Peter on them since I'm sure he understands all
> the various PostParse and Validate special conditions and various flags
> usage better than I do.
Thanks for notifying me.
In general, when parsing a "new" domain config as it's the case here,
NULL should be passed in.
The qemu driver registers the 'domainPostParseDataAlloc' callback to
qemuDomainPostParseDataAlloc. This callback is executed after the
'basic' post parse callback which fills in the emulator binary.
Passing an explicit qemuCaps is meant only for special cases e.g.
migration where we have a specific set of capabilities which need to be
used rather than a new one.
This patch does not seem to make sense with the justification provided
here as the post-parse callbacks fill in the proper caps here and this
part clearly uses a new domain configuration, so the default behaviour
should be used.
Changing this would also need that we change the normal parser path to
achieve the same results which is impossible as you don't have access to
priv->qemuCaps prior to creating the virDomainObj object.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] uml: Fix weird logic inside umlConnectOpen() function.
I reworded the commit subject a little and pushed. One more thing to note is that we don't put a 'period' at the end of a commit subject, I've noticed it in a few of your patches, it's just I kept removing them intuitively and always forgot to mention it :). Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tests: reintroduce tests for libxl's legacy nested setting
On Thu, Sep 27, 2018 at 08:25:53AM -0600, Jim Fehlig wrote: > On 9/27/18 3:29 AM, Erik Skultety wrote: > > On Wed, Sep 26, 2018 at 11:31:19AM -0600, Jim Fehlig wrote: > > > The preferred location for setting the nested CPU flag changed in > > > Xen 4.10 and is advertised via the LIBXL_HAVE_BUILDINFO_NESTED_HVM > > > define. Commit 95d19cd0 changed libxl to use the new preferred > > > location but unconditionally changed the tests, causing 'make check' > > > failures against Xen < 4.10 that do not contain the new location. > > > > > > Commit e94415d5 fixed the failures by only running the tests when > > > LIBXL_HAVE_BUILDINFO_NESTED_HVM is defined. Since libvirt supports > > > several versions of Xen that use the old nested location, it is > > > prudent to test the flag is set correctly. This patch reintroduces > > > the tests for the legacy location of the nested setting. > > > > > > Signed-off-by: Jim Fehlig > > > --- > > > > > > We could probably get by with one test for the old nested location, > > > in which case I'd drop vnuma-hvm-legacy-nest. Any opinions on that? > > > > I verified with a few different platforms. I don't have a better idea on > > what > > to do about the legacy tests, we either add more (even identical) test files > > or we figure out some black magic to do the same thing (not preferred). > > Anyway, to answer your question, even though it might be enough, I'd like to > > stay consistent and keep both, so that if one day someone is looking at the > > source they don't wonder why only one of them is being run in the legacy > > mode. > > I hope that makes sense. > > Yep, no problem. Should I push now or after release? Ah, sorry, we definitely want this in the release, so safe for freeze. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
