{util => monitor}/yank.c | 0 MAINTAINERS | 2 +- monitor/meson.build | 1 + util/meson.build | 1 - 4 files changed, 2 insertions(+), 2 deletions(-) rename {util => monitor}/yank.c (100%)
util/yank.c and stubs/yank.c are both in libqemuutil.a, even though
their external symbols conflict. The linker happens to pick the
former. This links a bunch of unneeded code into the executables that
actually want the latter: qemu-io, qemu-img, qemu-nbd, and several
tests. Amazingly, none of them fails to link.
To fix this, the non-stub yank.c from sourceset util_ss to sourceset
qmp_ss. This requires moving it from util/ to monitor/.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
{util => monitor}/yank.c | 0
MAINTAINERS | 2 +-
monitor/meson.build | 1 +
util/meson.build | 1 -
4 files changed, 2 insertions(+), 2 deletions(-)
rename {util => monitor}/yank.c (100%)
diff --git a/util/yank.c b/monitor/yank.c
similarity index 100%
rename from util/yank.c
rename to monitor/yank.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5ca3c9f851..d3174c0bb0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2799,7 +2799,7 @@ F: tests/unit/test-uuid.c
Yank feature
M: Lukas Straub <lukasstraub2@web.de>
S: Odd fixes
-F: util/yank.c
+F: monitor/yank.c
F: stubs/yank.c
F: include/qemu/yank.h
F: qapi/yank.json
diff --git a/monitor/meson.build b/monitor/meson.build
index 6d00985ace..1ce5909c88 100644
--- a/monitor/meson.build
+++ b/monitor/meson.build
@@ -1,4 +1,5 @@
qmp_ss.add(files('monitor.c', 'qmp.c', 'qmp-cmds-control.c'))
+qmp_ss.add(files('yank.c'))
softmmu_ss.add(files(
'hmp-cmds.c',
diff --git a/util/meson.build b/util/meson.build
index 984fba965f..fa0298adab 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -51,7 +51,6 @@ endif
if have_system
util_ss.add(files('crc-ccitt.c'))
util_ss.add(when: 'CONFIG_GIO', if_true: [files('dbus.c'), gio])
- util_ss.add(files('yank.c'))
util_ss.add(when: 'CONFIG_LINUX', if_true: files('userfaultfd.c'))
endif
--
2.26.2
On 3/16/21 8:59 AM, Markus Armbruster wrote: > util/yank.c and stubs/yank.c are both in libqemuutil.a, even though > their external symbols conflict. The linker happens to pick the > former. This links a bunch of unneeded code into the executables that > actually want the latter: qemu-io, qemu-img, qemu-nbd, and several > tests. Amazingly, none of them fails to link. > > To fix this, the non-stub yank.c from sourceset util_ss to sourceset > qmp_ss. This requires moving it from util/ to monitor/. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > {util => monitor}/yank.c | 0 > MAINTAINERS | 2 +- > monitor/meson.build | 1 + > util/meson.build | 1 - > 4 files changed, 2 insertions(+), 2 deletions(-) > rename {util => monitor}/yank.c (100%) I'm still determining if I need an NBD pull request for soft freeze today; if so, I'm happy to include this one if it doesn't make it into the tree elsewhere first. I also consider it to be a build issue and therefore suitable for inclusion in -rc1 if it misses the boat today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes: > On 3/16/21 8:59 AM, Markus Armbruster wrote: >> util/yank.c and stubs/yank.c are both in libqemuutil.a, even though >> their external symbols conflict. The linker happens to pick the >> former. This links a bunch of unneeded code into the executables that >> actually want the latter: qemu-io, qemu-img, qemu-nbd, and several >> tests. Amazingly, none of them fails to link. >> >> To fix this, the non-stub yank.c from sourceset util_ss to sourceset >> qmp_ss. This requires moving it from util/ to monitor/. >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> {util => monitor}/yank.c | 0 >> MAINTAINERS | 2 +- >> monitor/meson.build | 1 + >> util/meson.build | 1 - >> 4 files changed, 2 insertions(+), 2 deletions(-) >> rename {util => monitor}/yank.c (100%) > > I'm still determining if I need an NBD pull request for soft freeze > today; if so, I'm happy to include this one if it doesn't make it into > the tree elsewhere first. I also consider it to be a build issue and > therefore suitable for inclusion in -rc1 if it misses the boat today. It's not urgent, just something I spotted when I looked for buddies of another bug that bit me.
On 16/03/2021 14.59, Markus Armbruster wrote: > util/yank.c and stubs/yank.c are both in libqemuutil.a, even though > their external symbols conflict. The linker happens to pick the > former. This links a bunch of unneeded code into the executables that > actually want the latter: qemu-io, qemu-img, qemu-nbd, and several > tests. Amazingly, none of them fails to link. > > To fix this, the non-stub yank.c from sourceset util_ss to sourceset > qmp_ss. This requires moving it from util/ to monitor/. In another patch ("tests: Use the normal yank code instead of stubs in relevant tests"), Lukas now changed the tests to always explicitly link against the real yank.c code. That makes me wonder whether we need the yank stubs at all ... it's not that much code after all, and it's very much self-contained without references to other files, so I think it should also be ok if we simply always keep it in the utils library and ditch the stubs? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 16/03/2021 14.59, Markus Armbruster wrote: >> util/yank.c and stubs/yank.c are both in libqemuutil.a, even though >> their external symbols conflict. The linker happens to pick the >> former. This links a bunch of unneeded code into the executables that >> actually want the latter: qemu-io, qemu-img, qemu-nbd, and several >> tests. Amazingly, none of them fails to link. >> To fix this, the non-stub yank.c from sourceset util_ss to sourceset >> qmp_ss. This requires moving it from util/ to monitor/. > > In another patch ("tests: Use the normal yank code instead of stubs in > relevant tests"), Lukas now changed the tests to always explicitly > link against the real yank.c code. That makes me wonder whether we > need the yank stubs at all ... it's not that much code after all, and > it's very much self-contained without references to other files, so I > think it should also be ok if we simply always keep it in the utils > library and ditch the stubs? Any solution that links and doesn't put duplicate symbols in .a is fine with me.
On Tue, 16 Mar 2021 14:59:07 +0100 Markus Armbruster <armbru@redhat.com> wrote: > util/yank.c and stubs/yank.c are both in libqemuutil.a, even though > their external symbols conflict. The linker happens to pick the > former. This links a bunch of unneeded code into the executables that > actually want the latter: qemu-io, qemu-img, qemu-nbd, and several > tests. Amazingly, none of them fails to link. > > To fix this, the non-stub yank.c from sourceset util_ss to sourceset > qmp_ss. This requires moving it from util/ to monitor/. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > {util => monitor}/yank.c | 0 > MAINTAINERS | 2 +- > monitor/meson.build | 1 + > util/meson.build | 1 - > 4 files changed, 2 insertions(+), 2 deletions(-) > rename {util => monitor}/yank.c (100%) > > diff --git a/util/yank.c b/monitor/yank.c > similarity index 100% > rename from util/yank.c > rename to monitor/yank.c > diff --git a/MAINTAINERS b/MAINTAINERS > index 5ca3c9f851..d3174c0bb0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2799,7 +2799,7 @@ F: tests/unit/test-uuid.c > Yank feature > M: Lukas Straub <lukasstraub2@web.de> > S: Odd fixes > -F: util/yank.c > +F: monitor/yank.c > F: stubs/yank.c > F: include/qemu/yank.h > F: qapi/yank.json > diff --git a/monitor/meson.build b/monitor/meson.build > index 6d00985ace..1ce5909c88 100644 > --- a/monitor/meson.build > +++ b/monitor/meson.build > @@ -1,4 +1,5 @@ > qmp_ss.add(files('monitor.c', 'qmp.c', 'qmp-cmds-control.c')) > +qmp_ss.add(files('yank.c')) > > softmmu_ss.add(files( > 'hmp-cmds.c', > diff --git a/util/meson.build b/util/meson.build > index 984fba965f..fa0298adab 100644 > --- a/util/meson.build > +++ b/util/meson.build > @@ -51,7 +51,6 @@ endif > if have_system > util_ss.add(files('crc-ccitt.c')) > util_ss.add(when: 'CONFIG_GIO', if_true: [files('dbus.c'), gio]) > - util_ss.add(files('yank.c')) > util_ss.add(when: 'CONFIG_LINUX', if_true: files('userfaultfd.c')) > endif > Looks good to me, applied and fixed commit message. Regards, Lukas Straub --
On 3/16/21 8:59 AM, Markus Armbruster wrote: > util/yank.c and stubs/yank.c are both in libqemuutil.a, even though > their external symbols conflict. The linker happens to pick the > former. This links a bunch of unneeded code into the executables that > actually want the latter: qemu-io, qemu-img, qemu-nbd, and several > tests. Amazingly, none of them fails to link. > > To fix this, the non-stub yank.c from sourceset util_ss to sourceset This sentence no verb. > qmp_ss. This requires moving it from util/ to monitor/. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > {util => monitor}/yank.c | 0 > MAINTAINERS | 2 +- > monitor/meson.build | 1 + > util/meson.build | 1 - > 4 files changed, 2 insertions(+), 2 deletions(-) > rename {util => monitor}/yank.c (100%) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes: > On 3/16/21 8:59 AM, Markus Armbruster wrote: >> util/yank.c and stubs/yank.c are both in libqemuutil.a, even though >> their external symbols conflict. The linker happens to pick the >> former. This links a bunch of unneeded code into the executables that >> actually want the latter: qemu-io, qemu-img, qemu-nbd, and several >> tests. Amazingly, none of them fails to link. >> >> To fix this, the non-stub yank.c from sourceset util_ss to sourceset > > This sentence no verb. To fix this, move the non-stub ... >> qmp_ss. This requires moving it from util/ to monitor/. >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> {util => monitor}/yank.c | 0 >> MAINTAINERS | 2 +- >> monitor/meson.build | 1 + >> util/meson.build | 1 - >> 4 files changed, 2 insertions(+), 2 deletions(-) >> rename {util => monitor}/yank.c (100%) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
© 2016 - 2024 Red Hat, Inc.