On 18. 3. 2020 12:40, Peter Krempa wrote:
> qemuMonitorDelObject is often used in cleanup cases so we need to
> control whether to log errors.
>
> First patch actually prevents one of the spurious calls in cases we know
> it would be pointless.
>
> Peter Krempa (5):
> qemuDomainChangeEjectableMedia: Don't always remove managed PR daemon
> qemuMonitorJSON(Add|Del)Object: Refactor cleanup
> qemuMonitorJSONCheckError: Use g_autofree
> qemuMonitorJSONCheckError: Allow suppressing of error reporting
> qemu: Suppress error reporting from qemuMonitorDelObject in cleanup
> paths
>
> src/qemu/qemu_block.c| 10 ++---
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_hotplug.c | 34 +---
> src/qemu/qemu_monitor.c | 5 ++-
> src/qemu/qemu_monitor.h | 3 +-
> src/qemu/qemu_monitor_json.c | 77 ++--
> src/qemu/qemu_monitor_json.h | 3 +-
> 7 files changed, 71 insertions(+), 63 deletions(-)
>
Reviewed-by: Michal Privoznik
Michal
On Wed, 2020-03-18 at 18:01 +0100, Michal Prívozník wrote:
> On 18. 3. 2020 16:47, Andrea Bolognani wrote:
> > if I use either one of
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > both qemu:///embed instances try to use the same paths:
> >
> > /dev/hugepages/libvirt/qemu/$domid-$domname/
> >
> > /dev/shm/libvirt/qemu/$domid-$domname/
>
> This is because qemu driver uses virDomainDefGetShortName() to construct
> the path which is not embed driver aware. In fact, we use it on a few
> other places:
>
> libvirt.git $ git grep -C0 virDomainDefGetShortName -- src/qemu/ \
> | wc -l
> 15
>
> so we probably have more broken plaes. I will investigate and post a
> patch. Probably something among those lines that fixed machined name
> generation.
I'm not sure changing virDomainDefGetShortName() to include the embed
root hash all the time, which I assume is what you had in mind, is a
good enough solution.
That would work very well for /dev/shm and friends, but if the path
you're building is *inside* the embed root, then, you just added 15
characters to the path of any file. This wouldn't be too bad if not
for the fact that the path to a UNIX socket can only be up to 108
characters long, which is limiting enough already...
Can we make the whole thing smart enough that the embed root hash
will appear in paths that are outside of the embed root, but not in
those that are inside it?
--
Andrea Bolognani / Red Hat / Virtualization
On Thu, Mar 19, 2020 at 10:21:39AM +0100, Andrea Bolognani wrote:
> On Wed, 2020-03-18 at 18:01 +0100, Michal Prívozník wrote:
> > On 18. 3. 2020 16:47, Andrea Bolognani wrote:
> > > if I use either one of
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > both qemu:///embed instances try to use the same paths:
> > >
> > > /dev/hugepages/libvirt/qemu/$domid-$domname/
> > >
> > > /dev/shm/libvirt/qemu/$domid-$domname/
> >
> > This is because qemu driver uses virDomainDefGetShortName() to construct
> > the path which is not embed driver aware. In fact, we use it on a few
> > other places:
> >
> > libvirt.git $ git grep -C0 virDomainDefGetShortName -- src/qemu/ \
> > | wc -l
> > 15
> >
> > so we probably have more broken plaes. I will investigate and post a
> > patch. Probably something among those lines that fixed machined name
> > generation.
>
> I'm not sure changing virDomainDefGetShortName() to include the embed
> root hash all the time, which I assume is what you had in mind, is a
> good enough solution.
>
> That would work very well for /dev/shm and friends, but if the path
> you're building is *inside* the embed root, then, you just added 15
> characters to the path of any file. This wouldn't be too bad if not
> for the fact that the path to a UNIX socket can only be up to 108
> characters long, which is limiting enough already...
>
> Can we make the whole thing smart enough that the embed root hash
> will appear in paths that are outside of the embed root, but not in
> those that are inside it?
This is probably a matter of having 2 methods for building a unique
name. One for use cases interacting with global resources, and one
for use cases interacting with isolated resources. Then we'll need
to make sure the driver calls the right one in each context.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Since the removal of gnulib, HAVE_SOCKETPAIR is no more defined, making
these two tests effectively skipped.
Use the same strategy used in other generic library bits, i.e. exclude
the socketpair usage on Windows.
Semi-related change in virnetdaemontest.c to make it build: since
virutil.h does not include unistd.h anymore, we need to include it.
Signed-off-by: Pino Toscano
---
tests/virnetdaemontest.c | 4 +++-
tests/virnetserverclienttest.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c
index 825487f0a1..09d268627c 100644
--- a/tests/virnetdaemontest.c
+++ b/tests/virnetdaemontest.c
@@ -18,13 +18,15 @@
#include
+#include
+
#include "testutils.h"
#include "virerror.h"
#include "rpc/virnetdaemon.h"
#define VIR_FROM_THIS VIR_FROM_RPC
-#if defined(HAVE_SOCKETPAIR) && defined(WITH_YAJL)
+#if !defined(WIN32) && defined(WITH_YAJL)
struct testClientPriv {
int magic;
};
diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c
index a9a56c48d5..668fd02a1e 100644
--- a/tests/virnetserverclienttest.c
+++ b/tests/virnetserverclienttest.c
@@ -24,7 +24,7 @@
#define VIR_FROM_THIS VIR_FROM_RPC
-#ifdef HAVE_SOCKETPAIR
+#ifndef WIN32
static void *
testClientNew(virNetServerClientPtr client G_GNUC_UNUSED,
--
2.25.1
On Thu, Mar 19, 2020 at 12:05:17PM +0100, Pino Toscano wrote:
> Since the removal of gnulib, HAVE_SOCKETPAIR is no more defined, making
> these two tests effectively skipped.
Doh, I had compared the config.h file before & after gnulib removal
to identify HAVE_* macros that no longer existed, but guess I
missed this.
>
> Use the same strategy used in other generic library bits, i.e. exclude
> the socketpair usage on Windows.
>
> Semi-related change in virnetdaemontest.c to make it build: since
> virutil.h does not include unistd.h anymore, we need to include it.
>
> Signed-off-by: Pino Toscano
> ---
> tests/virnetdaemontest.c | 4 +++-
> tests/virnetserverclienttest.c | 2 +-
> 2 files changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
On a Tuesday in 2020, Christian Schoenebeck wrote:
Introduce new 'multidevs' option for filesystem.
I don't like the 'multidevs' name, but cannot think of anything
beter.
'collisions' maybe?
This option prevents misbheaviours on guest if a 9pfs export
contains multiple devices, due to the potential file ID collisions
this otherwise may cause.
Signed-off-by: Christian Schoenebeck
---
docs/formatdomain.html.in | 47 ++-
docs/schemas/domaincommon.rng | 10
src/conf/domain_conf.c| 30 ++
src/conf/domain_conf.h| 13 ++
src/qemu/qemu_command.c | 7 ++
5 files changed, 106 insertions(+), 1 deletion(-)
Please split the XML changes from the qemu driver changes.
Also missing:
* qemu_capabilities addition
* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
reject this setting for virtiofs
* qemuxml2xmltest addition
* qemuxml2argvtest addition
(no changes required for virschematest - it checks all the XML files in
the directories used by the above tests against the schema)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 594146009d..13c506988b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3967,7 +3967,7 @@
-
+
@@ -4084,13 +4084,58 @@
+
Since 5.2.0, the filesystem element
has an optional attribute model with supported values
"virtio-transitional", "virtio-non-transitional", or "virtio".
See Virtio transitional devices
for more details.
+
+
Unrelated change that can be split out.
+
+ The filesystem element has an optional attribute multidevs
+ which specifies how to deal with a filesystem export containing more than
+ one device, in order to avoid file ID collisions on guest when using 9pfs
+ (since 6.2.0, requires QEMU 4.2).
+ This attribute is not available for virtiofs. The possible values are:
+
+
+
+default
+
+Use QEMU's default setting (which currently is warn).
+
+remap
+
+This setting allows guest to access multiple devices per export without
+encountering misbehaviours. Inode numbers from host are automatically
+remapped on guest to actively prevent file ID collisions if guest
+accesses one export containing multiple devices.
+
+forbid
+
+Only allow to access one device per export by guest. Attempts to access
+additional devices on the same export will cause the individual
+filesystem access by guest to fail with an error and being logged
(once)
+as error on host side.
+
+warn
+
+This setting resembles the behaviour of 9pfs prior to QEMU 4.2, that is
+no action is performed to prevent any potential file ID collisions if
an
+export contains multiple devices, with the only exception: a warning is
+logged (once) on host side now. This setting may lead to misbehaviours
+on guest side if more than one device is exported per export, due to
the
+potential file ID collisions this may cause on guest side in that case.
+
+
+
+
+ The filesystem element may contain the following
subelements:
+
+
And so can this one.
driver
The optional driver element allows specifying further details
@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " model='%s'",
virDomainFSModelTypeToString(def->model));
}
+if (def->multidevs) {
+virBufferAsprintf(buf, " multidevs='%s'", multidevs);
+}
make syntax-check complains here:
Curly brackets around single-line body:
../src/conf/domain_conf.c:25452-25454:
if (def->multidevs) {
virBufferAsprintf(buf, " multidevs='%s'", multidevs);
}
Jano
signature.asc
Description: PGP signature
On Wed, Mar 18, 2020 at 06:32:14PM +0100, Michal Privoznik wrote:
> The @src is not always a file. It may also be a directory (for
> instance qemuDomainCreateDeviceRecursive() assumes that) - even
> though it doesn't happen usually. Anyway, mount() can mount only
> a dir onto a dir and a file onto a file.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virfile.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 0f31554323..9c175b041f 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3743,8 +3743,17 @@ int
> virFileBindMountDevice(const char *src,
> const char *dst)
> {
> -if (virFileTouch(dst, 0666) < 0)
> -return -1;
> +if (!virFileExists(dst)) {
> +if (virFileIsDir(src)) {
> +if (virFileMakePath(dst)) {
Personally, I'd consider
if (virFileMakePath(dst) < 0)
clearer here for a reader who's not familiar with the function and what exactly
it returns.
Reviewed-by: Pavel Mores
> +virReportSystemError(errno, _("Unable to make dir %s"), dst);
> +return -1;
> +}
> +} else {
> +if (virFileTouch(dst, 0666) < 0)
> +return -1;
> +}
> +}
>
> if (mount(src, dst, "none", MS_BIND, NULL) < 0) {
> virReportSystemError(errno, _("Failed to bind %s on to %s"), src,
> --
> 2.24.1
>
On Wed, Mar 18, 2020 at 06:32:11PM +0100, Michal Privoznik wrote:
> The virFileMakePathWithMode() which is our recursive version of
> mkdir() fails, it simply just returns a negative value with errno
> set. No error is reported (as compared to virFileTouch() for
> instance).
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e2252f6cf..48bf5ae559 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -14643,8 +14643,12 @@ qemuDomainCreateDeviceRecursive(const char *device,
> * proper owner and mode. Bind mount only after that. */
> } else if (isDir) {
> if (create &&
> -virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
> +virFileMakePathWithMode(devicePath, sb.st_mode) < 0) {
> +virReportSystemError(errno,
> + _("Unable to make dir %s"),
> + devicePath);
> goto cleanup;
> +}
> } else {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("unsupported device type %s 0%o"),
> --
> 2.24.1
>
Reviewed-by: Pavel Mores
On Thu, Mar 19, 2020 at 04:57:41PM +0100, Christian Schoenebeck wrote:
> On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote:
> > On a Tuesday in 2020, Christian Schoenebeck wrote:
> > >Introduce new 'multidevs' option for filesystem.
> > >
> > >
> >
> > I don't like the 'multidevs' name, but cannot think of anything
> > beter.
> >
> > 'collisions' maybe?
>
> Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :)
> And which collision would that be? At least IMO 'multidevs' is less ambigious.
> I have no problem though to change it to whatever name you might come up
> with.
> Just keep the resulting key-value pair set in mind:
>
> multidevs='default'
> multidevs='remap'
> multidevs='forbid'
> multidevs='warn'
>
> vs.
>
> collisions='default'
> collisions='remap' <- probably misleading what 'remap' means in this case
> collisions='forbid'
> collisions='warn' <- wrong, it warns about multiple devices, not about file
> ID
> collisions.
>
> So different key-name might also require different value-names.
>
> Another option would be the long form 'multi-devices=...'
I tried to come up with names when this was posted to QEMU, but didn't
think of much better than multidevs, so I think that's acceptable for
libvirt usage.
"collisions" isn't better enough to justify different naming from QEMU
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote:
> On a Tuesday in 2020, Christian Schoenebeck wrote:
> >Introduce new 'multidevs' option for filesystem.
> >
> >
>
> I don't like the 'multidevs' name, but cannot think of anything
> beter.
>
> 'collisions' maybe?
Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :)
And which collision would that be? At least IMO 'multidevs' is less ambigious.
I have no problem though to change it to whatever name you might come up with.
Just keep the resulting key-value pair set in mind:
multidevs='default'
multidevs='remap'
multidevs='forbid'
multidevs='warn'
vs.
collisions='default'
collisions='remap' <- probably misleading what 'remap' means in this case
collisions='forbid'
collisions='warn' <- wrong, it warns about multiple devices, not about file ID
collisions.
So different key-name might also require different value-names.
Another option would be the long form 'multi-devices=...'
> >
> >
> >
> >
> >
> >This option prevents misbheaviours on guest if a 9pfs export
> >contains multiple devices, due to the potential file ID collisions
> >this otherwise may cause.
> >
> >Signed-off-by: Christian Schoenebeck
> >---
> >
> > docs/formatdomain.html.in | 47 ++-
> > docs/schemas/domaincommon.rng | 10
> > src/conf/domain_conf.c| 30 ++
> > src/conf/domain_conf.h| 13 ++
> > src/qemu/qemu_command.c | 7 ++
> > 5 files changed, 106 insertions(+), 1 deletion(-)
>
> Please split the XML changes from the qemu driver changes.
Ok
> Also missing:
> * qemu_capabilities addition
AFAICS the common procedure is to add new capabilities always to the end of
the enum list. So I guess I will do that as well.
> * qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
> reject this setting for virtiofs
Good to know where that check is supposed to go to, thanks!
> * qemuxml2xmltest addition
> * qemuxml2argvtest addition
Ok, I have to read up how those tests work. AFAICS I need to add xml files to
their data subdirs.
Separate patches required for those 2 tests?
> (no changes required for virschematest - it checks all the XML files in
> the directories used by the above tests against the schema)
>
> >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >index 594146009d..13c506988b 100644
> >--- a/docs/formatdomain.html.in
> >+++ b/docs/formatdomain.html.in
> >@@ -3967,7 +3967,7 @@
> >
> >
> >
> >
> >
> >
> >-
> >+ >multidevs='remap'>>
> >
> >
> >
> >
> >@@ -4084,13 +4084,58 @@
> >
> >
> >
> >
> >+
> >
> > Since 5.2.0, the filesystem element
> > has an optional attribute model with supported values
> > "virtio-transitional", "virtio-non-transitional", or "virtio".
> > See Virtio transitional
> > devices
> > for more details.
> >
> >+
> >+
>
> Unrelated change that can be split out.
Ok, I'll make that the 1st preparatory patch then including ...
> >+
> >+ The filesystem element has an optional attribute
> >multidevs + which specifies how to deal with a
> >filesystem export containing more than + one device, in order to
> >avoid file ID collisions on guest when using 9pfs + ( >class="since">since 6.2.0, requires QEMU 4.2). + This
> >attribute is not available for virtiofs. The possible values are: +
> >
> >+
> >+
> >+default
> >+
> >+Use QEMU's default setting (which currently is warn).
> >+
> >+remap
> >+
> >+This setting allows guest to access multiple devices per export
> >without +encountering misbehaviours. Inode numbers from host are
> >automatically +remapped on guest to actively prevent file ID
> >collisions if guest +accesses one export containing multiple
> >devices.
> >+
> >+forbid
> >+
> >+Only allow to access one device per export by guest. Attempts to
> >access +additional devices on the same export will cause the
> >individual +filesystem access by guest to fail with an error and
> >being logged (once) +as error on host side.
> >+
> >+warn
> >+
> >+This setting resembles the behaviour of 9pfs prior to QEMU 4.2,
> >that is +no action is performed to prevent any potential file ID
> >collisions if an +export contains multiple devices, with the only
> >exception: a warning is +logged (once) on host side now. This
> >setting may lead to misbehaviours +on guest side if more than one
> >
On a Thursday in 2020, Christian Schoenebeck wrote:
On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote:
On a Tuesday in 2020, Christian Schoenebeck wrote:
>Introduce new 'multidevs' option for filesystem.
>
>
I don't like the 'multidevs' name, but cannot think of anything
beter.
'collisions' maybe?
Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :)
And which collision would that be? At least IMO 'multidevs' is less ambigious.
I have no problem though to change it to whatever name you might come up with.
Just keep the resulting key-value pair set in mind:
multidevs='default'
multidevs='remap'
multidevs='forbid'
multidevs='warn'
vs.
collisions='default'
collisions='remap' <- probably misleading what 'remap' means in this case
collisions='forbid'
collisions='warn' <- wrong, it warns about multiple devices, not about file ID
collisions.
So different key-name might also require different value-names.
Another option would be the long form 'multi-devices=...'
Okay, let's leave it at 'multidevs'.
>
>
>
>
>
>This option prevents misbheaviours on guest if a 9pfs export
>contains multiple devices, due to the potential file ID collisions
>this otherwise may cause.
>
>Signed-off-by: Christian Schoenebeck
>---
>
> docs/formatdomain.html.in | 47 ++-
> docs/schemas/domaincommon.rng | 10
> src/conf/domain_conf.c| 30 ++
> src/conf/domain_conf.h| 13 ++
> src/qemu/qemu_command.c | 7 ++
> 5 files changed, 106 insertions(+), 1 deletion(-)
Please split the XML changes from the qemu driver changes.
Ok
Also missing:
* qemu_capabilities addition
AFAICS the common procedure is to add new capabilities always to the end of
the enum list. So I guess I will do that as well.
* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
reject this setting for virtiofs
Good to know where that check is supposed to go to, thanks!
* qemuxml2xmltest addition
* qemuxml2argvtest addition
Ok, I have to read up how those tests work. AFAICS I need to add xml files to
their data subdirs.
Separate patches required for those 2 tests?
Usually xml2xmltest is in the same test as the XML parser/formatter
and xml2argvtest is a part of the qemu driver patch.
Jano
signature.asc
Description: PGP signature
In one of my previous commits I've introduced code that creates
all devices for given (possible) multipath target. But I've made
a mistake there - the code accesses src->path without checking if
the disk source is local. Note that the path is NULL if the
source is not local.
Fixes: a30078cb832646177defd256e77c632905f1e6d0
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0e2252f6cf..ef17829235 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
}
tmpPath = g_strdup(next->path);
+
+if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+errno != ENOSYS && errno != EBADF) {
+virReportSystemError(errno,
+ _("Unable to get devmapper targets for
%s"),
+ next->path);
+return -1;
+}
}
if (virStringListAdd(&paths, tmpPath) < 0)
return -1;
-if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
-errno != ENOSYS && errno != EBADF) {
-virReportSystemError(errno,
- _("Unable to get devmapper targets for %s"),
- next->path);
-return -1;
-}
-
if (virStringListMerge(&paths, &targetPaths) < 0)
return -1;
}
--
2.24.1
We special cased some behaviour and it broke on combination with other
special case.
Peter Krempa (4):
qemuxml2argvdata/disk-slices: Add test case for 'luks' encryption
qemu: block: Extract logic decision when to use a separate 'raw' layer
for slice
qemuBlockStorageSourceNeedsStorageSliceLayer: Deal with 'luks' files
qemu: block: Split up formatting of JSON props for 'raw' and 'luks'
drivers
src/qemu/qemu_block.c | 70 ++-
src/qemu/qemu_block.h | 3 +
src/qemu/qemu_domain.c| 3 +-
.../disk-slices.x86_64-latest.args| 41 +++
tests/qemuxml2argvdata/disk-slices.xml| 13
.../disk-slices.x86_64-latest.xml | 16 -
6 files changed, 112 insertions(+), 34 deletions(-)
--
2.24.1
On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
> In one of my previous commits I've introduced code that creates
> all devices for given (possible) multipath target. But I've made
> a mistake there - the code accesses src->path without checking if
> the disk source is local. Note that the path is NULL if the
> source is not local.
Well next->path 'may' be NULL if it's not local, but in this case it is
local because NVMe disks are local, but they don't have the path.
>
> Fixes: a30078cb832646177defd256e77c632905f1e6d0
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
Once the commit message makes sense:
Reviewed-by: Peter Krempa
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e2252f6cf..ef17829235 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
> }
>
> tmpPath = g_strdup(next->path);
> +
> +if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> +errno != ENOSYS && errno != EBADF) {
> +virReportSystemError(errno,
> + _("Unable to get devmapper targets for
> %s"),
> + next->path);
> +return -1;
> +}
> }
>
> if (virStringListAdd(&paths, tmpPath) < 0)
> return -1;
>
> -if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> -errno != ENOSYS && errno != EBADF) {
> -virReportSystemError(errno,
> - _("Unable to get devmapper targets for %s"),
> - next->path);
> -return -1;
> -}
> -
> if (virStringListMerge(&paths, &targetPaths) < 0)
> return -1;
Is this supposed to go after 'tmpPath' in the list? Otherwise you might
want to move it toghether with the call to virDevMapperGetTargets.
On a Thursday in 2020, Michal Privoznik wrote:
In one of my previous commits I've introduced code that creates
all devices for given (possible) multipath target. But I've made
a mistake there - the code accesses src->path without checking if
the disk source is local. Note that the path is NULL if the
source is not local.
Fixes: a30078cb832646177defd256e77c632905f1e6d0
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_domain.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
On a Thursday in 2020, Peter Krempa wrote:
We special cased some behaviour and it broke on combination with other
special case.
Peter Krempa (4):
qemuxml2argvdata/disk-slices: Add test case for 'luks' encryption
qemu: block: Extract logic decision when to use a separate 'raw' layer
for slice
qemuBlockStorageSourceNeedsStorageSliceLayer: Deal with 'luks' files
qemu: block: Split up formatting of JSON props for 'raw' and 'luks'
drivers
src/qemu/qemu_block.c | 70 ++-
src/qemu/qemu_block.h | 3 +
src/qemu/qemu_domain.c| 3 +-
.../disk-slices.x86_64-latest.args| 41 +++
tests/qemuxml2argvdata/disk-slices.xml| 13
.../disk-slices.x86_64-latest.xml | 16 -
6 files changed, 112 insertions(+), 34 deletions(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
1. Fix link to knowledge base article
2. Use to make sure kbase.html has page title
Signed-off-by Sebastian Mitterle
---
docs/formatbackup.html.in | 2 +-
docs/kbase.html.in| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/formatbackup.html.in b/docs/formatbackup.html.in
index 543d913072..55acd13ddc 100644
--- a/docs/formatbackup.html.in
+++ b/docs/formatbackup.html.in
@@ -14,7 +14,7 @@
description of the actions to perform, as well as an optional
second XML document describing a
checkpoint to create at the same point in time. See
- also a comparison between
+ also a comparison between
the various state capture APIs.
diff --git a/docs/kbase.html.in b/docs/kbase.html.in
index 7055f4fda4..c586e0f676 100644
--- a/docs/kbase.html.in
+++ b/docs/kbase.html.in
@@ -2,7 +2,7 @@
http://www.w3.org/1999/xhtml";>
-Knowledge base
+Knowledge base
--
2.21.0
On a Thursday in 2020, Peter Krempa wrote:
virStorageEncryptionSecretPtr may have a string inside it, thus we must
copy the string too. Use virSecretLookupDefCopy to do that.
Likely caused by 756b46ddd24.
Please remove the period from the end.
Also, at the times of that commit, memcpy was sufficient:
struct _virStorageEncryptionSecret {
int type; /* virStorageEncryptionSecretType */
unsigned char uuid[VIR_UUID_BUFLEN];
};
So this actually fixes commit 47e88b33b which switched to using
virSecretLookupDef.
https://bugzilla.redhat.com/show_bug.cgi?id=1814923
Signed-off-by: Peter Krempa
---
src/util/virstorageencryption.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
signature.asc
Description: PGP signature
On Thu, Mar 19, 2020 at 18:14:15 +0100, Ján Tomko wrote:
> On a Thursday in 2020, Peter Krempa wrote:
> > virStorageEncryptionSecretPtr may have a string inside it, thus we must
> > copy the string too. Use virSecretLookupDefCopy to do that.
> >
> > Likely caused by 756b46ddd24.
>
> Please remove the period from the end.
>
> Also, at the times of that commit, memcpy was sufficient:
> struct _virStorageEncryptionSecret {
> int type; /* virStorageEncryptionSecretType */
> unsigned char uuid[VIR_UUID_BUFLEN];
> };
>
> So this actually fixes commit 47e88b33b which switched to using
> virSecretLookupDef.
Oh, okay, I didn't look as closely into the history. I'll update the
problematic commit.
On 19. 3. 2020 17:43, Peter Krempa wrote:
> On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
>> In one of my previous commits I've introduced code that creates
>> all devices for given (possible) multipath target. But I've made
>> a mistake there - the code accesses src->path without checking if
>> the disk source is local. Note that the path is NULL if the
>> source is not local.
>
> Well next->path 'may' be NULL if it's not local, but in this case it is
> local because NVMe disks are local, but they don't have the path.
Local as in virStorageSourceIsLocalStorage() == true. And for NVMe we
return false exactly because src->path has to be NULL (it can't be set
to any meaningfull path).
How about this formulation:
Note that the path is NULL if the
source is not local as viewed by
virStorageSourceIsLocalStorage().
>
>>
>> Fixes: a30078cb832646177defd256e77c632905f1e6d0
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814947
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/qemu/qemu_domain.c | 16
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> Once the commit message makes sense:
>
> Reviewed-by: Peter Krempa
>
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 0e2252f6cf..ef17829235 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -15862,19 +15862,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
>> }
>>
>> tmpPath = g_strdup(next->path);
>> +
>> +if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> +errno != ENOSYS && errno != EBADF) {
>> +virReportSystemError(errno,
>> + _("Unable to get devmapper targets for
>> %s"),
>> + next->path);
>> +return -1;
>> +}
>> }
>>
>> if (virStringListAdd(&paths, tmpPath) < 0)
>> return -1;
>>
>> -if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
>> -errno != ENOSYS && errno != EBADF) {
>> -virReportSystemError(errno,
>> - _("Unable to get devmapper targets for
>> %s"),
>> - next->path);
>> -return -1;
>> -}
>> -
>> if (virStringListMerge(&paths, &targetPaths) < 0)
>> return -1;
>
> Is this supposed to go after 'tmpPath' in the list? Otherwise you might
> want to move it toghether with the call to virDevMapperGetTargets.
>
Good point, the order doesn't matter.
Michal
On a Thursday in 2020, Peter Krempa wrote:
On Thu, Mar 19, 2020 at 18:14:15 +0100, Ján Tomko wrote:
On a Thursday in 2020, Peter Krempa wrote:
> virStorageEncryptionSecretPtr may have a string inside it, thus we must
> copy the string too. Use virSecretLookupDefCopy to do that.
>
> Likely caused by 756b46ddd24.
Please remove the period from the end.
Also, at the times of that commit, memcpy was sufficient:
struct _virStorageEncryptionSecret {
int type; /* virStorageEncryptionSecretType */
unsigned char uuid[VIR_UUID_BUFLEN];
};
So this actually fixes commit 47e88b33b which switched to using
virSecretLookupDef.
Oh, okay, I didn't look as closely into the history. I'll update the
problematic commit.
Well, 756b46ddd24 is also problematic - something like this is easy to
overlook.
Jano
signature.asc
Description: PGP signature
On Thu, Mar 19, 2020 at 18:39:58 +0100, Michal Privoznik wrote:
> On 19. 3. 2020 17:43, Peter Krempa wrote:
> > On Thu, Mar 19, 2020 at 17:22:37 +0100, Michal Privoznik wrote:
> >> In one of my previous commits I've introduced code that creates
> >> all devices for given (possible) multipath target. But I've made
> >> a mistake there - the code accesses src->path without checking if
> >> the disk source is local. Note that the path is NULL if the
> >> source is not local.
> >
> > Well next->path 'may' be NULL if it's not local, but in this case it is
> > local because NVMe disks are local, but they don't have the path.
>
> Local as in virStorageSourceIsLocalStorage() == true. And for NVMe we
> return false exactly because src->path has to be NULL (it can't be set
> to any meaningfull path).
Yes. The meaning of virStorageSourceIsLocalStorage shifted after NVMe
disks were added.
>
> How about this formulation:
>
> Note that the path is NULL if the
> source is not local as viewed by
> virStorageSourceIsLocalStorage().
That is still misleading, because network disks can have non-null path.
I'd prefer:
'next->path' is NULL/doesn't make sense for VIR_STORAGE_TYPE_NVME
On Wed, Mar 18, 2020 at 06:32:12PM +0100, Michal Privoznik wrote:
> If building namespace fails somewhere in the middle (that is some
> files exists under devMountsSavePath[i]), then plain rmdir() is
> not enough to remove dir. Umount the temp location and use
> virFileDeleteTree() to remove the directory.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 48bf5ae559..2724607311 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15311,9 +15311,12 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
> ret = 0;
> cleanup:
> for (i = 0; i < ndevMountsPath; i++) {
> +#if defined(__linux__)
> +umount(devMountsSavePath[i]);
> +#endif /* defined(__linux__) */
> /* The path can be either a regular file or a dir. */
> if (virFileIsDir(devMountsSavePath[i]))
> -rmdir(devMountsSavePath[i]);
> +virFileDeleteTree(devMountsSavePath[i]);
> else
> unlink(devMountsSavePath[i]);
> }
> --
> 2.24.1
>
Reviewed-by: Pavel Mores
On Wed, 18 Mar 2020 at 20:24, John Snow wrote:
>
> The following changes since commit d649689a8ecb2e276cc20d3af6d416e3c299cb17:
>
> Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into
> staging (2020-03-17 18:33:05 +)
>
> are available in the Git repository at:
>
> https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to 2d00cbd8e222a4adc08f415c399e84590ee8ff9a:
>
> block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-18 14:03:46
> -0400)
>
>
> Pull request
>
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.
-- PMM
On 3/19/20 1:57 PM, Peter Maydell wrote:
> On Wed, 18 Mar 2020 at 20:24, John Snow wrote:
>>
>> The following changes since commit d649689a8ecb2e276cc20d3af6d416e3c299cb17:
>>
>> Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into
>> staging (2020-03-17 18:33:05 +)
>>
>> are available in the Git repository at:
>>
>> https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>>
>> for you to fetch changes up to 2d00cbd8e222a4adc08f415c399e84590ee8ff9a:
>>
>> block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty (2020-03-18 14:03:46
>> -0400)
>>
>>
>> Pull request
>>
>>
>
>
> Applied, thanks.
>
Wonderful, thanks!
> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
> for any user-visible changes.
>
> -- PMM
Will do.
On a Thursday in 2020, Sebastian Mitterle wrote:
1. Fix link to knowledge base article
2. Use to make sure kbase.html has page title
These are actually two fixes.
I've split them up using these bullet points as commit messages.
Signed-off-by Sebastian Mitterle
You can use 'git commit -s' to add your sign-off line.
---
docs/formatbackup.html.in | 2 +-
docs/kbase.html.in| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko
and pushed
Jano
signature.asc
Description: PGP signature
TL;DR - I'm not as anti-XML as the proposal seems to be, but also not
pro-XML. I also (after thinking about it) understand the advantage of
putting this in a separate library. So yeah, let's go it!
On 3/13/20 6:47 AM, Daniel P. Berrangé wrote:
On Fri, Mar 13, 2020 at 11:23:44AM +0200, Dan Kenigsberg wrote:
On Wed, 4 Mar 2020, 14:51 Daniel P. Berrangé, wrote:
We've been doing alot of refactoring of code in recent times, and also
have plans for significant infrastructure changes. We still need to
spend time delivering interesting features to users / applications.
This mail is to introduce an idea for a solution to an specific
area applications have had long term pain with libvirt's current
"mechanism, not policy" approach - device addressing. This is a way
for us to show brand new ideas & approaches for what the libvirt
project can deliver in terms of management APIs.
To set expectations straight: I have written no code for this yet,
merely identified the gap & conceptual solution.
The device addressing problem
=
One of the key jobs libvirt does when processing a new domain XML
configuration is to assign addresses to all devices that are present.
This involves adding various device controllers (PCI bridges, PCI root
ports, IDE/SCSI buses, USB controllers, etc) if they are not already
present, and then assigning PCI, USB, IDE, SCSI, etc, addresses to each
device so they are associated with controllers. When libvirt spawns a
QEMU guest, it will pass full address information to QEMU.
Libvirt, as a general rule, aims to avoid defining and implementing
policy around expansion of guest configuration / defaults, however, it
is inescapable in the case of device addressing due to the need to
guarantee a stable hardware ABI to make live migration and save/restore
to disk work. The policy that libvirt has implemented for device
addressing is, as much as possible, the same as the addressing scheme
QEMU would apply itself.
While libvirt succeeds in its goal of providing a stable hardware API,
the addressing scheme used is not well suited to all deployment
scenarios of QEMU. This is an inevitable result of having a specific
assignment policy implemented in libvirt which has to trade off mutually
incompatible use cases/goals.
When the libvirt addressing policy is not been sufficient, management
applications are forced to take on address assignment themselves,
which is a massive non-trivial job with many subtle problems to
consider.
Places where libvirt's addressing is insufficient for PCI include
* Setting up multiple guest NUMA nodes and associating devices to
specific nodes
* Pre-emptive creation of extra PCIe root ports, to allow for later
device hotplug on PCIe topologies
* Determining whether to place a device on a PCI or PCIe bridge
* Controlling whether a device is placed into a hotpluggable slot
* Controlling whether a PCIe root port supports hotplug or not
* Determining whether to places all devices on distinct slots or
buses, vs grouping them all into functions on the same slot
* Ability to expand the device addressing without being on the
hypervisor host
(I don't understand the last bullet point)
I'm not sure if this is still the case, but at some point in time
there was a desire from KubeVirt to be able to expand the users'
configuration when loaded in KubeVirt, filling in various defaults
for devices. This would run when the end user YAML/JSON config
was first posted to the k8s API for storage, some arbitrary amount
of time later the config gets chosen to run on a virtualization
host at which point it is turned into libvirt domain XML.
If I recall the discussion properly, the context was that we wanted
kubevirt to remember all the stuff like PCI addresses, MAC addresses,
exact machinetype to be "backfilled" from libvirt into the Kubevirt
config, but for them that's a one-way street. So having all these things
set by a separate API (even in a separate library) would definitely be
an advantage for them, as long as all the same info was available at
that time (e.g. you really need to know the machinetypes supported by
the specific qemu that is going to be used in order to set the exact
machinetype)
Libvirt wishes to avoid implementing many different address assignment
policies. It also wishes to keep the domain XML as a representation
of the virtual hardware, not add a bunch of properties to it which
merely serve as tunable input parameters for device addressing
algorithms.
There is thus a dilemma here. Management applications increasingly
need fine grained control over device addressing, while libvirt
doesn't want to expose fine grained policy controls via the XML.
The new libvirt-devaddr API
===
The way out of this is to define a brand new virt management API
which tackles this specific problem in a way that addresses all the
problems mgmt apps have with device addressing and explicitly
pr
See patch 4 for code and 6 for what we try to parse back.
Peter Krempa (6):
virBitmapNewEmpty: Use g_new0 to allocate and remove error checking
virJSONValueObjectDeflattenWorker: Refactor cleanup
util: json: Extract deflattening of keys into a separate function
virjson: Deflatten arrays generated by the json->commandline generator
jsontest: Add test cases for deflattening of arrays
tests: virstoragetest: validate that array deflattening works for
gluster
src/util/virbitmap.c | 14 +--
src/util/virhostcpu.c | 6 +-
src/util/virjson.c| 105 ++
src/util/virtpm.c | 3 +-
tests/virbitmaptest.c | 8 +-
.../deflatten-dotted-array-in.json| 27 +
.../deflatten-dotted-array-out.json | 43 +++
tests/virjsontest.c | 1 +
tests/virstoragetest.c| 18 +++
9 files changed, 182 insertions(+), 43 deletions(-)
create mode 100644 tests/virjsondata/deflatten-dotted-array-in.json
create mode 100644 tests/virjsondata/deflatten-dotted-array-out.json
--
2.24.1
The "" feature, although it's not available for pseries,
can be declared in the domain XML of ppc64 guests without errors.
But setting its 'eoi' attribute will break QEMU. For "":
qemu-kvm: Expected key=value format, found +kvm_pv_eoi
A similar error happens with eoi='off'.
One can argue that it's better to simply forbid launching ppc64
guests with "" declared in the ppc64 XML - it is a feature that
the architecture doesn't support and this would make it clearer
about it. This is sensible, but there are ppc64 guests that are running
with "" declared in the domain (and A LOT of guests running with
"" for that matter, probably reminiscent of x86 templates that
were reused for ppc64) that will stop working if we go this route.
A more subtle approach is to detect if the 'eoi' element is being set
for ppc64 guests, and warn the user about it with a better error
message than the one QEMU provides. This is the new error message
when any value is set for the 'eoi' element:
error: unsupported configuration: The 'eoi' attribute of the 'apic'
feature is not supported for architecture 'ppc64' or machine type
'pseries'.
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index edc8ba2ddb..381b62b96a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5262,8 +5262,26 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
}
break;
-case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_APIC:
+/* the declaration is harmless for ppc64, but
+ * its 'eoi' attribute isn't. To detect if 'eoi' was
+ * present in the XML we can check the tristate switch
+ * of def->apic_eoi */
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT &&
+ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The 'eoi' attribute of the '%s' feature "
+ "is not supported for architecture '%s' or "
+ "machine type '%s'"),
+ featureName,
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
+case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
--
2.25.1
The Perfomance Monitoring Unit (PMU) feature is not available for
the Power architecture. The "" feature will always have a value
'on' or 'off' after saving the domain XML, and both will be rejected
by QEMU when launching. This is the error message for
"":
qemu-kvm: can't apply global host-powerpc64-cpu.pmu=on: Property '.pmu' not
found
A similar error message is thrown for "".
This patch prevents the pseries guest from launching with any
pmu setting with a more informative error message:
error: unsupported configuration: The 'pmu' feature is not
supported for architecture 'ppc64' or machine type 'pseries'
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c67abec778..2e5f987a04 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5282,6 +5282,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
break;
case VIR_DOMAIN_FEATURE_PVSPINLOCK:
+case VIR_DOMAIN_FEATURE_PMU:
if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
ARCH_IS_PPC64(def->os.arch)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -5301,7 +5302,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
case VIR_DOMAIN_FEATURE_PRIVNET:
case VIR_DOMAIN_FEATURE_HYPERV:
case VIR_DOMAIN_FEATURE_CAPABILITIES:
-case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_MSRS:
case VIR_DOMAIN_FEATURE_LAST:
break;
--
2.25.1
PowerPC does not support the 'pvspinlock' feature. The ""
declaration will always have a value 'on' or 'off', and both will
break QEMU when launching. This is the error message for
"":
qemu-kvm: Expected key=value format, found +kvm_pv_unhalt
A similar error message is thrown for "".
This patch prevents the pseries guest from launching with any
pvspinlock setting with a more informative error message:
error: unsupported configuration: The 'pvspinlock' feature is not
supported for architecture 'ppc64' or machine type 'pseries'
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 381b62b96a..c67abec778 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5281,13 +5281,25 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
}
break;
+case VIR_DOMAIN_FEATURE_PVSPINLOCK:
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' feature is not supported for "
+ "architecture '%s' or machine type '%s'"),
+ featureName,
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
case VIR_DOMAIN_FEATURE_PRIVNET:
case VIR_DOMAIN_FEATURE_HYPERV:
-case VIR_DOMAIN_FEATURE_PVSPINLOCK:
case VIR_DOMAIN_FEATURE_CAPABILITIES:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_MSRS:
--
2.25.1
Hyperv features aren't supported in QEMU for ppc64. The
declaration in the XML by itself is benign, but any of its 14 current
features will break QEMU with an error like this:
qemu-kvm: Expected key=value format, found hv_relaxed
This is a more extreme case than the one for apic eoi because we
would need an extra 'switch' statement, with all current Hyperv
features in the body of qemuDomainDefValidateFeatures(), to
check if the user attempted to activate any of them. It's easier to
simply fail to launch with any 'hyperv' declaration in the XML for
ppc64 guests.
A fair disclaimer about Windows and PowerPC: the last Windows version
that ran in the architecture is the hall of famer Windows NT 4.0,
launched in 1996 and with end of extended support for the Server
version in 2004 [1]. I am acknowledging that there might be Windows
NT 4.0 users running in PowerPC, but not enough people running it
under KVM/QEMU to justify Libvirt allowing 'hyperv' to exist in the
domain XML of ppc64 domains.
[1] https://en.wikipedia.org/wiki/Windows_NT_4.0
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2e5f987a04..ba458bfde9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5295,12 +5295,23 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
}
break;
+case VIR_DOMAIN_FEATURE_HYPERV:
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Hyperv features are not supported for "
+ "architecture '%s' or machine type '%s'"),
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
case VIR_DOMAIN_FEATURE_PRIVNET:
-case VIR_DOMAIN_FEATURE_HYPERV:
case VIR_DOMAIN_FEATURE_CAPABILITIES:
case VIR_DOMAIN_FEATURE_MSRS:
case VIR_DOMAIN_FEATURE_LAST:
--
2.25.1
Hi,
This work was intended to fix a bug with APIC-EOI setting
only (patch 1). I decided to take a closer look and ended up
handling more cases.
I consider the first 3 patches to be straightforward: those
are conditions that QEMU will complain about and I'm simply
refusing to launch while giving a better error message than
the one QEMU provides.
Patch 4 is more debatable. declaration in
the XML is benign, but any of the 14 features being declared
will cause QEMU errors. Instead of putting more code (i.e.
a switch with 14 features in the middle of the code) to
prevent any of those features to be enabled, I made a call
to simply refuse to launch with any setting in
the XML. This is not because I have a beef with Windows
or MS or anything. This is about making an educated guess that
it's highly unlikable that anyone is running ppc64 guests
with declared in the XML, and it's not worth all
the code to support it. But I'm all ears to hear otherwise.
After this series, these are the features that aren't supported by
ppc64 but can be declared in a ppc64 guest without resulting
in a failed launch:
(without 'eoi' setting)
Daniel Henrique Barboza (4):
qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting
qemu_domain.c: do not launch ppc64 guests with 'pvspinlock' setting
qemu_domain.c: do not launch ppc64 guests with 'pmu' setting
qemu_domain.c: do not launch ppc64 guests with Hyperv settings
src/qemu/qemu_domain.c | 49 ++
1 file changed, 45 insertions(+), 4 deletions(-)
--
2.25.1
Hi! How hard implement listen type socket support for rdp graphics? VNC and
SPICE already have such options?
And does this support for rdp available for qemu driver? (I need to access
qemu vm via rdp)
Thanks!
--
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
