| Message ID | 20200306132648.27577-1-christian.ehrhardt@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | modules: load modules from versioned /var/run dir | expand |
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
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
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 > >
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 > > > > >
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
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 :| > >
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
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
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
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
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);
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(+)