* util/async-teardown.c: is it really needed for --disable-system build?
@ 2023-08-12 9:38 Michael Tokarev
2023-08-12 9:48 ` Michael Tokarev
0 siblings, 1 reply; 6+ messages in thread
From: Michael Tokarev @ 2023-08-12 9:38 UTC (permalink / raw)
To: QEMU Developers, Claudio Imbrenda; +Cc: Paolo Bonzini
Hi!
On Debian we're building qemu tools (eg qemu-img &Co) on systems which don't
support qemu-system. And after commit c891c24b1a "os-posix: asynchronous
teardown for shutdown on Linux", this fails on certain architectures too,
notable on ia64. This is because ia64 does not have the "traditional"
linux clone system call, it has clone2 with different stack setup.
But I wonder why this file is being compiled to begin with, when --disable-system
is specified?
os-posix.c (which calls init_async_teardown() from os_parse_cmd_args()) is marked
as part of blockdev_ss, not just system_ss.
qemu-nbd calls os_setup_early_signal_handling() from os-posix.c. This is, I guess,
why os-posix.c is part of blockdev_ss.
qemu-storage-daemon uses: os_set_daemonize(), os_setup_signal_handling(), os_setup_post().
And util/async-teardown.c is always built on linux.
It smells like, at the very least, os-posix.c should be split. We shouldn't include
a ton of qemu-system functionality (like very specific option parsing) into qemu-nbd
for example.
How about splitting os-posix.c into a few files in util/ (not in the root dir), and
adding them to util_ss in case of posix-os? Ditto for os-win32.c, I guess, but I
haven't looked at this.
And for the question in $subj, this one needs to be guarded by CONFIG_SOFTMMU.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: util/async-teardown.c: is it really needed for --disable-system build?
2023-08-12 9:38 util/async-teardown.c: is it really needed for --disable-system build? Michael Tokarev
@ 2023-08-12 9:48 ` Michael Tokarev
2023-08-14 7:01 ` Claudio Imbrenda
0 siblings, 1 reply; 6+ messages in thread
From: Michael Tokarev @ 2023-08-12 9:48 UTC (permalink / raw)
To: QEMU Developers, Claudio Imbrenda; +Cc: Paolo Bonzini
12.08.2023 12:38, Michael Tokarev wrote:
...
> It smells like, at the very least, os-posix.c should be split. We shouldn't include
> a ton of qemu-system functionality (like very specific option parsing) into qemu-nbd
> for example.
>
> How about splitting os-posix.c into a few files in util/ (not in the root dir), and
> adding them to util_ss in case of posix-os? Ditto for os-win32.c, I guess, but I
> haven't looked at this.
>
> And for the question in $subj, this one needs to be guarded by CONFIG_SOFTMMU.
Or maybe better yet, put the softmmu-specific functions (one very good example here
is os_parse_cmd_args() function - it clearly belongs to softmmu/, it should never
has been in global os-foo.c but in some softmmu-os-foo.c instead. This way,
async-teardown.c is moved to softmmu/ too, maybe os-linux-async-teardown.c.
/mjt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: util/async-teardown.c: is it really needed for --disable-system build?
2023-08-12 9:48 ` Michael Tokarev
@ 2023-08-14 7:01 ` Claudio Imbrenda
2023-08-14 7:12 ` Michael Tokarev
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2023-08-14 7:01 UTC (permalink / raw)
To: Michael Tokarev; +Cc: QEMU Developers, Paolo Bonzini
On Sat, 12 Aug 2023 12:48:14 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:
> 12.08.2023 12:38, Michael Tokarev wrote:
> ...
> > It smells like, at the very least, os-posix.c should be split. We shouldn't include
> > a ton of qemu-system functionality (like very specific option parsing) into qemu-nbd
> > for example.
> >
> > How about splitting os-posix.c into a few files in util/ (not in the root dir), and
> > adding them to util_ss in case of posix-os? Ditto for os-win32.c, I guess, but I
> > haven't looked at this.
> >
> > And for the question in $subj, this one needs to be guarded by CONFIG_SOFTMMU.
>
> Or maybe better yet, put the softmmu-specific functions (one very good example here
> is os_parse_cmd_args() function - it clearly belongs to softmmu/, it should never
> has been in global os-foo.c but in some softmmu-os-foo.c instead. This way,
> async-teardown.c is moved to softmmu/ too, maybe os-linux-async-teardown.c.
>
> /mjt
I think we could guard the offending item with CONFIG_SOFTMMU for now,
to immediately fix the issues you raised, and do the refactoring you
proposed later (e.g. next cycle).
what do you think?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: util/async-teardown.c: is it really needed for --disable-system build?
2023-08-14 7:01 ` Claudio Imbrenda
@ 2023-08-14 7:12 ` Michael Tokarev
2023-08-14 7:27 ` Claudio Imbrenda
0 siblings, 1 reply; 6+ messages in thread
From: Michael Tokarev @ 2023-08-14 7:12 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: QEMU Developers, Paolo Bonzini
14.08.2023 10:01, Claudio Imbrenda wrote:
> I think we could guard the offending item with CONFIG_SOFTMMU for now,
> to immediately fix the issues you raised, and do the refactoring you
> proposed later (e.g. next cycle).
I don't think rushing for the last-minute fix is necessary in this case.
It has real build problem on ia64 only which does not work for several
releases anyway, and the linking of unnecessary pieces happened for a
long time too.
/mjt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: util/async-teardown.c: is it really needed for --disable-system build?
2023-08-14 7:12 ` Michael Tokarev
@ 2023-08-14 7:27 ` Claudio Imbrenda
2023-08-14 8:31 ` Michael Tokarev
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2023-08-14 7:27 UTC (permalink / raw)
To: Michael Tokarev; +Cc: QEMU Developers, Paolo Bonzini
On Mon, 14 Aug 2023 10:12:35 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:
> 14.08.2023 10:01, Claudio Imbrenda wrote:
>
> > I think we could guard the offending item with CONFIG_SOFTMMU for now,
> > to immediately fix the issues you raised, and do the refactoring you
> > proposed later (e.g. next cycle).
>
> I don't think rushing for the last-minute fix is necessary in this case.
yes and no
it's a bug (which I introduced), and the quick fix seems to be
easy enough, so why not?
> It has real build problem on ia64 only which does not work for several
> releases anyway, and the linking of unnecessary pieces happened for a
> long time too.
>
> /mjt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: util/async-teardown.c: is it really needed for --disable-system build?
2023-08-14 7:27 ` Claudio Imbrenda
@ 2023-08-14 8:31 ` Michael Tokarev
0 siblings, 0 replies; 6+ messages in thread
From: Michael Tokarev @ 2023-08-14 8:31 UTC (permalink / raw)
To: Claudio Imbrenda; +Cc: QEMU Developers, Paolo Bonzini
14.08.2023 10:27, Claudio Imbrenda wrote:
> On Mon, 14 Aug 2023 10:12:35 +0300
> Michael Tokarev <mjt@tls.msk.ru> wrote:
>
>> 14.08.2023 10:01, Claudio Imbrenda wrote:
>>
>>> I think we could guard the offending item with CONFIG_SOFTMMU for now,
>>> to immediately fix the issues you raised, and do the refactoring you
>>> proposed later (e.g. next cycle).
>>
>> I don't think rushing for the last-minute fix is necessary in this case.
>
> yes and no
>
> it's a bug (which I introduced), and the quick fix seems to be
> easy enough, so why not?
The "quick fix" needs to add guards !USER_ONLY into include file, into
os-posix.c and into async-teardown.c, - that's just too ugly to my taste.
Also, the bug has been here for a very long time, - it is linking the
softmmu/vl.c options parsing part into a common code for qemu-nbd and
qemu-storage-daemon. Your change just discovered it.
No, I don't think this is necessary to apply a quick fix for 8.1. In
debian I can do that by applying debian-specific patch, and since no one
complained so far, I guess no one else is interested in ia64.
Thanks,
/mjt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-14 8:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-12 9:38 util/async-teardown.c: is it really needed for --disable-system build? Michael Tokarev
2023-08-12 9:48 ` Michael Tokarev
2023-08-14 7:01 ` Claudio Imbrenda
2023-08-14 7:12 ` Michael Tokarev
2023-08-14 7:27 ` Claudio Imbrenda
2023-08-14 8:31 ` Michael Tokarev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).