[PATCH] modules: load modules from versioned /var/run dir

[PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

On upgrades the old .so files usually are replaced. But on the other
hand since a qemu process represents a guest instance it is usually kept
around.

That makes late addition of dynamic features e.g. 'hot-attach of a ceph
disk' fail by trying to load a new version of e.f. block-rbd.so into an
old still running qemu binary.

This adds a fallback to also load modules from a versioned directory in the
temporary /var/run path. That way qemu is providing a way for packaging
to store modules of an upgraded qemu package as needed until the next reboot.

An example how that can then be used in packaging can be seen in:
https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU

Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 util/module.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/module.c b/util/module.c
index 236a7bb52a..d2446104be 100644
--- a/util/module.c
+++ b/util/module.c
@@ -19,6 +19,7 @@
 #endif
 #include "qemu/queue.h"
 #include "qemu/module.h"
+#include "qemu-version.h"
 
 typedef struct ModuleEntry
 {
@@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
 #ifdef CONFIG_MODULES
     char *fname = NULL;
     char *exec_dir;
+    char *version_dir;
     const char *search_dir;
     char *dirs[4];
     char *module_name;
@@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
     dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
     dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
     dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
+    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
+                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
+                             '_');
+    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+
     assert(n_dirs <= ARRAY_SIZE(dirs));
 
     g_free(exec_dir);
-- 
2.25.1

Re: [PATCH] modules: load modules from versioned /var/run dir

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200306132648.27577-1-christian.ehrhardt@canonical.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

File: "/tmp/qemu-nsis\qemu-doc.html" -> no files found.
Usage: File [/nonfatal] [/a] ([/r] [/x filespec [...]] filespec [...] |
   /oname=outfile one_file_only)
Error in script "/tmp/qemu-test/src/qemu.nsi" on line 180 -- aborting creation process
make: *** [Makefile:1162: qemu-setup-4.2.50.exe] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zbg82xrb/src/docker-src.2020-03-06-09.38.41.1852:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zbg82xrb/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m8.711s
user    0m7.969s


The full log is available at
http://patchew.org/logs/20200306132648.27577-1-christian.ehrhardt@canonical.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

On Fri, Mar 6, 2020 at 3:42 PM <no-reply@patchew.org> wrote:

> Patchew URL:
> https://patchew.org/QEMU/20200306132648.27577-1-christian.ehrhardt@canonical.com/
>
>
>
> Hi,
>
> This series failed the docker-mingw@fedora build test. Please find the
> testing commands and
> their output below. If you have Docker installed, you can probably
> reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #! /bin/bash
> export ARCH=x86_64
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-mingw@fedora J=14 NETWORK=1
> === TEST SCRIPT END ===
>
> File: "/tmp/qemu-nsis\qemu-doc.html" -> no files found.
> Usage: File [/nonfatal] [/a] ([/r] [/x filespec [...]] filespec [...] |
>    /oname=outfile one_file_only)
> Error in script "/tmp/qemu-test/src/qemu.nsi" on line 180 -- aborting
> creation process
> make: *** [Makefile:1162: qemu-setup-4.2.50.exe] Error 1
> Traceback (most recent call last):
>   File "./tests/docker/docker.py", line 664, in <module>
>     sys.exit(main())
> ---
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run',
> '--label', 'com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1', '-u',
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e',
> 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14',
> '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache',
> '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v',
> '/var/tmp/patchew-tester-tmp-zbg82xrb/src/docker-src.2020-03-06-09.38.41.1852:/var/tmp/qemu:z,ro',
> 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit
> status 2.
>
> filter=--filter=label=com.qemu.instance.uuid=d78ae1bf94024f7c8a0255eb0d63c8f1
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zbg82xrb/src'
> make: *** [docker-run-test-mingw@fedora] Error 2
>
> real    3m8.711s
> user    0m7.969s
>
>
> The full log is available at
>
> http://patchew.org/logs/20200306132648.27577-1-christian.ehrhardt@canonical.com/testing.docker-mingw@fedora/?type=message
> .
>

This is a false-positive - at least not due to my proposed patch.
I've not even remotely touched qemu-doc.

The test build even runs with config:
  module support    no
Which even means all my changes except the include are removed by the
preprocessor.

Do I need to do anything to reset the state of this patch to not be marked
as failed?


> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 4140 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> On upgrades the old .so files usually are replaced. But on the other
> hand since a qemu process represents a guest instance it is usually kept
> around.
> 
> That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> disk' fail by trying to load a new version of e.f. block-rbd.so into an
> old still running qemu binary.
> 
> This adds a fallback to also load modules from a versioned directory in the
> temporary /var/run path. That way qemu is providing a way for packaging
> to store modules of an upgraded qemu package as needed until the next reboot.
> 
> An example how that can then be used in packaging can be seen in:
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> 
> Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  util/module.c | 7 +++++++
>  1 file changed, 7 insertions(+)

CCing Debian, Fedora, and Red Hat package maintainers in case they have
comments.

The use of /var/run makes me a little uneasy.  I guess it's related to
wanting to uninstall the old package so the .so in their original
location cannot be used (even if they had a versioned path)?

I'm not a package maintainer though so I hope the others will make
suggestions if there are other solutions :).

> 
> diff --git a/util/module.c b/util/module.c
> index 236a7bb52a..d2446104be 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -19,6 +19,7 @@
>  #endif
>  #include "qemu/queue.h"
>  #include "qemu/module.h"
> +#include "qemu-version.h"
>  
>  typedef struct ModuleEntry
>  {
> @@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
>  #ifdef CONFIG_MODULES
>      char *fname = NULL;
>      char *exec_dir;
> +    char *version_dir;
>      const char *search_dir;
>      char *dirs[4];
>      char *module_name;
> @@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
>      dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
>      dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
>      dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
> +    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
> +                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
> +                             '_');
> +    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
> +
>      assert(n_dirs <= ARRAY_SIZE(dirs));
>  
>      g_free(exec_dir);
> -- 
> 2.25.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

On Tue, Mar 10, 2020 at 10:39 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > On upgrades the old .so files usually are replaced. But on the other
> > hand since a qemu process represents a guest instance it is usually kept
> > around.
> >
> > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > old still running qemu binary.
> >
> > This adds a fallback to also load modules from a versioned directory in
> the
> > temporary /var/run path. That way qemu is providing a way for packaging
> > to store modules of an upgraded qemu package as needed until the next
> reboot.
> >
> > An example how that can then be used in packaging can be seen in:
> >
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  util/module.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
>
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
>

Yeah that seems worth, thanks Stefan.

The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?
>

Yes you'd want to uninstall the old bits from disk - even if there would be
a versioned path.
/var/run was considered a nice place as it is auto-cleaned on a reboot
avoiding a massive
and most likely broken logic trying to detect which qemu versions still
have running binaries.

And no distro will have so many qemu updates that N*~<100k for the .so
files will really grow too big.

Also if the path would end up to be the major concern and we can't agree
on a single one we can consider making this:
1. not checking an extra path if not configured
2. on configure packaging can set a path of their choice

I have not done so yet as I was hoping for a simpler patch and
everyone knowing what to expect across e.g. distros.
It also will make isolation easier for example I also have apparmor changes
for libvirt allowing that path which is more complex if it ends up
configurable.

And finally this has to be considered an "offer" by qemu to the packagers
to fix a real field issue.
The packaging does not "have to" exploit this, every Distro is free to just
ignore it.

I'm not a package maintainer though so I hope the others will make
> suggestions if there are other solutions :).
>
> >
> > diff --git a/util/module.c b/util/module.c
> > index 236a7bb52a..d2446104be 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -19,6 +19,7 @@
> >  #endif
> >  #include "qemu/queue.h"
> >  #include "qemu/module.h"
> > +#include "qemu-version.h"
> >
> >  typedef struct ModuleEntry
> >  {
> > @@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char
> *lib_name)
> >  #ifdef CONFIG_MODULES
> >      char *fname = NULL;
> >      char *exec_dir;
> > +    char *version_dir;
> >      const char *search_dir;
> >      char *dirs[4];
> >      char *module_name;
> > @@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char
> *lib_name)
> >      dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
> >      dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
> >      dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
> > +    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
> > +                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS
> "+-.~",
> > +                             '_');
> > +    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
> > +
> >      assert(n_dirs <= ARRAY_SIZE(dirs));
> >
> >      g_free(exec_dir);
> > --
> > 2.25.1
> >
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 5837 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Daniel P. Berrangé]

On Tue, Mar 10, 2020 at 09:39:10AM +0000, Stefan Hajnoczi wrote:
> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > On upgrades the old .so files usually are replaced. But on the other
> > hand since a qemu process represents a guest instance it is usually kept
> > around.
> > 
> > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > old still running qemu binary.
> > 
> > This adds a fallback to also load modules from a versioned directory in the
> > temporary /var/run path. That way qemu is providing a way for packaging
> > to store modules of an upgraded qemu package as needed until the next reboot.
> > 
> > An example how that can then be used in packaging can be seen in:
> > https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> > 
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  util/module.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> 
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
> 
> The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?
> 
> I'm not a package maintainer though so I hope the others will make
> suggestions if there are other solutions :).

My first concern is that this only partially solves the quoted problem.

Consider the QEMU RBD module which is

   /usr/lib64/qemu/block-rbd.so

This links to

   /usr/lib64/librbd.so.1

On host OS upgrade, just before uninstalling old QEMU we copy RBD
module into:

   /var/run/qemu-$VER/block-rbd.so

....but the host OS upgrade also upgrades RBD itself to a new
major version which ships

   /usr/lib64/librbd.so.2

We've got /var/run/qemu-$VER/block-rbd.so saved, but we didn't
transitively save all the things that this linked to, so there's
no guarantee it will still be possible to load it.

IOW, this approach of saving QEMU block modules to a scratch dir
works, *provided* everything else that this QEMU block module needs
still exists in a compatible form.

Some distros may have a policy that no incompatible soname bumps
are permitted in updates, but that's not universal.

On the other hand this is not a big amount of new code, so is not
a huge maint problem even if only a few people ever use it. I would
be a bit more comfortable if this search path addition was perhaps
enabled by an opt-in configure argument, rather than being always
present.


I'm also uneasy about the idea of using a search path ordering for
doing this.  This means that an old running QEMU is always going
to first try to load the block-rbd.so from the new QEMU, expect
to see that fail, and only then fallback to load the block-rbd.so
that actually matches its build.

IIRC, we have an embedded build-id in the modules that should
guarantee that the first load attempt always fails, but I'm still
uneasy about that at a conceptual level.

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] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]

On Tue, Mar 10, 2020 at 1:10 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Mar 10, 2020 at 09:39:10AM +0000, Stefan Hajnoczi wrote:
> > On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> > > On upgrades the old .so files usually are replaced. But on the other
> > > hand since a qemu process represents a guest instance it is usually
> kept
> > > around.
> > >
> > > That makes late addition of dynamic features e.g. 'hot-attach of a ceph
> > > disk' fail by trying to load a new version of e.f. block-rbd.so into an
> > > old still running qemu binary.
> > >
> > > This adds a fallback to also load modules from a versioned directory
> in the
> > > temporary /var/run path. That way qemu is providing a way for packaging
> > > to store modules of an upgraded qemu package as needed until the next
> reboot.
> > >
> > > An example how that can then be used in packaging can be seen in:
> > >
> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
> > >
> > > Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > ---
> > >  util/module.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> >
> > CCing Debian, Fedora, and Red Hat package maintainers in case they have
> > comments.
> >
> > The use of /var/run makes me a little uneasy.  I guess it's related to
> > wanting to uninstall the old package so the .so in their original
> > location cannot be used (even if they had a versioned path)?
> >
> > I'm not a package maintainer though so I hope the others will make
> > suggestions if there are other solutions :).
>
> My first concern is that this only partially solves the quoted problem.
>
> Consider the QEMU RBD module which is
>
>    /usr/lib64/qemu/block-rbd.so
>
> This links to
>
>    /usr/lib64/librbd.so.1
>
> On host OS upgrade, just before uninstalling old QEMU we copy RBD
> module into:
>
>    /var/run/qemu-$VER/block-rbd.so
>
> ....but the host OS upgrade also upgrades RBD itself to a new
> major version which ships
>
>    /usr/lib64/librbd.so.2



We've got /var/run/qemu-$VER/block-rbd.so saved, but we didn't
> transitively save all the things that this linked to, so there's
> no guarantee it will still be possible to load it.
>
> IOW, this approach of saving QEMU block modules to a scratch dir
> works, *provided* everything else that this QEMU block module needs
> still exists in a compatible form.
>
> Some distros may have a policy that no incompatible soname bumps
> are permitted in updates, but that's not universal.
>

Hi Daniel,
Yeah for "us" being Ubuntu that would certainly be true.
An upgrade of librbd.so.1 to librbd.so.2 would only happen at a major
upgrade.
For example in Ubuntu terms e.g. 18.04 to 20.04. Those upgrades usually
come with a requirement to reboot anyway - I'm not trying to solve these.

The much more common and frequent small updates to qemu with individual
fixes are what triggers this bug and what my proposal would solve.

On the other hand this is not a big amount of new code, so is not
> a huge maint problem even if only a few people ever use it. I would
> be a bit more comfortable if this search path addition was perhaps
> enabled by an opt-in configure argument, rather than being always
> present.
>

I already suggested opt-in configure before.
I'd keep this just enable/disable unless we really can't agree on a path.
That should be no big issue to add this in v2.

I'm also uneasy about the idea of using a search path ordering for
> doing this.  This means that an old running QEMU is always going
> to first try to load the block-rbd.so from the new QEMU, expect
> to see that fail, and only then fallback to load the block-rbd.so
> that actually matches its build.
>
> IIRC, we have an embedded build-id in the modules that should
> guarantee that the first load attempt always fails, but I'm still

uneasy about that at a conceptual level.
>

Yes that build-id is what prevents it loading the modules of the new
package.

The intention was to keep this change small and not a burden for anyone.
I'm not yet having an idea for a conceptual change that would avoid to rely
on the build-id AND at the same time not make this a huge patch with
probably many new issues we'll find out only much later.

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 :|
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 7485 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Michael Tokarev]

10.03.2020 12:39, Stefan Hajnoczi wrote:
> On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
>> On upgrades the old .so files usually are replaced. But on the other
>> hand since a qemu process represents a guest instance it is usually kept
>> around.
>>
>> That makes late addition of dynamic features e.g. 'hot-attach of a ceph
>> disk' fail by trying to load a new version of e.f. block-rbd.so into an
>> old still running qemu binary.
>>
>> This adds a fallback to also load modules from a versioned directory in the
>> temporary /var/run path. That way qemu is providing a way for packaging
>> to store modules of an upgraded qemu package as needed until the next reboot.
>>
>> An example how that can then be used in packaging can be seen in:
>> https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU
>>
>> Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
>> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
>> ---
>>  util/module.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
> 
> CCing Debian, Fedora, and Red Hat package maintainers in case they have
> comments.
> 
> The use of /var/run makes me a little uneasy.  I guess it's related to
> wanting to uninstall the old package so the .so in their original
> location cannot be used (even if they had a versioned path)?

BTW, this is /run nowadays, not /var/run, as far as I can see.

/mjt

Re: [PATCH] modules: load modules from versioned /var/run dir

[Paolo Bonzini]

On 13/03/20 08:34, Michael Tokarev wrote:
>> The use of /var/run makes me a little uneasy.  I guess it's related to
>> wanting to uninstall the old package so the .so in their original
>> location cannot be used (even if they had a versioned path)?
> 
> BTW, this is /run nowadays, not /var/run, as far as I can see.

/var/run is still symlinked to /run.  QEMU generally uses /var/run,
though we could consider switching sooner or later.

Paolo

Re: [PATCH] modules: load modules from versioned /var/run dir

[Daniel P. Berrangé]

On Fri, Mar 13, 2020 at 08:45:21AM +0100, Paolo Bonzini wrote:
> On 13/03/20 08:34, Michael Tokarev wrote:
> >> The use of /var/run makes me a little uneasy.  I guess it's related to
> >> wanting to uninstall the old package so the .so in their original
> >> location cannot be used (even if they had a versioned path)?
> > 
> > BTW, this is /run nowadays, not /var/run, as far as I can see.
> 
> /var/run is still symlinked to /run.  QEMU generally uses /var/run,
> though we could consider switching sooner or later.

Only Linux commonly uses /run, others still use /var/run. 

We really only need a "configure --rundir=/run"  arg to override the
default, in the same way distros pick /etc and /var, instead of
/usr/etc and /usr/var.

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] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Tue, Mar 10, 2020 at 12:47:49PM +0100, Christian Ehrhardt wrote:
> On Tue, Mar 10, 2020 at 10:39 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Fri, Mar 06, 2020 at 02:26:48PM +0100, Christian Ehrhardt wrote:
> And finally this has to be considered an "offer" by qemu to the packagers
> to fix a real field issue.
> The packaging does not "have to" exploit this, every Distro is free to just
> ignore it.

I understand.  My intention is just to draw the attention of the right
people so that other distros are aware of the problem and improvements
can be discussed.

From my own perspective it seems fine to merge this or a similar patch
into qemu.git.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On Fri, Mar 6, 2020 at 11:54 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Mar 04, 2020 at 10:39:46AM +0100, Christian Ehrhardt wrote:
>
> Please start a new email thread.  Patches sent as replies to existing
> email threads are easily missed by humans and tooling also doesn't
> recognize them.
>

Sure, thanks Stefan for the hint about how that will be processed/looked at
by maintainers and reviewers.

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

[-- Attachment #2: Type: text/html, Size: 900 bytes --]

Re: [PATCH] modules: load modules from versioned /var/run dir

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

On Wed, Mar 04, 2020 at 10:39:46AM +0100, Christian Ehrhardt wrote:

Please start a new email thread.  Patches sent as replies to existing
email threads are easily missed by humans and tooling also doesn't
recognize them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] modules: load modules from versioned /var/run dir

[Christian Ehrhardt]

On upgrades the old .so files usually are replaced. But on the other
hand since a qemu process represents a guest instance it is usually kept
around.

That makes late addition of dynamic features e.g. 'hot-attach of a ceph
disk' fail by trying to load a new version of e.f. block-rbd.so into an
old still running qemu binary.

This adds a fallback to also load modules from a versioned directory in the
temporary /var/run path. That way qemu is providing a way for packaging
to store modules of an upgraded qemu package as needed until the next reboot.

An example how that can then be used in packaging can be seen in:
https://git.launchpad.net/~paelzer/ubuntu/+source/qemu/log/?h=bug-1847361-miss-old-so-on-upgrade-UBUNTU

Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1847361
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 util/module.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/util/module.c b/util/module.c
index 236a7bb52a..d2446104be 100644
--- a/util/module.c
+++ b/util/module.c
@@ -19,6 +19,7 @@
 #endif
 #include "qemu/queue.h"
 #include "qemu/module.h"
+#include "qemu-version.h"
 
 typedef struct ModuleEntry
 {
@@ -170,6 +171,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
 #ifdef CONFIG_MODULES
     char *fname = NULL;
     char *exec_dir;
+    char *version_dir;
     const char *search_dir;
     char *dirs[4];
     char *module_name;
@@ -201,6 +203,11 @@ bool module_load_one(const char *prefix, const char *lib_name)
     dirs[n_dirs++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
     dirs[n_dirs++] = g_strdup_printf("%s/..", exec_dir ? : "");
     dirs[n_dirs++] = g_strdup_printf("%s", exec_dir ? : "");
+    version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
+                             G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
+                             '_');
+    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+
     assert(n_dirs <= ARRAY_SIZE(dirs));
 
     g_free(exec_dir);
-- 
2.25.1