| Message ID | 20210316135907.3646901-1-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | yank: Avoid linking into executables that don't want it | expand |
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>
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 <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!
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 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 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.
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
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%)