Re: [PATCH 03/11] MAINTAINERS: add a section for policy documents

[Warner Losh]

Alex,

On Thu, Mar 30, 2023 at 12:11 PM Alex Bennée  wrote:

> We don't update these often but if you are the sort of person who
> enjoys debating and tuning project policies you could now add yourself
> as a reviewer here so you don't miss the next debate over tabs vs
> spaces ;-)
>
> Who's with me?
>
> Signed-off-by: Alex Bennée 
> Cc: Thomas Huth 
> Cc: Daniel P. Berrangé 
> Cc: Markus Armbruster 
> Cc: Kashyap Chamarthy 
> Cc: Paolo Bonzini 
> Cc: Peter Maydell 
> Cc: Philippe Mathieu-Daudé 
> Cc: Bernhard Beschow 
>

Reviewed-by: Warner Losh 

Since I'm not on the list, I approve :). I had enough dealing with code of
conduct issues with FreeBSD to last for any number of years...

Warner


> ---
> v2
>   - s/your/you are/
>   - add some willing victims
> ---
>  MAINTAINERS | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9e1a60ea24..2c173dbd96 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -64,6 +64,19 @@ L: qemu-de...@nongnu.org
>  F: *
>  F: */
>
> +Project policy and developer guides
> +R: Alex Bennée 
> +R: Daniel P. Berrangé 
> +R: Thomas Huth 
> +R: Markus Armbruster 
> +W: https://www.qemu.org/docs/master/devel/index.html
> +S: Odd Fixes
> +F: docs/devel/style.rst
> +F: docs/devel/code-of-conduct.rst
> +F: docs/devel/conflict-resolution.rst
> +F: docs/devel/submitting-a-patch.rst
> +F: docs/devel/submitting-a-pull-request.rst
> +
>  Responsible Disclosure, Reporting Security Issues
>  -
>  W: https://wiki.qemu.org/SecurityProcess
> --
> 2.39.2
>
>

Re: [PATCH v4 03/19] scripts/clean-includes: Skip symbolic links

[Warner Losh]

On Fri, Jan 27, 2023 at 3:47 PM Eric Blake  wrote:

> On Thu, Jan 19, 2023 at 07:59:43AM +0100, Markus Armbruster wrote:
> > When a symbolic link points to a file that needs cleaning, the script
> > replaces the link with a cleaned regular file.  Not wanted; skip them.
> >
> > We have a few symbolic links under subprojects/libvduse/ and
> > subprojects/libvhost-user/.
> >
> > Signed-off-by: Markus Armbruster 
> > ---
> >  scripts/clean-includes | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/scripts/clean-includes b/scripts/clean-includes
> > index 8e8420d785..f0466a6262 100755
> > --- a/scripts/clean-includes
> > +++ b/scripts/clean-includes
> > @@ -113,6 +113,10 @@ EOT
> >
> >  files=
> >  for f in "$@"; do
> > +  if [ -L "$f" ]; then
>
> I don't see -L used with test very often, but POSIX requires it, so it
> is safe for our choice of /bin/sh.
>

FYI: -L is in FreeBSD, NetBSD, OpenBSD, etc. It's been in all these trees
since the mid 90s. It wasn't in 4.4BSD, but all these projects have
imported the code from pdksh's test.

So in addition to POSIX, it's been widely implemented, at least in the BSD
world, for over 20 years.

Warner

Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Warner Losh]

On Thu, Jan 19, 2023 at 9:42 AM Markus Armbruster  wrote:

> Warner Losh  writes:
>
> > On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster 
> > wrote:
> >
> >> Clean up includes so that osdep.h is included first and headers
> >> which it implies are not included manually.
> >>
> >> This commit was created with scripts/clean-includes.
> >>
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  bsd-user/bsd-proc.h   | 4 
> >>  bsd-user/qemu.h   | 1 -
> >>  bsd-user/arm/signal.c | 1 +
> >>  bsd-user/arm/target_arch_cpu.c| 2 ++
> >>  bsd-user/freebsd/os-sys.c | 1 +
> >>  bsd-user/i386/signal.c| 1 +
> >>  bsd-user/i386/target_arch_cpu.c   | 3 +--
> >>  bsd-user/main.c   | 4 +---
> >>  bsd-user/strace.c | 1 -
> >>  bsd-user/x86_64/signal.c  | 1 +
> >>  bsd-user/x86_64/target_arch_cpu.c | 3 +--
> >>  11 files changed, 9 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
> >> index 68b66e571d..a1061bffb8 100644
> >> --- a/bsd-user/bsd-proc.h
> >> +++ b/bsd-user/bsd-proc.h
> >> @@ -20,11 +20,7 @@
> >>  #ifndef BSD_PROC_H_
> >>  #define BSD_PROC_H_
> >>
> >> -#include 
> >> -#include 
> >> -#include 
> >>  #include 
> >>
> >
> > Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill
> > where all includes are independent, but much of that hasn't been merged
> to
> > 12 yet. sys/types.h, at least, is documented as a prereq for both
> > sys/stat.h and sys/resource.h.
> >
> > I know many of these are in os-dep.h, and I've had trouble in the
> bsd-user
> > fork with that and the layout of the code which has arguably way too much
> > in the .h files.
>
> No, I didn't test on FreeBSD 12.
>

OK. Fair enough. If it passes the CI tests, then it's likely fine (though
if I hit issues, I'll submit patches).


> Any .c must include qemu/osdep.h *first*.  Any further inclusions of
> headers qemu/osdep.h includes already are no-ops unless the headers in
> question lack multiple inclusion guards.
>

OK.


> > Also, why didn't you move sys/resource.h and other such files to
> os-dep.h?
> > I'm struggling to understand the rules around what is or isn't included
> > where?
>
> This series is "run scripts/clean-includes, split it into reviewable
> chunks, tidy up blank lines".  Moving more includes into qemu/osdep.h is
> out of scope.
>

OK. Fair point. I'm happy with that answer since it tells me I could move
things there too, if need be.


> I sympathize with your complaint that QEMU does too much in headers in
> general, and in qemu/osdep.h in particular.
>

Yea, I'm not entirely sure  it's a complaint, or if it's an observation of
difficulties relative to other code bases... I go back and forth on my
opinion of it...


> >> -#include 
> >>
> >>  /* exit(2) */
> >>  static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
> >> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> >> index be6105385e..0ceecfb6df 100644
> >> --- a/bsd-user/qemu.h
> >> +++ b/bsd-user/qemu.h
> >> @@ -17,7 +17,6 @@
> >>  #ifndef QEMU_H
> >>  #define QEMU_H
> >>
> >> -#include "qemu/osdep.h"
> >>  #include "cpu.h"
> >>  #include "qemu/units.h"
> >>  #include "exec/cpu_ldst.h"
> >> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> >> index 2b1dd745d1..9734407543 100644
> >> --- a/bsd-user/arm/signal.c
> >> +++ b/bsd-user/arm/signal.c
> >> @@ -17,6 +17,7 @@
> >>   *  along with this program; if not, see <http://www.gnu.org/licenses/
> >.
> >>   */
> >>
> >> +#include "qemu/osdep.h"
> >>  #include "qemu.h"
> >>
> >>  /*
> >> diff --git a/bsd-user/arm/target_arch_cpu.c
> >> b/bsd-user/arm/target_arch_cpu.c
> >> index 02bf9149d5..fe38ae2210 100644
> >> --- a/bsd-user/arm/target_arch_cpu.c
> >> +++ b/bsd-user/arm/target_arch_cpu.c
> >> @@ -16,6 +16,8 @@
> >>   *  You should have received a copy of the GNU General Public License
> >>   *  along with this program; if not, see <http://www.gnu.org/licenses/
> >.
> >>   */
> >> +
> >> +#include "q

Re: [PATCH v4 04/19] bsd-user: Clean up includes

[Warner Losh]

On Thu, Jan 19, 2023 at 12:00 AM Markus Armbruster 
wrote:

> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
>
> This commit was created with scripts/clean-includes.
>
> Signed-off-by: Markus Armbruster 
> ---
>  bsd-user/bsd-proc.h   | 4 
>  bsd-user/qemu.h   | 1 -
>  bsd-user/arm/signal.c | 1 +
>  bsd-user/arm/target_arch_cpu.c| 2 ++
>  bsd-user/freebsd/os-sys.c | 1 +
>  bsd-user/i386/signal.c| 1 +
>  bsd-user/i386/target_arch_cpu.c   | 3 +--
>  bsd-user/main.c   | 4 +---
>  bsd-user/strace.c | 1 -
>  bsd-user/x86_64/signal.c  | 1 +
>  bsd-user/x86_64/target_arch_cpu.c | 3 +--
>  11 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
> index 68b66e571d..a1061bffb8 100644
> --- a/bsd-user/bsd-proc.h
> +++ b/bsd-user/bsd-proc.h
> @@ -20,11 +20,7 @@
>  #ifndef BSD_PROC_H_
>  #define BSD_PROC_H_
>
> -#include 
> -#include 
> -#include 
>  #include 
>

Did you test this on FreeBSD 12? FreeBSD 13 has started to climb the hill
where all includes are independent, but much of that hasn't been merged to
12 yet. sys/types.h, at least, is documented as a prereq for both
sys/stat.h and sys/resource.h.

I know many of these are in os-dep.h, and I've had trouble in the bsd-user
fork with that and the layout of the code which has arguably way too much
in the .h files.

Also, why didn't you move sys/resource.h and other such files to os-dep.h?
I'm struggling to understand the rules around what is or isn't included
where?


> -#include 
>
>  /* exit(2) */
>  static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index be6105385e..0ceecfb6df 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -17,7 +17,6 @@
>  #ifndef QEMU_H
>  #define QEMU_H
>
> -#include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "qemu/units.h"
>  #include "exec/cpu_ldst.h"
> diff --git a/bsd-user/arm/signal.c b/bsd-user/arm/signal.c
> index 2b1dd745d1..9734407543 100644
> --- a/bsd-user/arm/signal.c
> +++ b/bsd-user/arm/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> diff --git a/bsd-user/arm/target_arch_cpu.c
> b/bsd-user/arm/target_arch_cpu.c
> index 02bf9149d5..fe38ae2210 100644
> --- a/bsd-user/arm/target_arch_cpu.c
> +++ b/bsd-user/arm/target_arch_cpu.c
> @@ -16,6 +16,8 @@
>   *  You should have received a copy of the GNU General Public License
>   *  along with this program; if not, see .
>   */
> +
> +#include "qemu/osdep.h"
>  #include "target_arch.h"
>
>  void target_cpu_set_tls(CPUARMState *env, target_ulong newtls)
> diff --git a/bsd-user/freebsd/os-sys.c b/bsd-user/freebsd/os-sys.c
> index 309e27b9d6..1676ec10f8 100644
> --- a/bsd-user/freebsd/os-sys.c
> +++ b/bsd-user/freebsd/os-sys.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>  #include "target_arch_sysarch.h"
>
> diff --git a/bsd-user/i386/signal.c b/bsd-user/i386/signal.c
> index 5dd975ce56..a3131047b8 100644
> --- a/bsd-user/i386/signal.c
> +++ b/bsd-user/i386/signal.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> diff --git a/bsd-user/i386/target_arch_cpu.c
> b/bsd-user/i386/target_arch_cpu.c
> index d349e45299..2a3af2ddef 100644
> --- a/bsd-user/i386/target_arch_cpu.c
> +++ b/bsd-user/i386/target_arch_cpu.c
> @@ -17,9 +17,8 @@
>   *  along with this program; if not, see .
>   */
>
> -#include 
> -
>  #include "qemu/osdep.h"
> +
>  #include "cpu.h"
>  #include "qemu.h"
>  #include "qemu/timer.h"
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d65..41290e16f9 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -18,12 +18,10 @@
>   *  along with this program; if not, see .
>   */
>
> -#include 
> -#include 
> +#include "qemu/osdep.h"
>  #include 
>  #include 
>
> -#include "qemu/osdep.h"
>  #include "qemu/help-texts.h"
>  #include "qemu/units.h"
>  #include "qemu/accel.h"
> diff --git a/bsd-user/strace.c b/bsd-user/strace.c
> index a77d10dd6b..96499751eb 100644
> --- a/bsd-user/strace.c
> +++ b/bsd-user/strace.c
> @@ -20,7 +20,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>
>  #include "qemu.h"
>
> diff --git a/bsd-user/x86_64/signal.c b/bsd-user/x86_64/signal.c
> index c3875bc4c6..46cb865180 100644
> --- a/bsd-user/x86_64/signal.c
> +++ b/bsd-user/x86_64/signal.c
> @@ -16,6 +16,7 @@
>   *  along with this program; if not, see .
>   */
>
> +#include "qemu/osdep.h"
>  #include "qemu.h"
>
>  /*
> di

Re: [PATCH v2 08/15] scripts/qapi: add required system includes to visitor

[Warner Losh]

On Tue, Jul 12, 2022 at 3:36 AM  wrote:

> From: Marc-André Lureau 
>
> The generated visitor code includes abort() & assert(), we shouldn't
> rely on the global "-i" headers to include the necessary system headers.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  scripts/qapi/visit.py | 2 ++
>  1 file changed, 2 insertions(+)
>

Reviewed-by: Warner Losh 


> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 1ff464c0360f..4aba5ddd8af4 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -342,6 +342,8 @@ def _begin_user_module(self, name: str) -> None:
>  visit = self._module_basename('qapi-visit', name)
>  self._genc.preamble_add(mcgen('''
>  %(include)s
> +#include 
> +#include 
>
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> --
> 2.37.0.rc0
>
>

Re: [PATCH v2 04/15] error-report: introduce overridable error_is_detailed()

[Warner Losh]

On Tue, Jul 12, 2022 at 3:36 AM  wrote:

> From: Marc-André Lureau 
>
> Remove the direct dependency from error-report to monitor code.
> This will allow to move error-report to a subproject.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  include/qemu/error-report.h | 2 ++
>  softmmu/vl.c| 5 +
>  stubs/error-is-detailed.c   | 7 +++
>  util/error-report.c | 3 +--
>  stubs/meson.build   | 1 +
>  5 files changed, 16 insertions(+), 2 deletions(-)
>  create mode 100644 stubs/error-is-detailed.c
>

Reviewed-by: Warner Losh 


> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..6ab25d458350 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -30,6 +30,8 @@ void loc_set_none(void);
>  void loc_set_cmdline(char **argv, int idx, int cnt);
>  void loc_set_file(const char *fname, int lno);
>
> +bool error_is_detailed(void);
> +
>  int error_vprintf(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
>  int error_printf(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 3f264d4b0930..f94efc56e9d6 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2589,6 +2589,11 @@ void qmp_x_exit_preconfig(Error **errp)
>  }
>  }
>
> +bool error_is_detailed(void)
> +{
> +return !monitor_cur();
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>  QemuOpts *opts;
> diff --git a/stubs/error-is-detailed.c b/stubs/error-is-detailed.c
> new file mode 100644
> index ..c47cd236932f
> --- /dev/null
> +++ b/stubs/error-is-detailed.c
> @@ -0,0 +1,7 @@
> +#include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +
> +bool error_is_detailed(void)
> +{
> +return TRUE;
> +}
> diff --git a/util/error-report.c b/util/error-report.c
> index c43227a975e2..4d1d66fc0650 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -11,7 +11,6 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
>  #include "qemu/error-report.h"
>
>  /*
> @@ -195,7 +194,7 @@ real_time_iso8601(void)
>   */
>  static void vreport(report_type type, const char *fmt, va_list ap)
>  {
> -bool detailed = !monitor_cur();
> +bool detailed = error_is_detailed();
>  gchar *timestr;
>
>  if (message_with_timestamp && detailed) {
> diff --git a/stubs/meson.build b/stubs/meson.build
> index d8f3fd5c44f2..0f3a782824f9 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -9,6 +9,7 @@ stub_ss.add(files('cpus-get-virtual-clock.c'))
>  stub_ss.add(files('qemu-timer-notify-cb.c'))
>  stub_ss.add(files('icount.c'))
>  stub_ss.add(files('dump.c'))
> +stub_ss.add(files('error-is-detailed.c'))
>  stub_ss.add(files('error-printf.c'))
>  stub_ss.add(files('fdset.c'))
>  stub_ss.add(files('gdbstub.c'))
> --
> 2.37.0.rc0
>
>

Re: [PATCH v2 12/15] qemu-common: move glib-compat.h

[Warner Losh]

On Tue, Jul 12, 2022 at 3:37 AM  wrote:

> From: Marc-André Lureau 
>
> qemu-common will have compatible dependency requirements with QEMU.
>
> Since qemu-common won't have a toplevel qemu/osdep.h which would include
> various system headers, include stdbool.h (bool is used for some
> declarations here).
>
> Replace getenv() with g_getenv() to avoid extra header inclusion.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  {include => subprojects/qemu-common/include}/glib-compat.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>  rename {include => subprojects/qemu-common/include}/glib-compat.h (97%)
>

Reviewed-by: Warner Losh 


>
> diff --git a/include/glib-compat.h
> b/subprojects/qemu-common/include/glib-compat.h
> similarity index 97%
> rename from include/glib-compat.h
> rename to subprojects/qemu-common/include/glib-compat.h
> index 43a562974d80..2b0f2962f322 100644
> --- a/include/glib-compat.h
> +++ b/subprojects/qemu-common/include/glib-compat.h
> @@ -30,6 +30,8 @@
>  #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>
>  #include 
> +#include 
> +
>  #if defined(G_OS_UNIX)
>  #include 
>  #include 
> @@ -133,7 +135,7 @@ qemu_g_test_slow(void)
>  {
>  static int cached = -1;
>  if (cached == -1) {
> -cached = g_test_slow() || getenv("G_TEST_SLOW") != NULL;
> +cached = g_test_slow() || g_getenv("G_TEST_SLOW") != NULL;
>  }
>  return cached;
>  }
> --
> 2.37.0.rc0
>
>

Re: [PATCH v2 10/15] qemu-common: introduce a common subproject

[Warner Losh]

On Tue, Jul 12, 2022 at 3:36 AM  wrote:

> From: Marc-André Lureau 
>
> Add a new meson subproject to provide common code and scripts for QEMU
> and tools. Initially, it will offer QAPI/QMP code generation and
> common utilities.
>
> libvhost-user & libvduse will make use of the subproject to avoid having
> include/ links to common headers.
>
> The other targeted user is qemu-ga, which will also be converted to a
> subproject (so it can be built, moved, released etc independent from QEMU).
>
> Other projects such as qemu-storage-daemon could be built standalone
> eventually in the future.
>
> Note that with meson subprojects are "global". Projects will share
> subprojects (
> https://mesonbuild.com/Subprojects.html#subprojects-depending-on-other-subprojects
> ).
> We will add extra subprojects/ links to allow standalone subproject
> compilation though.
>
> This initial commit simply set the stage to build and link against it.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  meson.build  | 9 -
>  .../qemu-common/include}/qemu/help-texts.h   | 0
>  linux-user/meson.build   | 4 ++--
>  subprojects/libvduse/meson.build | 2 ++
>  subprojects/libvduse/subprojects/qemu-common | 1 +
>  subprojects/libvhost-user/meson.build| 2 ++
>  subprojects/libvhost-user/subprojects/qemu-common| 1 +
>  subprojects/qemu-common/meson.build  | 8 
>  8 files changed, 24 insertions(+), 3 deletions(-)
>  rename {include => subprojects/qemu-common/include}/qemu/help-texts.h
> (100%)
>  create mode 12 subprojects/libvduse/subprojects/qemu-common
>  create mode 12 subprojects/libvhost-user/subprojects/qemu-common
>  create mode 100644 subprojects/qemu-common/meson.build
>
> diff --git a/meson.build b/meson.build
> index bc5569ace159..254eb1263a66 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -167,6 +167,10 @@ if 'dtrace' in get_option('trace_backends')
>endif
>  endif
>
> +add_project_arguments('-I' + meson.current_source_dir() /
> 'subprojects/qemu-common/include',
> +  language: ['c', 'cpp', 'objc'],
> +)
> +
>  if get_option('iasl') == ''
>iasl = find_program('iasl', required: false)
>  else
> @@ -1577,6 +1581,9 @@ if libbpf.found() and not cc.links('''
>endif
>  endif
>
> +qemu_common = subproject('qemu-common')
> +qemu_common = qemu_common.get_variable('qemu_common_dep')
> +
>  #
>  # config-host.h #
>  #
> @@ -3052,7 +3059,7 @@ util_ss.add_all(trace_ss)
>  util_ss = util_ss.apply(config_all, strict: false)
>  libqemuutil = static_library('qemuutil',
>   sources: util_ss.sources() +
> stub_ss.sources() + genh,
> - dependencies: [util_ss.dependencies(), libm,
> threads, glib, socket, malloc, pixman])
> + dependencies: [util_ss.dependencies(), libm,
> threads, glib, socket, malloc, pixman, qemu_common])
>  qemuutil = declare_dependency(link_with: libqemuutil,
>sources: genh + version_res,
>dependencies: [event_loop_base])
> diff --git a/include/qemu/help-texts.h
> b/subprojects/qemu-common/include/qemu/help-texts.h
> similarity index 100%
> rename from include/qemu/help-texts.h
> rename to subprojects/qemu-common/include/qemu/help-texts.h
> diff --git a/linux-user/meson.build b/linux-user/meson.build
> index de4320af053c..fc6cdb55d657 100644
> --- a/linux-user/meson.build
> +++ b/linux-user/meson.build
> @@ -7,7 +7,7 @@ linux_user_ss = ss.source_set()
>  common_user_inc += include_directories('include/host/' / host_arch)
>  common_user_inc += include_directories('include')
>
> -linux_user_ss.add(files(
> +linux_user_ss.add([files(
>'elfload.c',
>'exit.c',
>'fd-trans.c',
> @@ -20,7 +20,7 @@ linux_user_ss.add(files(
>'thunk.c',
>'uaccess.c',
>'uname.c',
> -))
> +), qemu_common])
>

Question: Why does linux-user need these, but bsd-user does not?

Warner


>  linux_user_ss.add(rt)
>
>  linux_user_ss.add(when: 'TARGET_HAS_BFLT', if_true: files('flatload.c'))
> diff --git a/subprojects/libvduse/meson.build
> b/subprojects/libvduse/meson.build
> index ba08f5ee1a03..841509ecb996 100644
> --- a/subprojects/libvduse/meson.build
> +++ b/subprojects/libvduse/meson.build
> @@ -2,6 +2,8 @@ project('libvduse', 'c',
>  license: 'GPL-2.0-or-later',
>  default_options: ['c_std=gnu99'])
>
> +qemu_common = subproject('qemu-common')
> +
>  libvduse = static_library('vduse',
>files('libvduse.c'),
>c_args: '-D_GNU_SOURCE')
> diff --git a/subprojects/libvduse/subprojects/qemu-common
> b/subprojects/libvduse/subprojects/qemu-common
> new file mode 12
> index ..4c1c87018a7a
> --- /dev/null
> +++ b/subprojects/libvduse/subprojects/qemu-common
> @@ -0,0 +1 @@
> +../

Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[Warner Losh]

On Thu, Jun 16, 2022 at 6:41 AM  wrote:

> From: Marc-André Lureau 
>
> Remove monitor dependency from error printing code, by allowing programs
> to set a callback for when to use "detailed" reporting or not.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  include/qemu/error-report.h  | 4 +++-
>  bsd-user/main.c  | 2 +-
>  linux-user/main.c| 2 +-
>  qemu-img.c   | 2 +-
>  qemu-io.c| 2 +-
>  qemu-nbd.c   | 2 +-
>  scsi/qemu-pr-helper.c| 2 +-
>  softmmu/vl.c | 7 ++-
>  storage-daemon/qemu-storage-daemon.c | 7 ++-
>  util/error-report.c  | 8 +---
>  10 files changed, 26 insertions(+), 12 deletions(-)
>

Reviewed-by: Warner Losh 

While all the changes look good, I'm only confident about the *-user
changes if that matters.

Warner


> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..e2e630f207f0 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_ERROR_REPORT_H
>  #define QEMU_ERROR_REPORT_H
>
> +typedef bool (*ErrorReportDetailedFunc)(void);
> +
>  typedef struct Location {
>  /* all members are private to qemu-error.c */
>  enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char
> *fmt, ...)
>  bool warn_report_once_cond(bool *printed, const char *fmt, ...)
>  G_GNUC_PRINTF(2, 3);
>
> -void error_init(const char *argv0);
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
>
>  /*
>   * Similar to error_report(), except it prints the message just once.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d6541..d5f8fca863d7 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -292,7 +292,7 @@ int main(int argc, char **argv)
>
>  save_proc_pathname(argv[0]);
>
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  qemu_init_cpu_list();
>  module_call_init(MODULE_INIT_QOM);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 651e32f5f248..84f380bd366d 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -646,7 +646,7 @@ int main(int argc, char **argv, char **envp)
>  unsigned long max_reserved_va;
>  bool preserve_argv0;
>
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  qemu_init_cpu_list();
>  module_call_init(MODULE_INIT_QOM);
> diff --git a/qemu-img.c b/qemu-img.c
> index 4cf4d2423df8..1f27a9fc70f6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -5396,7 +5396,7 @@ int main(int argc, char **argv)
>  #endif
>
>  socket_init();
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  qemu_init_exec_dir(argv[0]);
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 2bd7bfb65073..b5cdc7c922a7 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -539,7 +539,7 @@ int main(int argc, char **argv)
>  #endif
>
>  socket_init();
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  qemu_init_exec_dir(argv[0]);
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 0cd5aa6f02bc..6bc632c93611 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -587,7 +587,7 @@ int main(int argc, char **argv)
>  #endif
>
>  socket_init();
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  qcrypto_init(&error_fatal);
>
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 196b78c00df5..8d80e58d4498 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -910,7 +910,7 @@ int main(int argc, char **argv)
>
>  signal(SIGPIPE, SIG_IGN);
>
> -error_init(argv[0]);
> +error_init(argv[0], NULL);
>  module_call_init(MODULE_INIT_TRACE);
>  module_call_init(MODULE_INIT_QOM);
>  qemu_add_opts(&qemu_trace_opts);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 54e920ada1a1..3b46fc9c1fc5 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
>  }
>  }
>
> +static bool error_is_detailed(void)
> +{
> +return !monitor_cur();
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>  QemuOpts *opts;
> @@ -2634,7 +2639,7 @@ void qemu_init(int argc, cha

Re: [PATCH 19/41] compiler.h: replace QEMU_NORETURN with G_NORETURN

[Warner Losh]

On Wed, Apr 20, 2022 at 7:28 AM  wrote:

> From: Marc-André Lureau 
>
> G_NORETURN was introduced in glib 2.68, fallback to G_GNUC_NORETURN in
> glib-compat.
>
> Note that this attribute must be placed before the function declaration
> (bringing a bit of consistency in qemu codebase usage).
>
> Signed-off-by: Marc-André Lureau 
>

Reviewed-by: Warner Losh 

Most of this looks mechanical, but I only looked closely at the bsd-user
changes.



> ---
>  accel/tcg/internal.h |  3 +--
>  include/exec/exec-all.h  | 20 +-
>  include/exec/helper-head.h   |  2 +-
>  include/glib-compat.h|  4 
>  include/hw/core/cpu.h|  2 +-
>  include/hw/core/tcg-cpu-ops.h|  6 +++---
>  include/hw/hw.h  |  2 +-
>  include/qemu/compiler.h  |  2 --
>  include/qemu/osdep.h |  3 ++-
>  include/qemu/thread.h|  2 +-
>  include/tcg/tcg-ldst.h   |  4 ++--
>  include/tcg/tcg.h|  2 +-
>  linux-user/user-internals.h  |  2 +-
>  scripts/cocci-macro-file.h   |  2 +-
>  target/alpha/cpu.h   | 10 -
>  target/arm/internals.h   | 12 +--
>  target/hppa/cpu.h|  2 +-
>  target/i386/tcg/helper-tcg.h | 24 ++---
>  target/microblaze/cpu.h  |  6 +++---
>  target/mips/tcg/tcg-internal.h   | 17 ---
>  target/nios2/cpu.h   |  6 +++---
>  target/openrisc/exception.h  |  2 +-
>  target/ppc/cpu.h | 14 ++---
>  target/ppc/internal.h|  6 +++---
>  target/riscv/cpu.h   | 10 -
>  target/s390x/s390x-internal.h|  6 +++---
>  target/s390x/tcg/tcg_s390x.h | 12 +--
>  target/sh4/cpu.h |  6 +++---
>  target/sparc/cpu.h   | 10 -
>  target/xtensa/cpu.h  |  6 +++---
>  accel/stubs/tcg-stub.c   |  4 ++--
>  bsd-user/signal.c|  3 ++-
>  hw/misc/mips_itu.c   |  3 ++-
>  linux-user/signal.c  |  3 ++-
>  monitor/hmp.c|  4 ++--
>  qemu-img.c   | 12 +++
>  target/alpha/helper.c| 10 -
>  target/arm/pauth_helper.c|  4 ++--
>  target/arm/tlb_helper.c  |  7 ---
>  target/hexagon/op_helper.c   |  9 
>  target/hppa/cpu.c|  8 +++
>  target/hppa/op_helper.c  |  4 ++--
>  target/i386/tcg/bpt_helper.c |  2 +-
>  target/i386/tcg/excp_helper.c| 31 ++--
>  target/i386/tcg/misc_helper.c|  6 +++---
>  target/i386/tcg/sysemu/misc_helper.c |  7 ---
>  target/openrisc/exception.c  |  2 +-
>  target/openrisc/exception_helper.c   |  3 ++-
>  target/riscv/op_helper.c |  4 ++--
>  target/rx/op_helper.c| 22 +++-
>  target/s390x/tcg/excp_helper.c   | 22 +++-
>  target/sh4/op_helper.c   |  5 +++--
>  target/sparc/mmu_helper.c|  8 +++
>  target/tricore/op_helper.c   |  6 +++---
>  tcg/tcg.c|  3 ++-
>  tests/fp/fp-bench.c  |  3 ++-
>  tests/fp/fp-test.c   |  3 ++-
>  scripts/checkpatch.pl|  2 +-
>  58 files changed, 214 insertions(+), 191 deletions(-)
>
> diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
> index 881bc1ede0b1..3092bfa96430 100644
> --- a/accel/tcg/internal.h
> +++ b/accel/tcg/internal.h
> @@ -14,8 +14,7 @@
>  TranslationBlock *tb_gen_code(CPUState *cpu, target_ulong pc,
>target_ulong cs_base, uint32_t flags,
>int cflags);
> -
> -void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
> +G_NORETURN void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
>  void page_init(void);
>  void tb_htable_init(void);
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index d2cb0981f405..311e5fb422a3 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -58,10 +58,10 @@ void restore_state_to_opc(CPUArchState *env,
> TranslationBlock *tb,
>   */
>  bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool
> will_exit);
>
> -void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
> -void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
> -void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
> -void QEMU_NORET

Re: [PATCH 06/41] include: rename qemu-common.h qemu/copyright.h

[Warner Losh]

On Wed, Apr 20, 2022 at 7:26 AM  wrote:

> From: Marc-André Lureau 
>
> Suggested-by: Peter Maydell 
> Signed-off-by: Marc-André Lureau 
>

Reviewed-by: Warner Losh 


> ---
>  include/{qemu-common.h => qemu/copyright.h} | 0
>  bsd-user/main.c | 2 +-
>  linux-user/main.c   | 2 +-
>  qemu-img.c  | 2 +-
>  qemu-io.c   | 2 +-
>  qemu-nbd.c  | 2 +-
>  qga/main.c  | 2 +-
>  scsi/qemu-pr-helper.c   | 2 +-
>  softmmu/vl.c| 2 +-
>  storage-daemon/qemu-storage-daemon.c| 2 +-
>  tools/virtiofsd/passthrough_ll.c| 2 +-
>  ui/cocoa.m  | 2 +-
>  12 files changed, 11 insertions(+), 11 deletions(-)
>  rename include/{qemu-common.h => qemu/copyright.h} (100%)
>
> diff --git a/include/qemu-common.h b/include/qemu/copyright.h
> similarity index 100%
> rename from include/qemu-common.h
> rename to include/qemu/copyright.h
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 88d347d05ebf..aaab3f278534 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -24,7 +24,7 @@
>  #include 
>
>  #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qemu/units.h"
>  #include "qemu/accel.h"
>  #include "sysemu/tcg.h"
> diff --git a/linux-user/main.c b/linux-user/main.c
> index fbc9bcfd5f5f..744d216b1e8e 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -18,7 +18,7 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qemu/units.h"
>  #include "qemu/accel.h"
>  #include "sysemu/tcg.h"
> diff --git a/qemu-img.c b/qemu-img.c
> index 116e05867558..a2b1d3653a1e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -25,7 +25,7 @@
>  #include "qemu/osdep.h"
>  #include 
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qemu/qemu-progress.h"
>  #include "qemu-version.h"
>  #include "qapi/error.h"
> diff --git a/qemu-io.c b/qemu-io.c
> index eb8afc8b413b..952a36643b0c 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -15,7 +15,7 @@
>  #include 
>  #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qapi/error.h"
>  #include "qemu-io.h"
>  #include "qemu/error-report.h"
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 713e7557a9eb..f4d121c0c40e 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -21,7 +21,7 @@
>  #include 
>  #include 
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "sysemu/block-backend.h"
> diff --git a/qga/main.c b/qga/main.c
> index ac63d8e47802..8994f73e4735 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -18,7 +18,7 @@
>  #include 
>  #include 
>  #endif
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qapi/qmp/json-parser.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qjson.h"
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index f281daeced8d..e7549ffb3bc9 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -36,7 +36,7 @@
>  #include 
>  #endif
>
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
>  #include "qemu/main-loop.h"
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 46aba6a039c4..b0bf16e16aaa 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -23,7 +23,7 @@
>   */
>
>  #include "qemu/osdep.h"
> -#include "qemu-common.h"
> +#include "qemu/copyright.h"
>  #include "qemu/datadir.h"
>  #include "qemu/units.h"
>  #include "exec/cpu-common.h"
> diff --git a/storage-daemon/qemu-storage-daemon.c
> b/storage-daemon/qemu-storage-daemon.c
> index eb724072579a..a4415e8c995b 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -42,7 +42,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/qobject-input-visitor.h"
>
> -#include "qemu-common.h

Re: [PATCH v4 4/8] tests: Refresh lcitool submodule

[Warner Losh]

On Fri, Jan 21, 2022 at 4:47 AM Daniel P. Berrangé 
wrote:

> On Fri, Jan 21, 2022 at 12:40:48PM +0100, Thomas Huth wrote:
> > On 21/01/2022 11.36, Philippe Mathieu-Daudé wrote:
> > > Refresh lcitool submodule and the generated files by running:
> > >
> > >$ make lcitool-refresh
> > >
> > > Reviewed-by: Daniel P. Berrangé 
> > > Signed-off-by: Philippe Mathieu-Daudé 
> > > ---
> > >   .gitlab-ci.d/cirrus/freebsd-12.vars   | 2 +-
> > >   .gitlab-ci.d/cirrus/freebsd-13.vars   | 2 +-
> > >   tests/docker/dockerfiles/alpine.docker| 3 ++-
> > >   tests/docker/dockerfiles/centos8.docker   | 3 +--
> > >   tests/docker/dockerfiles/fedora.docker| 3 +--
> > >   tests/docker/dockerfiles/opensuse-leap.docker | 2 +-
> > >   tests/docker/dockerfiles/ubuntu1804.docker| 2 +-
> > >   tests/docker/dockerfiles/ubuntu2004.docker| 2 +-
> > >   tests/lcitool/libvirt-ci  | 2 +-
> > >   9 files changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/.gitlab-ci.d/cirrus/freebsd-12.vars
> b/.gitlab-ci.d/cirrus/freebsd-12.vars
> > > index 9c52266811f..bdcce578edf 100644
> > > --- a/.gitlab-ci.d/cirrus/freebsd-12.vars
> > > +++ b/.gitlab-ci.d/cirrus/freebsd-12.vars
> > > @@ -11,6 +11,6 @@ MAKE='/usr/local/bin/gmake'
> > >   NINJA='/usr/local/bin/ninja'
> > >   PACKAGING_COMMAND='pkg'
> > >   PIP3='/usr/local/bin/pip-3.8'
> > > -PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache
> cdrkit-genisoimage ctags curl cyrus-sasl dbus diffutils dtc gettext git
> glib gmake gnutls gsed gtk3 libepoxy libffi libgcrypt libjpeg-turbo libnfs
> libspice-server libssh libtasn1 libxml2 llvm lttng-ust lzo2 meson ncurses
> nettle ninja opencv p5-Test-Harness perl5 pixman pkgconf png py38-numpy
> py38-pillow py38-pip py38-sphinx py38-sphinx_rtd_theme py38-virtualenv
> py38-yaml python3 rpm2cpio sdl2 sdl2_image snappy spice-protocol tesseract
> texinfo usbredir virglrenderer vte3 zstd'
> > > +PKGS='alsa-lib bash bzip2 ca_root_nss capstone4 ccache
> cdrkit-genisoimage ctags curl cyrus-sasl dbus diffutils dtc fusefs-libs3
> gettext git glib gmake gnutls gsed gtk3 libepoxy libffi libgcrypt
> libjpeg-turbo libnfs libspice-server libssh libtasn1 libxml2 llvm lttng-ust
> lzo2 meson ncurses nettle ninja opencv p5-Test-Harness perl5 pixman pkgconf
> png py38-numpy py38-pillow py38-pip py38-sphinx py38-sphinx_rtd_theme
> py38-virtualenv py38-yaml python3 rpm2cpio sdl2 sdl2_image snappy
> spice-protocol tesseract texinfo usbredir virglrenderer vte3 zstd'
> >
> > Aren't the FreeBSD jobs currently failing due to lttng-ust not being
> > available anymore? ... I'd somehow expected that this update might fix
> this,
> > too, but I still see lttng-ust in the list here?
>
> I had prepped an update to drop it, but @bsdimp indicated that it ought
> to be a transient problem so I never applied it, hoping it would fix
> itself.
>
> https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/211
>
> It has been broken for ~2 weeks now though, and lttng-ust is not an
> especially critical feature so we should probably just go ahead and
> disable it regardless.
>

I agree. I got some back and forth from the FreeBSD community about whether
or not
it was coming back. I think we should drop the package until that back and
forth results
in it being available again.

Warner

Re: [RFC 0/2] tls: add macros for coroutine-safe TLS variables

[Warner Losh]

On Mon, Oct 25, 2021 at 10:18 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 10/25/21 7:07 AM, Stefan Hajnoczi wrote:
> > This is a preview of how we can solve the coroutines TLS problem.
> Coroutines
> > re-entered from another thread sometimes see stale TLS values. This
> happens
> > because compilers may cache values across yield points, so a value from
> the
> > previous thread will be used when the coroutine is re-entered in another
> > thread.
>
> I'm not thrilled by this, but I guess it does work.
>
> It could be worthwhile to add some inline asm instead for specific hosts
> -- one
> instruction instead of an out-of-line call.
>
>
> > Serge Guelton developed this technique, see the first patch for details.
> I'm
> > submitting it for discussion before I go ahead with a full conversion of
> the
> > source tree.
> >
> > Todo:
> > - Convert all uses of __thread
> > - Extend checkpatch.pl to reject code that uses __thread
>
> Absolutely not.  *Perhaps* one or two tls variables which are accessible
> by coroutines,
> but there are plenty that have absolutely no relation.  Especially
> everything related to
> user-only execution.
>

I had the same worry. I'd also worry that the hoops that are jumped through
for
coroutines would somehow conflict with the low-level user-only execution
environment. I mean, it should be fine, but I know I'd be cranky if I traced
obscure regressions to being forced to use this construct...

Warner


> r~
>
>

Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Warner Losh]

> On Jun 24, 2021, at 4:56 AM, Peter Maydell  wrote:
> 
> On Thu, 24 Jun 2021 at 11:27, Tom Yan  wrote:
>> I really think we should get (/ have gotten) things clear first. What
>> exactly is the bug we have been talking about here? I mean like, where
>> does it occur and what's the nature of it.
>> 
>> 1. Is it specific to a certain type / model of backend / physical
>> storage device that will be made use of by qemu for the emulated
>> storage? (I presume not since you mention about image, unless you
>> irrationally related/bound the emulated storage type and the physical
>> storage type together.)
>> 
>> 2. Does it have anything to do with a certain flaw in qemu itself?
>> Like the code that does read/write operation is flawed that it cannot
>> be handled by a certain *proper* backend device?
>> 
>> 3. Or is it actually a bug in a certain driver / firmware blob that
>> will be used by an *emulated* device in the guest? In that case, can
>> we emulate another model so that it won't be using the problematic
>> driver / firmware?
> 
> Definitely agreed -- before we start changing QEMU code we need
> to identify clearly (a) what the real hardware does and (b) what
> the situation was we were originally trying to fix.

Real SD and MMC cards do not have power-of-two constraints in the
number of blocks. None of the SD/MMC bridges I’ve ever dealt with
have a power-of-two constraint on the size of the card. The only
constraint on the size related to the card is the block size.

If non-power-of-2 sized cards fail, that’s a cat-1 sev-1 bug that needs
to be fixed, rather than a warning be generated.

Warner


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Warner Losh]

On Wed, May 5, 2021, 5:10 PM Eric Blake  wrote:

> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
> > +Eric
> >
> > On 5/5/21 11:22 PM, Keith Busch wrote:
> >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
> >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
> >>> a constant! Help it by using a definitions instead.
> >>
> >> I don't understand.
> >
> > Neither do I TBH...
> >
> >> It's labeled 'const', so any reasonable compiler
> >> will place it in the 'text' segment of the executable rather than on the
> >> stack. While that's compiler specific, is there really a compiler doing
> >> something bad with this? If not, I do prefer the 'const' here if only
> >> because it limits the symbol scope ('static const' is also preferred if
> >> that helps).
> >
> > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
> >
> > Both static+const / const trigger:
> >
> > hw/block/nvme.c: In function ‘nvme_map_sgl’:
> > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
> > ‘segment’ [-Werror=vla]
> >   818 | NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
> >   | ^
> > cc1: all warnings being treated as errors
>
> C99 6.7.5.2 paragraph 4:
> "If the size is an integer constant expression and the element type has
> a known constant size, the array type is not a variable length array
> type; otherwise, the array type is a variable length array type."
>
> 6.6 paragraph 6:
> "An integer constant expression shall have integer type and shall only
> have operands that are integer constants, enumeration constants,
> character constants, sizeof expressions whose results are integer
> constants, and floating constants that are the immediate operands of
> casts. Cast operators in an integer constant expression shall only
> convert arithmetic types to integer types, except as part of an operand
> to the sizeof operator."
>
> Notably absent from that list are 'const int' variables, which even
> though they act as constants (in that the name always represents the
> same value), do not actually qualify as such under C99 rules.  Yes, it's
> a pain.  What's more, 6.6 paragraph 10:
>
> "An implementation may accept other forms of constant expressions."
>
> which means it _should_ be possible for the compiler to do what we want.
>  But just because it is permitted does not make it actually work. :(
>
> And while C17 expands the list of integer constant expressions to
> include _Alignof expressions, it does not add any wording to permit
> const variables.
>
>
> https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c
> helps with this explanation:
> "The thing to remember (and yes, this is a bit counterintuitive) is that
> const doesn't mean constant. A constant expression is, roughly, one that
> can be evaluated at compile time (like 2+2 or 42). The const type
> qualifier, even though its name is obviously derived from the English
> word "constant", really means "read-only".
>
> Consider, for example, that these are a perfectly valid declarations:
>
> const int r = rand();
> const time_t now = time(NULL);
>
> The const just means that you can't modify the value of r or now after
> they've been initialized. Those values clearly cannot be determined
> until execution time."
>
> And C++ _does_ support named constants, but we're using C, not C++.
>

Enum is as close as it gets in C if you are eschewing #define.

Warner

-- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>
>
>

Re: [PATCH 2/3] hw/ide: Add Kconfig dependency MICRODRIVE -> PCMCIA

[Warner Losh]

On Sat, Apr 24, 2021 at 4:24 PM Philippe Mathieu-Daudé 
wrote:

> The Microdrive Compact Flash can be plugged on a PCMCIA bus.
> Express the dependency using the 'depends on' Kconfig expression.
>

Reviewed-by: Warner Losh 


> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
> index 5d9106b1ac2..8e2c8934549 100644
> --- a/hw/ide/Kconfig
> +++ b/hw/ide/Kconfig
> @@ -41,6 +41,7 @@ config IDE_VIA
>  config MICRODRIVE
>  bool
>  select IDE_QDEV
> +depends on PCMCIA
>
>  config AHCI
>  bool
> --
> 2.26.3
>
>
>

Re: [PATCH 1/3] hw/arm/pxa2xx: Declare PCMCIA bus with Kconfig

[Warner Losh]

On Sat, Apr 24, 2021 at 4:22 PM Philippe Mathieu-Daudé 
wrote:

> The Intel XScale PXA chipsets provide a PCMCIA controller,
> which expose a PCMCIA (IDE) bus. Express this dependency using
> the Kconfig 'select' expression.
>

I'd consider dropping the (IDE) in the description of the PCMCIA
bus since I think it's more confusing than helpful...

Reviewed-by: Warner Losh 


> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 8c37cf00da7..b887f6a5b17 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -142,6 +142,7 @@ config PXA2XX
>  select SD
>  select SSI
>  select USB_OHCI
> +select PCMCIA
>
>  config GUMSTIX
>  bool
> --
> 2.26.3
>
>
>

Re: [RFC PATCH 3/3] hw/block/nvme: add nvme_inject_state HMP command

[Warner Losh]

On Wed, Feb 10, 2021, 9:26 PM Keith Busch  wrote:

> On Thu, Feb 11, 2021 at 12:38:48PM +0900, Minwoo Im wrote:
> > On 21-02-11 12:00:11, Keith Busch wrote:
> > > But I would prefer to see advanced retry tied to real errors that can
> be
> > > retried, like if we got an EBUSY or EAGAIN errno or something like
> that.
> >
> > I have seen a thread [1] about ACRE.  Forgive me If I misunderstood this
> > thread or missed something after this thread.  It looks like CRD field in
> > the CQE can be set for any NVMe error state which means it *may* depend
> on
> > the device status.
>
> Right! Setting CRD values is at the controller's discretion for any
> error status as long as the host enables ACRE.
>
> > And this patch just introduced a internal temporarily error state of
> > the controller by returning Command Intrrupted status.
>
> It's just purely synthetic, though. I was hoping something more natural
> could trigger the status. That might not provide the deterministic
> scenario you're looking for, though.
>
> I'm not completely against using QEMU as a development/test vehicle for
> corner cases like this, but we are introducing a whole lot of knobs
> recently, and you practically need to be a QEMU developer to even find
> them. We probably should step up the documentation in the wiki along
> with these types of features.
>

I'd love that too... I need to test FreeBSD's nvme driver for different
error conditions. I know qemu can help, but it's a bit obscure.

Warner

> I think, in this stage, we can go with some errors in the middle of the
> > AIO (nvme_aio_err()) for advanced retry.  Shouldn't AIO errors are
> > retry-able and supposed to be retried ?
>
> Sure, we can assume that receiving an error in the AIO callback means
> the lower layers exhausted available recovery mechanisms.
>
>

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne  wrote:

> Some BSD platforms do not have this header.
>
> Signed-off-by: Joelle van Dyne 
> ---
>  meson.build| 1 +
>  block.c| 2 +-
>  block/file-posix.c | 2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 27110075df..6818d97df5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
>  config_host_data.set('HAVE_SYS_IOCCOM_H', cc.has_header('sys/ioccom.h'))
>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
>
>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> diff --git a/block.c b/block.c
> index 8b9d457546..c4cf391dea 100644
> --- a/block.c
> +++ b/block.c
> @@ -54,7 +54,7 @@
>  #ifdef CONFIG_BSD
>  #include 
>  #include 
> -#ifndef __DragonFly__
> +#if defined(HAVE_SYS_DISK_H)
>  #include 
>  #endif
>  #endif
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 11d2021346..666d3e7504 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2320,7 +2320,7 @@ again:
>  }
>  if (size == 0)
>  #endif
> -#if defined(__APPLE__) && defined(__MACH__)
> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
>

Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
__MACH__

Warner


>  {
>  uint64_t sectors = 0;
>  uint32_t sector_size = 0;
> --
> 2.28.0
>
>
>

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

On Tue, Jan 26, 2021 at 12:08 AM Philippe Mathieu-Daudé 
wrote:

> On 1/26/21 6:55 AM, Joelle van Dyne wrote:
> > Previously, the only case where sys/disk.h does not exist is on
> > platforms that define __DragonFly__. However, iOS also does not have
> > this header. Previously, I had it as
> >
> > #if defined(__DragonFly__) || defined(CONFIG_IOS)
> >
> > But there was a code review comment that we should use feature flags
> > instead of platform defines. So I added the HAS_SYS_DISK_H flag.
>
> On technical lists, it's best to avoid top-posting, and to
> instead reply inline to make the conversation easier to follow.
>
> >
> > -j
> >
> > On Mon, Jan 25, 2021 at 8:35 PM Warner Losh  wrote:
> >>
> >>
> >>
> >> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne  wrote:
> >>>
> >>> Some BSD platforms do not have this header.
> >>>
> >>> Signed-off-by: Joelle van Dyne 
> >>> ---
> >>>  meson.build| 1 +
> >>>  block.c| 2 +-
> >>>  block/file-posix.c | 2 +-
> >>>  3 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/meson.build b/meson.build
> >>> index 27110075df..6818d97df5 100644
> >>> --- a/meson.build
> >>> +++ b/meson.build
> >>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
> >>>  config_host_data.set('HAVE_SYS_IOCCOM_H',
> cc.has_header('sys/ioccom.h'))
> >>>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> >>>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> >>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
> >>>
> >>>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
> >>>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> >>> diff --git a/block.c b/block.c
> >>> index 8b9d457546..c4cf391dea 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -54,7 +54,7 @@
> >>>  #ifdef CONFIG_BSD
> >>>  #include 
> >>>  #include 
> >>> -#ifndef __DragonFly__
> >>> +#if defined(HAVE_SYS_DISK_H)
> >>>  #include 
> >>>  #endif
> >>>  #endif
> >>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>> index 11d2021346..666d3e7504 100644
> >>> --- a/block/file-posix.c
> >>> +++ b/block/file-posix.c
> >>> @@ -2320,7 +2320,7 @@ again:
> >>>  }
> >>>  if (size == 0)
> >>>  #endif
> >>> -#if defined(__APPLE__) && defined(__MACH__)
> >>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) &&
> defined(__MACH__)
> >>
> >>
> >> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
> __MACH__
>
> Hmm we could also add:
>
>   config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT', cc.compiles(...))
>
> Then this block would be easier to read:
>
>   #if defined(HAVE_DKIOCGETBLOCKCOUNT)
>   ...
>
> (Maybe this is what Warner meant?)
>

Close. I'd test it more directly since DKIOCGETBLOCKCOUNT is already a
#define, and is unlikely to change...

When I saw Joelle's response, I realized I'd been needlessly cryptic in my
comments, so posted what I had in mind for cleanup. I'm not sure if the
norms of qemu code reviews would say my suggestion was too big to be in
scope, or not.

Warner

>>
> >> Warner
> >>
> >>>
> >>>  {
> >>>  uint64_t sectors = 0;
> >>>  uint32_t sector_size = 0;
> >>> --
> >>> 2.28.0
> >>>
> >>>
> >
>
>

Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Warner Losh]

On Mon, Jan 25, 2021 at 10:55 PM Joelle van Dyne  wrote:

> Previously, the only case where sys/disk.h does not exist is on
> platforms that define __DragonFly__. However, iOS also does not have
> this header. Previously, I had it as
>
> #if defined(__DragonFly__) || defined(CONFIG_IOS)
>
> But there was a code review comment that we should use feature flags
> instead of platform defines. So I added the HAS_SYS_DISK_H flag.
>

Right. I like that the #include is now protected like that. However,
sys/disk.h never was standardized and varies considerably among the systems
that it exists on.


> -j
>
> On Mon, Jan 25, 2021 at 8:35 PM Warner Losh  wrote:
> >
> >
> >
> > On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne  wrote:
> >>
> >> Some BSD platforms do not have this header.
> >>
> >> Signed-off-by: Joelle van Dyne 
> >> ---
> >>  meson.build| 1 +
> >>  block.c| 2 +-
> >>  block/file-posix.c | 2 +-
> >>  3 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index 27110075df..6818d97df5 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H',
> cc.has_header('pty.h'))
> >>  config_host_data.set('HAVE_SYS_IOCCOM_H',
> cc.has_header('sys/ioccom.h'))
> >>  config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h'))
> >>  config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device)
> >> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h'))
> >>
> >>  ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target
> >>  arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST',
> 'CONFIG_BDRV_RO_WHITELIST']
> >> diff --git a/block.c b/block.c
> >> index 8b9d457546..c4cf391dea 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -54,7 +54,7 @@
> >>  #ifdef CONFIG_BSD
> >>  #include 
> >>  #include 
> >> -#ifndef __DragonFly__
> >> +#if defined(HAVE_SYS_DISK_H)
> >>  #include 
> >>  #endif
> >>  #endif
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index 11d2021346..666d3e7504 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -2320,7 +2320,7 @@ again:
> >>  }
> >>  if (size == 0)
> >>  #endif
> >> -#if defined(__APPLE__) && defined(__MACH__)
> >> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && defined(__MACH__)
> >
> >
> > Why is this needed? __DragonFly__ doesn't define either __APPLE__ or
> __MACH_
>

Which is why I asked this question...

Let me ask it another way. Why not base this on the
ioctl  DKIOCGETBLOCKCOUNT like the rest of this function? It's simple and
on platforms that don't have that ioctl, it won't be used.  I don't even
know how to read the proposed change logically. If IOS doesn't have this
interface, then you'll need another #else  to work reliably
anyway, since the seek trick that's used there may or may not work. However
that starts to get kinda nested and twisty.  So maybe something more like
the following would make it clearer... though that might be beyond the
scope of what you're trying to do.

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..704ded68b0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2295,8 +2295,10 @@ static int64_t raw_getlength(BlockDriverState *bs)
 again:
 #endif
 if (!fstat(fd, &sb) && (S_IFCHR & sb.st_mode)) {
+size = 0;
 #ifdef DIOCGMEDIASIZE
 if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size))
+size = 0;
 #elif defined(DIOCGPART)
 {
 struct partinfo pi;
@@ -2305,9 +2307,7 @@ again:
 else
 size = 0;
 }
-if (size == 0)
-#endif
-#if defined(__APPLE__) && defined(__MACH__)
+#elif defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)
 {
 uint64_t sectors = 0;
 uint32_t sector_size = 0;
@@ -2315,19 +2315,15 @@ again:
 if (ioctl(fd, DKIOCGETBLOCKCOUNT, §ors) == 0
&& ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) == 0) {
 size = sectors * sector_size;
-} else {
-size = lseek(fd, 0LL, SEEK_END);
-if (size < 0) {
-return -errno;
-}
 }
 }
-#else
-size = lseek(fd, 0LL, SEEK_END);
+#endif
+if (size == 0) {
+size = lseek(fd, 0LL, SEEK_END);
+}
 if (size < 0) {
 return -errno;
 }
-#endif
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 switch(s->type) {
 case FTYPE_CD:

Warner

>
> >>
> >>  {
> >>  uint64_t sectors = 0;
> >>  uint32_t sector_size = 0;
> >> --
> >> 2.28.0
> >>
> >>
>