[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH vOther2 1/1] qemu-nbd: Restore "qemu-nbd -v --fork" output
From: |
Eric Blake |
Subject: |
Re: [PATCH vOther2 1/1] qemu-nbd: Restore "qemu-nbd -v --fork" output |
Date: |
Mon, 11 Sep 2023 09:48:57 -0500 |
User-agent: |
NeoMutt/20230517 |
On Sun, Sep 10, 2023 at 11:40:25AM +0300, Michael Tokarev wrote:
> 25.08.2023 23:08, Denis V. Lunev wrote:
> > + /* Remember parent's stderr if we will be restoring it. */
> > + if (verbose /* fork_process is set */) {
> > + opts.stderr = dup(STDERR_FILENO);
> > + if (opts.stderr < 0) {
> > + error_report("Could not dup stdedd: %s",
> > strerror(errno));
> > + exit(EXIT_FAILURE);
> > + }
> > + }
> > +
> > ret = qemu_daemon(1, 0);
>
> I haven't looked closely to this development.
>
> To me it all looks.. backwards.
>
> Instead of saving stderr around qemu_daemon() call, it might be more
> productive to tell qemu_daemon() to stop redirecting stderr (and maybe
> instrumenting it to do so).
I tried to do that in an earlier revision of this patch, but it
changed iotest output. We've already had a lot of churn on patches
that were supposed to fix a regression but in turn caused another
regression.
>
> Besides, qemu has 2 daemon implementations, one is qemu_daemon()
> in util/oslib-posix.c and another is os_daemonize() in os-posix.c.
> Note os_daemonize() does the right thing wrt logging already, -
>
> /* In case -D is given do not redirect stderr to /dev/null */
> if (!qemu_log_enabled()) {
> dup2(fd, 2);
> }
>
> but I guess nbd does not use qemu_log_enabled() et al.
>
> Also, qemu-nbd can benefit from using -runas/-chroot too.
>
> Ideally this whole thing should be consolidated. I already took a
> step towards this by moving softmmu-specific stuff from os-posix.c
> to softmmu/, this work should continue. When it's done, we can
> revert this band-aid change for a real solution.
Indeed, there may still be further cleanups to do once the os-posix.c
cleanups are in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org