[PATCH] yank: Avoid linking into executables that don't want it

[PATCH] yank: Avoid linking into executables that don't want it

[Markus Armbruster]

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

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Eric Blake]

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

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Eric Blake]

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

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Markus Armbruster]

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!

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Markus Armbruster]

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.

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Lukas Straub]

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

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

-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Thomas Huth]

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

Re: [PATCH] yank: Avoid linking into executables that don't want it

[Markus Armbruster]

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.