Re: [PATCH v2 1/2] util: Rework virFileFlock() to be unambiguous
On Fri, Jul 10, 2020 at 09:45:25PM +0200, Martin Kletzander wrote:
The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
a lot of confusion when reading the callers. The new approach is explicit and
unambiguous.
Signed-off-by: Martin Kletzander
Reviewed-by: Andrea Bolognani
---
src/util/virfile.c| 27 ++-
src/util/virfile.h| 9 -
src/util/virresctrl.c | 4 ++--
3 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 213acdbcaa2b..554011eecb25 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -465,23 +465,33 @@ int virFileUnlock(int fd, off_t start, off_t len)
/**
* virFileFlock:
* @fd: file descriptor to call flock on
- * @lock: true for lock, false for unlock
- * @shared: true if shared, false for exclusive, ignored if `@lock == false`
+ * @op: operation to perform
*
* This is just a simple wrapper around flock(2) that errors out on unsupported
* platforms.
*
* The lock will be released when @fd is closed or this function is called with
- * `@lock == false`.
+ * `@op == VIR_FILE_FLOCK_UNLOCK`.
*
* Returns 0 on success, -1 otherwise (with errno set)
*/
-int virFileFlock(int fd, bool lock, bool shared)
+int virFileFlock(int fd, virFileFlockOperation op)
{
-if (lock)
-return flock(fd, shared ? LOCK_SH : LOCK_EX);
+int flock_op = -1;
-return flock(fd, LOCK_UN);
+switch (op) {
+case VIR_FILE_FLOCK_SHARED:
+flock_op = LOCK_SH;
+break;
+case VIR_FILE_FLOCK_EXCLUSIVE:
+flock_op = LOCK_EX;
+break;
+case VIR_FILE_FLOCK_UNLOCK:
+flock_op = LOCK_UN;
+break;
Yes, I know I forgot to add the default branch and it complicates matters even
more because it would set an error in a function that just sets errno... I'll
send a v3.
+}
+
+return flock(fd, flock_op);
}
#else /* WIN32 */
@@ -505,8 +515,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
int virFileFlock(int fd G_GNUC_UNUSED,
- bool lock G_GNUC_UNUSED,
- bool shared G_GNUC_UNUSED)
+ virFileFlockOperation op G_GNUC_UNUSED)
{
errno = ENOSYS;
return -1;
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 7a92364a5c9f..04428727fd3b 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -118,7 +118,14 @@ int virFileLock(int fd, bool shared, off_t start, off_t
len, bool waitForLock)
int virFileUnlock(int fd, off_t start, off_t len)
G_GNUC_NO_INLINE;
-int virFileFlock(int fd, bool lock, bool shared);
+
+typedef enum {
+VIR_FILE_FLOCK_EXCLUSIVE,
+VIR_FILE_FLOCK_SHARED,
+VIR_FILE_FLOCK_UNLOCK,
+} virFileFlockOperation;
+
+int virFileFlock(int fd, virFileFlockOperation op);
typedef int (*virFileRewriteFunc)(int fd, const void *opaque);
int virFileRewrite(const char *path,
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 3563fc560db5..b685b8bb6f7b 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
return -1;
}
-if (virFileFlock(fd, true, true) < 0) {
+if (virFileFlock(fd, VIR_FILE_FLOCK_SHARED) < 0) {
virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
VIR_FORCE_CLOSE(fd);
return -1;
@@ -485,7 +485,7 @@ virResctrlUnlock(int fd)
virReportSystemError(errno, "%s", _("Cannot close resctrl"));
/* Trying to save the already broken */
-if (virFileFlock(fd, false, false) < 0)
+if (virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) < 0)
virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
return -1;
--
2.27.0
signature.asc
Description: PGP signature
[PATCH v2 2/2] resctrl: Use exclusive lock for /sys/fs/resctrl
That's the way it should've been all the time. It was originally the case, but
then the rework to virFileFlock() made the function ambiguous when it was
created in commit 5a0a5f7fb5f5, and due to that it was misused in commit
657ddeff2313 and since then the lock being taken was shared rather than
exclusive.
Signed-off-by: Martin Kletzander
Reviewed-by: Andrea Bolognani
---
src/util/virresctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index b685b8bb6f7b..784a8d43bb2f 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
return -1;
}
-if (virFileFlock(fd, VIR_FILE_FLOCK_SHARED) < 0) {
+if (virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) < 0) {
virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
VIR_FORCE_CLOSE(fd);
return -1;
--
2.27.0
[PATCH v2 1/2] util: Rework virFileFlock() to be unambiguous
The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
a lot of confusion when reading the callers. The new approach is explicit and
unambiguous.
Signed-off-by: Martin Kletzander
Reviewed-by: Andrea Bolognani
---
src/util/virfile.c| 27 ++-
src/util/virfile.h| 9 -
src/util/virresctrl.c | 4 ++--
3 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 213acdbcaa2b..554011eecb25 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -465,23 +465,33 @@ int virFileUnlock(int fd, off_t start, off_t len)
/**
* virFileFlock:
* @fd: file descriptor to call flock on
- * @lock: true for lock, false for unlock
- * @shared: true if shared, false for exclusive, ignored if `@lock == false`
+ * @op: operation to perform
*
* This is just a simple wrapper around flock(2) that errors out on unsupported
* platforms.
*
* The lock will be released when @fd is closed or this function is called with
- * `@lock == false`.
+ * `@op == VIR_FILE_FLOCK_UNLOCK`.
*
* Returns 0 on success, -1 otherwise (with errno set)
*/
-int virFileFlock(int fd, bool lock, bool shared)
+int virFileFlock(int fd, virFileFlockOperation op)
{
-if (lock)
-return flock(fd, shared ? LOCK_SH : LOCK_EX);
+int flock_op = -1;
-return flock(fd, LOCK_UN);
+switch (op) {
+case VIR_FILE_FLOCK_SHARED:
+flock_op = LOCK_SH;
+break;
+case VIR_FILE_FLOCK_EXCLUSIVE:
+flock_op = LOCK_EX;
+break;
+case VIR_FILE_FLOCK_UNLOCK:
+flock_op = LOCK_UN;
+break;
+}
+
+return flock(fd, flock_op);
}
#else /* WIN32 */
@@ -505,8 +515,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
int virFileFlock(int fd G_GNUC_UNUSED,
- bool lock G_GNUC_UNUSED,
- bool shared G_GNUC_UNUSED)
+ virFileFlockOperation op G_GNUC_UNUSED)
{
errno = ENOSYS;
return -1;
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 7a92364a5c9f..04428727fd3b 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -118,7 +118,14 @@ int virFileLock(int fd, bool shared, off_t start, off_t
len, bool waitForLock)
int virFileUnlock(int fd, off_t start, off_t len)
G_GNUC_NO_INLINE;
-int virFileFlock(int fd, bool lock, bool shared);
+
+typedef enum {
+VIR_FILE_FLOCK_EXCLUSIVE,
+VIR_FILE_FLOCK_SHARED,
+VIR_FILE_FLOCK_UNLOCK,
+} virFileFlockOperation;
+
+int virFileFlock(int fd, virFileFlockOperation op);
typedef int (*virFileRewriteFunc)(int fd, const void *opaque);
int virFileRewrite(const char *path,
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 3563fc560db5..b685b8bb6f7b 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
return -1;
}
-if (virFileFlock(fd, true, true) < 0) {
+if (virFileFlock(fd, VIR_FILE_FLOCK_SHARED) < 0) {
virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
VIR_FORCE_CLOSE(fd);
return -1;
@@ -485,7 +485,7 @@ virResctrlUnlock(int fd)
virReportSystemError(errno, "%s", _("Cannot close resctrl"));
/* Trying to save the already broken */
-if (virFileFlock(fd, false, false) < 0)
+if (virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) < 0)
virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
return -1;
--
2.27.0
[PATCH v2 0/2] Fix resctrl locking
Le Blurb: Re-sending even when reviewed because I don't feel that confident, even after reading the commit messages after myself. Let's be honest, I'm resending it only because of the commit messages. Martin Kletzander (2): util: Rework virFileFlock() to be unambiguous resctrl: Use exclusive lock for /sys/fs/resctrl src/util/virfile.c| 27 ++- src/util/virfile.h| 9 - src/util/virresctrl.c | 4 ++-- 3 files changed, 28 insertions(+), 12 deletions(-) -- 2.27.0
Re: [PATCH] util: Rework virFileFlock() to be unambiguous
On Fri, Jul 10, 2020 at 03:50:07PM +0200, Andrea Bolognani wrote:
On Fri, 2020-07-10 at 15:20 +0200, Martin Kletzander wrote:
The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
a lot of confusion when reading the callers. The new approach is explicit and
unambiguous.
While at it, also change the only caller so that it acquires an exclusive lock
as it should've been all the time. This was caused because the function was
ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
misused in commit 657ddeff2313 and since then the lock being taken was shared
rather than exclusive.
I don't think you should do both things in the same patch: please
change the virFileFlock() interface, with no semantic changes, in one
patch, and fix virResctrlLockWrite() in a separate one.
+switch (op) {
+case VIR_FILE_FLOCK_SHARED:
+flock_op = LOCK_SH;
+break;
+case VIR_FILE_FLOCK_EXCLUSIVE:
+flock_op = LOCK_EX;
+break;
+case VIR_FILE_FLOCK_UNLOCK:
+flock_op = LOCK_UN;
+break;
+}
This switch() statement is missing
default:
virReportEnumRangeError(virFileFlockOperation, op);
Did that exist back in the old days when I was sending patches??? =D Anyway
yeah, it's an enum, not a sum type.
And I thought you liked Rust?!? :D
That match {} would not have to have the default, that's one of the reasons I
did not add it in the first place ;)
+++ b/src/util/virresctrl.c
@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
return -1;
}
-if (virFileFlock(fd, true, true) < 0) {
+if (virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) < 0) {
As mentioned above, use VIR_FILE_FLOCK_SHARED here and then change it
to VIR_FILE_FLOCK_EXCLUSIVE in a separate patch.
I agree with that in all other patches. In this one, where resctrl is (and most
probably will forever stay) the only user of virFileFlock()... I'll just do it
without agreeing.
With the default case added and the semantic-altering changes
removed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
signature.asc
Description: PGP signature
Re: [PATCH] resctrl: Do not open directory for writing
On Fri, Jul 10, 2020 at 04:00:13PM +0200, Michal Privoznik wrote: On 7/10/20 3:39 PM, Andrea Bolognani wrote: On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote: On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote: On 7/9/20 6:30 PM, Andrea Bolognani wrote: This is all bikeshedding, of course: what actually matters is making that lock exclusive once again :) Just realized that for exclusive (aka write) lock, the FD must be opened for writing (working on patches for the following report [1] and been experimenting a bit and that's what I'm seeing). Good point, but luckily not related to flock(2). That seems to be the case: according to flock(2), A shared or exclusive lock can be placed on a file regardless of the mode in which the file was opened. Michal, does that sound reasonable to you? D'oh! of course this is another case of file locking exemptions. The patches I sent earlier today fix code around virFileLock() which is fcntl() which is POSIX locking. flock(2) is BSD lock which may or may not be implementedusing POSIX locks. So I think we're okay on that front. Alternatively, we may switch to OFD (F_OFD_SETLK from fcntl(2)) and experience proper file locking. Those are Linux only (but so is resctrl). Unfortunately it is not stated anywhere that it would have to stay Linux-only and moreover the kernel recommendation is to use flock(2) which means there are other applications that will use flock(2) which means we cannot really switch to any other lock unless guaranteed that it is mutually exclusive with the current one. But it would be nice ;-) signature.asc Description: PGP signature
Re: [PATCH] storage: fix vstorage backend build
On Fri, 2020-07-10 at 18:26 +0100, Daniel P. Berrangé wrote: > On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote: > > That failed with > > > > Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz) > > Requires: libjson-c.so.2(libjson-c.so.2)(64bit) > > > > which is surprising because I have the json-c package installed. > > I think that's caused by a bug in your spec file - the name of the > > library should not appear in parentheses - but I can't seem to find > > the source package for vstorage-client under > > > > http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/ > > Yes, this dependancy is clearly broken, whcih is the reason why > we're not using the URL you show above, and instead using the older > release at: > > https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/ > > this URL is the last one that doesn't have the broken dependancies. Note that we're not just talking about different releases available from the same repository, but about two completely different repositories: the one we're using is under https://download.openvz.org/virtuozzo/releases/ whereas the one Nikolay pointed to for vstorage package is under http://repo.virtuozzo.com/vz/releases/ If you look at the list of releases, you'll see that the versions are similar but always a bit different: I couldn't find a single release which is available on both sites. If you look at the list of packages through the provided "repoview" files, the releases from the second repository contain many more: additional categories include things like "Readykernel", Virtuozzo High Availability" and, most interesting to us, "Virtuozzo Storage". As mentioned in my previous message, some of these packages appear to be released not under the (L)GPL but under a "Virtuozzo" license that I haven't been able to find anywhere, and I'm not entirely sure is actually open source. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] storage: fix vstorage backend build
On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote: > On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote: > > On 09.07.2020 19:06, Andrea Bolognani wrote: > > > One thing at a time, though. First, how do we get the vstorage > > > commands included in the CentOS 7 container? What packages need to > > > be installed, and from what repository? > > > > The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os > > > > vstorage binary is in vstorage-ctl package and vstorage-mount binary > > is in vstorage-client package. > > > > However vstorage binary is not used at all the driver. Also the openvz > > driver for example does not check for its binaries. Probably > > vstorage driver should be changed accordingly as binaries are not > > build requisites. > > I'm not sure how the best way to handle the situation is. You should > look at what we do with qemu-img for inspiration. > > > > As an aside, I'm still very confused by the vz/openvz dichotomy. > > > AFAICT, the latter can be (and in fact is) built unconditionally, > > > but the former requires the "Parallels SDK" packages to be installed: > > > baffingly enough, said SDK is obtained from the repository mentioned > > > above, which just so happens to include the string "openvz" twice in > > > its URL... > > > > Yeah, naming is confusing. Basically openvz manages system containers thru > > vzctl binary. Vz driver manages both VMs/containers thru single connection > > using prlsdk. And originally vz driver was called parallels driver but after > > company split we have to change the name. Also in the past prlsdk was only > > commercially available and now times changes and both vzctl and prlsdk are > > available under openvz name which is an umbrella for uncommercial projects. > > Thanks for the explanation, but I'm afraid that even with it the > relationship between the various projects and products is only > marginally clearer to me O:-) > > Anyway. > > Starting from our existing CentOS 7 build environment, I've added > > [vz] > baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/ > enabled=1 > gpgcheck=1 > priority=90 > includepkgs=vstorage*,libvz* > > to /etc/yum.repos.d/vz.repo and tried to install vstorage-client. > That failed with > > Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz) > Requires: libjson-c.so.2(libjson-c.so.2)(64bit) > > which is surprising because I have the json-c package installed. > I think that's caused by a bug in your spec file - the name of the > library should not appear in parentheses - but I can't seem to find > the source package for vstorage-client under > > http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/ Yes, this dependancy is clearly broken, whcih is the reason why we're not using the URL you show above, and instead using the older release at: https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/ this URL is the last one that doesn't have the broken dependancies. I asked about this at the time but didn't get any response, so I guess it was buried in the email torrent and then i forgot to ping about it. https://www.redhat.com/archives/libvir-list/2019-December/msg00437.html 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 :|
Re: [libvirt PATCH 9/9] rpc: use new virt-nc binary for remote tunnelling
On Thu, 2020-07-09 at 19:36 +0100, Daniel P. Berrangé wrote: > This wires up support for using the new virt-nc binary with the ssh, > libssh and libssh2 protocols. > > The new binary will be used preferentially if it is available in $PATH, > otherwise we fall back to traditional netcat. > > The "proxy" URI parameter can be used to force use of netcat e.g. > > qemu+ssh://host/system?proxy=netcat > > or the disable fallback e.g. > > qemu+ssh://host/system?proxy=virt-nc > > With use of virt-nc, we can now support remote session URIs > > qemu+ssh://host/session > > and this will only use virt-nc, with no fallback. This also lets the > libvirtd process be auto-started. Just a couple of comments about the UI: would it make sense to use something like qemu+ssh://host/system?tunnelcmd=virt-tunnel instead? virt-nc could be seen as a bit of a misnomer, considering that (by design) it doesn't do nearly as much as the actual netcat does; as for the 'proxy' argument, I'm afraid it might lead people to believe it's used for HTTP proxying or some other form of proxying *between the client and the host*, whereas it's really something that only affects operations on the host itself - not to mention that we also have a virtproxyd now, further increasing the potential for confusion... I'd also be remiss if I didn't point out that this is definitely a change worth documenting in our release notes :) -- Andrea Bolognani / Red Hat / Virtualization
Re: libvirt-6.5.0 breaks host passthrough migration
On Fri, Jul 10, 2020 at 07:48:26 -0400, Mark Mielke wrote: > On Fri, Jul 10, 2020 at 7:14 AM Jiri Denemark wrote: > > > On Sun, Jul 05, 2020 at 12:45:55 -0400, Mark Mielke wrote: > > > With 6.4.0, live migration was working fine with Qemu 5.0. After trying > > out > > > 6.5.0, migration broke with the following error: > > > > > > libvirt.libvirtError: internal error: unable to execute QEMU command > > > 'migrate': State blocked by non-migratable CPU device (invtsc flag) > > > > Could you please describe the reproducer steps? For example, was the > > domain you're trying to migrate already running when you upgrade libvirt > > or is it freshly started by the new libvirt? > > > > > The original case was: > > 1) Machine X running libvirt 6.4.0 + qemu 5.0 > 2) Machine Y running libvirt 6.5.0 + qemu 5.0 > 3) Live migration from X to Y works. Guest appears fine. > 4) Upgrade Machine X from libvirt 6.4.0 to 6.5.0 and reboot. > 5) Live migration from Y to X fails with the message shown. Oh I see, so I guess the bad default is chosen during the incoming migration to machine Y. I'll try to reproduce myself to see what's going on. > In each case, live migration was done with OpenStack Train directing > libvirt + qemu. > > > And it would be helpful to see the element as shown by virsh > > dumpxml before you try to start the domain as well as the QEMU command > > line libvirt used to start the domain (in > > /var/log/libvirt/qemu/$VM.log). > > > > The element looks like this: > > > > > > The QEMU command line is very long, and includes details I would avoid > publishing publicly unless you need them. The "-cpu" portion is just: > > -cpu host > > The QEMU command line itself is generated from libvirt, which is directed > by OpenStack Train. These are from machine X before step 3, right? Can you also share the same from machine Y before step 5? > I wasn't sure what QEMU_CAPS_CPU_MIGRATABLE represents. I initially > suspected what you are saying, but since it apparently did not work the way > I expected, I then presumed it does not work the way I expected. :-) > > Is QEMU_CAPS_CPU_MIGRATABLE only from the element? If so, doesn't > this mean that it is not explicitly listed for host-passthrough, and this > means the check is not detecting whether it is enabled or not properly? QEMU_CAPS_CPU_MIGRATABLE comes from the QEMU capability probing. Specifically, the capability is enabled when a given QEMU binary reports 'migratable' property for the CPU object. And the capability detection tests show we should be properly detecting this capability: tests/qemucapabilitiesdata $ git grep cpu.migratable caps_2.12.0.x86_64.xml: caps_3.0.0.x86_64.xml: caps_3.1.0.x86_64.xml: caps_4.0.0.x86_64.xml: caps_4.1.0.x86_64.xml: caps_4.2.0.x86_64.xml: caps_5.0.0.x86_64.xml: caps_5.1.0.x86_64.xml: > I think it can go either way. There is also convention over configuration > as a competing principle. However, I also prefer explicit. Just, it needs > to be correct, otherwise explicit can be very bad, as it seems in my case. > :-) Of course, the explicit default must match the implicit one. Jirka
Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it
On 10/07/20 18:02, Eduardo Habkost wrote: > On Fri, Jul 10, 2020 at 09:22:42AM +0200, Paolo Bonzini wrote: >> On 09/07/20 21:13, Eduardo Habkost wrote: Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE mode, so that the hypervisor can validate the high bits in the PDPTEs? >>> If the fix has additional overhead, is the additional overhead >>> bad enough to warrant making it optional? Most existing >>> GUEST_MAXPHYADDR < HOST_MAXPHYADDR guests already work today >>> without the fix. >> >> The problematic case is when host maxphyaddr is 52. That case wouldn't >> work at all without the fix. > > What can QEMU do to do differentiate "can't work at all without > the fix" from "not the best idea, but will probably work"? Blocking guest_maxphyaddr < host_maxphyaddr if maxphyaddr==52 would be a good start. However it would block the default configuration on IceLake processors (which is why Mohammed looked at this thing in the first place). Paolo
Re: [PATCH] storage: fix vstorage backend build
On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote: > On 09.07.2020 19:06, Andrea Bolognani wrote: > > One thing at a time, though. First, how do we get the vstorage > > commands included in the CentOS 7 container? What packages need to > > be installed, and from what repository? > > The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os > > vstorage binary is in vstorage-ctl package and vstorage-mount binary > is in vstorage-client package. > > However vstorage binary is not used at all the driver. Also the openvz > driver for example does not check for its binaries. Probably > vstorage driver should be changed accordingly as binaries are not > build requisites. I'm not sure how the best way to handle the situation is. You should look at what we do with qemu-img for inspiration. > > As an aside, I'm still very confused by the vz/openvz dichotomy. > > AFAICT, the latter can be (and in fact is) built unconditionally, > > but the former requires the "Parallels SDK" packages to be installed: > > baffingly enough, said SDK is obtained from the repository mentioned > > above, which just so happens to include the string "openvz" twice in > > its URL... > > Yeah, naming is confusing. Basically openvz manages system containers thru > vzctl binary. Vz driver manages both VMs/containers thru single connection > using prlsdk. And originally vz driver was called parallels driver but after > company split we have to change the name. Also in the past prlsdk was only > commercially available and now times changes and both vzctl and prlsdk are > available under openvz name which is an umbrella for uncommercial projects. Thanks for the explanation, but I'm afraid that even with it the relationship between the various projects and products is only marginally clearer to me O:-) Anyway. Starting from our existing CentOS 7 build environment, I've added [vz] baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/ enabled=1 gpgcheck=1 priority=90 includepkgs=vstorage*,libvz* to /etc/yum.repos.d/vz.repo and tried to install vstorage-client. That failed with Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz) Requires: libjson-c.so.2(libjson-c.so.2)(64bit) which is surprising because I have the json-c package installed. I think that's caused by a bug in your spec file - the name of the library should not appear in parentheses - but I can't seem to find the source package for vstorage-client under http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/ Moreover, when I try to get information about eg. vstorage-client, the output of 'yum info' contains License : Virtuozzo Is this an actual open source license that I can find anywhere? Until the packaging issue has been addressed and the license situation has been clarified, we can't move forward with building the vstorage driver as part of our CI pipeline. I did, however, reproduce the build failure you describe above and confirm that your patch fixes it in the process of writing this message, so Reviewed-by: Andrea Bolognani to that. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it
On Fri, Jul 10, 2020 at 09:22:42AM +0200, Paolo Bonzini wrote: > On 09/07/20 21:13, Eduardo Habkost wrote: > >> Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE > >> mode, so that the hypervisor can validate the high bits in the PDPTEs? > > If the fix has additional overhead, is the additional overhead > > bad enough to warrant making it optional? Most existing > > GUEST_MAXPHYADDR < HOST_MAXPHYADDR guests already work today > > without the fix. > > The problematic case is when host maxphyaddr is 52. That case wouldn't > work at all without the fix. What can QEMU do to do differentiate "can't work at all without the fix" from "not the best idea, but will probably work"? -- Eduardo
Re: [libvirt PATCH v2 4/5] m4: virt-xdr: rewrite XDR check
On Fri, 2020-07-10 at 14:12 +0200, Pavel Hrdina wrote:
> On Fri, Jul 10, 2020 at 01:13:00PM +0200, Andrea Bolognani wrote:
> > On Fri, 2020-07-10 at 12:25 +0200, Pavel Hrdina wrote:
> > > +%if 0%{?fedora} || 0%{?rhel}
> > > BuildRequires: libtirpc-devel
> > > %endif
> >
> > We only support building RPMs on Fedora and RHEL, so this check is
> > effectively a no-op.
>
> OK, I'll drop it before pushing.
Cool. Everything else looks reasonable, so under the assumption that
you've made sure it actually builds on all supported targets
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
[libvirt PATCH] docs: rename fig to svg in Makefile.am
Commit <9ad637c9651ff29955dd6aa8fe31f639b42b7315> converted all fig files into svg files but did not change the Makefile.am. Signed-off-by: Pavel Hrdina --- Pushed under build-breaker rule. docs/Makefile.am | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/Makefile.am b/docs/Makefile.am index 3fd8256e668..a480123e33f 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -343,15 +343,15 @@ api_DATA = \ libvirt-lxc-api.xml \ libvirt-admin-api.xml -fig = \ - libvirt-daemon-arch.fig \ - libvirt-driver-arch.fig \ - libvirt-object-model.fig \ - migration-managed-direct.fig \ - migration-managed-p2p.fig \ - migration-native.fig \ - migration-tunnel.fig \ - migration-unmanaged-direct.fig +svg = \ + libvirt-daemon-arch.svg \ + libvirt-driver-arch.svg \ + libvirt-object-model.svg \ + migration-managed-direct.svg \ + migration-managed-p2p.svg \ + migration-native.svg \ + migration-tunnel.svg \ + migration-unmanaged-direct.svg schemadir = $(pkgdatadir)/schemas schema_DATA = $(wildcard $(srcdir)/schemas/*.rng) @@ -359,7 +359,7 @@ schema_DATA = $(wildcard $(srcdir)/schemas/*.rng) EXTRA_DIST= \ site.xsl subsite.xsl newapi.xsl page.xsl \ $(dot_html_in) $(dot_rst) $(apipng) \ - $(fig) $(assets) \ + $(svg) $(assets) \ $(javascript) $(logofiles) \ $(internals_html_in) $(internals_rst) $(fonts) \ $(kbase_html_in) $(kbase_rst) \ -- 2.26.2
Re: [PATCH 17/18] qemu: caps: Enable QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI
On Fri, Jul 10, 2020 at 16:33:38 +0200, Peter Krempa wrote:
> Enable it when regular QEMU_CAPS_BLOCKDEV is present.
>
> Signed-off-by: Peter Krempa
> ---
> src/qemu/qemu_capabilities.c | 3 ++
> .../caps_4.2.0.aarch64.xml| 1 +
> .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 +
> .../caps_4.2.0.x86_64.xml | 1 +
> .../caps_5.0.0.aarch64.xml| 1 +
> .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 +
> .../caps_5.0.0.riscv64.xml| 1 +
> .../caps_5.0.0.x86_64.xml | 1 +
> .../caps_5.1.0.x86_64.xml | 1 +
> .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++
> ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46
> 11 files changed, 65 insertions(+), 44 deletions(-)
>
Kevin, Markus, could you please check, that he change from -drive
if=none to -blockdev is equivalent/makes sense in these cases.
Usage of SCSI hostdevs is finnicky as it tends to be used with weird
devices such as tape libraries. I've tested it only with scsi_debug.
Thanks.
> diff --git
> a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args
> b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args
> index 99b0117447..72980d58b8 100644
> --- a/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/hostdev-scsi-virtio-scsi.x86_64-latest.args
> @@ -34,40 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \
> -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\
> "file":"libvirt-1-storage"}' \
> -device
> ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1 \
> --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
> +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\
> +"node-name":"libvirt-hostdev0-backend","read-only":false}' \
> -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=8,\
> -drive=drive-hostdev0,id=hostdev0 \
> --drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \
> +drive=libvirt-hostdev0-backend,id=hostdev0 \
> +-blockdev '{"driver":"host_device","filename":"/dev/sg0",\
> +"node-name":"libvirt-hostdev1-backend","read-only":true}' \
> -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=4,lun=7,\
> -drive=drive-hostdev1,id=hostdev1 \
> --drive file.driver=iscsi,file.portal=example.org:3260,\
> -file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\
> -format=raw,id=drive-hostdev2 \
> +drive=libvirt-hostdev1-backend,id=hostdev1 \
> +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\
> +"target":"iqn.1992-01.com.example","lun":0,"transport":"tcp",\
> +"node-name":"libvirt-hostdev2-backend","read-only":false}' \
> -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\
> -drive=drive-hostdev2,id=hostdev2 \
> --drive file.driver=iscsi,file.portal=example.org:3260,\
> -file.target=iqn.1992-01.com.example,file.lun=1,file.transport=tcp,if=none,\
> -format=raw,id=drive-hostdev3 \
> +drive=libvirt-hostdev2-backend,id=hostdev2 \
> +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\
> +"target":"iqn.1992-01.com.example","lun":1,"transport":"tcp",\
> +"node-name":"libvirt-hostdev3-backend","read-only":false}' \
> -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\
> -drive=drive-hostdev3,id=hostdev3 \
> +drive=libvirt-hostdev3-backend,id=hostdev3 \
> -object secret,id=hostdev4-secret0,\
> data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
> keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
> --drive file.driver=iscsi,file.portal=example.org:3260,\
> -file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\
> -file.user=myname,file.password-secret=hostdev4-secret0,if=none,format=raw,\
> -id=drive-hostdev4 \
> +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\
> +"target":"iqn.1992-01.com.example:storage","lun":1,"transport":"tcp",\
> +"user":"myname","password-secret":"hostdev4-secret0",\
> +"node-name":"libvirt-hostdev4-backend","read-only":false}' \
> -device scsi-generic,bus=scsi0.0,channel=0,scsi-id=3,lun=4,\
> -drive=drive-hostdev4,id=hostdev4 \
> +drive=libvirt-hostdev4-backend,id=hostdev4 \
> -object secret,id=hostdev5-secret0,\
> data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\
> keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \
> --drive file.driver=iscsi,file.portal=example.org:3260,\
> -file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\
> -file.user=myname,file.password-secret=hostdev5-secret0,if=none,format=raw,\
> -id=drive-hostdev5 \
> +-blockdev '{"driver":"iscsi","portal":"example.org:3260",\
> +"target":"iqn.1992-01.com.example:storage","lun":2,"transport":"tcp",\
> +"user":"myname","password-secret":"hostdev5-secret0",\
> +"node-name":"libvirt-hostdev5-backend","read-only":false}' \
> -device
Re: [PATCH 2/4] qemu_domainjob: job funcitons moved out of `qemu_domain`
On 7/10/20 9:11 AM, Prathamesh Chavan wrote:
We move functions `qemuDomainObjPrivateXMLParseJob` and
`qemuDomainObjPrivateXMLFormatJob` to `qemu_domainjob`.
To make this happen, corresponding changes in their
parameters were made, as well as they were exposed
to be used in `qemu_domain` file.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_domain.c| 245 +-
src/qemu/qemu_domainjob.c | 244 +
src/qemu/qemu_domainjob.h | 8 ++
3 files changed, 254 insertions(+), 243 deletions(-)
static bool
qemuDomainHasSlirp(virDomainObjPtr vm)
{
@@ -2399,7 +2303,7 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
if (priv->lockState)
virBufferAsprintf(buf, "%s\n",
priv->lockState);
-if (qemuDomainObjPrivateXMLFormatJob(buf, vm, priv) < 0)
+if (qemuDomainObjPrivateXMLFormatJob(buf, vm) < 0)
This means, that this patch is not a plain code movement. Just for
future work, break patches into logical/semantic changes. For instance
in this case it could have been: one patch that drops the @priv argument
(since the function already gets @vm from which it can obtain @priv),
the other patch would be actual code movement.
I know it probably doesn't make much sense and I totally get it. I did
not make any sense to me when I was starting with libvirt - the end
result is the same, isn't it? What's the fuss then? but then I started
reviewing patches and realized very quickly how important it is. And
then I started backporting patches to RHEL and I realized it again :-)
But as you'll see in patch 3/4 we probably don't need to move everything.
Michal
Re: [GSoC][PATCH v3 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`
On 7/10/20 9:11 AM, Prathamesh Chavan wrote:
To remove dependecy of `qemuDomainJob` on job specific
paramters, a `privateData` pointer is introduced.
To handle it, structure of callback functions is
also introduced.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_domain.c | 4 +-
src/qemu/qemu_domain.h | 10 +
src/qemu/qemu_domainjob.c| 74 ++--
src/qemu/qemu_domainjob.h| 32 --
src/qemu/qemu_driver.c | 3 +-
src/qemu/qemu_migration.c| 28
src/qemu/qemu_migration_params.c | 9 ++--
src/qemu/qemu_process.c | 15 +--
8 files changed, 119 insertions(+), 56 deletions(-)
Almost.
+typedef void *(*qemuDomainObjPrivateJobAlloc)(void);
+typedef void (*qemuDomainObjPrivateJobFree)(void *);
+typedef int (*qemuDomainObjPrivateJobFormat)(virBufferPtr,
+ virDomainObjPtr);
+typedef int (*qemuDomainObjPrivateJobParse)(virDomainObjPtr,
+xmlXPathContextPtr);
+
+typedef struct _qemuDomainObjPrivateJobCallbacks
qemuDomainObjPrivateJobCallbacks;
+struct _qemuDomainObjPrivateJobCallbacks {
+ qemuDomainObjPrivateJobAlloc allocJobPrivate;
+ qemuDomainObjPrivateJobFree freeJobPrivate;
+ qemuDomainObjPrivateJobFormat formatJob;
+ qemuDomainObjPrivateJobParse parseJob;
+};
+
This looks correct.
typedef struct _qemuDomainJobObj qemuDomainJobObj;
typedef qemuDomainJobObj *qemuDomainJobObjPtr;
struct _qemuDomainJobObj {
@@ -182,14 +197,11 @@ struct _qemuDomainJobObj {
qemuDomainJobInfoPtr current; /* async job progress data */
qemuDomainJobInfoPtr completed; /* statistics data of a recently
completed job */
bool abortJob; /* abort of the job requested */
-bool spiceMigration;/* we asked for spice migration and we
- * should wait for it to finish */
-bool spiceMigrated; /* spice migration completed */
char *error;/* job event completion error */
-bool dumpCompleted; /* dump completed */
-
-qemuMigrationParamsPtr migParams;
unsigned long apiFlags; /* flags passed to the API which started the
async job */
+
+void *privateData; /* job specific collection of data */
+qemuDomainObjPrivateJobCallbacks cb;
And so does this. But these callbacks should be passed to
qemuDomainObjInitJob() which will save them into the cb like this:
int
qemuDomainObjInitJob(qemuDomainJobObjPtr job,
qemuDomainObjPrivateJobCallbacks cb)
{
memset(job, 0, sizeof(*job));
if (virCondInit(>cond) < 0)
return -1;
if (virCondInit(>asyncCond) < 0) {
virCondDestroy(>cond);
return -1;
}
job->cb = cb;
if (!(job->privateData = job->cb.allocJobPrivate())) {
qemuDomainObjFreeJob(job);
return -1;
}
return 0;
}
In general, nothing outside qemu_domainjob.c should be calling these
callbacks. Therefore for instance when formatting private XML it should
looks something like this:
static int
qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
virDomainObjPtr vm)
{
...
if (qemuDomainJobFormat(buf, priv->job, vm) < 0)
return -1;
...
}
This qemuDomainJobFormat() can look something like this:
int
qemuDomainObjJobFormat(virBufferPtr buf,
qemuDomainJobObjPtr job,
void *opaque)
{
g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
if (!qemuDomainTrackJob(job))
job = QEMU_JOB_NONE;
if (job == QEMU_JOB_NONE &&
jobObj->asyncJob == QEMU_ASYNC_JOB_NONE)
return 0;
virBufferAsprintf(, " type='%s' async='%s'",
qemuDomainJobTypeToString(job),
qemuDomainAsyncJobTypeToString(jobObj->asyncJob));
if (jobObj->phase) {
virBufferAsprintf(, " phase='%s'",
qemuDomainAsyncJobPhaseToString(jobObj->asyncJob,
jobObj->phase));
}
if (jobObj->asyncJob != QEMU_ASYNC_JOB_NONE)
virBufferAsprintf(, " flags='0x%lx'", jobObj->apiFlags);
if (job->cb.formatJob(, vm) < 0)
return -1;
virXMLFormatElement(buf, "job", , );
return 0;
}
This looks very close to qemuDomainObjPrivateXMLFormatJob() and that is
exactly how we want it. Except all "private" data is now formatted using
callback (migParams for instance). Does this make more sense?
Michal
Re: [GSoC][PATCH v3 1/4] qemu_domain: moved qemuDomainNamespace to `qemu_domain`
On 7/10/20 9:11 AM, Prathamesh Chavan wrote: While moving the code, qemuDomainNamespace also was moved to `qemu_domainjob`. Hence it is moved back to `qemu_domain` where it will be more appropriate. Signed-off-by: Prathamesh Chavan --- src/qemu/qemu_domain.c| 5 + src/qemu/qemu_domainjob.c | 6 -- 2 files changed, 5 insertions(+), 6 deletions(-) Oops, sorry for not spotting this in the original patchset. Reviewed-by: Michal Privoznik Michal
[PATCH 16/18] qemuDomainRemoveHostDevice: Use new infrastructure for (i)SCSI
Similarly to previous commits, modify the hostdev detach code to use
blockdev infrastructure to detach (i)SCSI hostdevs.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_hotplug.c | 20 +++-
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 06b3b94ff2..607f9fdc0f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4441,31 +4441,17 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr net = NULL;
size_t i;
qemuDomainObjPrivatePtr priv = vm->privateData;
-g_autofree char *drivealias = NULL;
-const char *secretObjAlias = NULL;
VIR_DEBUG("Removing host device %s from domain %p %s",
hostdev->info->alias, vm, vm->def->name);
if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
-virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
-virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
+g_autoptr(qemuBlockStorageSourceAttachData) detachscsi = NULL;
-if (!(drivealias = qemuAliasFromHostdev(hostdev)))
-return -1;
-
-if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
-qemuDomainStorageSourcePrivatePtr srcPriv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
-if (srcPriv && srcPriv->secinfo)
-secretObjAlias = srcPriv->secinfo->s.aes.alias;
-}
+detachscsi = qemuBuildHostdevSCSIDetachPrepare(hostdev,
priv->qemuCaps);
qemuDomainObjEnterMonitor(driver, vm);
-qemuMonitorDriveDel(priv->mon, drivealias);
-
-if (secretObjAlias)
-ignore_value(qemuMonitorDelObject(priv->mon, secretObjAlias,
false));
-
+qemuBlockStorageSourceAttachRollback(priv->mon, detachscsi);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
}
--
2.26.2
[PATCH 08/18] qemu: domain: Regenerate hostdev source private data
When upgrading from a libvirt which didn't format private data of a
virStorageSource representing an iSCSI hostdev source, we might need to
generate some internal data so that the code still works as if it was
present in the status XML.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c| 54 ++-
.../disk-secinfo-upgrade-in.xml | 20 +++
.../disk-secinfo-upgrade-out.xml | 30 +++
3 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3141a94136..41e4d5dd0b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5319,6 +5319,51 @@ qemuDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
}
+/**
+ * qemuDomainDeviceHostdevDefPostParseRestoreSecAlias:
+ *
+ * Re-generate aliases for objects related to the storage source if they
+ * were not stored in the status XML by an older libvirt.
+ *
+ * Note that qemuCaps should be always present for a status XML.
+ */
+static int
+qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(virDomainHostdevDefPtr
hostdev,
+ virQEMUCapsPtr qemuCaps,
+ unsigned int parseFlags)
+{
+qemuDomainStorageSourcePrivatePtr priv;
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
+g_autofree char *authalias = NULL;
+
+if (!(parseFlags & VIR_DOMAIN_DEF_PARSE_STATUS) ||
+!qemuCaps ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_SECRET))
+return 0;
+
+if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
+hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI ||
+scsisrc->protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI ||
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET) ||
+!qemuDomainStorageSourceHasAuth(iscsisrc->src))
+return 0;
+
+if (!(priv = qemuDomainStorageSourcePrivateFetch(iscsisrc->src)))
+return -1;
+
+if (priv->secinfo)
+return 0;
+
+authalias = g_strdup_printf("%s-secret0", hostdev->info->alias);
+
+if (qemuStorageSourcePrivateDataAssignSecinfo(>secinfo, )
< 0)
+return -1;
+
+return 0;
+}
+
+
static int
qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc,
virQEMUCapsPtr qemuCaps)
@@ -5336,10 +5381,15 @@
qemuDomainHostdevDefMdevPostParse(virDomainHostdevSubsysMediatedDevPtr mdevsrc,
static int
qemuDomainHostdevDefPostParse(virDomainHostdevDefPtr hostdev,
- virQEMUCapsPtr qemuCaps)
+ virQEMUCapsPtr qemuCaps,
+ unsigned int parseFlags)
{
virDomainHostdevSubsysPtr subsys = >source.subsys;
+if (qemuDomainDeviceHostdevDefPostParseRestoreSecAlias(hostdev, qemuCaps,
+ parseFlags) < 0)
+return -1;
+
if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV &&
qemuDomainHostdevDefMdevPostParse(>u.mdev, qemuCaps) < 0)
@@ -5414,7 +5464,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
break;
case VIR_DOMAIN_DEVICE_HOSTDEV:
-ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps);
+ret = qemuDomainHostdevDefPostParse(dev->data.hostdev, qemuCaps,
parseFlags);
break;
case VIR_DOMAIN_DEVICE_TPM:
diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml
b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml
index ce55a70637..8023857ffe 100644
--- a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml
+++ b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-in.xml
@@ -491,6 +491,26 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml
b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml
index 5d3287606f..d5534fb0f5 100644
--- a/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml
+++ b/tests/qemustatusxml2xmldata/disk-secinfo-upgrade-out.xml
@@ -513,6 +513,36 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
--
2.26.2
[PATCH 13/18] qemu: command: Create qemuBlockStorageSourceAttachData for (i)SCSI hostdevs
Add convertor for creating qemuBlockStorageSourceAttachData which will
allow reusing the infrastructure which we have for attaching disks also
for hostdevs.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 83 +
src/qemu/qemu_command.h | 8
2 files changed, 91 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 200decd6e0..6b3b1b39ca 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5062,6 +5062,89 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
}
+qemuBlockStorageSourceAttachData *
+qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDefPtr hostdev,
+ virQEMUCapsPtr qemuCaps)
+{
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
+g_autoptr(qemuBlockStorageSourceAttachData) ret =
g_new0(qemuBlockStorageSourceAttachData, 1);
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) {
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
+qemuDomainStorageSourcePrivatePtr srcpriv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+
+ret->storageNodeName = iscsisrc->src->nodestorage;
+
+if (srcpriv && srcpriv->secinfo &&
+srcpriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
+ret->authsecretAlias = g_strdup(srcpriv->secinfo->s.aes.alias);
+} else {
+ret->storageNodeNameCopy = g_strdup_printf("libvirt-%s-backend",
hostdev->info->alias);
+ret->storageNodeName = ret->storageNodeNameCopy;
+}
+
+ret->storageAttached = true;
+} else {
+ret->driveAlias = qemuAliasFromHostdev(hostdev);
+ret->driveAdded = true;
+}
+
+return g_steal_pointer();
+}
+
+
+qemuBlockStorageSourceAttachData *
+qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev,
+ const char **backendAlias,
+ virQEMUCapsPtr qemuCaps)
+{
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = >u.iscsi;
+virStorageSourcePtr src;
+g_autoptr(virStorageSource) localsrc = NULL;
+g_autoptr(qemuBlockStorageSourceAttachData) ret =
g_new0(qemuBlockStorageSourceAttachData, 1);
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI)) {
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
+src = iscsisrc->src;
+} else {
+g_autofree char *devstr = qemuBuildSCSIHostHostdevDrvStr(hostdev);
+
+if (!devstr)
+return NULL;
+
+if (!(src = localsrc = virStorageSourceNew()))
+return NULL;
+
+src->type = VIR_STORAGE_TYPE_BLOCK;
+src->readonly = hostdev->readonly;
+src->path = g_strdup_printf("/dev/%s", devstr);
+}
+
+src->nodestorage = g_strdup_printf("libvirt-%s-backend",
hostdev->info->alias);
+ret->storageNodeNameCopy = g_strdup(src->nodestorage);
+ret->storageNodeName = ret->storageNodeNameCopy;
+*backendAlias = ret->storageNodeNameCopy;
+
+if (!(ret->storageProps = qemuBlockStorageSourceGetBackendProps(src,
+
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)))
+return NULL;
+
+} else {
+ret->driveCmd = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps);
+ret->driveAlias = qemuAliasFromHostdev(hostdev);
+*backendAlias = ret->driveAlias;
+}
+
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI &&
+qemuBuildStorageSourceAttachPrepareCommon(iscsisrc->src, ret,
qemuCaps) < 0)
+return NULL;
+
+return g_steal_pointer();
+}
+
+
static int
qemuBuildHostdevSCSICommandLine(virCommandPtr cmd,
const virDomainDef *def,
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index d0de27d9b0..a43cdb2733 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -182,6 +182,14 @@ char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
const char *backendAlias);
+qemuBlockStorageSourceAttachData *
+qemuBuildHostdevSCSIAttachPrepare(virDomainHostdevDefPtr hostdev,
+ const char **backendAlias,
+ virQEMUCapsPtr qemuCaps);
+qemuBlockStorageSourceAttachData *
+qemuBuildHostdevSCSIDetachPrepare(virDomainHostdevDefPtr hostdev,
+ virQEMUCapsPtr qemuCaps);
+
char *
qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
--
2.26.2
[PATCH 15/18] qemuDomainAttachHostSCSIDevice: Use new infrastructure
Similarly to command line creation, use the blockdev helpers when
hotplugging an (i)SCSI hostdev.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_hotplug.c | 45 +++--
1 file changed, 7 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fbbd6a533c..06b3b94ff2 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2566,17 +2566,12 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
int ret = -1;
qemuDomainObjPrivatePtr priv = vm->privateData;
virErrorPtr orig_err;
+g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
+const char *backendalias = NULL;
g_autofree char *devstr = NULL;
-g_autofree char *drvstr = NULL;
-g_autofree char *drivealias = NULL;
-g_autofree char *secobjAlias = NULL;
bool teardowncgroup = false;
bool teardownlabel = false;
bool teardowndevice = false;
-bool driveAdded = false;
-virJSONValuePtr secobjProps = NULL;
-virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
-qemuDomainSecretInfoPtr secinfo = NULL;
/* Let's make sure the disk has a controller defined and loaded before
* trying to add it. The controller used by the disk must exist before a
@@ -2611,25 +2606,11 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
if (qemuDomainSecretHostdevPrepare(priv, hostdev) < 0)
goto cleanup;
-if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
-qemuDomainStorageSourcePrivatePtr srcPriv =
-QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
-if (srcPriv)
-secinfo = srcPriv->secinfo;
-}
-
-if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
-if (qemuBuildSecretInfoProps(secinfo, ) < 0)
-goto cleanup;
-}
-
-if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps)))
-goto cleanup;
-
-if (!(drivealias = qemuAliasFromHostdev(hostdev)))
+if (!(data = qemuBuildHostdevSCSIAttachPrepare(hostdev, ,
+ priv->qemuCaps)))
goto cleanup;
-if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, drivealias)))
+if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, backendalias)))
goto cleanup;
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
@@ -2637,14 +2618,9 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
qemuDomainObjEnterMonitor(driver, vm);
-if (secobjProps &&
-qemuMonitorAddObject(priv->mon, , ) < 0)
+if (qemuBlockStorageSourceAttachApply(priv->mon, data) < 0)
goto exit_monitor;
-if (qemuMonitorAddDrive(priv->mon, drvstr) < 0)
-goto exit_monitor;
-driveAdded = true;
-
if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
goto exit_monitor;
@@ -2670,18 +2646,11 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
VIR_WARN("Unable to remove host device from /dev");
}
qemuDomainSecretHostdevDestroy(hostdev);
-virJSONValueFree(secobjProps);
return ret;
exit_monitor:
virErrorPreserveLast(_err);
-if (driveAdded && qemuMonitorDriveDel(priv->mon, drivealias) < 0) {
-VIR_WARN("Unable to remove drive %s (%s) after failed "
- "qemuMonitorAddDevice",
- drvstr, devstr);
-}
-if (secobjAlias)
-ignore_value(qemuMonitorDelObject(priv->mon, secobjAlias, false));
+qemuBlockStorageSourceAttachRollback(priv->mon, data);
ignore_value(qemuDomainObjExitMonitor(driver, vm));
virErrorRestore(_err);
--
2.26.2
[PATCH 07/18] qemuDomainSecretHostdevDestroy: Don't clear secinfo alias
We need the alias to deal with hot-unplug of the hostdev. Use
qemuDomainSecretInfoDestroy which clears only the secrets and not the
alias. The same function is used also for handling disk secrets.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_domain.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 248e259634..3141a94136 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1216,8 +1216,8 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr
hostdev)
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
-if (srcPriv && srcPriv->secinfo)
-g_clear_pointer(>secinfo, qemuDomainSecretInfoFree);
+if (srcPriv)
+qemuDomainSecretInfoDestroy(srcPriv->secinfo);
}
}
}
--
2.26.2
[PATCH 01/18] qemuBlockStorageSourceGetBackendProps: Convert boolean arguments to flags
Upcoming commit will need to add another flag for the function so
convert it to a bitwise-or'd array of flags to prevent having 4
booleans.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 32 +---
src/qemu/qemu_block.h | 10 +++---
src/qemu/qemu_command.c | 3 ++-
tests/qemublocktest.c | 18 --
4 files changed, 42 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a2eabbcd64..10ddf53b3b 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1052,26 +1052,32 @@
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src,
/**
* qemuBlockStorageSourceGetBackendProps:
* @src: disk source
- * @legacy: use legacy formatting of attributes (for -drive / old qemus)
- * @onlytarget: omit any data which does not identify the image itself
- * @autoreadonly: use the auto-read-only feature of qemu
+ * @flags: bitwise-or of qemuBlockStorageSourceBackendPropsFlags
+ *
+ * Flags:
+ * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY:
+ * use legacy formatting of attributes (for -drive / old qemus)
+ * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY:
+ * omit any data which does not identify the image itself
+ * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY:
+ * use the auto-read-only feature of qemu
*
* Creates a JSON object describing the underlying storage or protocol of a
* storage source. Returns NULL on error and reports an appropriate error
message.
*/
virJSONValuePtr
qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
- bool legacy,
- bool onlytarget,
- bool autoreadonly)
+ unsigned int flags)
{
int actualType = virStorageSourceGetActualType(src);
g_autoptr(virJSONValue) fileprops = NULL;
const char *driver = NULL;
virTristateBool aro = VIR_TRISTATE_BOOL_ABSENT;
virTristateBool ro = VIR_TRISTATE_BOOL_ABSENT;
+bool onlytarget = flags &
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY;
+bool legacy = flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY;
-if (autoreadonly) {
+if (flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY) {
aro = VIR_TRISTATE_BOOL_YES;
} else {
if (src->readonly)
@@ -1576,15 +1582,18 @@
qemuBlockStorageSourceAttachPrepareBlockdev(virStorageSourcePtr src,
bool autoreadonly)
{
g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
+unsigned int backendpropsflags = 0;
+
+if (autoreadonly)
+backendpropsflags |=
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY;
if (VIR_ALLOC(data) < 0)
return NULL;
if (!(data->formatProps = qemuBlockStorageSourceGetBlockdevProps(src,
backingStore)) ||
-!(data->storageProps = qemuBlockStorageSourceGetBackendProps(src,
false,
- false,
-
autoreadonly)))
+!(data->storageProps = qemuBlockStorageSourceGetBackendProps(src,
+
backendpropsflags)))
return NULL;
data->storageNodeName = src->nodestorage;
@@ -2108,7 +2117,8 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr src,
}
/* use json: pseudo protocol otherwise */
-if (!(backingProps = qemuBlockStorageSourceGetBackendProps(src, false,
true, false)))
+if (!(backingProps = qemuBlockStorageSourceGetBackendProps(src,
+
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY)))
return NULL;
props = backingProps;
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index b1bdb39613..715739e59c 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -56,11 +56,15 @@ qemuBlockGetNodeData(virJSONValuePtr data);
bool
qemuBlockStorageSourceSupportsConcurrentAccess(virStorageSourcePtr src);
+typedef enum {
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY = 1 << 0,
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1,
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2,
+} qemuBlockStorageSourceBackendPropsFlags;
+
virJSONValuePtr
qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
- bool legacy,
- bool onlytarget,
- bool autoreadonly);
+ unsigned int flags);
virURIPtr
qemuBlockStorageSourceGetURI(virStorageSourcePtr src);
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24e99e13b8..c41976b903
[PATCH 11/18] qemuBuildSCSIHostdevDevStr: Pass in backend alias
Don't (re)generate the backend alias (alias of the -drive backend for
now) internally but rather pass it in. Later on it will be replaced by
the nodename when blockdev is used depending on the capabilities.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 14 --
src/qemu/qemu_command.h | 4 +++-
src/qemu/qemu_hotplug.c | 2 +-
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3d9479f863..200decd6e0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4641,11 +4641,11 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
char *
qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
- virDomainHostdevDefPtr dev)
+ virDomainHostdevDefPtr dev,
+ const char *backendAlias)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
int model = -1;
-g_autofree char *driveAlias = NULL;
const char *contAlias;
model = qemuDomainFindSCSIControllerModel(def, dev->info);
@@ -4687,9 +4687,7 @@ qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
dev->info->addr.drive.unit);
}
-if (!(driveAlias = qemuAliasFromHostdev(dev)))
-return NULL;
-virBufferAsprintf(, ",drive=%s,id=%s", driveAlias, dev->info->alias);
+virBufferAsprintf(, ",drive=%s,id=%s", backendAlias, dev->info->alias);
if (dev->info->bootIndex)
virBufferAsprintf(, ",bootindex=%u", dev->info->bootIndex);
@@ -5073,6 +5071,7 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd,
virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
g_autofree char *devstr = NULL;
g_autofree char *drvstr = NULL;
+g_autofree char *backendAlias = NULL;
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
@@ -5091,8 +5090,11 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, drvstr);
+if (!(backendAlias = qemuAliasFromHostdev(hostdev)))
+return -1;
+
virCommandAddArg(cmd, "-device");
-if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
+if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev, backendAlias)))
return -1;
virCommandAddArg(cmd, devstr);
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 55bc67072a..d0de27d9b0 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -179,7 +179,9 @@ char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
virQEMUCapsPtr qemuCaps);
char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
- virDomainHostdevDefPtr dev);
+ virDomainHostdevDefPtr dev,
+ const char *backendAlias);
+
char *
qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index cbd4c88abe..fbbd6a533c 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2629,7 +2629,7 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
if (!(drivealias = qemuAliasFromHostdev(hostdev)))
goto cleanup;
-if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev)))
+if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, drivealias)))
goto cleanup;
if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
--
2.26.2
[PATCH 04/18] virDomainHostdevDefFormatSubsys: Format private data for a virStorageSource
iSCSI subsystem hostdevs store the data as a virStorageSource. Format
the private data part of the virStorageSource in the appropriate place.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d14485f18d..bda9375f13 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25987,7 +25987,8 @@ static int
virDomainHostdevDefFormatSubsys(virBufferPtr buf,
virDomainHostdevDefPtr def,
unsigned int flags,
-bool includeTypeInAddr)
+bool includeTypeInAddr,
+virDomainXMLOptionPtr xmlopt)
{
bool closedSource = false;
virDomainHostdevSubsysUSBPtr usbsrc = >source.subsys.u.usb;
@@ -26086,6 +26087,10 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
if (iscsisrc->src->hosts[0].port)
virBufferAsprintf(buf, " port='%u'",
iscsisrc->src->hosts[0].port);
virBufferAddLit(buf, "/>\n");
+
+if (virDomainDiskSourceFormatPrivateData(buf, iscsisrc->src,
+ flags, xmlopt) < 0)
+return -1;
} else {
virBufferAsprintf(buf, "\n",
scsihostsrc->adapter);
@@ -26167,13 +26172,14 @@ static int
virDomainActualNetDefContentsFormat(virBufferPtr buf,
virDomainNetDefPtr def,
bool inSubelement,
-unsigned int flags)
+unsigned int flags,
+virDomainXMLOptionPtr xmlopt)
{
virDomainNetType actualType = virDomainNetGetActualType(def);
if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
if (virDomainHostdevDefFormatSubsys(buf,
virDomainNetGetActualHostdev(def),
-flags, true) < 0) {
+flags, true, xmlopt) < 0) {
return -1;
}
} else {
@@ -26253,7 +26259,8 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf,
static int
virDomainActualNetDefFormat(virBufferPtr buf,
virDomainNetDefPtr def,
-unsigned int flags)
+unsigned int flags,
+virDomainXMLOptionPtr xmlopt)
{
virDomainNetType type;
const char *typeStr;
@@ -26281,7 +26288,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
-if (virDomainActualNetDefContentsFormat(buf, def, true, flags) < 0)
+if (virDomainActualNetDefContentsFormat(buf, def, true, flags, xmlopt) < 0)
return -1;
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
@@ -26479,7 +26486,7 @@ virDomainNetDefFormat(virBufferPtr buf,
* the standard place... (this is for public reporting of
* interface status)
*/
-if (virDomainActualNetDefContentsFormat(buf, def, false, flags) < 0)
+if (virDomainActualNetDefContentsFormat(buf, def, false, flags,
xmlopt) < 0)
return -1;
} else {
/* ...but if we've asked for the inactive XML (rather than
@@ -26581,7 +26588,7 @@ virDomainNetDefFormat(virBufferPtr buf,
case VIR_DOMAIN_NET_TYPE_HOSTDEV:
if (virDomainHostdevDefFormatSubsys(buf, >data.hostdev.def,
-flags, true) < 0) {
+flags, true, xmlopt) < 0) {
return -1;
}
break;
@@ -26627,7 +26634,7 @@ virDomainNetDefFormat(virBufferPtr buf,
*/
if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
(flags & VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET) &&
-(virDomainActualNetDefFormat(buf, def, flags) < 0))
+(virDomainActualNetDefFormat(buf, def, flags, xmlopt) < 0))
return -1;
}
@@ -28204,7 +28211,8 @@ virDomainGraphicsDefFormat(virBufferPtr buf,
static int
virDomainHostdevDefFormat(virBufferPtr buf,
virDomainHostdevDefPtr def,
- unsigned int flags)
+ unsigned int flags,
+ virDomainXMLOptionPtr xmlopt)
{
const char *mode = virDomainHostdevModeTypeToString(def->mode);
virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
@@ -28283,7 +28291,7 @@ virDomainHostdevDefFormat(virBufferPtr buf,
switch (def->mode) {
case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
-if (virDomainHostdevDefFormatSubsys(buf, def, flags, false) < 0)
+if (virDomainHostdevDefFormatSubsys(buf, def, flags, false,
[PATCH 17/18] qemu: caps: Enable QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI
Enable it when regular QEMU_CAPS_BLOCKDEV is present.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 3 ++
.../caps_4.2.0.aarch64.xml| 1 +
.../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 +
.../caps_4.2.0.x86_64.xml | 1 +
.../caps_5.0.0.aarch64.xml| 1 +
.../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 +
.../caps_5.0.0.riscv64.xml| 1 +
.../caps_5.0.0.x86_64.xml | 1 +
.../caps_5.1.0.x86_64.xml | 1 +
.../hostdev-scsi-lsi.x86_64-latest.args | 52 +++
...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46
11 files changed, 65 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index cc54ad9283..d6bfec9996 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5124,6 +5124,9 @@ virQEMUCapsInitProcessCapsInterlock(virQEMUCapsPtr
qemuCaps)
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_STORAGE_WERROR)) {
virQEMUCapsClear(qemuCaps, QEMU_CAPS_STORAGE_WERROR);
}
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV))
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI);
}
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
index 98cee36669..11a964ed39 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
@@ -188,6 +188,7 @@
+
4001050
0
61700242
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml
b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml
index 0b174ffeec..76e2747b65 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml
@@ -152,6 +152,7 @@
+
4002000
0
39100242
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
index 91ce30ba44..fd63a0ee02 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml
@@ -235,6 +235,7 @@
+
4002000
0
43100242
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
index 9446ddc22a..928af2a01c 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml
@@ -198,6 +198,7 @@
+
500
0
61700241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
index 8c371a045b..e8668a25a9 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml
@@ -207,6 +207,7 @@
+
500
0
42900241
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
index 15c9d54332..85a8a46dac 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml
@@ -194,6 +194,7 @@
+
500
0
0
diff --git a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
index dafca26b89..546b9b0422 100644
--- a/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml
@@ -242,6 +242,7 @@
+
500
0
43100241
diff --git a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
index bd6e83ccaf..e05290fcfe 100644
--- a/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml
@@ -242,6 +242,7 @@
+
550
0
43100242
diff --git a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args
b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args
index 76a0708336..d4599f6002 100644
--- a/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/hostdev-scsi-lsi.x86_64-latest.args
@@ -34,34 +34,42 @@ file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \
-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw",\
"file":"libvirt-1-storage"}' \
-device ide-hd,bus=ide.0,unit=0,drive=libvirt-1-format,id=ide0-0-0,bootindex=1
\
--drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev0 \
--device scsi-generic,bus=scsi0.0,scsi-id=7,drive=drive-hostdev0,id=hostdev0 \
--drive file=/dev/sg0,if=none,format=raw,id=drive-hostdev1,readonly=on \
--device scsi-generic,bus=scsi0.0,scsi-id=6,drive=drive-hostdev1,id=hostdev1 \
--drive file.driver=iscsi,file.portal=example.org:3260,\
-file.target=iqn.1992-01.com.example,file.lun=0,file.transport=tcp,if=none,\
-format=raw,id=drive-hostdev2 \
--device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev2,id=hostdev2 \
[PATCH 06/18] qemustatusxml2xmltest: Add tests for iSCSI hostdev private data handling
Signed-off-by: Peter Krempa --- tests/qemustatusxml2xmldata/modern-in.xml | 18 ++ 1 file changed, 18 insertions(+) diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index 63ef9caed3..0a3990bd3e 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -458,6 +458,24 @@ + + + + + + + + + + + + + + + + + + -- 2.26.2
[PATCH 14/18] qemuBuildHostdevSCSICommandLine: Use new infrastructure
In preparation for instantiating (i)SCSI hostdevs via -blockdev,
refactor qemuBuildHostdevSCSICommandLine to use the new infrastructure
which will do it automatically.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 44 -
1 file changed, 4 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6b3b1b39ca..7f624a031e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -695,27 +695,6 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd,
}
-/* qemuBuildDiskSecinfoCommandLine:
- * @cmd: Pointer to the command string
- * @secinfo: Pointer to a possible secinfo
- *
- * Add the secret object for the disks that will be using it to perform
- * their authentication.
- *
- * Returns 0 on success, -1 w/ error on some sort of failure.
- */
-static int
-qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd,
-qemuDomainSecretInfoPtr secinfo)
-{
-/* Not necessary for non AES secrets */
-if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES)
-return 0;
-
-return qemuBuildObjectSecretCommandLine(cmd, secinfo);
-}
-
-
/* qemuBuildGeneralSecinfoURI:
* @uri: Pointer to the URI structure to add to
* @secinfo: Pointer to the secret info data (if present)
@@ -5151,29 +5130,14 @@ qemuBuildHostdevSCSICommandLine(virCommandPtr cmd,
virDomainHostdevDefPtr hostdev,
virQEMUCapsPtr qemuCaps)
{
-virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+g_autoptr(qemuBlockStorageSourceAttachData) data = NULL;
g_autofree char *devstr = NULL;
-g_autofree char *drvstr = NULL;
-g_autofree char *backendAlias = NULL;
+const char *backendAlias = NULL;
-if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
-virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
->u.iscsi;
-qemuDomainStorageSourcePrivatePtr srcPriv =
-QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
-
-if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ?
-srcPriv->secinfo :
-NULL) < 0)
-return -1;
-}
-
-virCommandAddArg(cmd, "-drive");
-if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
+if (!(data = qemuBuildHostdevSCSIAttachPrepare(hostdev, ,
qemuCaps)))
return -1;
-virCommandAddArg(cmd, drvstr);
-if (!(backendAlias = qemuAliasFromHostdev(hostdev)))
+if (qemuBuildBlockStorageSourceAttachDataCommandline(cmd, data) < 0)
return -1;
virCommandAddArg(cmd, "-device");
--
2.26.2
[PATCH 12/18] qemu: capabilities: Add QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI
We want to instantiate hostdevs via -blockdev too. Add a separate
capability for them for a clean transition. The new capability will be
enabled when QEMU_CAPS_BLOCKDEV is present once all code is prepared.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 1 +
src/qemu/qemu_capabilities.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b27a5e5c81..cc54ad9283 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -596,6 +596,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"intel-iommu.aw-bits",
"spapr-tpm-proxy",
"numa.hmat",
+ "blockdev-hostdev-scsi"
);
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index dac36b33d9..5d08941538 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -576,6 +576,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_INTEL_IOMMU_AW_BITS, /* intel-iommu.aw-bits */
QEMU_CAPS_DEVICE_SPAPR_TPM_PROXY, /* -device spapr-tpm-proxy */
QEMU_CAPS_NUMA_HMAT, /* -numa hmat */
+QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI, /* -blockdev used for (i)SCSI hostdevs */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.26.2
[PATCH 18/18] qemuBuildSCSIHostdevDrvStr: unexport
The function is no longer called from other modules.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_command.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7f624a031e..839c93bfb4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4589,7 +4589,7 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
return virBufferContentAndReset();
}
-char *
+static char *
qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
virQEMUCapsPtr qemuCaps)
{
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index a43cdb2733..b579817b44 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -175,9 +175,6 @@ char *qemuBuildUSBHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
virQEMUCapsPtr qemuCaps);
-char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
- virQEMUCapsPtr qemuCaps);
-
char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def,
virDomainHostdevDefPtr dev,
const char *backendAlias);
--
2.26.2
[PATCH 05/18] virDomainHostdevSubsysSCSIiSCSIDefParseXML: Parse private data of virStorageSource
We store the config of an iSCSI hostdev in a virStorageSource structure.
Parse the private data portion.
Signed-off-by: Peter Krempa
---
src/conf/domain_conf.c | 39 +--
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bda9375f13..ceaf73772d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8283,7 +8283,9 @@ virDomainHostdevSubsysSCSIHostDefParseXML(xmlNodePtr
sourcenode,
static int
virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr sourcenode,
virDomainHostdevSubsysSCSIPtr def,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ unsigned int flags,
+ virDomainXMLOptionPtr xmlopt)
{
int auth_secret_usage = -1;
xmlNodePtr cur;
@@ -8348,13 +8350,27 @@ virDomainHostdevSubsysSCSIiSCSIDefParseXML(xmlNodePtr
sourcenode,
}
cur = cur->next;
}
+
+if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) &&
+xmlopt && xmlopt->privateData.storageParse) {
+VIR_XPATH_NODE_AUTORESTORE(ctxt);
+
+ctxt->node = sourcenode;
+
+if ((ctxt->node = virXPathNode("./privateData", ctxt)) &&
+xmlopt->privateData.storageParse(ctxt, iscsisrc->src) < 0)
+return -1;
+}
+
return 0;
}
static int
virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode,
virDomainHostdevSubsysSCSIPtr scsisrc,
- xmlXPathContextPtr ctxt)
+ xmlXPathContextPtr ctxt,
+ unsigned int flags,
+ virDomainXMLOptionPtr xmlopt)
{
g_autofree char *protocol = NULL;
@@ -8370,7 +8386,8 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr
sourcenode,
}
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
-return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc,
ctxt);
+return virDomainHostdevSubsysSCSIiSCSIDefParseXML(sourcenode, scsisrc,
ctxt,
+ flags, xmlopt);
return virDomainHostdevSubsysSCSIHostDefParseXML(sourcenode, scsisrc);
}
@@ -8461,7 +8478,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
xmlXPathContextPtr ctxt,
const char *type,
virDomainHostdevDefPtr def,
- unsigned int flags)
+ unsigned int flags,
+ virDomainXMLOptionPtr xmlopt)
{
xmlNodePtr sourcenode;
int backend;
@@ -8633,7 +8651,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
-if (virDomainHostdevSubsysSCSIDefParseXML(sourcenode, scsisrc, ctxt) <
0)
+if (virDomainHostdevSubsysSCSIDefParseXML(sourcenode, scsisrc, ctxt,
flags, xmlopt) < 0)
return -1;
break;
@@ -11645,7 +11663,8 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
virDomainNetDefPtr parent,
virDomainActualNetDefPtr *def,
- unsigned int flags)
+ unsigned int flags,
+ virDomainXMLOptionPtr xmlopt)
{
virDomainActualNetDefPtr actual = NULL;
int ret = -1;
@@ -11750,7 +11769,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
addrtype = g_strdup("usb");
hostdev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
if (virDomainHostdevDefParseXMLSubsys(node, ctxt, addrtype,
- hostdev, flags) < 0) {
+ hostdev, flags, xmlopt) < 0) {
goto error;
}
} else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE ||
@@ -12124,7 +12143,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
def->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
virXMLNodeNameEqual(cur, "actual")) {
if (virDomainActualNetDefParseXML(cur, ctxt, def,
- , flags) < 0) {
+ , flags, xmlopt) < 0)
{
goto error;
}
} else if (virXMLNodeNameEqual(cur, "bandwidth")) {
@@ -12388,7 +12407,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
addrtype = g_strdup("usb");
hostdev->mode = VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
if
[PATCH 02/18] qemuBlockStorageSourceGetBackendProps: Allow skipping "discard":"unmap"
It doesn't make sense to format "discard" when doing a -blockdev backend
of scsi-generic used with SCSI hostdevs. Add a way to skip it.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 8
src/qemu/qemu_block.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 10ddf53b3b..bdd07d9925 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1061,6 +1061,9 @@
qemuBlockStorageSourceGetBlockdevGetCacheProps(virStorageSourcePtr src,
* omit any data which does not identify the image itself
* QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY:
* use the auto-read-only feature of qemu
+ * QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP:
+ * don't enable 'discard:unmap' option for passing through dicards
+ * (note that this is disabled also for _LEGACY and _TARGET_ONLY options)
*
* Creates a JSON object describing the underlying storage or protocol of a
* storage source. Returns NULL on error and reports an appropriate error
message.
@@ -1202,6 +1205,11 @@
qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src,
if (virJSONValueObjectAdd(fileprops,
"T:read-only", ro,
"T:auto-read-only", aro,
+ NULL) < 0)
+return NULL;
+
+if (!(flags & QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP)
&&
+virJSONValueObjectAdd(fileprops,
"s:discard", "unmap",
NULL) < 0)
return NULL;
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 715739e59c..0701fc18d1 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -60,6 +60,7 @@ typedef enum {
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_LEGACY = 1 << 0,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY = 1 << 1,
QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_AUTO_READONLY = 1 << 2,
+QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_SKIP_UNMAP = 1 << 3,
} qemuBlockStorageSourceBackendPropsFlags;
virJSONValuePtr
--
2.26.2
[PATCH 09/18] qemu: hotplug: Don't regenerate iSCSI secret alias
We now store the alias of the secrets in the status XML so there's no
need to generate it again.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_hotplug.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e897c059dd..cbd4c88abe 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4473,7 +4473,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
size_t i;
qemuDomainObjPrivatePtr priv = vm->privateData;
g_autofree char *drivealias = NULL;
-g_autofree char *objAlias = NULL;
+const char *secretObjAlias = NULL;
VIR_DEBUG("Removing host device %s from domain %p %s",
hostdev->info->alias, vm, vm->def->name);
@@ -4485,22 +4485,17 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
if (!(drivealias = qemuAliasFromHostdev(hostdev)))
return -1;
-/* Look for the markers that the iSCSI hostdev was added with a
- * secret object to manage the username/password. If present, let's
- * attempt to remove the object as well. */
-if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI &&
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET) &&
-qemuDomainStorageSourceHasAuth(iscsisrc->src)) {
-if (!(objAlias = qemuAliasForSecret(hostdev->info->alias, NULL)))
-return -1;
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
+qemuDomainStorageSourcePrivatePtr srcPriv =
QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+if (srcPriv && srcPriv->secinfo)
+secretObjAlias = srcPriv->secinfo->s.aes.alias;
}
qemuDomainObjEnterMonitor(driver, vm);
qemuMonitorDriveDel(priv->mon, drivealias);
-/* If it fails, then so be it - it was a best shot */
-if (objAlias)
-ignore_value(qemuMonitorDelObject(priv->mon, objAlias, false));
+if (secretObjAlias)
+ignore_value(qemuMonitorDelObject(priv->mon, secretObjAlias,
false));
if (qemuDomainObjExitMonitor(driver, vm) < 0)
return -1;
--
2.26.2
[PATCH 03/18] qemuBlockStorageSourceAttachData: Add field for ad-hoc storage node name
SCSI hostdevs don't have a virStorageSource associated with the backend
in certain cases. Adding a separate field to hold memory for a copy of
the nodename of the storage backend will allow reusing the blockdev
machinery also for SCSI hostdevs.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_block.c | 1 +
src/qemu/qemu_block.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index bdd07d9925..34406aa743 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -1557,6 +1557,7 @@
qemuBlockStorageSourceAttachDataFree(qemuBlockStorageSourceAttachDataPtr data)
virJSONValueFree(data->encryptsecretProps);
virJSONValueFree(data->tlsProps);
virJSONValueFree(data->tlsKeySecretProps);
+VIR_FREE(data->storageNodeNameCopy);
VIR_FREE(data->tlsAlias);
VIR_FREE(data->tlsKeySecretAlias);
VIR_FREE(data->authsecretAlias);
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 0701fc18d1..9aab620947 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -85,6 +85,7 @@ struct qemuBlockStorageSourceAttachData {
virJSONValuePtr storageProps;
const char *storageNodeName;
+char *storageNodeNameCopy; /* in some cases we don't have the
corresponding storage source */
bool storageAttached;
virJSONValuePtr storageSliceProps;
--
2.26.2
[PATCH 10/18] qemuBuildHostdevCommandLine: Extract (i)SCSI code
Move all (i)SCSI related code into a new function named
'qemuBuildHostdevSCSICommandLine'.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_command.c | 61 +
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c41976b903..3d9479f863 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5063,6 +5063,43 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
return virBufferContentAndReset();
}
+
+static int
+qemuBuildHostdevSCSICommandLine(virCommandPtr cmd,
+const virDomainDef *def,
+virDomainHostdevDefPtr hostdev,
+virQEMUCapsPtr qemuCaps)
+{
+virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
+g_autofree char *devstr = NULL;
+g_autofree char *drvstr = NULL;
+
+if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
+virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
+>u.iscsi;
+qemuDomainStorageSourcePrivatePtr srcPriv =
+QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
+
+if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ?
+srcPriv->secinfo :
+NULL) < 0)
+return -1;
+}
+
+virCommandAddArg(cmd, "-drive");
+if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
+return -1;
+virCommandAddArg(cmd, drvstr);
+
+virCommandAddArg(cmd, "-device");
+if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
+return -1;
+virCommandAddArg(cmd, devstr);
+
+return 0;
+}
+
+
static int
qemuBuildHostdevCommandLine(virCommandPtr cmd,
const virDomainDef *def,
@@ -5074,10 +5111,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
for (i = 0; i < def->nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = def->hostdevs[i];
virDomainHostdevSubsysPtr subsys = >source.subsys;
-virDomainHostdevSubsysSCSIPtr scsisrc = >source.subsys.u.scsi;
virDomainHostdevSubsysMediatedDevPtr mdevsrc = >u.mdev;
g_autofree char *devstr = NULL;
-g_autofree char *drvstr = NULL;
g_autofree char *vhostfdName = NULL;
unsigned int bootIndex = hostdev->info->bootIndex;
int vhostfd = -1;
@@ -5123,28 +5158,8 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
/* SCSI */
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
-if (scsisrc->protocol ==
VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
-virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc =
->u.iscsi;
-qemuDomainStorageSourcePrivatePtr srcPriv =
-QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
-
-if (qemuBuildDiskSecinfoCommandLine(cmd, srcPriv ?
-srcPriv->secinfo :
-NULL) < 0)
-return -1;
-}
-
-virCommandAddArg(cmd, "-drive");
-if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
+if (qemuBuildHostdevSCSICommandLine(cmd, def, hostdev, qemuCaps) <
0)
return -1;
-virCommandAddArg(cmd, drvstr);
-
-virCommandAddArg(cmd, "-device");
-if (!(devstr = qemuBuildSCSIHostdevDevStr(def, hostdev)))
-return -1;
-virCommandAddArg(cmd, devstr);
-
break;
/* SCSI_host */
--
2.26.2
[PATCH 00/18] qemu: Switch (i)SCSI host devices to -blockdev
Peter Krempa (18): qemuBlockStorageSourceGetBackendProps: Convert boolean arguments to flags qemuBlockStorageSourceGetBackendProps: Allow skipping "discard":"unmap" qemuBlockStorageSourceAttachData: Add field for ad-hoc storage node name virDomainHostdevDefFormatSubsys: Format private data for a virStorageSource virDomainHostdevSubsysSCSIiSCSIDefParseXML: Parse private data of virStorageSource qemustatusxml2xmltest: Add tests for iSCSI hostdev private data handling qemuDomainSecretHostdevDestroy: Don't clear secinfo alias qemu: domain: Regenerate hostdev source private data qemu: hotplug: Don't regenerate iSCSI secret alias qemuBuildHostdevCommandLine: Extract (i)SCSI code qemuBuildSCSIHostdevDevStr: Pass in backend alias qemu: capabilities: Add QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI qemu: command: Create qemuBlockStorageSourceAttachData for (i)SCSI hostdevs qemuBuildHostdevSCSICommandLine: Use new infrastructure qemuDomainAttachHostSCSIDevice: Use new infrastructure qemuDomainRemoveHostDevice: Use new infrastructure for (i)SCSI qemu: caps: Enable QEMU_CAPS_BLOCKDEV_HOSTDEV_SCSI qemuBuildSCSIHostdevDrvStr: unexport src/conf/domain_conf.c| 71 +--- src/qemu/qemu_block.c | 41 +++-- src/qemu/qemu_block.h | 12 +- src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 167 -- src/qemu/qemu_command.h | 15 +- src/qemu/qemu_domain.c| 58 +- src/qemu/qemu_hotplug.c | 70 ++-- tests/qemublocktest.c | 18 +- .../caps_4.2.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 + .../caps_4.2.0.x86_64.xml | 1 + .../caps_5.0.0.aarch64.xml| 1 + .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 + .../caps_5.0.0.riscv64.xml| 1 + .../caps_5.0.0.x86_64.xml | 1 + .../caps_5.1.0.x86_64.xml | 1 + .../disk-secinfo-upgrade-in.xml | 20 +++ .../disk-secinfo-upgrade-out.xml | 30 tests/qemustatusxml2xmldata/modern-in.xml | 18 ++ .../hostdev-scsi-lsi.x86_64-latest.args | 52 +++--- ...ostdev-scsi-virtio-scsi.x86_64-latest.args | 46 ++--- 23 files changed, 426 insertions(+), 205 deletions(-) -- 2.26.2
Re: [PATCH] resctrl: Do not open directory for writing
On 7/10/20 3:39 PM, Andrea Bolognani wrote: On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote: On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote: On 7/9/20 6:30 PM, Andrea Bolognani wrote: This is all bikeshedding, of course: what actually matters is making that lock exclusive once again :) Just realized that for exclusive (aka write) lock, the FD must be opened for writing (working on patches for the following report [1] and been experimenting a bit and that's what I'm seeing). Good point, but luckily not related to flock(2). That seems to be the case: according to flock(2), A shared or exclusive lock can be placed on a file regardless of the mode in which the file was opened. Michal, does that sound reasonable to you? D'oh! of course this is another case of file locking exemptions. The patches I sent earlier today fix code around virFileLock() which is fcntl() which is POSIX locking. flock(2) is BSD lock which may or may not be implementedusing POSIX locks. So I think we're okay on that front. Alternatively, we may switch to OFD (F_OFD_SETLK from fcntl(2)) and experience proper file locking. Those are Linux only (but so is resctrl). Michal
Re: [PATCH] util: Rework virFileFlock() to be unambiguous
On Fri, 2020-07-10 at 15:20 +0200, Martin Kletzander wrote:
> The boolean parameters for lock/unlock and read/write (shared/exclusive)
> caused
> a lot of confusion when reading the callers. The new approach is explicit and
> unambiguous.
>
> While at it, also change the only caller so that it acquires an exclusive lock
> as it should've been all the time. This was caused because the function was
> ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
> misused in commit 657ddeff2313 and since then the lock being taken was shared
> rather than exclusive.
I don't think you should do both things in the same patch: please
change the virFileFlock() interface, with no semantic changes, in one
patch, and fix virResctrlLockWrite() in a separate one.
> +switch (op) {
> +case VIR_FILE_FLOCK_SHARED:
> +flock_op = LOCK_SH;
> +break;
> +case VIR_FILE_FLOCK_EXCLUSIVE:
> +flock_op = LOCK_EX;
> +break;
> +case VIR_FILE_FLOCK_UNLOCK:
> +flock_op = LOCK_UN;
> +break;
> +}
This switch() statement is missing
default:
virReportEnumRangeError(virFileFlockOperation, op);
And I thought you liked Rust?!? :D
> +++ b/src/util/virresctrl.c
> @@ -463,7 +463,7 @@ virResctrlLockWrite(void)
> return -1;
> }
>
> -if (virFileFlock(fd, true, true) < 0) {
> +if (virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) < 0) {
As mentioned above, use VIR_FILE_FLOCK_SHARED here and then change it
to VIR_FILE_FLOCK_EXCLUSIVE in a separate patch.
With the default case added and the semantic-altering changes
removed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
Re: [libvirt PATCH v2 3/5] m4: virt-secdriver-selinux: drop obsolete function checks
On Fri, Jul 10, 2020 at 12:25:04PM +0200, Pavel Hrdina wrote: > All of the listed functions are available in libselinux version 2.2. > Our supported OSes start with version 2.5 so there is no need to check > it. > > Signed-off-by: Pavel Hrdina > --- > m4/virt-secdriver-selinux.m4| 24 ++-- > src/security/security_selinux.c | 18 +++--- > tests/securityselinuxhelper.c | 6 -- > 3 files changed, 5 insertions(+), 43 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 :|
Re: [libvirt PATCH v2 2/5] docs: convert FIG files into SVG format
On Fri, Jul 10, 2020 at 12:25:03PM +0200, Pavel Hrdina wrote: > Converted by using: > > fig2dev -L svg > > Signed-off-by: Pavel Hrdina > --- > docs/architecture.fig | 87 -- > docs/architecture.svg | 239 > docs/libvirt-daemon-arch.fig| 114 - > docs/libvirt-daemon-arch.svg| 185 + > docs/libvirt-driver-arch.fig| 62 > docs/libvirt-driver-arch.svg| 94 +++ > docs/libvirt-object-model.fig | 61 --- > docs/libvirt-object-model.svg | 138 > docs/libvirt-virConnect-example.fig | 58 --- > docs/libvirt-virConnect-example.svg | 138 > docs/migration-managed-direct.fig | 58 --- > docs/migration-managed-direct.svg | 107 + > docs/migration-managed-p2p.fig | 58 --- > docs/migration-managed-p2p.svg | 107 + > docs/migration-native.fig | 43 - > docs/migration-native.svg | 68 > docs/migration-tunnel.fig | 49 -- > docs/migration-tunnel.svg | 92 +++ > docs/migration-unmanaged-direct.fig | 58 --- > docs/migration-unmanaged-direct.svg | 107 + > docs/node.fig | 30 > docs/node.svg | 36 + > docs/structures.fig | 72 - > docs/structures.svg | 187 ++ > 24 files changed, 1498 insertions(+), 750 deletions(-) > delete mode 100644 docs/architecture.fig > create mode 100644 docs/architecture.svg > delete mode 100644 docs/libvirt-daemon-arch.fig > create mode 100644 docs/libvirt-daemon-arch.svg > delete mode 100644 docs/libvirt-driver-arch.fig > create mode 100644 docs/libvirt-driver-arch.svg > delete mode 100644 docs/libvirt-object-model.fig > create mode 100644 docs/libvirt-object-model.svg > delete mode 100644 docs/libvirt-virConnect-example.fig > create mode 100644 docs/libvirt-virConnect-example.svg > delete mode 100644 docs/migration-managed-direct.fig > create mode 100644 docs/migration-managed-direct.svg > delete mode 100644 docs/migration-managed-p2p.fig > create mode 100644 docs/migration-managed-p2p.svg > delete mode 100644 docs/migration-native.fig > create mode 100644 docs/migration-native.svg > delete mode 100644 docs/migration-tunnel.fig > create mode 100644 docs/migration-tunnel.svg > delete mode 100644 docs/migration-unmanaged-direct.fig > create mode 100644 docs/migration-unmanaged-direct.svg > delete mode 100644 docs/node.fig > create mode 100644 docs/node.svg > delete mode 100644 docs/structures.fig > create mode 100644 docs/structures.svg 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 :|
Re: [PATCH] resctrl: Do not open directory for writing
On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote: > On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote: > > On 7/9/20 6:30 PM, Andrea Bolognani wrote: > > > This is all bikeshedding, of course: what actually matters is making > > > that lock exclusive once again :) > > > > Just realized that for exclusive (aka write) lock, the FD must be opened > > for writing (working on patches for the following report [1] and been > > experimenting a bit and that's what I'm seeing). > > Good point, but luckily not related to flock(2). That seems to be the case: according to flock(2), A shared or exclusive lock can be placed on a file regardless of the mode in which the file was opened. Michal, does that sound reasonable to you? -- Andrea Bolognani / Red Hat / Virtualization
Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier
On Fri, Jul 10, 2020 at 02:34:11PM +0800, Jason Wang wrote: > > On 2020/7/9 下午10:10, Peter Xu wrote: > > On Thu, Jul 09, 2020 at 01:58:33PM +0800, Jason Wang wrote: > > > > > - If we care the performance, it's better to implement the MAP event > > > > > for > > > > > vhost, otherwise it could be a lot of IOTLB miss > > > > I feel like these are two things. > > > > > > > > So far what we are talking about is whether vt-d should have knowledge > > > > about > > > > what kind of events one iommu notifier is interested in. I still think > > > > we > > > > should keep this as answered in question 1. > > > > > > > > The other question is whether we want to switch vhost from UNMAP to > > > > MAP/UNMAP > > > > events even without vDMA, so that vhost can establish the mapping even > > > > before > > > > IO starts. IMHO it's doable, but only if the guest runs DPDK > > > > workloads. When > > > > the guest is using dynamic iommu page mappings, I feel like that can be > > > > even > > > > slower, because then the worst case is for each IO we'll need to vmexit > > > > twice: > > > > > > > > - The first vmexit caused by an invalidation to MAP the page > > > > tables, so vhost > > > > will setup the page table before IO starts > > > > > > > > - IO/DMA triggers and completes > > > > > > > > - The second vmexit caused by another invalidation to UNMAP the > > > > page tables > > > > > > > > So it seems to be worse than when vhost only uses UNMAP like right now. > > > > At > > > > least we only have one vmexit (when UNMAP). We'll have a vhost > > > > translate() > > > > request from kernel to userspace, but IMHO that's cheaper than the > > > > vmexit. > > > > > > Right but then I would still prefer to have another notifier. > > > > > > Since vtd_page_walk has nothing to do with device IOTLB. IOMMU have a > > > dedicated command for flushing device IOTLB. But the check for > > > vtd_as_has_map_notifier is used to skip the device which can do demand > > > paging via ATS or device specific way. If we have two different notifiers, > > > vhost will be on the device iotlb notifier so we don't need it at all? > > But we can still have iommu notifier that only registers to UNMAP even > > after we > > introduce dev-iotlb notifier? We don't want to do page walk for them as > > well. > > TCG should be the only one so far, but I don't know.. maybe there can still > > be > > new ones? > > > I think you're right. But looking at the codes, it looks like the check of > vtd_as_has_map_notifier() was only used in: > > 1) vtd_iommu_replay() > 2) vtd_iotlb_page_invalidate_notify() (PSI) > > For the replay, it's expensive anyhow. For PSI, I think it's just about one > or few mappings, not sure it will have obvious performance impact. > > And I had two questions: > > 1) The codes doesn't check map for DSI or GI, does this match what spec > said? (It looks to me the spec is unclear in this part) Both DSI/GI should cover maps too? E.g. vtd_sync_shadow_page_table() in vtd_iotlb_domain_invalidate(). > 2) for the replay() I don't see other implementations (either spapr or > generic one) that did unmap (actually they skip unmap explicitly), any > reason for doing this in intel IOMMU? I could be wrong, but I'd guess it's because vt-d implemented the caching mode by leveraging the same invalidation strucuture, so it's harder to make all things right (IOW, we can't clearly identify MAP with UNMAP when we receive an invalidation request, because MAP/UNMAP requests look the same). I didn't check others, but I believe spapr is doing it differently by using some hypercalls to deliver IOMMU map/unmap requests, which seems a bit close to what virtio-iommu is doing. Anyway, the point is if we have explicit MAP/UNMAP from the guest, logically the replay indeed does not need to do any unmap because we don't need to call replay() on an already existing device but only for e.g. hot plug. VT-d does not have that clear interface, so VT-d needs to maintain its own mapping structures, and also vt-d is using the same replay & page_walk operations to sync all these structures, which complicated the vt-d replay a bit. With that, we assume replay() can be called anytime on a device, and we won't notify duplicated MAPs to lower layer like vfio if it is mapped before. At the meantime, since we'll compare the latest mapping with the one we cached in the iova tree, UNMAP becomes possible too. Thanks, -- Peter Xu
[PATCH] util: Rework virFileFlock() to be unambiguous
The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
a lot of confusion when reading the callers. The new approach is explicit and
unambiguous.
While at it, also change the only caller so that it acquires an exclusive lock
as it should've been all the time. This was caused because the function was
ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
misused in commit 657ddeff2313 and since then the lock being taken was shared
rather than exclusive.
Signed-off-by: Martin Kletzander
---
src/util/virfile.c| 27 ++-
src/util/virfile.h| 9 -
src/util/virresctrl.c | 4 ++--
3 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 213acdbcaa2b..554011eecb25 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -465,23 +465,33 @@ int virFileUnlock(int fd, off_t start, off_t len)
/**
* virFileFlock:
* @fd: file descriptor to call flock on
- * @lock: true for lock, false for unlock
- * @shared: true if shared, false for exclusive, ignored if `@lock == false`
+ * @op: operation to perform
*
* This is just a simple wrapper around flock(2) that errors out on unsupported
* platforms.
*
* The lock will be released when @fd is closed or this function is called with
- * `@lock == false`.
+ * `@op == VIR_FILE_FLOCK_UNLOCK`.
*
* Returns 0 on success, -1 otherwise (with errno set)
*/
-int virFileFlock(int fd, bool lock, bool shared)
+int virFileFlock(int fd, virFileFlockOperation op)
{
-if (lock)
-return flock(fd, shared ? LOCK_SH : LOCK_EX);
+int flock_op = -1;
-return flock(fd, LOCK_UN);
+switch (op) {
+case VIR_FILE_FLOCK_SHARED:
+flock_op = LOCK_SH;
+break;
+case VIR_FILE_FLOCK_EXCLUSIVE:
+flock_op = LOCK_EX;
+break;
+case VIR_FILE_FLOCK_UNLOCK:
+flock_op = LOCK_UN;
+break;
+}
+
+return flock(fd, flock_op);
}
#else /* WIN32 */
@@ -505,8 +515,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
int virFileFlock(int fd G_GNUC_UNUSED,
- bool lock G_GNUC_UNUSED,
- bool shared G_GNUC_UNUSED)
+ virFileFlockOperation op G_GNUC_UNUSED)
{
errno = ENOSYS;
return -1;
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 7a92364a5c9f..04428727fd3b 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -118,7 +118,14 @@ int virFileLock(int fd, bool shared, off_t start, off_t
len, bool waitForLock)
int virFileUnlock(int fd, off_t start, off_t len)
G_GNUC_NO_INLINE;
-int virFileFlock(int fd, bool lock, bool shared);
+
+typedef enum {
+VIR_FILE_FLOCK_EXCLUSIVE,
+VIR_FILE_FLOCK_SHARED,
+VIR_FILE_FLOCK_UNLOCK,
+} virFileFlockOperation;
+
+int virFileFlock(int fd, virFileFlockOperation op);
typedef int (*virFileRewriteFunc)(int fd, const void *opaque);
int virFileRewrite(const char *path,
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 3563fc560db5..784a8d43bb2f 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
return -1;
}
-if (virFileFlock(fd, true, true) < 0) {
+if (virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) < 0) {
virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
VIR_FORCE_CLOSE(fd);
return -1;
@@ -485,7 +485,7 @@ virResctrlUnlock(int fd)
virReportSystemError(errno, "%s", _("Cannot close resctrl"));
/* Trying to save the already broken */
-if (virFileFlock(fd, false, false) < 0)
+if (virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) < 0)
virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
return -1;
--
2.27.0
Re: [PATCH] resctrl: Do not open directory for writing
On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote: On 7/9/20 6:30 PM, Andrea Bolognani wrote: On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote: On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote: it seems to me that this change entirely flipped the semantics of the lock. Yep, you're right. When you have a lock that has boolean as a parameter I think the boolean should indicate whether the lock is writable, but maybe the proper and most readable solution would be virFileFlockShareable() and virFileFlockExclusive() to avoid any misconception in the future[2]. Given the fact that there are no (and most probably won't be) any other users of flock(2) and given the fact that resctrl is also pretty niche feature, I don't have any preference. Also I feel like after some time I'm a little bit rusty with C and the libvirt codebase (and most importantly the reasoning and decisions) has changed a bit, so I'll leave the decision on how to deal with that on someone else. I'm happy to provide the patches when clear decision is made. Either virFileFlockExclusive(fd) virFileFlockShared(fd) or virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) virFileFlock(fd, VIR_FILE_FLOCK_SHARED) would work. I like the latter better because it's closer to the original flock(), which it ultimately acts as a very thin wrapper around of. I'm actually unclear why we'd have the last argument in the first place: personally I'd just use virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) and keep things as unambiguous as they can be. This is all bikeshedding, of course: what actually matters is making that lock exclusive once again :) Just realized that for exclusive (aka write) lock, the FD must be opened for writing (working on patches for the following report [1] and been experimenting a bit and that's what I'm seeing). Good point, but luckily not related to flock(2). 1: https://www.redhat.com/archives/libvir-list/2020-July/msg00451.html Michal signature.asc Description: PGP signature
Re: Setting 'nodatacow' on VM image when on Btrfs
On Thu, Jul 09, 2020 at 02:15:50PM -0600, Chris Murphy wrote: > On Thu, Jul 9, 2020 at 12:27 PM Daniel P. Berrangé > wrote: > > > > On Thu, Jul 09, 2020 at 08:30:14AM -0600, Chris Murphy wrote: > > > It's generally recommended by upstream Btrfs development to set > > > 'nodatacow' using 'chattr +C' on the containing directory for VM > > > images. By setting it on the containing directory it means new VM > > > images inherit the attribute, including images copied to this > > > location. > > > > > > But 'nodatacow' also implies no compression and no data checksums. > > > Could there be use cases in which it's preferred to do compression and > > > checksumming on VM images? In that case the fragmentation problem can > > > be mitigated by periodic defragmentation. > > > > Setting nodatacow makes sense particularly when using qcow2, since > > qcow2 is already potentially performing copy-on-write. > > > > Skipping compression and data checksums is reasonable in many cases, > > as the OS inside the VM is often going to be able todo this itself, > > so we don't want double checksums or double compression as it just > > wastes performance. > > Good point. I am open to suggestion/recommendation by Felipe Borges > about GNOME Boxes' ability to do installs by injecting kickstarts. It > might sound nutty but it's quite sane to consider the guest do > something like plain XFS (no LVM). But all of my VM's are as you > describe: guest is btrfs with the checksums and compression, on a raw > file with chattr +C set. GNOME Boxes / virt-install both use libosinfo for doing the automated kickstart installs. In the case of Fedora that's driven by a template common across all versions. We already have to cope with switch from ext3 to ext4 way back in Fedora 10. If new Fedora decides on btrfs by default, we'll need to update the kickstart to follow that recmmendation too https://gitlab.com/libosinfo/osinfo-db/-/blob/master/data/install-script/fedoraproject.org/fedora-kickstart-jeos.xml.in#L80 > > > Is this something libvirt can and should do? Possibly by default with > > > a way for users to opt out? > > > > The default /var/lib/libvirt/images directory is created by RPM > > at install time. Not sure if there's a way to get RPM to set > > attributes on the dir at this time ? > > For lives it's rsync today. I'm not certain if rsync carries over > file attributes. tar does not. Also not sure if squashfs and > unsquashfs do either. So this might mean an anaconda post-install > script is a more reliable way to go, since Anaconda can support rsync > (Live) and rpm (netinstall,dvd) installs. And there is a proposal > dangling in the wind to use plain squashfs (no nested ext4 as today). Hmm, tricky, so many different scenarios to consider - traditional Anaconda install, install from live CD, plain RPM post-install, and pre-built disk image. > > Libvirt's storage pool APIs has support for setting "nodatacow" > > on a per-file basis when creating the images. This was added for > > btrfs benefit, but this is opt in and I'm not sure any mgmt tools > > actually use this right now. So in practice it probably dooesn't > > have any out of the box benefit. > > > > The storage pool APIs don't have any feature to set nodatacow > > for the directory as a whole, but probably we should add this. > > systemd-journald checks for btrfs and sets +C on /var/log/journal if > it is. This can be inhibited by 'touch > /etc/tmpfiles.d/journal-nocow.conf' > > > Another option is to have the installer set 'chattr +C' in the short > > > term. But this doesn't help GNOME Boxes, since the user home isn't > > > created at installation time. > > > > > > Three advantages of libvirt awareness of Btrfs: > > > > > > (a) GNOME Boxes Cockpit, and other users of libvirt can then use this > > > same mechanism, and apply to their VM image locations. > > > > > > (b) Create the containing directory as a subvolume instead of a directory > > > (1) btrfs snapshots are not recursive, therefore making this a > > > subvolume would prevent it from being snapshot, and thus (temporarily) > > > resuming datacow. > > > (2) in heavy workloads there can be lock contention on file > > > btrees; a subvolume is a dedicated file btree; so this would reduce > > > tendency for lock contention in heavy workloads (this is not likely a > > > desktop/laptop use case) > > > > Being able to create subvolumes sounds like a reasonable idea. We already > > have a ZFS specific storage driver that can do the ZFS equivalent. > > > > Again though we'll also need mgmt tools modified to take advantage of > > this. Not sure how we would make this all work out of the box, with > > the way we let RPM pre-create /var/lib/libvirt/images, as we'd need > > different behaviour depending on what filesystem you install the RPM > > onto. > > It seems reasonable to me that libvirtd can "own" > /var/lib/libvirt/images and make these decisions. i.e. if it's empty, > and if btrfs, then delete it and
Re: [libvirt PATCH v2 4/5] m4: virt-xdr: rewrite XDR check
On Fri, Jul 10, 2020 at 01:13:00PM +0200, Andrea Bolognani wrote:
> On Fri, 2020-07-10 at 12:25 +0200, Pavel Hrdina wrote:
> > +%if 0%{?fedora} || 0%{?rhel}
> > BuildRequires: libtirpc-devel
> > %endif
>
> We only support building RPMs on Fedora and RHEL, so this check is
> effectively a no-op.
OK, I'll drop it before pushing.
Thanks
Pavel
signature.asc
Description: PGP signature
Re: [libvirt PATCH 9/9] rpc: use new virt-nc binary for remote tunnelling
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote:
This wires up support for using the new virt-nc binary with the ssh,
libssh and libssh2 protocols.
The new binary will be used preferentially if it is available in $PATH,
otherwise we fall back to traditional netcat.
The "proxy" URI parameter can be used to force use of netcat e.g.
qemu+ssh://host/system?proxy=netcat
or the disable fallback e.g.
qemu+ssh://host/system?proxy=virt-nc
With use of virt-nc, we can now support remote session URIs
qemu+ssh://host/session
and this will only use virt-nc, with no fallback. This also lets the
libvirtd process be auto-started.
Signed-off-by: Daniel P. Berrangé
---
docs/uri.html.in| 18 ++
src/remote/remote_driver.c | 30 +++-
src/remote/remote_sockets.c | 8 -
src/rpc/virnetclient.c | 70 ++---
src/rpc/virnetclient.h | 30 +---
tests/virnetsockettest.c| 7 ++--
6 files changed, 136 insertions(+), 27 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index cd1bcc3ab3..5939f74e62 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -50,6 +50,10 @@ enum {
VIR_NET_CLIENT_MODE_COMPLETE,
};
+VIR_ENUM_IMPL(virNetClientProxy,
+ VIR_NET_CLIENT_PROXY_LAST,
+ "auto", "netcat", "virt-nc");
+
struct _virNetClientCall {
int mode;
@@ -414,20 +418,50 @@ virNetClientDoubleEscapeShell(const char *str)
}
char *
-virNetClientSSHHelperCommand(const char *netcatPath,
- const char *socketPath)
+virNetClientSSHHelperCommand(virNetClientProxy proxy,
+ const char *netcatPath,
+ const char *socketPath,
+ const char *driverURI,
+ bool readonly)
{
g_autofree char *netcatPathSafe =
virNetClientDoubleEscapeShell(netcatPath);
+g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI);
+g_autofree char *nccmd = NULL;
+g_autofree char *virtnccmd = NULL;
-return g_strdup_printf(
-"sh -c "
-"'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
"
- "ARG=-q0;"
+nccmd = g_strdup_printf(
+"if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then
"
+"ARG=-q0;"
"else "
- "ARG=;"
+"ARG=;"
"fi;"
-"'%s' $ARG -U %s'",
+"'%s' $ARG -U %s",
netcatPathSafe, netcatPathSafe, socketPath);
+
+virtnccmd = g_strdup_printf("%s '%s'",
+readonly ? "virt-nc -r" : "virt-nc",
+driverURISafe);
+
+switch (proxy) {
+case VIR_NET_CLIENT_PROXY_AUTO:
+return g_strdup_printf("sh -c 'which virt-nc 1>/dev/null 2>&1; "
+ "if test $? = 0; then "
+ "%s; "
+ "else"
+ "%s; "
+ "fi'", virtnccmd, nccmd);
+
+case VIR_NET_CLIENT_PROXY_NETCAT:
+return g_strdup_printf("sh -c '%s'", nccmd);
+
+case VIR_NET_CLIENT_PROXY_VIRT_NC:
+return g_strdup_printf("sh -c '%s'", virtnccmd);
+
+case VIR_NET_CLIENT_PROXY_LAST:
+default:
+virReportEnumRangeError(virNetClientProxy, proxy);
+return NULL;
+}
}
This needs to be coupled with virnetsockettest update because the
expected output of executed command changes.
Michal
Re: [libvirt PATCH 7/9] remote: introduce virtd-nc helper binary
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote: When accessing libvirtd over a SSH tunnel, the remote driver must spawn the remote 'nc' process, pointing it to the libvirtd socket path. This is problematic for a number of reasons: - The socket path varies according to the --prefix chosen at build time. The remote client is seeing the local prefix, but what we need is the remote prefix - The socket path varies according to remote env variables, such as the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR value, but what we need is the remote value (if any) - We can not able to autospawn the libvirtd daemon for session mode access To address these problems this patch introduces the 'virtd-nc' helper program which takes the URI for the remote driver as a CLI parameter. It then figures out the socket path to connect to using the same code as the remote driver does on the remote host. Signed-off-by: Daniel P. Berrangé --- build-aux/syntax-check.mk | 2 +- po/POTFILES.in | 1 + src/remote/Makefile.inc.am | 30 +++ src/remote/remote_nc.c | 424 + src/rpc/virnetsocket.h | 1 + 5 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 src/remote/remote_nc.c The spec file needs to be updated too. Michal
Re: [libvirt PATCH 0/9] remote: introduce a custom netcat impl for ssh tunnelling
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote: We have long had a problem with use of netcat for ssh tunnelling because there's no guarantee the UNIX socket path the client builds will match the UNIX socket path the remote host uses. We don't even allow session mode SSH tunnelling for this reason. We also can't easily auto-spawn libvirtd in session mode. With the introduction of modular daemons we also have potential for two completely different UNIX socket paths even for system mode, and the client can't know which to use. The solution to all these problems is to introduce a custom netcat impl. Instead passing the UNIX socket path, we pass the libvirt driver URI. The custom netcat then decides which socket path to use based on the remote build host environment. We still have to support netcat for interoperability with legacy libvirt versions, but we can default to the new virt-nc. Daniel P. Berrangé (9): rpc: merge logic for generating remote SSH shell script remote: split off enums into separate source file remote: split out function for parsing URI scheme remote: parse the remote transport string earlier remote: split out function for constructing socket path remote: extract logic for determining daemon to connect to remote: introduce virtd-nc helper binary rpc: switch order of args in virNetClientNewSSH rpc: use new virt-nc binary for remote tunnelling build-aux/syntax-check.mk | 2 +- docs/uri.html.in| 18 ++ po/POTFILES.in | 2 + src/libvirt_remote.syms | 1 + src/remote/Makefile.inc.am | 32 +++ src/remote/remote_driver.c | 323 +-- src/remote/remote_nc.c | 424 src/remote/remote_sockets.c | 277 +++ src/remote/remote_sockets.h | 70 ++ src/rpc/virnetclient.c | 151 - src/rpc/virnetclient.h | 29 ++- src/rpc/virnetsocket.c | 37 +--- src/rpc/virnetsocket.h | 4 +- tests/virnetsockettest.c| 12 +- 14 files changed, 1018 insertions(+), 364 deletions(-) create mode 100644 src/remote/remote_nc.c create mode 100644 src/remote/remote_sockets.c create mode 100644 src/remote/remote_sockets.h If you fix small problems I've raised then: Reviewed-by: Michal Privoznik Michal
Re: [libvirt PATCH v2 5/5] wireshark: fix compilation errors
On a Friday in 2020, Pavel Hrdina wrote: With meson introduction which is using the same CFLAGS for the whole project some compilation errors were discovered. The wireshark plugin library is the only one in tools directory that is not using AM_CFLAGS. With the AM_CFLAGS we get these errors: ../../tools/wireshark/src/packet-libvirt.c: In function 'dissect_libvirt_fds': ../../tools/wireshark/src/packet-libvirt.c:348:31: error: unused parameter 'tvb' [-Werror=unused-parameter] 348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds) | ~~^~~ ../../tools/wireshark/src/packet-libvirt.c:348:41: error: unused parameter 'start' [-Werror=unused-parameter] 348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds) |~^ ../../tools/wireshark/src/packet-libvirt.c:348:55: error: unused parameter 'nfds' [-Werror=unused-parameter] 348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds) |~~~^~~~ At top level: ../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_bool' defined but not used [-Werror=unused-function] 64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf) \ | ^~~~ ../../tools/wireshark/src/packet-libvirt.c:88:1: note: in expansion of macro 'XDR_PRIMITIVE_DISSECTOR' 88 | XDR_PRIMITIVE_DISSECTOR(bool,bool_t, boolean) | ^~~ ../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_float' defined but not used [-Werror=unused-function] 64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf) \ | ^~~~ ../../tools/wireshark/src/packet-libvirt.c:86:1: note: in expansion of macro 'XDR_PRIMITIVE_DISSECTOR' 86 | XDR_PRIMITIVE_DISSECTOR(float, gfloat, float) | ^~~ ../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_short' defined but not used [-Werror=unused-function] 64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf) \ | ^~~~ ../../tools/wireshark/src/packet-libvirt.c:80:1: note: in expansion of macro 'XDR_PRIMITIVE_DISSECTOR' 80 | XDR_PRIMITIVE_DISSECTOR(short, gint16, int) | ^~~ ../../tools/wireshark/src/packet-libvirt.c: In function 'dissect_libvirt_message': ../../tools/wireshark/src/packet-libvirt.c:423:34: error: null pointer dereference [-Werror=null-dereference] 423 | vir_xdr_dissector_t xd = find_payload_dissector(proc, type, get_program_data(prog, VIR_PROGRAM_DISSECTORS), | ^~ 424 | *(gsize *)get_program_data(prog, VIR_PROGRAM_DISSECTORS_LEN)); | ~ Signed-off-by: Pavel Hrdina --- src/internal.h | 4 tools/Makefile.am| 2 +- tools/wireshark/src/packet-libvirt.c | 17 ++--- 3 files changed, 19 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: libvirt-6.5.0 breaks host passthrough migration
On Fri, Jul 10, 2020 at 7:14 AM Jiri Denemark wrote:
> On Sun, Jul 05, 2020 at 12:45:55 -0400, Mark Mielke wrote:
> > With 6.4.0, live migration was working fine with Qemu 5.0. After trying
> out
> > 6.5.0, migration broke with the following error:
> >
> > libvirt.libvirtError: internal error: unable to execute QEMU command
> > 'migrate': State blocked by non-migratable CPU device (invtsc flag)
>
> Could you please describe the reproducer steps? For example, was the
> domain you're trying to migrate already running when you upgrade libvirt
> or is it freshly started by the new libvirt?
>
The original case was:
1) Machine X running libvirt 6.4.0 + qemu 5.0
2) Machine Y running libvirt 6.5.0 + qemu 5.0
3) Live migration from X to Y works. Guest appears fine.
4) Upgrade Machine X from libvirt 6.4.0 to 6.5.0 and reboot.
5) Live migration from Y to X fails with the message shown.
In each case, live migration was done with OpenStack Train directing
libvirt + qemu.
And it would be helpful to see the element as shown by virsh
> dumpxml before you try to start the domain as well as the QEMU command
> line libvirt used to start the domain (in
> /var/log/libvirt/qemu/$VM.log).
>
The element looks like this:
The QEMU command line is very long, and includes details I would avoid
publishing publicly unless you need them. The "-cpu" portion is just:
-cpu host
The QEMU command line itself is generated from libvirt, which is directed
by OpenStack Train.
> > commit 201bd5db639c063862b0c1b1abfab9a9a7c92591
> > Author: Jiri Denemark
> > Date: Tue Jun 2 15:34:07 2020 +0200
> >
> > qemu: Fill default value in //cpu/@migratable attribute
> >
> > Before QEMU introduced migratable CPU property, "-cpu host" included
> all
> > features that could be enabled on the host, even those which would
> block
> > migration. In other words, the default was equivalent to
> migratable=off.
> > When the migratable property was introduced, the default changed to
> > migratable=on. Let's record the default in domain XML.
> >
> > Signed-off-by: Jiri Denemark
> > Reviewed-by: Michal Privoznik
> >
> > Before this change, qemu was still being launched with "-cpu host", which
> > for any somewhat modern version of qemu, defaults to migratable=on. The
> > above comment acknowledges this, however, the implementation chooses the
> > pessimistic and ancient (and no longer applicable!) value of
> migratable=off:
> >
> > +if (qemuCaps &&
> > +def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH &&
> > +!def->cpu->migratable) {
> > +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_MIGRATABLE))
> > +def->cpu->migratable = VIR_TRISTATE_SWITCH_ON;
> >
> > *+else if (ARCH_IS_X86(def->os.arch))+
> > def->cpu->migratable = VIR_TRISTATE_SWITCH_OFF;*
> > +}
>
> The implementation seems to be doing exactly what the commit message
> says. The migratable=off default should be used only when QEMU does not
> support -cpu host,migratable=on|off, that is only when QEMU is very old.
> Every non-ancient version of libvirt should have the
> QEMU_CAPS_CPU_MIGRATABLE set and thus this code should choose
> migrateble=on default.
>
I wasn't sure what QEMU_CAPS_CPU_MIGRATABLE represents. I initially
suspected what you are saying, but since it apparently did not work the way
I expected, I then presumed it does not work the way I expected. :-)
Is QEMU_CAPS_CPU_MIGRATABLE only from the element? If so, doesn't
this mean that it is not explicitly listed for host-passthrough, and this
means the check is not detecting whether it is enabled or not properly?
> I think it is not a requirement for "migratable=XXX" to be explicit in
> > libvirt. However, if there is some reason I am unaware of, and it is
> > important for libvirt to know, then I think it is important for libvirt
> to
> > find out the authoritative state rather than guessing.
> Explicit defaults are always better for two reasons: they are visible to
> users and they don't silently change.
>
I think it can go either way. There is also convention over configuration
as a competing principle. However, I also prefer explicit. Just, it needs
to be correct, otherwise explicit can be very bad, as it seems in my case.
:-)
Thanks,
--
Mark Mielke
Re: [libvirt PATCH v2 3/5] m4: virt-secdriver-selinux: drop obsolete function checks
On a Friday in 2020, Pavel Hrdina wrote: All of the listed functions are available in libselinux version 2.2. Our supported OSes start with version 2.5 so there is no need to check it. Signed-off-by: Pavel Hrdina --- m4/virt-secdriver-selinux.m4| 24 ++-- src/security/security_selinux.c | 18 +++--- tests/securityselinuxhelper.c | 6 -- 3 files changed, 5 insertions(+), 43 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: libvirt-6.5.0 breaks host passthrough migration
On Fri, Jul 10, 2020 at 05:53:47 -0400, Mark Mielke wrote: > Hi all: > > Any thoughts on this, or not time enough to look? > > I'm trying to decide what the right solution is on my end to deal with this > breaking change, before more widely deploying it. If I know what the fix > will be, I would like to align my strategy against it. Or, if we discuss > and the libvirt team disagrees with my analysis, then I will have to decide > whether to enable some sort of conversion mechanism to safely migrate > machines from libvirt-6.4.0 and before, to libvirt-6.5.0 without losing the > migration capability. Ah, sorry about the delay, I was on vacation. Anyway, I just replied to your original email... Jirka
Re: libvirt-6.5.0 breaks host passthrough migration
On Sun, Jul 05, 2020 at 12:45:55 -0400, Mark Mielke wrote:
> Hi all:
>
> With 6.4.0, live migration was working fine with Qemu 5.0. After trying out
> 6.5.0, migration broke with the following error:
>
> libvirt.libvirtError: internal error: unable to execute QEMU command
> 'migrate': State blocked by non-migratable CPU device (invtsc flag)
Could you please describe the reproducer steps? For example, was the
domain you're trying to migrate already running when you upgrade libvirt
or is it freshly started by the new libvirt?
And it would be helpful to see the element as shown by virsh
dumpxml before you try to start the domain as well as the QEMU command
line libvirt used to start the domain (in
/var/log/libvirt/qemu/$VM.log).
> I believe I traced the error back to this commit:
>
> commit 201bd5db639c063862b0c1b1abfab9a9a7c92591
> Author: Jiri Denemark
> Date: Tue Jun 2 15:34:07 2020 +0200
>
> qemu: Fill default value in //cpu/@migratable attribute
>
> Before QEMU introduced migratable CPU property, "-cpu host" included all
> features that could be enabled on the host, even those which would block
> migration. In other words, the default was equivalent to migratable=off.
> When the migratable property was introduced, the default changed to
> migratable=on. Let's record the default in domain XML.
>
> Signed-off-by: Jiri Denemark
> Reviewed-by: Michal Privoznik
>
>
> Before this change, qemu was still being launched with "-cpu host", which
> for any somewhat modern version of qemu, defaults to migratable=on. The
> above comment acknowledges this, however, the implementation chooses the
> pessimistic and ancient (and no longer applicable!) value of migratable=off:
>
> +if (qemuCaps &&
> +def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH &&
> +!def->cpu->migratable) {
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_MIGRATABLE))
> +def->cpu->migratable = VIR_TRISTATE_SWITCH_ON;
>
> *+else if (ARCH_IS_X86(def->os.arch))+
> def->cpu->migratable = VIR_TRISTATE_SWITCH_OFF;*
> +}
The implementation seems to be doing exactly what the commit message
says. The migratable=off default should be used only when QEMU does not
support -cpu host,migratable=on|off, that is only when QEMU is very old.
Every non-ancient version of libvirt should have the
QEMU_CAPS_CPU_MIGRATABLE set and thus this code should choose
migrateble=on default.
> I think it is not a requirement for "migratable=XXX" to be explicit in
> libvirt. However, if there is some reason I am unaware of, and it is
> important for libvirt to know, then I think it is important for libvirt to
> find out the authoritative state rather than guessing.
Explicit defaults are always better for two reasons: they are visible to
users and they don't silently change.
Jirka
Re: [libvirt PATCH v2 4/5] m4: virt-xdr: rewrite XDR check
On Fri, 2020-07-10 at 12:25 +0200, Pavel Hrdina wrote:
> +%if 0%{?fedora} || 0%{?rhel}
> BuildRequires: libtirpc-devel
> %endif
We only support building RPMs on Fedora and RHEL, so this check is
effectively a no-op.
--
Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 0/2] virSecurityManagerMetadataLock: Ignore RO filesystem
On a Friday in 2020, Michal Privoznik wrote: See 2/2 for explanation. Michal Prívozník (2): virSecurityManagerMetadataLock: Clarify directory locking comment virSecurityManagerMetadataLock: Ignore RO filesystem src/security/security_manager.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 0/7] qemu: Stop chowning domain restore file
On Wed, Jul 01, 2020 at 06:15:00PM +0200, Michal Privoznik wrote:
> See 4/7 for detailed explanation.
>
> Michal Prívozník (7):
> security: Reintroduce virSecurityManager{Set,Restore}SavedStateLabel
> qemu_security: Implement
> virSecurityManager{Set,Restore}SavedStateLabel
> security_selinux: Implement
> virSecurityManager{Set,Restore}SavedStateLabel
> qemu: Use qemuSecuritySetSavedStateLabel() to label restore path
> Revert "qemuSecurityDomainRestorePathLabel: Introduce @ignoreNS
> argument"
> secdrivers: Rename @stdin_path argument of
> virSecurityDomainSetAllLabel()
> qemu_security: Complete renaming of virSecurityManagerSetAllLabel()
> argument
I gave it a try, compared it to both 6.5.0 and to the much older 4.6.0 as per
the bugzilla in 4/7 and it worked.
Reviewed-by: Erik Skultety
Re: [PATCH 7/7] qemu_security: Complete renaming of virSecurityManagerSetAllLabel() argument
On Wed, Jul 01, 2020 at 06:15:07PM +0200, Michal Privoznik wrote:
> Just like in the previous commit, the stdin_path argument of
> virSecurityManagerSetAllLabel() is renamed to incomingPath.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_security.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 34cfcd3256..621523f086 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -32,7 +32,7 @@ VIR_LOG_INIT("qemu.qemu_security");
> int
> qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -const char *stdin_path,
> +const char *incomingPath,
> bool migrated)
The same change should be performed in qemu_security.h
I also think since all of the stdin_path occurrences are security related, you
can squash 6 and 7 together. While doing that you might as well toss in an
identical patch for da863c2ad15 which also missed the same change in the header
file.
Erik
Re: [RFC PATCH 0/2] hw/sd: Deprecate the SPI mode and the SPI to SD adapter
Hi Philippe, On Mon, Jul 6, 2020 at 6:07 AM Philippe Mathieu-Daudé wrote: > > I tried to maintain the SPI mode because it is useful in > tiny embedded devices, and thought it would be helpful for > the AVR MCUs. > As AVR was blocked, I thought it was wise to deprecate the > SPI mode as users are interested in the faster MMC mode. > Today Thomas surprised me by posting an update of it! > https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html > > I'm still posting this as RFC to discuss, but I'm reconsiderating > keeping this mode a bit more. > AFAIK, SiFive folks (Pragnesh in cc) are investigating supporting QSPI model on "sifive_u" machine, and it will definitely use this SPI over SD model. In fact, the QSPI is the last big gap in the "sifive_u" machine to make it a complete platform for hardware replacement. Regards, Bin
Re: [PATCH] docs: point out that locals should be defined at the top of a block of code
On Thu, Jul 09, 2020 at 06:41:21PM -0400, Laine Stump wrote: > Although we have nothing in make syntax-check to enforce this, and > apparently there are places where it isn't the case (according to > Dan), we should discourage the practice of defining new variables in > the middle of a block of code. > > https://www.redhat.com/archives/libvir-list/2020-July/msg00433.html > Signed-off-by: Laine Stump > --- > docs/coding-style.rst | 38 ++ > 1 file changed, 38 insertions(+) > > diff --git a/docs/coding-style.rst b/docs/coding-style.rst > index 03b89c86e5..b9b4a16987 100644 > --- a/docs/coding-style.rst > +++ b/docs/coding-style.rst > @@ -541,6 +541,44 @@ diligent about this, when you see a non-const pointer, > you're > guaranteed that it is used to modify the storage it points to, or > it is aliased to another pointer that is. > > +Defining Local Variables > + > + > +Always define local variables at the top of the block in which they > +are used (before any pure code). Although modern C compilers allow > +defining a local variable in the middle of a block of code, this > +practice can lead to bugs, and must be avoided in all libvirt > +code. (As indicated in these examples, it is okay to initialize > +variables where they are defined, even if the initialization involves > +calling another function.) > + > +:: > + > + GOOD: > +int > +Bob(char *loblaw) Since we are nitpicking I don't think we allow the first letter of the function name to be uppercase. :) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature
[libvirt PATCH v2 4/5] m4: virt-xdr: rewrite XDR check
The current code to check XDR support was obsolete and way to
complicated.
On linux we can use pkg-config to check for libtirpc and have
the CFLAGS and LIBS configured by it as well.
On MinGW there is portablexdr library which installs header files
directly into system include directory.
On FreeBSD and macOS XDR functions are part of libc so there is
no library needed, we just need to call AM_CONDITIONAL to silence
configure which otherwise complains about missing WITH_XDR.
Signed-off-by: Pavel Hrdina
---
libvirt.spec.in | 4
m4/virt-xdr.m4 | 39 +++--
src/Makefile.am | 4 +++-
src/admin/Makefile.inc.am | 1 +
src/locking/Makefile.inc.am | 2 ++
src/logging/Makefile.inc.am | 1 +
src/remote/Makefile.inc.am | 1 +
7 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 9f24e06aa46..6413d0e4e7a 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -406,8 +406,12 @@ BuildRequires: wireshark-devel >= 2.4.0
BuildRequires: libssh-devel >= 0.7.0
%endif
+# On RHEL-7 rpcgen is still part of glibc-common package
%if 0%{?fedora} || 0%{?rhel} > 7
BuildRequires: rpcgen
+%endif
+
+%if 0%{?fedora} || 0%{?rhel}
BuildRequires: libtirpc-devel
%endif
diff --git a/m4/virt-xdr.m4 b/m4/virt-xdr.m4
index 83754157d9a..09d0c2ba2f4 100644
--- a/m4/virt-xdr.m4
+++ b/m4/virt-xdr.m4
@@ -18,37 +18,20 @@ dnl
Re: libvirt opens kernel+initrd in read-write mode
On 7/9/20 8:27 PM, Olaf Hering wrote: Am Thu, 9 Jul 2020 19:00:18 +0200 schrieb Michal Privoznik : do you see an actual libvirt error? I think this may come from secdrivers trying to remember the original owner of kernel/initrd files. Jul 09 16:10:42 libvirtd[5741]: internal error: child reported (status=125): unable to open /path/to/initrd: Read-only file system Jul 09 16:10:42 libvirtd[5741]: unable to open /path/to/initrd: Read-only file system Jul 09 16:10:42 libvirtd[5741]: internal error: child reported (status=125): unable to open /path/to/initrd: Read-only file system Jul 09 16:10:42 libvirtd[5741]: unable to open /path/to/initrd: Read-only file system If you set remember_owner=0 in /etc/libvirt/qemu.conf (and restart libvirtd) then does it fix your problem? Yes, this helps as a workaround. Patch proposed here: https://www.redhat.com/archives/libvir-list/2020-July/msg00530.html Michal
[libvirt PATCH v2 1/5] docs: drop %.png: %.fig rule
convert bin is part of ImageMagick package and uses uniconvertor to
create png file from fig files.
Unfortunately uniconvertor is python2 only and not available in most
recent distributions which makes the convert command fail with:
sh: uniconvertor: command not found
/usr/bin/mv: cannot stat '/tmp/magick-1397138DRT8Pzx4Qmoc.svg': No such file or
directory
convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg'
'%o'' @ error/delegate.c/InvokeDelegate/1958.
convert: unable to open file `/tmp/magick-1397138S8ARueJXLXkc': No such file or
directory @ error/constitute.c/ReadImage/605.
convert: no images defined `docs/migration-managed-direct.png' @
error/convert.c/ConvertImageCommand/3226.
It looks like that there are plans to somehow port uniconvertor into
python3 but as part of different project color-picker but the job is
far from complete.
Signed-off-by: Pavel Hrdina
Reviewed-by: Ján Tomko
---
docs/Makefile.am | 3 ---
1 file changed, 3 deletions(-)
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 07a7d7a369c..3fd8256e668 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -406,9 +406,6 @@ hvsupport.html.in: $(top_srcdir)/scripts/hvsupport.py
$(api_DATA) \
$(AM_V_GEN)$(RUNUTF8) $(PYTHON) $(top_srcdir)/scripts/hvsupport.py \
$(top_srcdir) $(top_builddir) > $@ || { rm $@ && exit 1; }
-%.png: %.fig
- convert -rotate 90 $< $@
-
manpages/%.html.in: manpages/%.rst
$(AM_V_GEN)$(MKDIR_P) `dirname $@` && \
grep -v '^:Manual ' < $< | \
--
2.26.2
Re: [PATCH] docs: point out that locals should be defined at the top of a block of code
On Thu, Jul 09, 2020 at 06:41:21PM -0400, Laine Stump wrote: > Although we have nothing in make syntax-check to enforce this, and > apparently there are places where it isn't the case (according to > Dan), we should discourage the practice of defining new variables in > the middle of a block of code. > > https://www.redhat.com/archives/libvir-list/2020-July/msg00433.html > Signed-off-by: Laine Stump > --- > docs/coding-style.rst | 38 ++ > 1 file changed, 38 insertions(+) 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 :|
[libvirt PATCH v2 0/5] fixes and cleanups for current build system
While working on rewrite to Meson I discovered some parts of our current build system that could be improved to help with the transition to Meson. It will make the review of the Meson patches a bit easier. Changes in v2: - dropped patches: src: unify virFileActivateDirOverride() tools: virsh-secret: fix compilation error - fixed pointed out issues - added a patch to covert FIG files into SVG files Pavel Hrdina (5): docs: drop %.png: %.fig rule docs: convert FIG files into SVG format m4: virt-secdriver-selinux: drop obsolete function checks m4: virt-xdr: rewrite XDR check wireshark: fix compilation errors docs/Makefile.am | 3 - docs/architecture.fig| 87 -- docs/architecture.svg| 239 +++ docs/libvirt-daemon-arch.fig | 114 - docs/libvirt-daemon-arch.svg | 185 + docs/libvirt-driver-arch.fig | 62 --- docs/libvirt-driver-arch.svg | 94 +++ docs/libvirt-object-model.fig| 61 --- docs/libvirt-object-model.svg| 138 docs/libvirt-virConnect-example.fig | 58 --- docs/libvirt-virConnect-example.svg | 138 docs/migration-managed-direct.fig| 58 --- docs/migration-managed-direct.svg| 107 docs/migration-managed-p2p.fig | 58 --- docs/migration-managed-p2p.svg | 107 docs/migration-native.fig| 43 - docs/migration-native.svg| 68 docs/migration-tunnel.fig| 49 -- docs/migration-tunnel.svg| 92 +++ docs/migration-unmanaged-direct.fig | 58 --- docs/migration-unmanaged-direct.svg | 107 docs/node.fig| 30 docs/node.svg| 36 docs/structures.fig | 72 docs/structures.svg | 187 + libvirt.spec.in | 4 + m4/virt-secdriver-selinux.m4 | 24 +-- m4/virt-xdr.m4 | 39 ++--- src/Makefile.am | 4 +- src/admin/Makefile.inc.am| 1 + src/internal.h | 4 + src/locking/Makefile.inc.am | 2 + src/logging/Makefile.inc.am | 1 + src/remote/Makefile.inc.am | 1 + src/security/security_selinux.c | 18 +- tests/securityselinuxhelper.c| 6 - tools/Makefile.am| 2 +- tools/wireshark/src/packet-libvirt.c | 17 +- 38 files changed, 1545 insertions(+), 829 deletions(-) delete mode 100644 docs/architecture.fig create mode 100644 docs/architecture.svg delete mode 100644 docs/libvirt-daemon-arch.fig create mode 100644 docs/libvirt-daemon-arch.svg delete mode 100644 docs/libvirt-driver-arch.fig create mode 100644 docs/libvirt-driver-arch.svg delete mode 100644 docs/libvirt-object-model.fig create mode 100644 docs/libvirt-object-model.svg delete mode 100644 docs/libvirt-virConnect-example.fig create mode 100644 docs/libvirt-virConnect-example.svg delete mode 100644 docs/migration-managed-direct.fig create mode 100644 docs/migration-managed-direct.svg delete mode 100644 docs/migration-managed-p2p.fig create mode 100644 docs/migration-managed-p2p.svg delete mode 100644 docs/migration-native.fig create mode 100644 docs/migration-native.svg delete mode 100644 docs/migration-tunnel.fig create mode 100644 docs/migration-tunnel.svg delete mode 100644 docs/migration-unmanaged-direct.fig create mode 100644 docs/migration-unmanaged-direct.svg delete mode 100644 docs/node.fig create mode 100644 docs/node.svg delete mode 100644 docs/structures.fig create mode 100644 docs/structures.svg -- 2.26.2
[PATCH 1/2] virSecurityManagerMetadataLock: Clarify directory locking comment
In the light of recent commit of 9d83281382 fix the comment that
says directories can't be locked. Well, in general they can, but
not in our case.
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index ad1938caeb..d26d3a0527 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1353,7 +1353,8 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
continue;
if (S_ISDIR(sb.st_mode)) {
-/* Directories can't be locked */
+/* We need to open the path for writing because we need exclusive
+ * (write) lock. But directories can't be opened for writing. */
continue;
}
--
2.26.2
[PATCH 2/2] virSecurityManagerMetadataLock: Ignore RO filesystem
When locking files for metadata change, we open() them for R/W
access. The write access is needed because we want to acquire
exclusive (write) lock (to mutually exclude with other daemons
trying to modify XATTRs on the same file). Anyway, the open()
might fail if the file lives on a RO filesystem. Well, if that's
the case, ignore the error and continue with the next file on the
list. We won't change any seclabel on the file anyway - there is
nothing to remember then.
Reported-by: Olaf Hering
Signed-off-by: Michal Privoznik
---
src/security/security_manager.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index d26d3a0527..252cfefcff 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -1359,6 +1359,11 @@ virSecurityManagerMetadataLock(virSecurityManagerPtr mgr
G_GNUC_UNUSED,
}
if ((fd = open(p, O_RDWR)) < 0) {
+if (errno == EROFS) {
+/* There is nothing we can do for RO filesystem. */
+continue;
+}
+
#ifndef WIN32
if (S_ISSOCK(sb.st_mode)) {
/* Sockets can be opened only if there exists the
--
2.26.2
Re: [PATCH] docs: point out that locals should be defined at the top of a block of code
On Thu, 2020-07-09 at 18:41 -0400, Laine Stump wrote:
> +Defining Local Variables
> +
> +
> +Always define local variables at the top of the block in which they
> +are used (before any pure code). Although modern C compilers allow
> +defining a local variable in the middle of a block of code, this
> +practice can lead to bugs, and must be avoided in all libvirt
> +code. (As indicated in these examples, it is okay to initialize
> +variables where they are defined, even if the initialization involves
> +calling another function.)
The parentheses around the last sentence are unnecessary, please
drop them.
> + GOOD:
> +int
> +Bob(char *loblaw)
> +{
> +int x;
> +int y = lawBlog(loblaw);
I believe this should be
int y = lawBlog();
but note that I haven't compile-tested this alternative version.
> + BAD:
> +int
> +Bob(char *loblaw)
> +{
> +int x;
> +int y = lawBlog(loblaw);
> +
> +x = y + 20;
> +
> +char *z = NULL; <===
Please add // in front of the ASCII arrow.
It's pretty weird how we use C++-style comments throughout our style
guide, at the same time as *the style guide itself* instructs
developers to use C-style comments instead, but addressing that is a
job for another patch :)
With the nits fixed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
[libvirt PATCH v2 5/5] wireshark: fix compilation errors
With meson introduction which is using the same CFLAGS for the whole
project some compilation errors were discovered. The wireshark plugin
library is the only one in tools directory that is not using AM_CFLAGS.
With the AM_CFLAGS we get these errors:
../../tools/wireshark/src/packet-libvirt.c: In function 'dissect_libvirt_fds':
../../tools/wireshark/src/packet-libvirt.c:348:31: error: unused parameter
'tvb' [-Werror=unused-parameter]
348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
| ~~^~~
../../tools/wireshark/src/packet-libvirt.c:348:41: error: unused parameter
'start' [-Werror=unused-parameter]
348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
|~^
../../tools/wireshark/src/packet-libvirt.c:348:55: error: unused parameter
'nfds' [-Werror=unused-parameter]
348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
|~~~^~~~
At top level:
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_bool'
defined but not used [-Werror=unused-function]
64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int
hf) \
| ^~~~
../../tools/wireshark/src/packet-libvirt.c:88:1: note: in expansion of macro
'XDR_PRIMITIVE_DISSECTOR'
88 | XDR_PRIMITIVE_DISSECTOR(bool,bool_t, boolean)
| ^~~
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_float'
defined but not used [-Werror=unused-function]
64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int
hf) \
| ^~~~
../../tools/wireshark/src/packet-libvirt.c:86:1: note: in expansion of macro
'XDR_PRIMITIVE_DISSECTOR'
86 | XDR_PRIMITIVE_DISSECTOR(float, gfloat, float)
| ^~~
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_short'
defined but not used [-Werror=unused-function]
64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int
hf) \
| ^~~~
../../tools/wireshark/src/packet-libvirt.c:80:1: note: in expansion of macro
'XDR_PRIMITIVE_DISSECTOR'
80 | XDR_PRIMITIVE_DISSECTOR(short, gint16, int)
| ^~~
../../tools/wireshark/src/packet-libvirt.c: In function
'dissect_libvirt_message':
../../tools/wireshark/src/packet-libvirt.c:423:34: error: null pointer
dereference [-Werror=null-dereference]
423 | vir_xdr_dissector_t xd = find_payload_dissector(proc, type,
get_program_data(prog, VIR_PROGRAM_DISSECTORS),
|
^~
424 | *(gsize
*)get_program_data(prog, VIR_PROGRAM_DISSECTORS_LEN));
|
~
Signed-off-by: Pavel Hrdina
---
src/internal.h | 4
tools/Makefile.am| 2 +-
tools/wireshark/src/packet-libvirt.c | 17 ++---
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/internal.h b/src/internal.h
index c054e3ce96f..3bab6b4bfe0 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -174,6 +174,10 @@
_Pragma ("GCC diagnostic push")
#endif
+#define VIR_WARNINGS_NO_UNUSED_FUNCTION \
+_Pragma ("GCC diagnostic push") \
+_Pragma ("GCC diagnostic ignored \"-Wunused-function\"")
+
/* Workaround bogus GCC 6.0 for logical 'or' equal expression warnings.
* (GCC bz 69602) */
#if BROKEN_GCC_WLOGICALOP_EQUAL_EXPR
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 53df930e0ac..eb8f269b486 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -378,7 +378,7 @@ if WITH_WIRESHARK_DISSECTOR
ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la
wireshark_src_libvirt_la_CFLAGS = \
- -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) $(XDR_CFLAGS)
+ -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) $(XDR_CFLAGS)
$(AM_CFLAGS)
wireshark_src_libvirt_la_LDFLAGS = -avoid-version -module
wireshark_src_libvirt_la_SOURCES = \
wireshark/src/packet-libvirt.h \
diff --git a/tools/wireshark/src/packet-libvirt.c
b/tools/wireshark/src/packet-libvirt.c
index 20b7a3ec812..2b499d2cf29 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -75,6 +75,8 @@ static gint ett_libvirt_stream_hole = -1;
} \
}
+VIR_WARNINGS_NO_UNUSED_FUNCTION
+
XDR_PRIMITIVE_DISSECTOR(int, gint32, int)
XDR_PRIMITIVE_DISSECTOR(u_int, guint32, uint)
XDR_PRIMITIVE_DISSECTOR(short, gint16, int)
@@ -87,6 +89,8 @@ XDR_PRIMITIVE_DISSECTOR(float, gfloat, float)
XDR_PRIMITIVE_DISSECTOR(double, gdouble, double)
XDR_PRIMITIVE_DISSECTOR(bool,bool_t, boolean)
+VIR_WARNINGS_RESET
+
typedef gboolean
Re: [libvirt PATCH 27/31] src: unify virFileActivateDirOverride()
On Thu, Jul 09, 2020 at 04:21:32PM +0100, Daniel P. Berrangé wrote: > On Thu, Jul 02, 2020 at 02:25:25PM +0200, Pavel Hrdina wrote: > > We have two functions to set that we want to override directory where to > > look for libvirt files, one checks the binary name and the other one > > checks env variable. > > > > This patch removes the binary name check to simplify code by using only > > the env variable. > > > > This change will be required for Meson introduction where the binary > > name doesn't have any prefix by default. > > This means that we won't be able to run "./src/libvirtd" directly from > the build dir - we'll *always* have to use "./run ./src/libvirtd" to > get the env var set IIUC. > > Instead of checking the binary name, can we check for existance of a > known valid build file in the directory we're running the binary from. > eg look for "Makefile" or "ninja.build" or something like that. This will not work, with autotools we are running from: $abs_build_dir/src/.libs/libvirtd where there are no Makefile or any other autotools related files, but that's not an issue as I will drop this patch for now. We actually have to fix it only for Meson. However, with Meson it will not work as well because the ninja.build file is only in the top-level build directory. My idea is to canonicalize PWD/argv0 and check if it matches abs_build_dir as prefix. Pavel signature.asc Description: PGP signature
Re: [PATCH] resctrl: Do not open directory for writing
On 7/9/20 6:30 PM, Andrea Bolognani wrote: On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote: On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote: it seems to me that this change entirely flipped the semantics of the lock. Yep, you're right. When you have a lock that has boolean as a parameter I think the boolean should indicate whether the lock is writable, but maybe the proper and most readable solution would be virFileFlockShareable() and virFileFlockExclusive() to avoid any misconception in the future[2]. Given the fact that there are no (and most probably won't be) any other users of flock(2) and given the fact that resctrl is also pretty niche feature, I don't have any preference. Also I feel like after some time I'm a little bit rusty with C and the libvirt codebase (and most importantly the reasoning and decisions) has changed a bit, so I'll leave the decision on how to deal with that on someone else. I'm happy to provide the patches when clear decision is made. Either virFileFlockExclusive(fd) virFileFlockShared(fd) or virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE) virFileFlock(fd, VIR_FILE_FLOCK_SHARED) would work. I like the latter better because it's closer to the original flock(), which it ultimately acts as a very thin wrapper around of. I'm actually unclear why we'd have the last argument in the first place: personally I'd just use virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) and keep things as unambiguous as they can be. This is all bikeshedding, of course: what actually matters is making that lock exclusive once again :) Just realized that for exclusive (aka write) lock, the FD must be opened for writing (working on patches for the following report [1] and been experimenting a bit and that's what I'm seeing). 1: https://www.redhat.com/archives/libvir-list/2020-July/msg00451.html Michal
Re: libvirt-6.5.0 breaks host passthrough migration
Hi all:
Any thoughts on this, or not time enough to look?
I'm trying to decide what the right solution is on my end to deal with this
breaking change, before more widely deploying it. If I know what the fix
will be, I would like to align my strategy against it. Or, if we discuss
and the libvirt team disagrees with my analysis, then I will have to decide
whether to enable some sort of conversion mechanism to safely migrate
machines from libvirt-6.4.0 and before, to libvirt-6.5.0 without losing the
migration capability.
On Sun, Jul 5, 2020 at 12:45 PM Mark Mielke wrote:
> Hi all:
>
> With 6.4.0, live migration was working fine with Qemu 5.0. After trying
> out 6.5.0, migration broke with the following error:
>
> libvirt.libvirtError: internal error: unable to execute QEMU command
> 'migrate': State blocked by non-migratable CPU device (invtsc flag)
>
>
> I believe I traced the error back to this commit:
>
> commit 201bd5db639c063862b0c1b1abfab9a9a7c92591
> Author: Jiri Denemark
> Date: Tue Jun 2 15:34:07 2020 +0200
>
> qemu: Fill default value in //cpu/@migratable attribute
>
> Before QEMU introduced migratable CPU property, "-cpu host" included
> all
> features that could be enabled on the host, even those which would
> block
> migration. In other words, the default was equivalent to
> migratable=off.
> When the migratable property was introduced, the default changed to
> migratable=on. Let's record the default in domain XML.
>
> Signed-off-by: Jiri Denemark
> Reviewed-by: Michal Privoznik
>
>
> Before this change, qemu was still being launched with "-cpu host", which
> for any somewhat modern version of qemu, defaults to migratable=on. The
> above comment acknowledges this, however, the implementation chooses the
> pessimistic and ancient (and no longer applicable!) value of migratable=off:
>
> +if (qemuCaps &&
> +def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH &&
> +!def->cpu->migratable) {
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_MIGRATABLE))
> +def->cpu->migratable = VIR_TRISTATE_SWITCH_ON;
>
> *+else if (ARCH_IS_X86(def->os.arch))+
> def->cpu->migratable = VIR_TRISTATE_SWITCH_OFF;*
> +}
>
> Consequently, after live migration qemu is started with "-cpu
> host,migratable=off" and this enables invtsc, which then prevents migration
> as when invtsc is enabled it activates a migration blocker.
>
> I think this patch should be reverted. If the migratable state cannot be
> determined, then it should not be guessed. It should be left as "absent",
> just as it has been working fine prior to libvirt-6.5.0. Qemu still knows
> what flags are enabled, so Qemu remains the authority on what can be safely
> migrated, and what cannot be. I reverted this patch and re-built, and this
> seems to clear the migration problems. If the user chooses to explicitly
> specify migratable=yes or migratable=no, then this value should be
> preserved and passed through to the Qemu "-cpu host,XXX" option.
>
> I think it is not a requirement for "migratable=XXX" to be explicit in
> libvirt. However, if there is some reason I am unaware of, and it is
> important for libvirt to know, then I think it is important for libvirt to
> find out the authoritative state rather than guessing.
>
> Please start by reverting this patch, so that other people do not get
> broken in the same way, and I don't need to carry my revert patch.
> Personally, I think this is important enough to build a 6.5.1. However,
> since I have a local revert patch in place, I am not waiting for this.
>
> Thanks!
>
> --
> Mark Mielke
>
>
--
Mark Mielke
[PATCH 0/2] virSecurityManagerMetadataLock: Ignore RO filesystem
See 2/2 for explanation. Michal Prívozník (2): virSecurityManagerMetadataLock: Clarify directory locking comment virSecurityManagerMetadataLock: Ignore RO filesystem src/security/security_manager.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 2.26.2
[libvirt PATCH v2 2/5] docs: convert FIG files into SVG format
Converted by using: fig2dev -L svg Signed-off-by: Pavel Hrdina --- docs/architecture.fig | 87 -- docs/architecture.svg | 239 docs/libvirt-daemon-arch.fig| 114 - docs/libvirt-daemon-arch.svg| 185 + docs/libvirt-driver-arch.fig| 62 docs/libvirt-driver-arch.svg| 94 +++ docs/libvirt-object-model.fig | 61 --- docs/libvirt-object-model.svg | 138 docs/libvirt-virConnect-example.fig | 58 --- docs/libvirt-virConnect-example.svg | 138 docs/migration-managed-direct.fig | 58 --- docs/migration-managed-direct.svg | 107 + docs/migration-managed-p2p.fig | 58 --- docs/migration-managed-p2p.svg | 107 + docs/migration-native.fig | 43 - docs/migration-native.svg | 68 docs/migration-tunnel.fig | 49 -- docs/migration-tunnel.svg | 92 +++ docs/migration-unmanaged-direct.fig | 58 --- docs/migration-unmanaged-direct.svg | 107 + docs/node.fig | 30 docs/node.svg | 36 + docs/structures.fig | 72 - docs/structures.svg | 187 ++ 24 files changed, 1498 insertions(+), 750 deletions(-) delete mode 100644 docs/architecture.fig create mode 100644 docs/architecture.svg delete mode 100644 docs/libvirt-daemon-arch.fig create mode 100644 docs/libvirt-daemon-arch.svg delete mode 100644 docs/libvirt-driver-arch.fig create mode 100644 docs/libvirt-driver-arch.svg delete mode 100644 docs/libvirt-object-model.fig create mode 100644 docs/libvirt-object-model.svg delete mode 100644 docs/libvirt-virConnect-example.fig create mode 100644 docs/libvirt-virConnect-example.svg delete mode 100644 docs/migration-managed-direct.fig create mode 100644 docs/migration-managed-direct.svg delete mode 100644 docs/migration-managed-p2p.fig create mode 100644 docs/migration-managed-p2p.svg delete mode 100644 docs/migration-native.fig create mode 100644 docs/migration-native.svg delete mode 100644 docs/migration-tunnel.fig create mode 100644 docs/migration-tunnel.svg delete mode 100644 docs/migration-unmanaged-direct.fig create mode 100644 docs/migration-unmanaged-direct.svg delete mode 100644 docs/node.fig create mode 100644 docs/node.svg delete mode 100644 docs/structures.fig create mode 100644 docs/structures.svg diff --git a/docs/architecture.fig b/docs/architecture.fig deleted file mode 100644 index 37ac719cfd1..000 --- a/docs/architecture.fig +++ /dev/null @@ -1,87 +0,0 @@ -#FIG 3.2 -Landscape -Center -Inches -Letter -100.00 -Single --2 -1200 2 -2 2 0 1 0 7 50 -1 -1 0.000 0 0 -1 0 0 5 -1050 7500 9375 7500 9375 8700 1050 8700 1050 7500 -2 4 0 1 0 7 50 -1 -1 0.000 0 0 7 0 0 5 -3525 7275 3525 4125 1050 4125 1050 7275 3525 7275 -2 1 1 2 0 7 50 -1 -1 6.000 0 0 -1 0 0 2 -1050 6540 3540 6525 -2 4 0 1 0 7 50 -1 -1 4.000 0 0 7 0 0 5 -1590 6900 1590 6645 1140 6645 1140 6900 1590 6900 -2 4 0 1 0 7 50 -1 -1 4.000 0 0 7 0 0 5 -1590 7185 1590 6930 1140 6930 1140 7185 1590 7185 -2 1 0 4 0 7 50 -1 -1 0.000 0 0 -1 1 1 2 - 1 1 2.00 120.00 240.00 - 1 1 2.00 120.00 240.00 -1875 7725 8625 7725 -2 2 0 1 0 7 50 -1 -1 0.000 0 0 -1 0 0 5 -1650 5625 3000 5625 3000 6375 1650 6375 1650 5625 -2 1 0 4 0 7 50 -1 -1 0.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -2850 7725 2850 6375 -2 4 0 1 0 7 50 -1 -1 0.000 0 0 7 0 0 5 -6450 7275 6450 4125 3975 4125 3975 7275 6450 7275 -2 4 0 1 0 7 50 -1 -1 0.000 0 0 7 0 0 5 -9300 7275 9300 4125 6825 4125 6825 7275 9300 7275 -2 1 1 2 0 7 50 -1 -1 6.000 0 0 -1 0 0 2 -3975 6540 6465 6525 -2 1 1 2 0 7 50 -1 -1 6.000 0 0 -1 0 0 2 -6825 6540 9315 6525 -2 1 0 4 0 7 50 -1 -1 0.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -5400 7725 5400 7050 -2 1 0 4 0 7 50 -1 -1 0.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -8025 7725 8025 7050 -2 2 0 1 0 7 50 -1 -1 0.000 0 0 -1 0 0 5 -1050 8925 9375 8925 9375 9900 1050 9900 1050 8925 -2 2 0 1 0 7 50 -1 -1 0.000 0 0 -1 0 0 5 -2100 4575 3450 4575 3450 5325 2100 5325 2100 4575 -2 1 1 3 0 7 50 -1 -1 2.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -3225 5325 3225 8325 -2 1 1 3 0 7 50 -1 -1 2.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -6225 6900 6225 8250 -2 1 1 3 0 7 50 -1 -1 2.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -8925 6900 8925 8250 -2 1 1 3 0 7 50 -1 -1 2.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -1725 7125 1725 8325 -2 1 0 4 0 7 50 -1 -1 0.000 0 0 -1 1 1 2 - 1 1 2.00 120.00 240.00 - 1 1 2.00 120.00 240.00 -2850 5850 2850 5025 -2 1 1 3 0 7 50 -1 -1 2.000 0 0 -1 1 0 2 - 1 1 2.00 120.00 240.00 -
[libvirt PATCH v2 3/5] m4: virt-secdriver-selinux: drop obsolete function checks
All of the listed functions are available in libselinux version 2.2.
Our supported OSes start with version 2.5 so there is no need to check
it.
Signed-off-by: Pavel Hrdina
---
m4/virt-secdriver-selinux.m4| 24 ++--
src/security/security_selinux.c | 18 +++---
tests/securityselinuxhelper.c | 6 --
3 files changed, 5 insertions(+), 43 deletions(-)
diff --git a/m4/virt-secdriver-selinux.m4 b/m4/virt-secdriver-selinux.m4
index a48569fc33a..4174249a510 100644
--- a/m4/virt-secdriver-selinux.m4
+++ b/m4/virt-secdriver-selinux.m4
@@ -32,28 +32,8 @@ AC_DEFUN([LIBVIRT_SECDRIVER_CHECK_SELINUX], [
AC_MSG_ERROR([You must install the libselinux development package and
enable SELinux with the --with-selinux=yes in order to compile libvirt
--with-secdriver-selinux=yes])
fi
elif test "$with_secdriver_selinux" != "no"; then
-old_CFLAGS="$CFLAGS"
-old_LIBS="$LIBS"
-CFLAGS="$CFLAGS $SELINUX_CFLAGS"
-LIBS="$CFLAGS $SELINUX_LIBS"
-
-fail=0
-AC_CHECK_FUNC([selinux_virtual_domain_context_path], [], [fail=1])
-AC_CHECK_FUNC([selinux_virtual_image_context_path], [], [fail=1])
-AC_CHECK_FUNCS([selinux_lxc_contexts_path])
-CFLAGS="$old_CFLAGS"
-LIBS="$old_LIBS"
-
-if test "$fail" = "1" ; then
- if test "$with_secdriver_selinux" = "check" ; then
-with_secdriver_selinux=no
- else
-AC_MSG_ERROR([You must install libselinux development package >=
2.0.82 in order to compile libvirt --with-secdriver-selinux=yes])
- fi
-else
- with_secdriver_selinux=yes
- AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SELINUX], 1, [whether SELinux
security driver is available])
-fi
+with_secdriver_selinux=yes
+AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SELINUX], 1, [whether SELinux security
driver is available])
fi
AM_CONDITIONAL([WITH_SECDRIVER_SELINUX], [test "$with_secdriver_selinux" !=
"no"])
])
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index f8c1a0a2f1a..67dc6ce09a4 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -639,7 +639,6 @@ virSecuritySELinuxGenNewContext(const char *basecontext,
}
-#ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH
static int
virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr)
{
@@ -702,15 +701,6 @@ virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr)
virHashFree(data->mcs);
return -1;
}
-#else
-static int
-virSecuritySELinuxLXCInitialize(virSecurityManagerPtr mgr G_GNUC_UNUSED)
-{
-virReportSystemError(ENOSYS, "%s",
- _("libselinux does not support LXC contexts path"));
-return -1;
-}
-#endif
static int
@@ -1018,11 +1008,9 @@ virSecuritySELinuxDriverProbe(const char *virtDriver)
if (is_selinux_enabled() <= 0)
return SECURITY_DRIVER_DISABLE;
-if (virtDriver && STREQ(virtDriver, "LXC")) {
-#if HAVE_SELINUX_LXC_CONTEXTS_PATH
-if (!virFileExists(selinux_lxc_contexts_path()))
-#endif
-return SECURITY_DRIVER_DISABLE;
+if (virtDriver && STREQ(virtDriver, "LXC") &&
+!virFileExists(selinux_lxc_contexts_path())) {
+return SECURITY_DRIVER_DISABLE;
}
return SECURITY_DRIVER_ENABLE;
diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c
index 0556241fd55..c3d7f8c1cee 100644
--- a/tests/securityselinuxhelper.c
+++ b/tests/securityselinuxhelper.c
@@ -48,9 +48,7 @@ static int (*real_is_selinux_enabled)(void);
static const char *(*real_selinux_virtual_domain_context_path)(void);
static const char *(*real_selinux_virtual_image_context_path)(void);
-#ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH
static const char *(*real_selinux_lxc_contexts_path)(void);
-#endif
static struct selabel_handle *(*real_selabel_open)(unsigned int backend,
const struct selinux_opt
*opts,
@@ -73,9 +71,7 @@ static void init_syms(void)
VIR_MOCK_REAL_INIT(selinux_virtual_domain_context_path);
VIR_MOCK_REAL_INIT(selinux_virtual_image_context_path);
-#ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH
VIR_MOCK_REAL_INIT(selinux_lxc_contexts_path);
-#endif
VIR_MOCK_REAL_INIT(selabel_open);
VIR_MOCK_REAL_INIT(selabel_close);
@@ -273,7 +269,6 @@ const char *selinux_virtual_image_context_path(void)
return abs_srcdir "/securityselinuxhelperdata/virtual_image_context";
}
-#ifdef HAVE_SELINUX_LXC_CONTEXTS_PATH
const char *selinux_lxc_contexts_path(void)
{
init_syms();
@@ -283,7 +278,6 @@ const char *selinux_lxc_contexts_path(void)
return abs_srcdir "/securityselinuxhelperdata/lxc_contexts";
}
-#endif
struct selabel_handle *
selabel_open(unsigned int backend,
--
2.26.2
Re: [RFC PATCH 0/2] hw/sd: Deprecate the SPI mode and the SPI to SD adapter
On 7/10/20 11:27 AM, Bin Meng wrote: > Hi Philippe, > > On Mon, Jul 6, 2020 at 6:07 AM Philippe Mathieu-Daudé wrote: >> >> I tried to maintain the SPI mode because it is useful in >> tiny embedded devices, and thought it would be helpful for >> the AVR MCUs. >> As AVR was blocked, I thought it was wise to deprecate the >> SPI mode as users are interested in the faster MMC mode. >> Today Thomas surprised me by posting an update of it! >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html >> >> I'm still posting this as RFC to discuss, but I'm reconsiderating >> keeping this mode a bit more. >> > > AFAIK, SiFive folks (Pragnesh in cc) are investigating supporting QSPI > model on "sifive_u" machine, and it will definitely use this SPI over > SD model. > > In fact, the QSPI is the last big gap in the "sifive_u" machine to > make it a complete platform for hardware replacement. Good news! I have some idea about the design, but don't have the time to work on it. Help in this area is welcomed, and I am happy to review the patches. The way I'd do it is keep the generic sd.c code and make it an abstract class (or interface), and split SD / SPI protocols handling in different implementation files. Only the negotiation at reset is common, once you switch to a protocol mode you can't switch to another without resetting the device. Also this would make the MMC protocol (already implemented by Xilinx) upstreamable more easily. Having different files also allows different maintenance granularity. I haven't looked at QSPI, but I expect it to be quite different that the old SPI mode. Regards, Phil.
Re: [libvirt PATCH 30/31] tools: virsh-secret: fix compilation error
On Thu, Jul 09, 2020 at 05:25:03PM +0200, Ján Tomko wrote: > On a Thursday in 2020, Pavel Hrdina wrote: > > ../tools/virsh-secret.c: In function ‘cmdSecretSetValue’: > > ../tools/virsh-secret.c:262:15: error: pointer targets in assignment from > > ‘char *’ to ‘unsigned char *’ differ in signedness [-Werror=pointer-sign] > > 262 | value = g_steal_pointer(_buf); > > | ^ > > cc1: all warnings being treated as errors > > > > Where do you get this error? I was not able to hit this error again after dropping this patch so probably there was something missing in meson, I'll drop this patch. Pavel signature.asc Description: PGP signature
Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it
On 09/07/20 21:13, Eduardo Habkost wrote: >> Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE >> mode, so that the hypervisor can validate the high bits in the PDPTEs? > If the fix has additional overhead, is the additional overhead > bad enough to warrant making it optional? Most existing > GUEST_MAXPHYADDR < HOST_MAXPHYADDR guests already work today > without the fix. The problematic case is when host maxphyaddr is 52. That case wouldn't work at all without the fix. Paolo
Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it
On 09/07/20 19:00, Jim Mattson wrote: >> >> Mostly fine. Some edge cases, like different page fault errors for >> addresses above GUEST_MAXPHYADDR and below HOST_MAXPHYADDR. Which I >> think Mohammed fixed in the kernel recently. > Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE > mode, so that the hypervisor can validate the high bits in the PDPTEs? In theory yes, but in practice it just means we'd use the AMD behavior of loading guest PDPT entries on demand during address translation (because the PDPT would point to nonexistent memory and cause an EPT violation on the PDE). Paolo
[GSoC][PATCH v3 3/4] qemu_domainjob: introduce `privateData` for `qemuDomainJob`
To remove dependecy of `qemuDomainJob` on job specific
paramters, a `privateData` pointer is introduced.
To handle it, structure of callback functions is
also introduced.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_domain.c | 4 +-
src/qemu/qemu_domain.h | 10 +
src/qemu/qemu_domainjob.c| 74 ++--
src/qemu/qemu_domainjob.h| 32 --
src/qemu/qemu_driver.c | 3 +-
src/qemu/qemu_migration.c| 28
src/qemu/qemu_migration_params.c | 9 ++--
src/qemu/qemu_process.c | 15 +--
8 files changed, 119 insertions(+), 56 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 10d2033db1..4ece07d141 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2303,7 +2303,7 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
if (priv->lockState)
virBufferAsprintf(buf, "%s\n", priv->lockState);
-if (qemuDomainObjPrivateXMLFormatJob(buf, vm) < 0)
+if (priv->job.cb.formatJob(buf, vm) < 0)
return -1;
if (priv->fakeReboot)
@@ -2962,7 +2962,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
priv->lockState = virXPathString("string(./lockstate)", ctxt);
-if (qemuDomainObjPrivateXMLParseJob(vm, ctxt) < 0)
+if (priv->job.cb.parseJob(vm, ctxt) < 0)
goto error;
priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index e524fd0002..bb9b414a46 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -492,6 +492,16 @@ struct _qemuDomainXmlNsDef {
char **capsdel;
};
+typedef struct _qemuDomainJobPrivate qemuDomainJobPrivate;
+typedef qemuDomainJobPrivate *qemuDomainJobPrivatePtr;
+struct _qemuDomainJobPrivate {
+bool spiceMigration;/* we asked for spice migration and we
+ * should wait for it to finish */
+bool spiceMigrated; /* spice migration completed */
+bool dumpCompleted; /* dump completed */
+qemuMigrationParamsPtr migParams;
+};
+
int qemuDomainObjStartWorker(virDomainObjPtr dom);
void qemuDomainObjStopWorker(virDomainObjPtr dom);
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 454a5a2c23..d79b8d49f6 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -159,23 +159,32 @@ qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
virObjectEventStateQueue(driver->domainEventState, event);
}
-
-int
-qemuDomainObjInitJob(qemuDomainJobObjPtr job)
+static void *
+qemuJobAllocPrivate(void)
{
-memset(job, 0, sizeof(*job));
+qemuDomainJobPrivatePtr priv;
+if (VIR_ALLOC(priv) < 0)
+return NULL;
+return (void *)priv;
+}
-if (virCondInit(>cond) < 0)
-return -1;
-if (virCondInit(>asyncCond) < 0) {
-virCondDestroy(>cond);
-return -1;
-}
-
-return 0;
+static void
+qemuJobFreePrivateData(qemuDomainJobPrivatePtr priv)
+{
+priv->spiceMigration = false;
+priv->spiceMigrated = false;
+priv->dumpCompleted = false;
+qemuMigrationParamsFree(priv->migParams);
+priv->migParams = NULL;
}
+static void
+qemuJobFreePrivate(void *opaque)
+{
+qemuDomainJobObjPtr job = (qemuDomainJobObjPtr) opaque;
+qemuJobFreePrivateData(job->privateData);
+}
static void
qemuDomainObjResetJob(qemuDomainJobObjPtr job)
@@ -207,13 +216,9 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObjPtr job)
job->phase = 0;
job->mask = QEMU_JOB_DEFAULT_MASK;
job->abortJob = false;
-job->spiceMigration = false;
-job->spiceMigrated = false;
-job->dumpCompleted = false;
VIR_FREE(job->error);
g_clear_pointer(>current, qemuDomainJobInfoFree);
-qemuMigrationParamsFree(job->migParams);
-job->migParams = NULL;
+job->cb.freeJobPrivate(job);
job->apiFlags = 0;
}
@@ -229,7 +234,7 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj,
job->asyncJob = priv->job.asyncJob;
job->asyncOwner = priv->job.asyncOwner;
job->phase = priv->job.phase;
-job->migParams = g_steal_pointer(>job.migParams);
+job->privateData = g_steal_pointer(>job.privateData);
job->apiFlags = priv->job.apiFlags;
qemuDomainObjResetJob(>job);
@@ -1240,12 +1245,13 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr
buf,
}
-int
+static int
qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuDomainJobObjPtr jobObj = >job;
+qemuDomainJobPrivatePtr jobPriv = jobObj->privateData;
g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
qemuDomainJob job = jobObj->active;
@@ -1274,8 +1280,8 @@ qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
[PATCH 2/4] qemu_domainjob: job funcitons moved out of `qemu_domain`
We move functions `qemuDomainObjPrivateXMLParseJob` and
`qemuDomainObjPrivateXMLFormatJob` to `qemu_domainjob`.
To make this happen, corresponding changes in their
parameters were made, as well as they were exposed
to be used in `qemu_domain` file.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_domain.c| 245 +-
src/qemu/qemu_domainjob.c | 244 +
src/qemu/qemu_domainjob.h | 8 ++
3 files changed, 254 insertions(+), 243 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2d9d8226d6..10d2033db1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2168,102 +2168,6 @@ qemuDomainObjPrivateXMLFormatPR(virBufferPtr buf,
}
-static int
-qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf,
-virStorageSourcePtr src,
-virDomainXMLOptionPtr xmlopt)
-{
-g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
-g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
-
-virBufferAsprintf(, " type='%s' format='%s'",
- virStorageTypeToString(src->type),
- virStorageFileFormatTypeToString(src->format));
-
-if (virDomainDiskSourceFormat(, src, "source", 0, false,
- VIR_DOMAIN_DEF_FORMAT_STATUS,
- false, false, xmlopt) < 0)
-return -1;
-
-virXMLFormatElement(buf, "migrationSource", , );
-
-return 0;
-}
-
-
-static int
-qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr buf,
- virDomainObjPtr vm)
-{
-qemuDomainObjPrivatePtr priv = vm->privateData;
-size_t i;
-virDomainDiskDefPtr disk;
-qemuDomainDiskPrivatePtr diskPriv;
-
-for (i = 0; i < vm->def->ndisks; i++) {
-g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
-g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
-disk = vm->def->disks[i];
-diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-
-virBufferAsprintf(, " dev='%s' migrating='%s'",
- disk->dst, diskPriv->migrating ? "yes" : "no");
-
-if (diskPriv->migrSource &&
-qemuDomainObjPrivateXMLFormatNBDMigrationSource(,
-
diskPriv->migrSource,
-
priv->driver->xmlopt) < 0)
-return -1;
-
-virXMLFormatElement(buf, "disk", , );
-}
-
-return 0;
-}
-
-
-static int
-qemuDomainObjPrivateXMLFormatJob(virBufferPtr buf,
- virDomainObjPtr vm,
- qemuDomainObjPrivatePtr priv)
-{
-g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
-g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
-qemuDomainJob job = priv->job.active;
-
-if (!qemuDomainTrackJob(job))
-job = QEMU_JOB_NONE;
-
-if (job == QEMU_JOB_NONE &&
-priv->job.asyncJob == QEMU_ASYNC_JOB_NONE)
-return 0;
-
-virBufferAsprintf(, " type='%s' async='%s'",
- qemuDomainJobTypeToString(job),
- qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
-
-if (priv->job.phase) {
-virBufferAsprintf(, " phase='%s'",
- qemuDomainAsyncJobPhaseToString(priv->job.asyncJob,
- priv->job.phase));
-}
-
-if (priv->job.asyncJob != QEMU_ASYNC_JOB_NONE)
-virBufferAsprintf(, " flags='0x%lx'", priv->job.apiFlags);
-
-if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT &&
-qemuDomainObjPrivateXMLFormatNBDMigration(, vm) < 0)
-return -1;
-
-if (priv->job.migParams)
-qemuMigrationParamsFormat(, priv->job.migParams);
-
-virXMLFormatElement(buf, "job", , );
-
-return 0;
-}
-
-
static bool
qemuDomainHasSlirp(virDomainObjPtr vm)
{
@@ -2399,7 +2303,7 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
if (priv->lockState)
virBufferAsprintf(buf, "%s\n", priv->lockState);
-if (qemuDomainObjPrivateXMLFormatJob(buf, vm, priv) < 0)
+if (qemuDomainObjPrivateXMLFormatJob(buf, vm) < 0)
return -1;
if (priv->fakeReboot)
@@ -2899,151 +2803,6 @@ qemuDomainObjPrivateXMLParsePR(xmlXPathContextPtr ctxt,
}
-static int
-qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node,
- xmlXPathContextPtr ctxt,
- virDomainDiskDefPtr disk,
- virDomainXMLOptionPtr xmlopt)
-{
-VIR_XPATH_NODE_AUTORESTORE(ctxt);
-qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
-g_autofree char *format = NULL;
-g_autofree char *type = NULL;
-g_autoptr(virStorageSource) migrSource = NULL;
-
[GSoC][PATCH v3 4/4] qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo`
To remove dependecy of `qemuDomainJobInfo` on job specific
paramters, a `privateData` pointer is introduced.
To handle it, structure of callback functions is
also introduced.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_backup.c | 15 --
src/qemu/qemu_domain.h | 18 +++
src/qemu/qemu_domainjob.c| 92
src/qemu/qemu_domainjob.h| 30 ++-
src/qemu/qemu_driver.c | 18 ---
src/qemu/qemu_migration.c| 14 +++--
src/qemu/qemu_migration_cookie.c | 7 ++-
src/qemu/qemu_process.c | 11 ++--
8 files changed, 147 insertions(+), 58 deletions(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index b711f8f623..a0832b4587 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -529,6 +529,7 @@ qemuBackupJobTerminate(virDomainObjPtr vm,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
+qemuDomainJobInfoPrivatePtr jobInfoPriv;
size_t i;
qemuDomainJobInfoUpdateTime(priv->job.current);
@@ -536,10 +537,13 @@ qemuBackupJobTerminate(virDomainObjPtr vm,
g_clear_pointer(>job.completed, qemuDomainJobInfoFree);
priv->job.completed = qemuDomainJobInfoCopy(priv->job.current);
-priv->job.completed->stats.backup.total = priv->backup->push_total;
-priv->job.completed->stats.backup.transferred =
priv->backup->push_transferred;
-priv->job.completed->stats.backup.tmp_used = priv->backup->pull_tmp_used;
-priv->job.completed->stats.backup.tmp_total = priv->backup->pull_tmp_total;
+
+jobInfoPriv = priv->job.completed->privateData;
+
+jobInfoPriv->stats.backup.total = priv->backup->push_total;
+jobInfoPriv->stats.backup.transferred = priv->backup->push_transferred;
+jobInfoPriv->stats.backup.tmp_used = priv->backup->pull_tmp_used;
+jobInfoPriv->stats.backup.tmp_total = priv->backup->pull_tmp_total;
priv->job.completed->status = jobstatus;
priv->job.completed->errmsg = g_strdup(priv->backup->errmsg);
@@ -1069,7 +1073,8 @@ qemuBackupGetJobInfoStats(virQEMUDriverPtr driver,
virDomainObjPtr vm,
qemuDomainJobInfoPtr jobInfo)
{
-qemuDomainBackupStats *stats = >stats.backup;
+qemuDomainJobInfoPrivatePtr jobInfoPriv = jobInfo->privateData;
+qemuDomainBackupStats *stats = >stats.backup;
qemuDomainObjPrivatePtr priv = vm->privateData;
qemuMonitorJobInfoPtr *blockjobs = NULL;
size_t nblockjobs = 0;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index bb9b414a46..7fcbefd0c0 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -502,6 +502,24 @@ struct _qemuDomainJobPrivate {
qemuMigrationParamsPtr migParams;
};
+typedef struct _qemuDomainBackupStats qemuDomainBackupStats;
+struct _qemuDomainBackupStats {
+unsigned long long transferred;
+unsigned long long total;
+unsigned long long tmp_used;
+unsigned long long tmp_total;
+};
+
+typedef struct _qemuDomainJobInfoPrivate qemuDomainJobInfoPrivate;
+typedef qemuDomainJobInfoPrivate *qemuDomainJobInfoPrivatePtr;
+struct _qemuDomainJobInfoPrivate {
+union {
+qemuMonitorMigrationStats mig;
+qemuMonitorDumpStats dump;
+qemuDomainBackupStats backup;
+} stats;
+};
+
int qemuDomainObjStartWorker(virDomainObjPtr dom);
void qemuDomainObjStopWorker(virDomainObjPtr dom);
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index d79b8d49f6..d8913f1e70 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -115,22 +115,68 @@ qemuDomainAsyncJobPhaseFromString(qemuDomainAsyncJob job,
return -1;
}
+static void
+qemuJobInfoFreePrivateData(qemuDomainJobInfoPrivatePtr priv)
+{
+g_free(>stats);
+}
+
+static void
+qemuJobInfoFreePrivate(void *opaque)
+{
+qemuDomainJobInfoPtr jobInfo = (qemuDomainJobInfoPtr) opaque;
+qemuJobInfoFreePrivateData(jobInfo->privateData);
+}
void
qemuDomainJobInfoFree(qemuDomainJobInfoPtr info)
{
+info->cb.freeJobInfoPrivate(info);
g_free(info->errmsg);
g_free(info);
}
+static void *
+qemuDomainJobInfoPrivateAlloc(void)
+{
+qemuDomainJobInfoPrivatePtr retPriv = g_new0(qemuDomainJobInfoPrivate, 1);
+return (void *)retPriv;
+}
+
+static void
+qemuDomainJobInfoPrivateCopy(qemuDomainJobInfoPtr src,
+ qemuDomainJobInfoPtr dest)
+{
+memcpy(dest->privateData, src->privateData,
+ sizeof(qemuDomainJobInfoPrivate));
+}
+
+static qemuDomainJobInfoPtr
+qemuDomainJobInfoAlloc(void)
+{
+qemuDomainJobInfoPtr ret = g_new0(qemuDomainJobInfo, 1);
+ret->cb.allocJobInfoPrivate =
+ret->cb.freeJobInfoPrivate =
+ret->cb.copyJobInfoPrivate =
+ret->privateData = ret->cb.allocJobInfoPrivate();
+return ret;
+}
+
+static void
+qemuDomainCurrentJobInfoInit(qemuDomainJobObjPtr job)
+{
+job->current = qemuDomainJobInfoAlloc();
+job->current->status =
[GSoC][PATCH v3 1/4] qemu_domain: moved qemuDomainNamespace to `qemu_domain`
While moving the code, qemuDomainNamespace also was moved
to `qemu_domainjob`. Hence it is moved back to `qemu_domain`
where it will be more appropriate.
Signed-off-by: Prathamesh Chavan
---
src/qemu/qemu_domain.c| 5 +
src/qemu/qemu_domainjob.c | 6 --
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 248e259634..2d9d8226d6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -83,6 +83,11 @@
VIR_LOG_INIT("qemu.qemu_domain");
+VIR_ENUM_IMPL(qemuDomainNamespace,
+ QEMU_DOMAIN_NS_LAST,
+ "mount",
+);
+
/**
* qemuDomainObjFromDomain:
* @domain: Domain pointer that has to be looked up
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 7111acadda..654f1c4f90 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -63,12 +63,6 @@ VIR_ENUM_IMPL(qemuDomainAsyncJob,
"backup",
);
-VIR_ENUM_IMPL(qemuDomainNamespace,
- QEMU_DOMAIN_NS_LAST,
- "mount",
-);
-
-
const char *
qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
int phase G_GNUC_UNUSED)
--
2.17.1
[GSoC][PATCH v3 0/4] removal of qemu_domainjob dependencies
The following series of patches work on isolating the qemu_domainjob from its dependency on other files such as `qemu_migration_params`, `qemu_monitor`, etc. This is done by the introduction of a `privateData` structure, which is further handled by a structure of callback functions. Previous version of this patch can be found here[1]. As the previous patch was a bulky one, this series of patches are the result of splitting the previous one. [1]: https://www.redhat.com/archives/libvir-list/2020-July/msg00423.html Prathamesh Chavan (4): qemu_domain: moved qemuDomainNamespace to `qemu_domain` qemu_domainjob: job funcitons moved out of `qemu_domain` qemu_domainjob: introduce `privateData` for `qemuDomainJob` qemu_domainjob: introduce `privateData` for `qemuDomainJobInfo` src/qemu/qemu_backup.c | 15 +- src/qemu/qemu_domain.c | 250 +-- src/qemu/qemu_domain.h | 28 +++ src/qemu/qemu_domainjob.c| 406 +++ src/qemu/qemu_domainjob.h| 54 ++-- src/qemu/qemu_driver.c | 21 +- src/qemu/qemu_migration.c| 42 ++-- src/qemu/qemu_migration_cookie.c | 7 +- src/qemu/qemu_migration_params.c | 9 +- src/qemu/qemu_process.c | 26 +- 10 files changed, 510 insertions(+), 348 deletions(-) -- 2.17.1
Re: [PATCH] storage: fix vstorage backend build
On 09.07.2020 19:06, Andrea Bolognani wrote:
> On Tue, 2020-07-07 at 15:00 +0300, Nikolay Shirokovskiy wrote:
>> Add headers with declarations of geteuid/getegid
>> and virGetUserName/virGetGroupName.
>
> Can you share the exact error message you're hitting? Our CI seems to
> be perfectly happy with the current status quo. Specifically, the
> x86-centos-7 job, the only one which is supposed to be able to build
> the OpenVZ driver, is green.
Hi! Sure.
../../src/storage/storage_backend_vstorage.c: In function
'virStorageBackendVzPoolStart':
../../src/storage/storage_backend_vstorage.c:52:9: error: implicit declaration
of function 'geteuid' [-Werror=implicit-function-declaration]
def->target.perms.uid = geteuid();
^
../../src/storage/storage_backend_vstorage.c:52:9: error: nested extern
declaration of 'geteuid' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:54:9: error: implicit declaration
of function 'getegid' [-Werror=implicit-function-declaration]
def->target.perms.gid = getegid();
^
../../src/storage/storage_backend_vstorage.c:54:9: error: nested extern
declaration of 'getegid' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:58:5: error: implicit declaration
of function 'virGetGroupName' [-Werror=implicit-function-declaration]
if (!(grp_name = virGetGroupName(def->target.perms.gid)))
^
../../src/storage/storage_backend_vstorage.c:58:5: error: nested extern
declaration of 'virGetGroupName' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:58:20: error: assignment makes
pointer from integer without a cast [-Werror]
if (!(grp_name = virGetGroupName(def->target.perms.gid)))
^
../../src/storage/storage_backend_vstorage.c:61:5: error: implicit declaration
of function 'virGetUserName' [-Werror=implicit-function-declaration]
if (!(usr_name = virGetUserName(def->target.perms.uid)))
^
../../src/storage/storage_backend_vstorage.c:61:5: error: nested extern
declaration of 'virGetUserName' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:61:20: error: assignment makes
pointer from integer without a cast [-Werror]
if (!(usr_name = virGetUserName(def->target.perms.uid)))
^
>
> Looking at the raw log[1] for the latest instance of the job, you can
> see that the vz driver is enabled (vz: yes), but the vstorage driver
> is not (Virtuozzo storage: no). From the configure output, the reason
> seems clear:
>
> checking for vstorage... no
> checking for vstorage-mount... no
>
> The CentOS 7 container that we use for builds has libprlsdk-devel and
> friends installed from the official repository[2]... Although, now
> that I look at it there's apparently a newer release[3] out, so we
> should probably switch to it.
vstorage and vz driver are quite unrelated. For the former vstorage
and vstorage-mount binaries are build requirements.
>
>
> Another thing: the spec file calls configure with hardcoded
>
> --without-vz
> --without-storage-vstorage
>
> and there is no binary package definition for anything like
>
> libvirt-daemon-driver-vz
> libvirt-daemon-driver-storage-vstorage
>
> which would probably be nice to have enabled at least on CentOS 7?
> Funnily enough, we actually already have
>
> %if 0%{?rhel}
> %define with_vz 0
> %endif
>
> so clearly at some point there was the intention to conditionally
> include or exclude these drivers from the build.
>
>
> One thing at a time, though. First, how do we get the vstorage
> commands included in the CentOS 7 container? What packages need to
> be installed, and from what repository?
The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os
vstorage binary is in vstorage-ctl package and vstorage-mount binary
is in vstorage-client package.
However vstorage binary is not used at all the driver. Also the openvz
driver for example does not check for its binaries. Probably
vstorage driver should be changed accordingly as binaries are not
build requisites.
>
>
> As an aside, I'm still very confused by the vz/openvz dichotomy.
> AFAICT, the latter can be (and in fact is) built unconditionally,
> but the former requires the "Parallels SDK" packages to be installed:
> baffingly enough, said SDK is obtained from the repository mentioned
> above, which just so happens to include the string "openvz" twice in
> its URL...
Yeah, naming is confusing. Basically openvz manages system containers thru
vzctl binary. Vz driver manages both VMs/containers thru single connection
using prlsdk. And originally vz driver was called parallels driver but after
company split we have to change the name. Also in the past prlsdk was only
commercially available and now times changes and both vzctl and prlsdk are
available under openvz name which is an umbrella for uncommercial projects.
Nikolay
>
>
> [1]
>
Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier
On 2020/7/9 下午10:10, Peter Xu wrote: On Thu, Jul 09, 2020 at 01:58:33PM +0800, Jason Wang wrote: - If we care the performance, it's better to implement the MAP event for vhost, otherwise it could be a lot of IOTLB miss I feel like these are two things. So far what we are talking about is whether vt-d should have knowledge about what kind of events one iommu notifier is interested in. I still think we should keep this as answered in question 1. The other question is whether we want to switch vhost from UNMAP to MAP/UNMAP events even without vDMA, so that vhost can establish the mapping even before IO starts. IMHO it's doable, but only if the guest runs DPDK workloads. When the guest is using dynamic iommu page mappings, I feel like that can be even slower, because then the worst case is for each IO we'll need to vmexit twice: - The first vmexit caused by an invalidation to MAP the page tables, so vhost will setup the page table before IO starts - IO/DMA triggers and completes - The second vmexit caused by another invalidation to UNMAP the page tables So it seems to be worse than when vhost only uses UNMAP like right now. At least we only have one vmexit (when UNMAP). We'll have a vhost translate() request from kernel to userspace, but IMHO that's cheaper than the vmexit. Right but then I would still prefer to have another notifier. Since vtd_page_walk has nothing to do with device IOTLB. IOMMU have a dedicated command for flushing device IOTLB. But the check for vtd_as_has_map_notifier is used to skip the device which can do demand paging via ATS or device specific way. If we have two different notifiers, vhost will be on the device iotlb notifier so we don't need it at all? But we can still have iommu notifier that only registers to UNMAP even after we introduce dev-iotlb notifier? We don't want to do page walk for them as well. TCG should be the only one so far, but I don't know.. maybe there can still be new ones? I think you're right. But looking at the codes, it looks like the check of vtd_as_has_map_notifier() was only used in: 1) vtd_iommu_replay() 2) vtd_iotlb_page_invalidate_notify() (PSI) For the replay, it's expensive anyhow. For PSI, I think it's just about one or few mappings, not sure it will have obvious performance impact. And I had two questions: 1) The codes doesn't check map for DSI or GI, does this match what spec said? (It looks to me the spec is unclear in this part) 2) for the replay() I don't see other implementations (either spapr or generic one) that did unmap (actually they skip unmap explicitly), any reason for doing this in intel IOMMU? Thanks Thanks,
