[PATCH 0/4] migration: Replace gemu_log with qemu_log

Josh Kunz posted 4 patches 2 weeks ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200114030138.260347-1-jkz@google.com
Maintainers: Riku Voipio <riku.voipio@iki.fi>, Laurent Vivier <laurent@vivier.eu>
bsd-user/main.c           |  13 +-
bsd-user/qemu.h           |   2 -
bsd-user/strace.c         |  32 ++-
bsd-user/syscall.c        |  31 ++-
include/qemu/log.h        |  15 ++
linux-user/arm/cpu_loop.c |   5 +-
linux-user/fd-trans.c     |  55 +++--
linux-user/main.c         |  50 +++-
linux-user/qemu.h         |   2 -
linux-user/signal.c       |   3 +-
linux-user/strace.c       | 479 +++++++++++++++++++-------------------
linux-user/syscall.c      |  43 ++--
linux-user/vm86.c         |   3 +-
util/log.c                |   5 +
14 files changed, 404 insertions(+), 334 deletions(-)

[PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Josh Kunz 2 weeks ago
The linux-user and bsd-user trees both widely use a function called
`gemu_log` (notice the 'g') for miscellaneous and strace logging. This
function predates the newer `qemu_log` function, and has a few drawbacks
compared to `qemu_log`:

  1. Always logs to `stderr`, no logging redirection.
  2. "Miscellaneous" logging cannot be disabled, so it may mix with guest
     logging.
  3. Inconsistency with other parts of the QEMU codebase, and a
     confusing name.

The second issue is especially troubling because it can interfere with
programs that expect to communicate via stderr.

This change introduces two new logging masks to the `qemu_log` subsystem
to support its use for user-mode logging. The `LOG_USER` mask, for
miscellaneous logs, and the `LOG_STRACE` mask for strace-specific
logging. Further, it replaces all existing uses of `gemu_log` with the
appropriate `qemu_log_mask(LOG_{USER,STRACE}, ...)` based on the log
message. To avoid potential backwards incompatibility, this patch series
also introduces a new flag: `-no-force-user-mode-logging`. Since "user
mode" logging was always enabled before this change, by-default the
LOG_USER mask is always set. The flag is provided for users that want to
disable LOG_USER. Hopefully, this flag can be removed, or turned into a
no-op in the futre, and -d/QEMU_LOG can be used directly to enable
user-mode logging if desired.

Unfortunately, this change introduces one small backwards incompatibility
for the linux-user tree: If an explicit `-D` option is provided, or the
`QEMU_LOG_FILENAME`_ environment variable is set, strace and user-mode
logging statements will log to the QEMU log file instead of stderr.

Tested:
  * Built with clang 9 and g++ 8.3
  * `make check` run with clang 9 build 
  * Verified:
    * QEMU_STRACE/-strace still works for linux-user
    * -no-force-user-mode-logging uses QEMU_LOG + `user_mode` or
      `strace` can be set in QEMU_LOG to trigger the appropriate
      logging.

Not tested:
  * Build/logging with bsd-user. I do not have easy access to a BSD system.

Josh Kunz (4):
  linux-user: Use `qemu_log' for non-strace logging
  linux-user: Use `qemu_log' for strace
  linux-user: remove gemu_log from the linux-user tree
  bsd-user: Replace gemu_log with qemu_log

 bsd-user/main.c           |  13 +-
 bsd-user/qemu.h           |   2 -
 bsd-user/strace.c         |  32 ++-
 bsd-user/syscall.c        |  31 ++-
 include/qemu/log.h        |  15 ++
 linux-user/arm/cpu_loop.c |   5 +-
 linux-user/fd-trans.c     |  55 +++--
 linux-user/main.c         |  50 +++-
 linux-user/qemu.h         |   2 -
 linux-user/signal.c       |   3 +-
 linux-user/strace.c       | 479 +++++++++++++++++++-------------------
 linux-user/syscall.c      |  43 ++--
 linux-user/vm86.c         |   3 +-
 util/log.c                |   5 +
 14 files changed, 404 insertions(+), 334 deletions(-)

-- 
2.25.0.rc1.283.g88dfdc4193-goog


Re: [PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Alex Bennée 2 weeks ago
Josh Kunz <jkz@google.com> writes:

<snip>
>
> Not tested:
>   * Build/logging with bsd-user. I do not have easy access to a BSD
> system.

If you have the time we have vm-build-netbsd:

  make vm-build-netbsd EXTRA_CONFIGURE_OPTS="--disable-system"
  
Which will create a NetBSD image for you and run the build through it.
Other images are available ;-)

-- 
Alex Bennée

Re: [PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Josh Kunz 1 week ago
On Tue, Jan 14, 2020 at 3:02 AM Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Josh Kunz <jkz@google.com> writes:
>
> <snip>
> >
> > Not tested:
> >   * Build/logging with bsd-user. I do not have easy access to a BSD
> > system.
>
> If you have the time we have vm-build-netbsd:
>
>   make vm-build-netbsd EXTRA_CONFIGURE_OPTS="--disable-system"
>
> Which will create a NetBSD image for you and run the build through it.
> Other images are available ;-)
>

This works, but it looks like it only runs the tests on BSD, even with
`configure --enable-bsd-user` first. I don't see the bsd-user binaries
being produced in the output of this command.

Re: [PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Alex Bennée 1 week ago
Josh Kunz <jkz@google.com> writes:

> On Tue, Jan 14, 2020 at 3:02 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>>
>> Josh Kunz <jkz@google.com> writes:
>>
>> <snip>
>> >
>> > Not tested:
>> >   * Build/logging with bsd-user. I do not have easy access to a BSD
>> > system.
>>
>> If you have the time we have vm-build-netbsd:
>>
>>   make vm-build-netbsd EXTRA_CONFIGURE_OPTS="--disable-system"
>>
>> Which will create a NetBSD image for you and run the build through it.
>> Other images are available ;-)
>>
>
> This works, but it looks like it only runs the tests on BSD, even with
> `configure --enable-bsd-user` first. I don't see the bsd-user binaries
> being produced in the output of this command.

Ahh the default build target for the BSDs is "check" but as bsd-user
doesn't have any checks it doesn't end up building. You can force it
with

  make vm-build-netbsd EXTRA_CONFIGURE_OPTS="--disable-system" BUILD_TARGET="all"

It would be worth plumbing in the tests/tcg tests at some point. I
suspect most of the user-mode tests are more POSIX than Linux.

-- 
Alex Bennée

Re: [PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Warner Losh 2 weeks ago
On Mon, Jan 13, 2020, 8:03 PM Josh Kunz <jkz@google.com> wrote:

> The linux-user and bsd-user trees both widely use a function called
> `gemu_log` (notice the 'g') for miscellaneous and strace logging. This
> function predates the newer `qemu_log` function, and has a few drawbacks
> compared to `qemu_log`:
>
>   1. Always logs to `stderr`, no logging redirection.
>   2. "Miscellaneous" logging cannot be disabled, so it may mix with guest
>      logging.
>   3. Inconsistency with other parts of the QEMU codebase, and a
>      confusing name.
>
> The second issue is especially troubling because it can interfere with
> programs that expect to communicate via stderr.
>
> This change introduces two new logging masks to the `qemu_log` subsystem
> to support its use for user-mode logging. The `LOG_USER` mask, for
> miscellaneous logs, and the `LOG_STRACE` mask for strace-specific
> logging. Further, it replaces all existing uses of `gemu_log` with the
> appropriate `qemu_log_mask(LOG_{USER,STRACE}, ...)` based on the log
> message. To avoid potential backwards incompatibility, this patch series
> also introduces a new flag: `-no-force-user-mode-logging`. Since "user
> mode" logging was always enabled before this change, by-default the
> LOG_USER mask is always set. The flag is provided for users that want to
> disable LOG_USER. Hopefully, this flag can be removed, or turned into a
> no-op in the futre, and -d/QEMU_LOG can be used directly to enable
> user-mode logging if desired.
>
> Unfortunately, this change introduces one small backwards incompatibility
> for the linux-user tree: If an explicit `-D` option is provided, or the
> `QEMU_LOG_FILENAME`_ environment variable is set, strace and user-mode
> logging statements will log to the QEMU log file instead of stderr.
>
> Tested:
>   * Built with clang 9 and g++ 8.3
>   * `make check` run with clang 9 build
>   * Verified:
>     * QEMU_STRACE/-strace still works for linux-user
>     * -no-force-user-mode-logging uses QEMU_LOG + `user_mode` or
>       `strace` can be set in QEMU_LOG to trigger the appropriate
>       logging.
>
> Not tested:
>   * Build/logging with bsd-user. I do not have easy access to a BSD system.
>

The bsd-user that is in tree doesn't work. I've been trying to catch up to
qemu head of tree, but I'm only up to 3.2... chances are the bsd-user
changes will not change the state of things...

Warner

Josh Kunz (4):
>   linux-user: Use `qemu_log' for non-strace logging
>   linux-user: Use `qemu_log' for strace
>   linux-user: remove gemu_log from the linux-user tree
>   bsd-user: Replace gemu_log with qemu_log
>
>  bsd-user/main.c           |  13 +-
>  bsd-user/qemu.h           |   2 -
>  bsd-user/strace.c         |  32 ++-
>  bsd-user/syscall.c        |  31 ++-
>  include/qemu/log.h        |  15 ++
>  linux-user/arm/cpu_loop.c |   5 +-
>  linux-user/fd-trans.c     |  55 +++--
>  linux-user/main.c         |  50 +++-
>  linux-user/qemu.h         |   2 -
>  linux-user/signal.c       |   3 +-
>  linux-user/strace.c       | 479 +++++++++++++++++++-------------------
>  linux-user/syscall.c      |  43 ++--
>  linux-user/vm86.c         |   3 +-
>  util/log.c                |   5 +
>  14 files changed, 404 insertions(+), 334 deletions(-)
>
> --
> 2.25.0.rc1.283.g88dfdc4193-goog
>
>
>

Re: [PATCH 0/4] migration: Replace gemu_log with qemu_log

Posted by Josh Kunz 1 week ago
On Mon, Jan 13, 2020 at 7:06 PM Warner Losh <imp@bsdimp.com> wrote:

> The bsd-user that is in tree doesn't work. I've been trying to catch up to
> qemu head of tree, but I'm only up to 3.2... chances are the bsd-user
> changes will not change the state of things...
>

Ah good to know. Would you all prefer I don't modify it at all, to try and
keep the code "pristine"? Or is it OK to keep the patch as is?