[RFC] qemu: Check for changes in qemu modules directory
Add a configuration option for specifying location of the qemu modules directory, defaulting to /usr/lib64/qemu. Then use this location to check for changes in the directory, indicating that a qemu module has changed and capabilities need to be reprobed. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- This is a mildly tested impl of the idea discussed here (and elsewhere in the thread) https://www.redhat.com/archives/libvir-list/2020-August/msg00684.html I've checked that both system and session caches are updated when a qemu modules package is updated. I'll do some more testing but wanted to send the patch for early comments. Testing in other environments would be much appreciated. Thanks! meson.build | 6 ++++++ meson_options.txt | 1 + src/qemu/qemu_capabilities.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/meson.build b/meson.build index a72d0c0e85..16928482aa 100644 --- a/meson.build +++ b/meson.build @@ -1745,6 +1745,12 @@ if not get_option('driver_qemu').disabled() if use_qemu conf.set('WITH_QEMU', 1) + qemu_moddir = get_option('qemu_moddir') + if qemu_moddir == '' + qemu_moddir = '/usr/lib64/qemu' + endif + conf.set_quoted('QEMU_MODDIR', qemu_moddir) + if host_machine.system() in ['freebsd', 'darwin'] default_qemu_user = 'root' default_qemu_group = 'wheel' diff --git a/meson_options.txt b/meson_options.txt index c538d323c1..cedbd79491 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -59,6 +59,7 @@ option('driver_openvz', type: 'feature', value: 'auto', description: 'OpenVZ dri option('driver_qemu', type: 'feature', value: 'auto', description: 'QEMU/KVM driver') option('qemu_user', type: 'string', value: '', description: 'username to run QEMU system instance as') option('qemu_group', type: 'string', value: '', description: 'groupname to run QEMU system instance as') +option('qemu_moddir', type: 'string', value: '', description: 'set the directory where QEMU modules are located') option('driver_remote', type: 'feature', value: 'enabled', description: 'remote driver') option('remote_default_mode', type: 'combo', choices: ['legacy', 'direct'], value: 'legacy', description: 'remote driver default mode') option('driver_secrets', type: 'feature', value: 'auto', description: 'local secrets management driver') diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ff6ba8c9e9..a00853c753 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -677,6 +677,7 @@ struct _virQEMUCaps { char *binary; time_t ctime; time_t libvirtCtime; + time_t modDirMtime; bool invalidation; virBitmapPtr flags; @@ -4194,6 +4195,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) * <qemuCaps> * <emulator>/some/path</emulator> * <qemuctime>234235253</qemuctime> + * <qemumoddirmtime>234235253</qemumoddirmtime> * <selfctime>234235253</selfctime> * <selfvers>1002016</selfvers> * <flag name='foo'/> @@ -4283,6 +4285,9 @@ virQEMUCapsLoadCache(virArch hostArch, } qemuCaps->ctime = (time_t)l; + if (virXPathLongLong("string(./qemumoddirmtime)", ctxt, &l) == 0) + qemuCaps->modDirMtime = (time_t)l; + if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities flags")); @@ -4615,6 +4620,10 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) qemuCaps->binary); virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); + if (qemuCaps->modDirMtime > 0) { + virBufferAsprintf(&buf, "<qemumoddirmtime>%llu</qemumoddirmtime>\n", + (long long)qemuCaps->modDirMtime); + } virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", (long long)qemuCaps->libvirtCtime); virBufferAsprintf(&buf, "<selfvers>%lu</selfvers>\n", @@ -4881,6 +4890,23 @@ virQEMUCapsIsValid(void *data, if (!qemuCaps->binary) return true; + if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + VIR_DEBUG("Failed to stat QEMU module directory '%s': %s", + QEMU_MODDIR, + g_strerror(errno)); + return false; + } + + if (sb.st_mtime != qemuCaps->modDirMtime) { + VIR_DEBUG("Outdated capabilities for '%s': QEMU modules " + "directory '%s' changed (%lld vs %lld)", + qemuCaps->binary, QEMU_MODDIR, + (long long)sb.st_mtime, (long long)qemuCaps->modDirMtime); + return false; + } + } + if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " @@ -5463,6 +5489,15 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } + if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + virReportSystemError(errno, _("Cannot check QEMU module directory %s"), + QEMU_MODDIR); + goto error; + } + qemuCaps->modDirMtime = sb.st_mtime; + } + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) goto error; -- 2.28.0
Hi All, Any comments on this RFC patch? Is this something we would like to see in the 6.7.0 release? On 8/20/20 5:03 PM, Jim Fehlig wrote:
Add a configuration option for specifying location of the qemu modules directory, defaulting to /usr/lib64/qemu. Then use this location to check for changes in the directory, indicating that a qemu module has changed and capabilities need to be reprobed.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is a mildly tested impl of the idea discussed here (and elsewhere in the thread)
https://www.redhat.com/archives/libvir-list/2020-August/msg00684.html
I've checked that both system and session caches are updated when a qemu modules package is updated. I'll do some more testing but wanted to send the patch for early comments. Testing in other environments would be much appreciated. Thanks!
I've tried some more configurations and haven't noticed any problems with the patch thus far. The request for additional eyeballs and testing still valid though :-). Regards, Jim
meson.build | 6 ++++++ meson_options.txt | 1 + src/qemu/qemu_capabilities.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/meson.build b/meson.build index a72d0c0e85..16928482aa 100644 --- a/meson.build +++ b/meson.build @@ -1745,6 +1745,12 @@ if not get_option('driver_qemu').disabled() if use_qemu conf.set('WITH_QEMU', 1)
+ qemu_moddir = get_option('qemu_moddir') + if qemu_moddir == '' + qemu_moddir = '/usr/lib64/qemu' + endif + conf.set_quoted('QEMU_MODDIR', qemu_moddir) + if host_machine.system() in ['freebsd', 'darwin'] default_qemu_user = 'root' default_qemu_group = 'wheel' diff --git a/meson_options.txt b/meson_options.txt index c538d323c1..cedbd79491 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -59,6 +59,7 @@ option('driver_openvz', type: 'feature', value: 'auto', description: 'OpenVZ dri option('driver_qemu', type: 'feature', value: 'auto', description: 'QEMU/KVM driver') option('qemu_user', type: 'string', value: '', description: 'username to run QEMU system instance as') option('qemu_group', type: 'string', value: '', description: 'groupname to run QEMU system instance as') +option('qemu_moddir', type: 'string', value: '', description: 'set the directory where QEMU modules are located') option('driver_remote', type: 'feature', value: 'enabled', description: 'remote driver') option('remote_default_mode', type: 'combo', choices: ['legacy', 'direct'], value: 'legacy', description: 'remote driver default mode') option('driver_secrets', type: 'feature', value: 'auto', description: 'local secrets management driver') diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ff6ba8c9e9..a00853c753 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -677,6 +677,7 @@ struct _virQEMUCaps { char *binary; time_t ctime; time_t libvirtCtime; + time_t modDirMtime; bool invalidation;
virBitmapPtr flags; @@ -4194,6 +4195,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) * <qemuCaps> * <emulator>/some/path</emulator> * <qemuctime>234235253</qemuctime> + * <qemumoddirmtime>234235253</qemumoddirmtime> * <selfctime>234235253</selfctime> * <selfvers>1002016</selfvers> * <flag name='foo'/> @@ -4283,6 +4285,9 @@ virQEMUCapsLoadCache(virArch hostArch, } qemuCaps->ctime = (time_t)l;
+ if (virXPathLongLong("string(./qemumoddirmtime)", ctxt, &l) == 0) + qemuCaps->modDirMtime = (time_t)l; + if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities flags")); @@ -4615,6 +4620,10 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) qemuCaps->binary); virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); + if (qemuCaps->modDirMtime > 0) { + virBufferAsprintf(&buf, "<qemumoddirmtime>%llu</qemumoddirmtime>\n", + (long long)qemuCaps->modDirMtime); + } virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", (long long)qemuCaps->libvirtCtime); virBufferAsprintf(&buf, "<selfvers>%lu</selfvers>\n", @@ -4881,6 +4890,23 @@ virQEMUCapsIsValid(void *data, if (!qemuCaps->binary) return true;
+ if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + VIR_DEBUG("Failed to stat QEMU module directory '%s': %s", + QEMU_MODDIR, + g_strerror(errno)); + return false; + } + + if (sb.st_mtime != qemuCaps->modDirMtime) { + VIR_DEBUG("Outdated capabilities for '%s': QEMU modules " + "directory '%s' changed (%lld vs %lld)", + qemuCaps->binary, QEMU_MODDIR, + (long long)sb.st_mtime, (long long)qemuCaps->modDirMtime); + return false; + } + } + if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " @@ -5463,6 +5489,15 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; }
+ if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + virReportSystemError(errno, _("Cannot check QEMU module directory %s"), + QEMU_MODDIR); + goto error; + } + qemuCaps->modDirMtime = sb.st_mtime; + } + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) goto error;
On Tue, Aug 25, 2020 at 11:23:13AM -0600, Jim Fehlig wrote:
Hi All,
Any comments on this RFC patch? Is this something we would like to see in the 6.7.0 release?
Not done a full review yet, but it looks reasonable as an approach and the code isn't too complex.
On 8/20/20 5:03 PM, Jim Fehlig wrote:
Add a configuration option for specifying location of the qemu modules directory, defaulting to /usr/lib64/qemu. Then use this location to check for changes in the directory, indicating that a qemu module has changed and capabilities need to be reprobed.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is a mildly tested impl of the idea discussed here (and elsewhere in the thread)
https://www.redhat.com/archives/libvir-list/2020-August/msg00684.html
I've checked that both system and session caches are updated when a qemu modules package is updated. I'll do some more testing but wanted to send the patch for early comments. Testing in other environments would be much appreciated. Thanks!
I've tried some more configurations and haven't noticed any problems with the patch thus far. The request for additional eyeballs and testing still valid though :-).
Regards, Jim
meson.build | 6 ++++++ meson_options.txt | 1 + src/qemu/qemu_capabilities.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/meson.build b/meson.build index a72d0c0e85..16928482aa 100644 --- a/meson.build +++ b/meson.build @@ -1745,6 +1745,12 @@ if not get_option('driver_qemu').disabled() if use_qemu conf.set('WITH_QEMU', 1) + qemu_moddir = get_option('qemu_moddir') + if qemu_moddir == '' + qemu_moddir = '/usr/lib64/qemu' + endif + conf.set_quoted('QEMU_MODDIR', qemu_moddir) + if host_machine.system() in ['freebsd', 'darwin'] default_qemu_user = 'root' default_qemu_group = 'wheel' diff --git a/meson_options.txt b/meson_options.txt index c538d323c1..cedbd79491 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -59,6 +59,7 @@ option('driver_openvz', type: 'feature', value: 'auto', description: 'OpenVZ dri option('driver_qemu', type: 'feature', value: 'auto', description: 'QEMU/KVM driver') option('qemu_user', type: 'string', value: '', description: 'username to run QEMU system instance as') option('qemu_group', type: 'string', value: '', description: 'groupname to run QEMU system instance as') +option('qemu_moddir', type: 'string', value: '', description: 'set the directory where QEMU modules are located') option('driver_remote', type: 'feature', value: 'enabled', description: 'remote driver') option('remote_default_mode', type: 'combo', choices: ['legacy', 'direct'], value: 'legacy', description: 'remote driver default mode') option('driver_secrets', type: 'feature', value: 'auto', description: 'local secrets management driver') diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ff6ba8c9e9..a00853c753 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -677,6 +677,7 @@ struct _virQEMUCaps { char *binary; time_t ctime; time_t libvirtCtime; + time_t modDirMtime; bool invalidation; virBitmapPtr flags; @@ -4194,6 +4195,7 @@ virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) * <qemuCaps> * <emulator>/some/path</emulator> * <qemuctime>234235253</qemuctime> + * <qemumoddirmtime>234235253</qemumoddirmtime> * <selfctime>234235253</selfctime> * <selfvers>1002016</selfvers> * <flag name='foo'/> @@ -4283,6 +4285,9 @@ virQEMUCapsLoadCache(virArch hostArch, } qemuCaps->ctime = (time_t)l; + if (virXPathLongLong("string(./qemumoddirmtime)", ctxt, &l) == 0) + qemuCaps->modDirMtime = (time_t)l; + if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities flags")); @@ -4615,6 +4620,10 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) qemuCaps->binary); virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); + if (qemuCaps->modDirMtime > 0) { + virBufferAsprintf(&buf, "<qemumoddirmtime>%llu</qemumoddirmtime>\n", + (long long)qemuCaps->modDirMtime); + } virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", (long long)qemuCaps->libvirtCtime); virBufferAsprintf(&buf, "<selfvers>%lu</selfvers>\n", @@ -4881,6 +4890,23 @@ virQEMUCapsIsValid(void *data, if (!qemuCaps->binary) return true; + if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + VIR_DEBUG("Failed to stat QEMU module directory '%s': %s", + QEMU_MODDIR, + g_strerror(errno)); + return false; + } + + if (sb.st_mtime != qemuCaps->modDirMtime) { + VIR_DEBUG("Outdated capabilities for '%s': QEMU modules " + "directory '%s' changed (%lld vs %lld)", + qemuCaps->binary, QEMU_MODDIR, + (long long)sb.st_mtime, (long long)qemuCaps->modDirMtime); + return false; + } + } + if (qemuCaps->libvirtCtime != virGetSelfLastChanged() || qemuCaps->libvirtVersion != LIBVIR_VERSION_NUMBER) { VIR_DEBUG("Outdated capabilities for '%s': libvirt changed " @@ -5463,6 +5489,15 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, goto error; } + if (virFileExists(QEMU_MODDIR)) { + if (stat(QEMU_MODDIR, &sb) < 0) { + virReportSystemError(errno, _("Cannot check QEMU module directory %s"), + QEMU_MODDIR); + goto error; + } + qemuCaps->modDirMtime = sb.st_mtime; + } + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) goto error;
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Aug 20, 2020 at 05:03:44PM -0600, Jim Fehlig wrote:
Add a configuration option for specifying location of the qemu modules directory, defaulting to /usr/lib64/qemu. Then use this location to check for changes in the directory, indicating that a qemu module has changed and capabilities need to be reprobed.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is a mildly tested impl of the idea discussed here (and elsewhere in the thread)
https://www.redhat.com/archives/libvir-list/2020-August/msg00684.html
I've checked that both system and session caches are updated when a qemu modules package is updated. I'll do some more testing but wanted to send the patch for early comments. Testing in other environments would be much appreciated. Thanks!
meson.build | 6 ++++++ meson_options.txt | 1 + src/qemu/qemu_capabilities.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/meson.build b/meson.build index a72d0c0e85..16928482aa 100644 --- a/meson.build +++ b/meson.build @@ -1745,6 +1745,12 @@ if not get_option('driver_qemu').disabled() if use_qemu conf.set('WITH_QEMU', 1)
+ qemu_moddir = get_option('qemu_moddir') + if qemu_moddir == '' + qemu_moddir = '/usr/lib64/qemu'
We sholdn't hardcode lib64 - use libdir variable, eg "/usr" / libdir / "qemu"
+ endif + conf.set_quoted('QEMU_MODDIR', qemu_moddir) +
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 8/26/20 3:31 AM, Daniel P. Berrangé wrote:
On Thu, Aug 20, 2020 at 05:03:44PM -0600, Jim Fehlig wrote:
Add a configuration option for specifying location of the qemu modules directory, defaulting to /usr/lib64/qemu. Then use this location to check for changes in the directory, indicating that a qemu module has changed and capabilities need to be reprobed.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
This is a mildly tested impl of the idea discussed here (and elsewhere in the thread)
https://www.redhat.com/archives/libvir-list/2020-August/msg00684.html
I've checked that both system and session caches are updated when a qemu modules package is updated. I'll do some more testing but wanted to send the patch for early comments. Testing in other environments would be much appreciated. Thanks!
meson.build | 6 ++++++ meson_options.txt | 1 + src/qemu/qemu_capabilities.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)
diff --git a/meson.build b/meson.build index a72d0c0e85..16928482aa 100644 --- a/meson.build +++ b/meson.build @@ -1745,6 +1745,12 @@ if not get_option('driver_qemu').disabled() if use_qemu conf.set('WITH_QEMU', 1)
+ qemu_moddir = get_option('qemu_moddir') + if qemu_moddir == '' + qemu_moddir = '/usr/lib64/qemu'
We sholdn't hardcode lib64 - use libdir variable, eg
"/usr" / libdir / "qemu"
Good point. I've squashed in the below diff. While testing it I remembered to also test specifying the option, e.g. meson configure -Dqemu_moddir=/var/run/qemu/modules diff --git a/meson.build b/meson.build index 928f6a0bad..70a33cf5e4 100644 --- a/meson.build +++ b/meson.build @@ -1760,7 +1760,7 @@ if not get_option('driver_qemu').disabled() qemu_moddir = get_option('qemu_moddir') if qemu_moddir == '' - qemu_moddir = '/usr/lib64/qemu' + qemu_moddir = '/usr' / libdir / 'qemu' endif conf.set_quoted('QEMU_MODDIR', qemu_moddir)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks. I see we've entered freeze for 6.7.0. I'll wait until after the release before pushing this and the xen PCI permissive patches you reviewed. Regards, Jim
participants (2)
-
Daniel P. Berrangé -
Jim Fehlig