Re: [Qemu-devel] [PATCH v2 0/2] Add tests for the TPM TIS

[Marc-Andre Lureau]

On Thu, Feb 15, 2018 at 3:10 PM, Stefan Berger
 wrote:
> This series of patches adds tests for the TPM TIS.
>
>Stefan
>
> v1->v2:
>  - moved common TPM test code into tpm-emu.c
>  - clearified a few comments in tpm-tis-test.c, no code changes
>
> Stefan Berger (2):
>   tests: Move common TPM test code into tpm-emu.c
>   tests: add test for TPM TIS device

series:

Reviewed-by: Marc-André Lureau 




>
>  MAINTAINERS|   1 +
>  hw/tpm/tpm_tis.c   | 101 --
>  include/hw/acpi/tpm.h  | 105 +++
>  tests/Makefile.include |   4 +-
>  tests/tpm-crb-test.c   | 174 +-
>  tests/tpm-emu.c| 175 ++
>  tests/tpm-emu.h|  38 
>  tests/tpm-tis-test.c   | 486 
> +
>  8 files changed, 811 insertions(+), 273 deletions(-)
>  create mode 100644 tests/tpm-emu.c
>  create mode 100644 tests/tpm-emu.h
>  create mode 100644 tests/tpm-tis-test.c
>
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH RFC 14/14] qapi: Add #if conditions to commands, events, types, visitors

[Marc-Andre Lureau]

Hi

On Mon, Feb 12, 2018 at 8:22 AM, Markus Armbruster  wrote:
> Example change to generated code:
>
> diff -rup test-qapi-events.h.old test-qapi-events.h
> --- test-qapi-events.h.old  2018-02-12 07:02:45.672737544 +0100
> +++ test-qapi-events.h  2018-02-12 07:03:01.128517669 +0100
> @@ -30,8 +30,10 @@ void qapi_event_send_event_e(UserDefZero
>  void qapi_event_send_event_f(UserDefAlternate *arg, Error **errp);
>
>  void qapi_event_send___org_qemu_x_event(__org_qemu_x_Enum 
> __org_qemu_x_member1, const char *__org_qemu_x_member2, bool has_q_wchar_t, 
> int64_t q_wchar_t, Error **errp);
> +#if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
>
>  void qapi_event_send_testifevent(TestIfStruct *foo, Error **errp);
> +#endif /* defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) */
>
>  typedef enum test_QAPIEvent {
>  TEST_QAPI_EVENT_EVENT_A = 0,
>
> TODO nice blank lines before #if and after #endif
> FIXME unclean access of protected members in commands.py
>
> Signed-off-by: Markus Armbruster 

I reviewed the RFC series, and your approach to visit ifcond instead
of having them as attribute argument for the visitor (something you
suggested me initially iirc).

I think the approach is interesting but that single patch shows the
complexity involved. The decorator approach still looks cleaner and
simpler to me. Furthermore, I don't fancy much having to redo and tune
the generation *again* to fix the inden, extra-spaces etc that were
fixed after several revisions (it takes hours to get there, it's
boring). Can't we go first with my approach and then look at replacing
it? Can't one add a "FIXME: replace the decorator with something less
magic" at ifcond_decorator definition for now? Is this code so
critical that it has to be the way you want in the first place? The
approach to take it first and improve it worked very well for
qapi2texi, it just took a few more days for you (and reviewers) to
improve it. I'd suggest we work that way instead of having me rewrite
and rewrite until you are happy (which is something I can't do right
without many painful iterations for you and me).

thanks

> ---
>  scripts/qapi/commands.py |  7 ++-
>  scripts/qapi/common.py   | 37 +
>  tests/test-qmp-cmds.c|  4 ++--
>  3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 21a7e0dbe6..439a8714e2 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -276,8 +276,13 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  self._visited_ret_types[self._genc].add(ret_type)
>  self._genc.add(gen_marshal_output(ret_type))
>  self._genh.add(gen_marshal_decl(name))
> -self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> +self._genc.add(gen_marshal(name, arg_type, boxed, ret_type,))
> +# FIXME unclean access of protected members
> +if self._genc._open_ifcond:
> +self._regy += gen_if(self._genc._open_ifcond)
>  self._regy += gen_register_command(name, success_response)
> +if self._genc._open_ifcond:
> +self._regy += gen_endif(self._genc._open_ifcond)
>
>
>  def gen_commands(schema, output_dir, prefix):
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 1a78dfaf3f..164d3e2daa 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -2122,6 +2122,9 @@ class QAPIGenC(QAPIGen):
>  self._blurb = blurb.strip('\n')
>  self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>re.MULTILINE))
> +self._open_ifcond = None
> +self._preamble_needs_ifcond = False
> +self._body_needs_ifcond = False
>
>  def _top(self, fname):
>  return mcgen('''
> @@ -2139,6 +2142,36 @@ class QAPIGenC(QAPIGen):
>  ''',
>   blurb=self._blurb, copyright=self._copyright)
>
> +def ifcond(self, ifcond, begin):
> +if begin:
> +assert not self._open_ifcond
> +self._open_ifcond = ifcond
> +self._preamble_needs_ifcond = True
> +self._body_needs_ifcond = True
> +else:
> +assert self._open_ifcond == ifcond
> +if not self._preamble_needs_ifcond:
> +QAPIGen.preamble_add(self, gen_endif(ifcond))
> +# TODO emit blank line
> +if not self._body_needs_ifcond:
> +QAPIGen.add(self, gen_endif(ifcond))
> +# TODO emit blank line
> +self._open_ifcond = None
> +
> +def preamble_add(self, text):
> +if self._open_ifcond and self._preamble_needs_ifcond:
> +# TODO emit blank line
> +QAPIGen.preamble_add(self, gen_if(self._open_ifcond))
> +self._preamble_needs_ifcond = False
> 

Re: [Qemu-devel] [PATCH v2 29/29] qapi: Don't create useless directory qapi-generated

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> We used to generate first test and later QGA QAPI code into
> qapi-generated/.  Commit b93b63f574 moved the test code to tests/.
> Commit 54c2e50205 moved the QGA code to qga/qapi-generated/.  The
> directory has been unused since.
>
> Signed-off-by: Markus Armbruster 

good catch,
Reviewed-by: Marc-André Lureau 


> ---
>  .gitignore | 1 -
>  Makefile   | 1 -
>  configure  | 1 -
>  3 files changed, 3 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index dabfe6bea8..4055e12ee8 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -27,7 +27,6 @@
>  /libuser
>  /linux-headers/asm
>  /qga/qapi-generated
> -/qapi-generated
>  /qapi-gen-timestamp
>  /qapi/qapi-builtin-types.[ch]
>  /qapi/qapi-builtin-visit.[ch]
> diff --git a/Makefile b/Makefile
> index 84411ee6ab..7eb1ede6f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -726,7 +726,6 @@ clean:
> rm -f trace/generated-tracers-dtrace.h*
> rm -f $(foreach f,$(GENERATED_FILES),$(f) $(f)-timestamp)
> rm -f qapi-gen-timestamp
> -   rm -rf qapi-generated
> rm -rf qga/qapi-generated
> for d in $(ALL_SUBDIRS); do \
> if test -d $$d; then $(MAKE) -C $$d $@ || exit 1; fi; \
> diff --git a/configure b/configure
> index 62562f08cf..f564a1639e 100755
> --- a/configure
> +++ b/configure
> @@ -7015,7 +7015,6 @@ DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 
> tests/libqos tests/qapi-sche
>  DIRS="$DIRS docs docs/interop fsdev scsi"
>  DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
>  DIRS="$DIRS roms/seabios roms/vgabios"
> -DIRS="$DIRS qapi-generated"
>  FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
>  FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
>  FILES="$FILES tests/tcg/lm32/Makefile tests/tcg/xtensa/Makefile po/Makefile"
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 25/29] docs/devel/writing-qmp-commands: Update for modular QAPI

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> With modular code generation, putting stuff right into
> qapi-schema.json is a bad idea.  Update writing-qmp-commands.txt
> accordingly.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  docs/devel/writing-qmp-commands.txt | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/docs/devel/writing-qmp-commands.txt 
> b/docs/devel/writing-qmp-commands.txt
> index 4f5b24c0c4..776b3b41ca 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -15,8 +15,8 @@ start with docs/interop/qmp-intro.txt.
>  Generally speaking, the following steps should be taken in order to write a
>  new QMP command.
>
> -1. Write the command's and type(s) specification in the QAPI schema file
> -   (qapi-schema.json in the root source directory)
> +1. Define the command and any types it needs in the appropriate QAPI
> +   schema module.
>
>  2. Write the QMP command itself, which is a regular C function. Preferably,
> the command should be exported by some QEMU subsystem. But it can also be
> @@ -88,8 +88,9 @@ command carries some meaningful action in QEMU but here it 
> will just print
>  Our command will be called "hello-world". It takes no arguments, nor does it
>  return any data.
>
> -The first step is to add the following line to the bottom of the
> -qapi-schema.json file:
> +The first step is defining the command in the appropriate QAPI schema
> +module.  We pick module qapi/misc.json, and add the following line at
> +the bottom:
>
>  { 'command': 'hello-world' }
>
> @@ -245,7 +246,7 @@ This is very important. No QMP command will be accepted 
> in QEMU without proper
>  documentation.
>
>  There are many examples of such documentation in the schema file already, but
> -here goes "hello-world"'s new entry for the qapi-schema.json file:
> +here goes "hello-world"'s new entry for qapi/misc.json:
>
>  ##
>  # @hello-world
> @@ -425,8 +426,7 @@ There are a number of things to be noticed:
> allocated by the implementation. This is so because the QAPI also 
> generates
> a function to free its types and it cannot distinguish between dynamically
> or statically allocated strings
> -6. You have to include the "qmp-commands.h" header file in qemu-timer.c,
> -   otherwise qemu won't build
> +6. You have to include "qapi/qmp-commands-misc.h" in qemu-timer.c
>
>  Time to test the new command. Build qemu, run it as described in the 
> "Testing"
>  section and try this:
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 28/29] Fix up dangling references to qmp-commands.* in comment and doc

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> Fix up the reference to qmp-commands.hx in qmp.c.  Missed in commit
> 5032a16d1d.
>
> Fix up the reference to qmp-commands.txt in
> docs/xen-save-devices-state.txt.  Missed in commit 4d8bb958fa.
>
> Signed-off-by: Markus Armbruster 
> ---
>  docs/xen-save-devices-state.txt |  3 +--
>  qmp.c   | 14 +++---
>  2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/docs/xen-save-devices-state.txt b/docs/xen-save-devices-state.txt
> index a72ecc8081..1912ecad25 100644
> --- a/docs/xen-save-devices-state.txt
> +++ b/docs/xen-save-devices-state.txt
> @@ -8,8 +8,7 @@ These operations are normally used with migration (see 
> migration.txt),
>  however it is also possible to save the state of all devices to file,
>  without saving the RAM or the block devices of the VM.
>
> -This operation is called "xen-save-devices-state" (see
> -qmp-commands.txt)
> +The save operation is available as QMP command xen-save-devices-state.
>
>
>  The binary format used in the file is the following:
> diff --git a/qmp.c b/qmp.c
> index a8d4eba973..ba82e1df9f 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -147,13 +147,13 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>
>  #ifndef CONFIG_SPICE
>  /*
> - * qmp-commands.hx ensures that QMP command query-spice exists only
> - * #ifdef CONFIG_SPICE.  Necessary for an accurate query-commands
> - * result.  However, the QAPI schema is blissfully unaware of that,
> - * and the QAPI code generator happily generates a dead
> - * qmp_marshal_query_spice() that calls qmp_query_spice().  Provide it
> - * one, or else linking fails.  FIXME Educate the QAPI schema on
> - * CONFIG_SPICE.
> + * qmp_unregister_commands_hack() ensures that QMP command query-spice
> + * exists only #ifdef CONFIG_SPICE.  Necessary for an accurate
> + * query-commands result.  However, the QAPI schema is blissfully
> + * unaware of that, and the QAPI code generator happily generates a
> + * dead qmp_marshal_query_spice() that calls qmp_query_spice().
> + * Provide it one, or else linking fails.  FIXME Educate the QAPI
> + * schema on CONFIG_SPICE.
>   */

I hope this comment will go away soon,
Reviewed-by: Marc-André Lureau 


>  SpiceInfo *qmp_query_spice(Error **errp)
>  {
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 27/29] qapi: Move qapi-schema.json to qapi/, rename generated files

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> Move qapi-schema.json to qapi/, so it's next to its modules, and all
> files get generated to qapi/, not just the ones generated for modules.
>
> Consistently name the generated files qapi-MODULE.EXT:
> qmp-commands.[ch] become qapi-commands.[ch], qapi-event.[ch] become
> qapi-events.[ch], and qmp-introspect.[ch] become qapi-introspect.[ch].
> This gets rid of the temporary hacks in scripts/qapi/commands.py and
> scripts/qapi/events.py.
>
> Signed-off-by: Markus Armbruster 


Reviewed-by: Marc-André Lureau 


> ---
>  .gitignore| 16 ++--
>  Makefile  | 42 
> +++
>  Makefile.objs | 21 
>  backends/hostmem.c|  2 +-
>  docs/devel/qapi-code-gen.txt  | 30 +++---
>  docs/devel/writing-qmp-commands.txt   |  2 +-
>  docs/interop/qmp-intro.txt|  2 +-
>  hmp.c |  2 +-
>  include/qapi/qmp/qobject.h|  2 +-
>  include/qapi/visitor.h|  2 +-
>  include/qom/object.h  |  2 +-
>  monitor.c |  6 ++---
>  net/filter-buffer.c   |  2 +-
>  qapi/misc.json|  4 +--
>  qapi-schema.json => qapi/qapi-schema.json | 32 +++
>  qga/Makefile.objs |  2 +-
>  qga/commands-posix.c  |  2 +-
>  qga/commands-win32.c  |  2 +-
>  qga/commands.c|  2 +-
>  qga/main.c|  2 +-
>  qom/object.c  |  2 +-
>  scripts/qapi/commands.py  |  7 --
>  scripts/qapi/events.py|  9 +--
>  scripts/qapi/introspect.py|  4 +--
>  scripts/qapi/types.py |  6 ++---
>  scripts/qapi/visit.py |  6 ++---
>  tests/.gitignore  |  6 ++---
>  tests/Makefile.include| 14 +--
>  tests/test-qmp-cmds.c |  2 +-
>  tests/test-qmp-event.c|  2 +-
>  tests/test-qobject-input-visitor.c|  6 ++---
>  tpm.c |  1 -
>  ui/cocoa.m|  2 +-
>  ui/vnc.c  |  2 +-
>  34 files changed, 116 insertions(+), 130 deletions(-)
>  rename qapi-schema.json => qapi/qapi-schema.json (85%)
>
> diff --git a/.gitignore b/.gitignore
> index 7f162e862f..dabfe6bea8 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -29,8 +29,8 @@
>  /qga/qapi-generated
>  /qapi-generated
>  /qapi-gen-timestamp
> -/qapi-builtin-types.[ch]
> -/qapi-builtin-visit.[ch]
> +/qapi/qapi-builtin-types.[ch]
> +/qapi/qapi-builtin-visit.[ch]
>  /qapi/qapi-commands-block-core.[ch]
>  /qapi/qapi-commands-block.[ch]
>  /qapi/qapi-commands-char.[ch]
> @@ -47,6 +47,7 @@
>  /qapi/qapi-commands-trace.[ch]
>  /qapi/qapi-commands-transaction.[ch]
>  /qapi/qapi-commands-ui.[ch]
> +/qapi/qapi-commands.[ch]
>  /qapi/qapi-events-block-core.[ch]
>  /qapi/qapi-events-block.[ch]
>  /qapi/qapi-events-char.[ch]
> @@ -63,6 +64,8 @@
>  /qapi/qapi-events-trace.[ch]
>  /qapi/qapi-events-transaction.[ch]
>  /qapi/qapi-events-ui.[ch]
> +/qapi/qapi-events.[ch]
> +/qapi/qapi-introspect.[ch]
>  /qapi/qapi-types-block-core.[ch]
>  /qapi/qapi-types-block.[ch]
>  /qapi/qapi-types-char.[ch]
> @@ -79,7 +82,7 @@
>  /qapi/qapi-types-trace.[ch]
>  /qapi/qapi-types-transaction.[ch]
>  /qapi/qapi-types-ui.[ch]
> -/qapi-types.[ch]
> +/qapi/qapi-types.[ch]
>  /qapi/qapi-visit-block-core.[ch]
>  /qapi/qapi-visit-block.[ch]
>  /qapi/qapi-visit-char.[ch]
> @@ -96,11 +99,8 @@
>  /qapi/qapi-visit-trace.[ch]
>  /qapi/qapi-visit-transaction.[ch]
>  /qapi/qapi-visit-ui.[ch]
> -/qapi-visit.[ch]
> -/qapi-event.[ch]
> -/qapi-doc.texi
> -/qmp-commands.[ch]
> -/qmp-introspect.[ch]
> +/qapi/qapi-visit.[ch]
> +/qapi/qapi-doc.texi
>  /qemu-doc.html
>  /qemu-doc.info
>  /qemu-doc.txt
> diff --git a/Makefile b/Makefile
> index 50eb194877..84411ee6ab 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -90,8 +90,8 @@ endif
>  include $(SRC_PATH)/rules.mak
>
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> -GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
> -GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi/qapi-builtin-types.h qapi/qapi-builtin-types.c
> +GENERATED_FILES += qapi/qapi-types.h qapi/qapi-types.c
>  GENERATED_FILES += qapi/qapi-types-block-core.h qapi/qapi-types-block-core.c
>  GENERATED_FILES += qapi/qapi-types-block.h qapi/qapi-types-block.c
>  GENERATED_FILES += qapi/qapi-types-char.h qapi/qapi-types-char.c
> @@ -108,8 +108,8 @@ GENERATED_FILES += qapi/qapi-types-tpm.h 
> 

Re: [Qemu-devel] [PATCH v2 26/29] docs: Correct outdated information on QAPI

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> * Fix guidance on error classes
>
> * Point to generated documentation
>
> * Drop plea for documentation, because the QAPI code generator
>   enforces it since commit 3313b6124b
>
> * Minor tweaks here and there
>
> Signed-off-by: Markus Armbruster 

looks good to me,
Reviewed-by: Marc-André Lureau 


> ---
>  docs/devel/writing-qmp-commands.txt | 25 +
>  docs/interop/qmp-intro.txt  |  3 ++-
>  2 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/docs/devel/writing-qmp-commands.txt 
> b/docs/devel/writing-qmp-commands.txt
> index 776b3b41ca..50385eff27 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -36,9 +36,9 @@ very simple and get more complex as we progress.
>  For all the examples in the next sections, the test setup is the same and is
>  shown here.
>
> -First, QEMU should be started as:
> +First, QEMU should be started like this:
>
> -# /path/to/your/source/qemu [...] \
> +# qemu-system-TARGET [...] \
>  -chardev socket,id=qmp,port=,host=localhost,server \
>  -mon chardev=qmp,mode=control,pretty=on
>
> @@ -179,7 +179,7 @@ described in the "Testing" section and then send two 
> commands:
>  }
>  }
>
> -You should see "Hello, world" and "we love qemu" in the terminal running 
> qemu,
> +You should see "Hello, world" and "We love qemu" in the terminal running 
> qemu,
>  if you don't see these strings, then something went wrong.
>
>  === Errors ===
> @@ -221,30 +221,23 @@ The QMP server's response should be:
>  }
>  }
>
> -As a general rule, all QMP errors should use ERROR_CLASS_GENERIC_ERROR
> -(done by default when using error_setg()). There are two exceptions to
> -this rule:
> +Note that error_setg() produces a "GenericError" class.  In general,
> +all QMP errors should have that error class.  There are two exceptions
> +to this rule:
>
> - 1. A non-generic ErrorClass value exists* for the failure you want to report
> -(eg. DeviceNotFound)
> + 1. To support a management application's need to recognize a specific
> +error for special handling
>
> - 2. Management applications have to take special action on the failure you
> -want to report, hence you have to add a new ErrorClass value so that they
> -can check for it
> + 2. Backward compatibility
>
>  If the failure you want to report falls into one of the two cases above,
>  use error_set() with a second argument of an ErrorClass value.
>
> - * All existing ErrorClass values are defined in the qapi-schema.json file
> -
>  === Command Documentation ===
>
>  There's only one step missing to make "hello-world"'s implementation 
> complete,
>  and that's its documentation in the schema file.
>
> -This is very important. No QMP command will be accepted in QEMU without 
> proper
> -documentation.
> -
>  There are many examples of such documentation in the schema file already, but
>  here goes "hello-world"'s new entry for qapi/misc.json:
>
> diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt
> index adbc94abb1..430fe1b747 100644
> --- a/docs/interop/qmp-intro.txt
> +++ b/docs/interop/qmp-intro.txt
> @@ -78,7 +78,8 @@ Escape character is '^]'.
>  }
>  }
>
> -Please, refer to the qapi-schema.json file for a complete command reference.
> +Please refer to docs/interop/qemu-qmp-ref.* for a complete command
> +reference, generated from qapi-schema.json.
>
>  QMP wiki page
>  -
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 23/29] Include less of the generated modular QAPI headers

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:36 AM, Markus Armbruster  wrote:
> In my "build everything" tree, a change to the types in
> qapi-schema.json triggers a recompile of about 4800 out of 5100
> objects.
>
> The previous commit split up qmp-commands.h, qmp-event.h, qmp-visit.h,
> qapi-types.h.  Each of these headers still includes all its shards.
> Reduce compile time by including just the shards we actually need.
>
> To illustrate the benefits: adding a type to qapi/migration.json now
> recompiles some 2300 instead of 4800 objects.  The next commit will
> improve it further.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  backends/cryptodev.c |  1 -
>  backends/hostmem.c   |  3 ++-
>  block.c  |  1 -
>  block/block-backend.c|  2 +-
>  block/crypto.c   |  2 +-
>  block/nbd.c  |  2 +-
>  block/nfs.c  |  2 +-
>  block/qapi.c |  4 ++--
>  block/qcow2.c|  3 +--
>  block/quorum.c   |  2 +-
>  block/sheepdog.c |  2 +-
>  block/ssh.c  |  2 +-
>  block/throttle-groups.c  |  2 +-
>  block/write-threshold.c  |  4 ++--
>  blockdev-nbd.c   |  2 +-
>  blockdev.c   |  5 +++--
>  blockjob.c   |  2 +-
>  chardev/char-fe.c|  1 -
>  chardev/char-ringbuf.c   |  2 +-
>  chardev/char-socket.c|  1 +
>  chardev/char.c   |  3 +--
>  cpus.c   |  2 +-
>  crypto/cipherpriv.h  |  2 +-
>  hmp.c|  2 +-
>  hw/acpi/core.c   |  2 +-
>  hw/block/block.c |  1 +
>  hw/block/hd-geometry.c   |  1 +
>  hw/char/virtio-console.c |  2 +-
>  hw/core/machine.c|  2 +-
>  hw/i386/pc.c |  2 +-
>  hw/mem/nvdimm.c  |  1 -
>  hw/net/rocker/qmp-norocker.c |  2 +-
>  hw/net/rocker/rocker.c   |  2 +-
>  hw/net/rocker/rocker_fp.c|  2 +-
>  hw/net/rocker/rocker_of_dpa.c|  2 +-
>  hw/net/virtio-net.c  |  2 +-
>  hw/ppc/spapr_rtas.c  |  1 -
>  hw/tpm/tpm_emulator.c|  1 +
>  hw/tpm/tpm_passthrough.c |  1 +
>  hw/watchdog/watchdog.c   |  2 +-
>  include/block/block.h|  2 +-
>  include/block/dirty-bitmap.h |  2 +-
>  include/block/nbd.h  |  2 +-
>  include/chardev/char.h   |  1 +
>  include/crypto/cipher.h  |  2 +-
>  include/crypto/hash.h|  2 +-
>  include/crypto/hmac.h|  2 +-
>  include/crypto/secret.h  |  1 +
>  include/crypto/tlscreds.h|  1 +
>  include/hw/block/block.h |  2 +-
>  include/hw/block/fdc.h   |  2 +-
>  include/hw/ppc/spapr_drc.h   |  1 +
>  include/hw/qdev-properties.h |  1 +
>  include/io/dns-resolver.h|  1 +
>  include/migration/colo.h |  2 +-
>  include/migration/failover.h |  2 +-
>  include/migration/global_state.h |  1 +
>  include/monitor/monitor.h|  1 +
>  include/net/filter.h |  1 +
>  include/net/net.h|  2 +-
>  include/qapi/clone-visitor.h |  1 -
>  include/qapi/error.h |  2 +-
>  include/qapi/qmp/qobject.h   |  2 +-
>  include/qapi/visitor.h   |  2 +-
>  include/qemu/sockets.h   |  2 +-
>  include/qemu/throttle.h  |  2 +-
>  include/qom/cpu.h|  1 +
>  include/qom/object.h |  2 +-
>  include/sysemu/dump.h|  2 ++
>  include/sysemu/hostmem.h |  1 +
>  include/sysemu/replay.h  |  1 +
>  include/sysemu/sysemu.h  |  1 +
>  include/sysemu/tpm.h |  1 +
>  include/sysemu/watchdog.h|  2 +-
>  include/ui/input.h   |  2 +-
>  io/channel-socket.c  |  1 +
>  io/dns-resolver.c|  1 +
>  migration/colo-failover.c|  2 +-
>  migration/colo.c |  2 +-
>  migration/migration.c|  4 ++--
>  migration/migration.h|  1 +
>  migration/ram.c  |  2 +-
>  migration/ram.h  |  2 +-
>  net/colo-compare.c   |  1 -
>  net/filter-buffer.c  |  2 +-
>  net/filter-mirror.c  |  1 -
>  net/filter-rewriter.c|  1 -
>  net/net.c|  4 ++--
>  net/tap_int.h|  2 +-
>  net/vhost-user.c |  2 +-
>  qemu-img.c   |  2 +-
>  qom/object.c |  2 +-
>  qom/object_interfaces.c  |  1 -
>  replay/replay-input.c|  1 +
>  replication.h|  1 +
>  scripts/qapi/commands.py | 14 --
>  scripts/qapi/events.py   | 10 ++
>  

Re: [Qemu-devel] [PATCH v2 20/29] qapi/types qapi/visit: Generate built-in stuff into separate files

[Marc-Andre Lureau]

Hi

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> Linking code from multiple separate QAPI schemata into the same
> program is possible, but involves some weirdness around built-in
> types:
>
> * We generate code for built-in types into .c only with option
>   --builtins.  The user is responsible for generating code for exactly
>   one QAPI schema per program with --builtins.
>
> * We generate code for built-in types into .h regardless of
>   --builtins, but guarded by #ifndef QAPI_VISIT_BUILTIN.  Because all
>   copies of this code are exactly the same, including any combination
>   of these headers works.
>
> Replace this contraption by something more conventional: generate code
> for built-in types into their very own files: qapi-builtin-types.c,
> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
> only with --builtins.  Obey --output-dir, but ignore --prefix for
> them.
>
> Make qapi-types.h include qapi-builtin-types.h.  With multiple
> schemata you now have multiple qapi-types.[ch], but only one
> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
> qapi-builtin-visit.[ch].
>
> Bonus: if all you need is built-in stuff, you can include a much
> smaller header.  To be exploited shortly.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 

> ---
>  .gitignore |  2 ++
>  Makefile   | 13 ++
>  Makefile.objs  |  2 ++
>  scripts/qapi/common.py | 61 ---
>  scripts/qapi/types.py  | 61 +++
>  scripts/qapi/visit.py  | 64 
> +-
>  6 files changed, 116 insertions(+), 87 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 7d783e6e66..9477a08b6b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -29,6 +29,8 @@
>  /qga/qapi-generated
>  /qapi-generated
>  /qapi-gen-timestamp
> +/qapi-builtin-types.[ch]
> +/qapi-builtin-visit.[ch]
>  /qapi-types.[ch]
>  /qapi-visit.[ch]
>  /qapi-event.[ch]
> diff --git a/Makefile b/Makefile
> index 164a38578e..60ddc9c945 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -90,10 +90,13 @@ endif
>  include $(SRC_PATH)/rules.mak
>
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> -GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
> -GENERATED_FILES += qmp-commands.c qapi-types.c qapi-visit.c qapi-event.c
> -GENERATED_FILES += qmp-introspect.h
> -GENERATED_FILES += qmp-introspect.c
> +GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
> +GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
> +GENERATED_FILES += qapi-visit.h qapi-visit.c
> +GENERATED_FILES += qmp-commands.h qmp-commands.c
> +GENERATED_FILES += qapi-event.h qapi-event.c
> +GENERATED_FILES += qmp-introspect.c qmp-introspect.h
>  GENERATED_FILES += qapi-doc.texi
>
>  GENERATED_FILES += trace/generated-tcg-tracers.h
> @@ -520,7 +523,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/qapi/common.json \
> $(SRC_PATH)/qapi/transaction.json \
> $(SRC_PATH)/qapi/ui.json
>
> +qapi-builtin-types.c qapi-builtin-types.h \
>  qapi-types.c qapi-types.h \
> +qapi-builtin-visit.c qapi-builtin-visit.h \
>  qapi-visit.c qapi-visit.h \
>  qmp-commands.h qmp-commands.c \
>  qapi-event.c qapi-event.h \
> diff --git a/Makefile.objs b/Makefile.objs
> index d255aaf194..2813e984fd 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,6 +2,8 @@
>  # Common libraries for tools and emulators
>  stub-obj-y = stubs/ crypto/
>  util-obj-y = util/ qobject/ qapi/
> +util-obj-y += qapi-builtin-types.o
> +util-obj-y += qapi-builtin-visit.o
>  util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
>
>  chardev-obj-y = chardev/
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 31d2f73e7e..de12f8469a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1531,11 +1531,10 @@ class QAPISchema(object):
>
>  def _def_builtin_type(self, name, json_type, c_type):
>  self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> -# TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
> -# qapi-types.h from a single .c, all arrays of builtins must be
> -# declared in the first file whether or not they are used.  Nicer
> -# would be to use lazy instantiation, while figuring out how to
> -# avoid compilation issues with multiple qapi-types.h.
> +# Instantiating only the arrays that are actually used would
> +# be nice, but we can't as long as their generated code
> +# (qapi-builtin-types.[ch]) may be shared by some other
> +# schema.
>  self._make_array_type(name, None)
>
>  def _def_predefineds(self):
> @@ -1992,14 +1991,15 @@ class QAPIGen(object):
> 

Re: [Qemu-devel] [PATCH v2 04/29] qapi: Rename variable holding the QAPISchemaGenFOOVisitor

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> Rename the variable holding the QAPISchemaGenFOOVisitor from gen to
> vis, to avoid confusion in the next commit.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-commands.py   | 8 
>  scripts/qapi-event.py  | 8 
>  scripts/qapi-introspect.py | 8 
>  scripts/qapi-types.py  | 8 
>  scripts/qapi-visit.py  | 8 
>  5 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 84a980d882..c3aa52fce1 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -291,9 +291,9 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenCommandVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenCommandVisitor()
> +schema.visit(vis)
> +fdef.write(vis.defn)
> +fdecl.write(vis.decl)
>
>  close_output(fdef, fdecl)
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 0a098803e2..edb9ddb650 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -203,9 +203,9 @@ fdecl.write(mcgen('''
>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenEventVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenEventVisitor()
> +schema.visit(vis)
> +fdef.write(vis.defn)
> +fdecl.write(vis.decl)
>
>  close_output(fdef, fdecl)
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index bd9253a172..ebe8706f41 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -193,9 +193,9 @@ fdef.write(mcgen('''
>   prefix=prefix))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
> +schema.visit(vis)
> +fdef.write(vis.defn)
> +fdecl.write(vis.decl)
>
>  close_output(fdef, fdecl)
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 1103dbda2d..4db8424da1 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -273,9 +273,9 @@ fdecl.write(mcgen('''
>  '''))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenTypeVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenTypeVisitor()
> +schema.visit(vis)
> +fdef.write(vis.defn)
> +fdecl.write(vis.decl)
>
>  close_output(fdef, fdecl)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index e56b3c1256..3c1a0e2544 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -360,9 +360,9 @@ fdecl.write(mcgen('''
>prefix=prefix))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenVisitVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenVisitVisitor()
> +schema.visit(vis)
> +fdef.write(vis.defn)
> +fdecl.write(vis.decl)
>
>  close_output(fdef, fdecl)
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 18/29] qapi: Rename generated qmp-marshal.c to qmp-commands.c

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> All generated .c are named like their .h, except for qmp-marshal.c and
> qmp-commands.h.  To add to the confusion, tests-qmp-commands.c falsely
> matches generated test-qmp-commands.h.
>
> Get rid of this unnecessary complication.
>
> Signed-off-by: Markus Armbruster 

that makes more sense,

Reviewed-by: Marc-André Lureau 


> ---
>  .gitignore |  3 +--
>  Makefile   |  6 +++---
>  Makefile.objs  |  2 +-
>  docs/devel/qapi-code-gen.txt   |  6 +++---
>  qga/Makefile.objs  |  2 +-
>  scripts/qapi/commands.py   |  2 +-
>  tests/.gitignore   |  5 ++---
>  tests/Makefile.include | 10 +-
>  tests/{test-qmp-commands.c => test-qmp-cmds.c} |  0
>  9 files changed, 17 insertions(+), 19 deletions(-)
>  rename tests/{test-qmp-commands.c => test-qmp-cmds.c} (100%)
>
> diff --git a/.gitignore b/.gitignore
> index 2f9a92f6cc..7d783e6e66 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -33,9 +33,8 @@
>  /qapi-visit.[ch]
>  /qapi-event.[ch]
>  /qapi-doc.texi
> -/qmp-commands.h
> +/qmp-commands.[ch]
>  /qmp-introspect.[ch]
> -/qmp-marshal.c
>  /qemu-doc.html
>  /qemu-doc.info
>  /qemu-doc.txt
> diff --git a/Makefile b/Makefile
> index bd781c6aad..164a38578e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -91,7 +91,7 @@ include $(SRC_PATH)/rules.mak
>
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
>  GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
> -GENERATED_FILES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c
> +GENERATED_FILES += qmp-commands.c qapi-types.c qapi-visit.c qapi-event.c
>  GENERATED_FILES += qmp-introspect.h
>  GENERATED_FILES += qmp-introspect.c
>  GENERATED_FILES += qapi-doc.texi
> @@ -496,7 +496,7 @@ $(SRC_PATH)/scripts/qapi-gen.py
>
>  qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
>  qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h \
> -qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-marshal.c \
> +qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-commands.c \
>  qga/qapi-generated/qga-qapi-doc.texi: \
>  qga/qapi-generated/qapi-gen-timestamp ;
>  qga/qapi-generated/qapi-gen-timestamp: $(SRC_PATH)/qga/qapi-schema.json 
> $(qapi-py)
> @@ -522,7 +522,7 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/qapi/common.json \
>
>  qapi-types.c qapi-types.h \
>  qapi-visit.c qapi-visit.h \
> -qmp-commands.h qmp-marshal.c \
> +qmp-commands.h qmp-commands.c \
>  qapi-event.c qapi-event.h \
>  qmp-introspect.h qmp-introspect.c \
>  qapi-doc.texi: \
> diff --git a/Makefile.objs b/Makefile.objs
> index 2efba6d768..d255aaf194 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -78,7 +78,7 @@ common-obj-$(CONFIG_FDT) += device_tree.o
>  ##
>  # qapi
>
> -common-obj-y += qmp-marshal.o
> +common-obj-y += qmp-commands.o
>  common-obj-y += qmp-introspect.o
>  common-obj-y += qmp.o hmp.o
>  endif
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 1a1cbaea7b..ba1dc73298 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -1147,8 +1147,8 @@ declares qmp_COMMAND() that the user must implement.
>
>  The following files are generated:
>
> -$(prefix)qmp-marshal.c: Command marshal/dispatch functions for each
> -QMP command defined in the schema
> +$(prefix)qmp-commands.c: Command marshal/dispatch functions for each
> + QMP command defined in the schema
>
>  $(prefix)qmp-commands.h: Function prototypes for the QMP commands
>   specified in the schema
> @@ -1170,7 +1170,7 @@ Example:
>  void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp);
>
>  #endif
> -$ cat qapi-generated/example-qmp-marshal.c
> +$ cat qapi-generated/example-qmp-commands.c
>  [Uninteresting stuff omitted...]
>
>  static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject 
> **ret_out, Error **errp)
> diff --git a/qga/Makefile.objs b/qga/Makefile.objs
> index 1c5986c0bb..6151378ae4 100644
> --- a/qga/Makefile.objs
> +++ b/qga/Makefile.objs
> @@ -3,6 +3,6 @@ qga-obj-$(CONFIG_POSIX) += commands-posix.o channel-posix.o
>  qga-obj-$(CONFIG_WIN32) += commands-win32.o channel-win32.o service-win32.o
>  qga-obj-$(CONFIG_WIN32) += vss-win32.o
>  qga-obj-y += qapi-generated/qga-qapi-types.o qapi-generated/qga-qapi-visit.o
> -qga-obj-y += qapi-generated/qga-qmp-marshal.o
> +qga-obj-y += qapi-generated/qga-qmp-commands.o
>
>  qga-vss-dll-obj-$(CONFIG_QGA_VSS) += vss-win32/
> diff --git a/scripts/qapi/commands.py 

Re: [Qemu-devel] [PATCH v2 19/29] qapi: Make code-generating visitors use QAPIGen more

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> The use of QAPIGen is rather shallow so far: most of the output
> accumulation is not converted.  Take the next step: convert output
> accumulation in the code-generating visitor classes.  Helper functions
> outside these classes are not converted.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/commands.py   | 71 
>  scripts/qapi/common.py | 13 
>  scripts/qapi/doc.py| 74 --
>  scripts/qapi/events.py | 55 ---
>  scripts/qapi/introspect.py | 56 +---
>  scripts/qapi/types.py  | 81 
> +++---
>  scripts/qapi/visit.py  | 80 +++--
>  7 files changed, 188 insertions(+), 242 deletions(-)
>
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 05fe33a03b..46757db771 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -223,44 +223,15 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
>  return ret
>
>
> -class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> +class QAPISchemaGenCommandVisitor(QAPISchemaMonolithicCVisitor):
> +
>  def __init__(self, prefix):
> -self._prefix = prefix
> -self.decl = None
> -self.defn = None
> -self._regy = None
> -self._visited_ret_types = None
> -
> -def visit_begin(self, schema):
> -self.decl = ''
> -self.defn = ''
> +QAPISchemaMonolithicCVisitor.__init__(
> +self, prefix, 'qmp-commands',
> +' * Schema-defined QAPI/QMP commands', __doc__)
>  self._regy = ''
>  self._visited_ret_types = set()
> -
> -def visit_end(self):
> -self.defn += gen_registry(self._regy, self._prefix)
> -self._regy = None
> -self._visited_ret_types = None
> -
> -def visit_command(self, name, info, arg_type, ret_type,
> -  gen, success_response, boxed):
> -if not gen:
> -return
> -self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
> -if ret_type and ret_type not in self._visited_ret_types:
> -self._visited_ret_types.add(ret_type)
> -self.defn += gen_marshal_output(ret_type)
> -self.decl += gen_marshal_decl(name)
> -self.defn += gen_marshal(name, arg_type, boxed, ret_type)
> -self._regy += gen_register_command(name, success_response)
> -
> -
> -def gen_commands(schema, output_dir, prefix):
> -blurb = ' * Schema-defined QAPI/QMP commands'
> -genc = QAPIGenC(blurb, __doc__)
> -genh = QAPIGenH(blurb, __doc__)
> -
> -genc.add(mcgen('''
> +self._genc.add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/module.h"
> @@ -275,19 +246,33 @@ def gen_commands(schema, output_dir, prefix):
>  #include "%(prefix)sqmp-commands.h"
>
>  ''',
> -   prefix=prefix))
> -
> -genh.add(mcgen('''
> + prefix=prefix))
> +self._genh.add(mcgen('''
>  #include "%(prefix)sqapi-types.h"
>  #include "qapi/qmp/dispatch.h"
>
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -   prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> + prefix=prefix,
> + c_prefix=c_name(prefix, protect=False)))
>
> +def visit_end(self):
> +self._genc.add(gen_registry(self._regy, self._prefix))
> +
> +def visit_command(self, name, info, arg_type, ret_type,
> +  gen, success_response, boxed):
> +if not gen:
> +return
> +self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
> +if ret_type and ret_type not in self._visited_ret_types:
> +self._visited_ret_types.add(ret_type)
> +self._genc.add(gen_marshal_output(ret_type))
> +self._genh.add(gen_marshal_decl(name))
> +self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
> +self._regy += gen_register_command(name, success_response)
> +
> +
> +def gen_commands(schema, output_dir, prefix):
>  vis = QAPISchemaGenCommandVisitor(prefix)
>  schema.visit(vis)
> -genc.add(vis.defn)
> -genh.add(vis.decl)
> -genc.write(output_dir, prefix + 'qmp-commands.c')
> -genh.write(output_dir, prefix + 'qmp-commands.h')
> +vis.write(output_dir)
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 29d98ca934..31d2f73e7e 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -2049,3 +2049,16 @@ class QAPIGenDoc(QAPIGen):
>  def _top(self, fname):
>  return (QAPIGen._top(self, fname)
>

Re: [Qemu-devel] [PATCH v2 01/29] Include qapi/qmp/qerror.h exactly where needed

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  block.c | 1 -
>  block/qcow2.c   | 1 -
>  chardev/char-fe.c   | 1 +
>  chardev/char.c  | 1 +
>  qom/object_interfaces.c | 1 +
>  scripts/qapi-visit.py   | 2 +-
>  6 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index f94585b230..05a484b4b8 100644
> --- a/block.c
> +++ b/block.c
> @@ -32,7 +32,6 @@
>  #include "qemu/module.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
> -#include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/qstring.h"
>  #include "sysemu/block-backend.h"
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a64a572785..9245deac19 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -30,7 +30,6 @@
>  #include "block/qcow2.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> -#include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi-event.h"
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index c611b3fa3e..e5f870e4d2 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -24,6 +24,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qapi-visit.h"
>  #include "sysemu/replay.h"
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 01d979a1da..c9a4da5516 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -32,6 +32,7 @@
>  #include "qmp-commands.h"
>  #include "qapi-visit.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  #include "sysemu/replay.h"
>  #include "qemu/help_option.h"
>  #include "qemu/option.h"
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 80d09139be..43d9aa0946 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -1,6 +1,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qerror.h"
>  #include "qom/object_interfaces.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 7e1cfc13f0..bc2b8b581a 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -371,13 +371,13 @@ fdef.write(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
>   prefix=prefix))
>
>  fdecl.write(mcgen('''
>  #include "qapi/visitor.h"
> -#include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH v2 09/29] qapi-gen: Convert from getopt to argparse

[Marc-Andre Lureau]

On Sun, Feb 11, 2018 at 10:35 AM, Markus Armbruster  wrote:
> argparse is nicer to use than getopt, and gives us --help almost for
> free.
>
> Signed-off-by: Markus Armbruster 

nice,
Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-gen.py| 48 ++--
>  scripts/qapi/common.py | 43 ---
>  2 files changed, 30 insertions(+), 61 deletions(-)
>
> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index 2100ca1145..e5be484e3e 100755
> --- a/scripts/qapi-gen.py
> +++ b/scripts/qapi-gen.py
> @@ -4,8 +4,11 @@
>  # This work is licensed under the terms of the GNU GPL, version 2 or later.
>  # See the COPYING file in the top-level directory.
>
> +from __future__ import print_function
> +import argparse
> +import re
>  import sys
> -from qapi.common import parse_command_line, QAPISchema
> +from qapi.common import QAPISchema
>  from qapi.types import gen_types
>  from qapi.visit import gen_visit
>  from qapi.commands import gen_commands
> @@ -15,26 +18,35 @@ from qapi.doc import gen_doc
>
>
>  def main(argv):
> -(input_file, output_dir, prefix, opts) = \
> -parse_command_line('bu', ['builtins', 'unmask-non-abi-names'])
> +parser = argparse.ArgumentParser(
> +description='Generate code from a QAPI schema')
> +parser.add_argument('-b', '--builtins', action='store_true',
> +help="generate code for built-in types")
> +parser.add_argument('-o', '--output_dir', action='store', default='',
> +help="write output to directory OUTPUT_DIR")
> +parser.add_argument('-p', '--prefix', action='store', default='',
> +help="prefix for symbols")
> +parser.add_argument('-u', '--unmask-non-abi-names', action='store_true',
> +dest='unmask',
> +help="expose non-ABI names in introspection")
> +parser.add_argument('schema', action='store')
> +args = parser.parse_args()
>
> -opt_builtins = False
> -opt_unmask = False
> +match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', args.prefix)
> +if match.end() != len(args.prefix):
> +print("%s: 'funny character '%s' in argument of --prefix"
> +  % (sys.argv[0], args.prefix[match.end()]),
> +  file=sys.stderr)
> +sys.exit(1)
>
> -for o, a in opts:
> -if o in ('-b', '--builtins'):
> -opt_builtins = True
> -if o in ('-u', '--unmask-non-abi-names'):
> -opt_unmask = True
> +schema = QAPISchema(args.schema)
>
> -schema = QAPISchema(input_file)
> -
> -gen_types(schema, output_dir, prefix, opt_builtins)
> -gen_visit(schema, output_dir, prefix, opt_builtins)
> -gen_commands(schema, output_dir, prefix)
> -gen_events(schema, output_dir, prefix)
> -gen_introspect(schema, output_dir, prefix, opt_unmask)
> -gen_doc(schema, output_dir, prefix)
> +gen_types(schema, args.output_dir, args.prefix, args.builtins)
> +gen_visit(schema, args.output_dir, args.prefix, args.builtins)
> +gen_commands(schema, args.output_dir, args.prefix)
> +gen_events(schema, args.output_dir, args.prefix)
> +gen_introspect(schema, args.output_dir, args.prefix, args.unmask)
> +gen_doc(schema, args.output_dir, args.prefix)
>
>
>  if __name__ == '__main__':
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 868ec25deb..8290795dc1 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -13,7 +13,6 @@
>
>  from __future__ import print_function
>  import errno
> -import getopt
>  import os
>  import re
>  import string
> @@ -1924,48 +1923,6 @@ def build_params(arg_type, boxed, extra):
>
>
>  #
> -# Common command line parsing
> -#
> -
> -
> -def parse_command_line(extra_options='', extra_long_options=[]):
> -
> -try:
> -opts, args = getopt.gnu_getopt(sys.argv[1:],
> -   'p:o:' + extra_options,
> -   ['prefix=', 'output-dir=']
> -   + extra_long_options)
> -except getopt.GetoptError as err:
> -print("%s: %s" % (sys.argv[0], str(err)), file=sys.stderr)
> -sys.exit(1)
> -
> -output_dir = ''
> -prefix = ''
> -extra_opts = []
> -
> -for oa in opts:
> -o, a = oa
> -if o in ('-p', '--prefix'):
> -match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', a)
> -if match.end() != len(a):
> -print("%s: 'funny character '%s' in argument of --prefix" \
> -  % (sys.argv[0], a[match.end()]), file=sys.stderr)
> -sys.exit(1)
> -prefix = a
> -elif o in ('-o', '--output-dir'):
> -output_dir = a + '/'
> -else:
> -extra_opts.append(oa)
> -
> -if len(args) != 1:
> 

Re: [Qemu-devel] [PATCH v2 10/29] qapi: Touch generated files only when they change

[Marc-Andre Lureau]

Hi

On Mon, Feb 12, 2018 at 8:48 PM, Eric Blake  wrote:
> On 02/11/2018 03:35 AM, Markus Armbruster wrote:
>>
>> A massive number of objects depends on QAPI-generated headers.  In my
>> "build everything" tree, it's roughly 4800 out of 5100.  This is
>> particularly annoying when only some of the generated files change,
>> say for a doc fix.
>>
>> Improve qapi-gen.py to touch its output files only if they actually
>> change.  Rebuild time for a QAPI doc fix drops from many minutes to a
>> few seconds.  Rebuilds get faster for certain code changes, too.  For
>> instance, adding a simple QMP event now recompiles less than 200
>> instead of 4800 objects.  But adding a QAPI type is as bad as ever;
>> we've clearly got more work to do.
>>
>> Signed-off-by: Markus Armbruster 
>> Reviewed-by: Eric Blake 
>> ---
>>   scripts/qapi/common.py | 11 +--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 8290795dc1..2e58573a39 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -1951,9 +1951,16 @@ class QAPIGen(object):
>>   except os.error as e:
>>   if e.errno != errno.EEXIST:
>>   raise
>> -f = open(os.path.join(output_dir, fname), 'w')
>> -f.write(self._top(fname) + self._preamble + self._body
>> +fd = os.open(os.path.join(output_dir, fname),
>> + os.O_RDWR | os.O_CREAT, 0666)
>
>
> patchew complained here for mingw; I'm not sure why.

python3 syntax error.
https://stackoverflow.com/questions/1837874/invalid-token-when-using-octal-numbers

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

Re: [Qemu-devel] [PATCH v3 1/1] dump.c: allow fd_write_vmcore to return errno on failure

[Marc-Andre Lureau]

Hi

On Mon, Feb 12, 2018 at 3:25 PM, Daniel Henrique Barboza
 wrote:
> From: Yasmin Beatriz 
>
> fd_write_vmcore can fail to execute for a lot of reasons that can be
> retrieved by errno, but it only returns -1. This makes difficult for
> the caller to know what happened and only a generic error message is
> propagated back to the user. This is an example using dump-guest-memory:
>
> (qemu) dump-guest-memory /home/yasmin/mnt/test.dump
> dump: failed to save memory
>
> All callers of fd_write_vmcore of dump.c does error handling via
> error_setg(), so at first it seems feasible to add the Error pointer as
> an argument of fd_write_vmcore. This proved to be more complex than it
> first looked. fd_write_vmcore is used by write_elf64_notes and
> write_elf32_notes as a WriteCoreDumpFunction prototype. WriteCoreDumpFunction
> is declared in include/qom/cpu.h and is used all around the code. This
> leaves us with few alternatives:
>
> - change the WriteCoreDumpFunction prototype to include an error pointer.
> This would require to change all functions that implements this prototype
> to also receive an Error pointer;
>
> - change both write_elf64_notes and write_elf32_notes to no use the
> WriteCoreDumpFunction. These functions use not only fd_write_vmcore
> but also buf_write_note, so this would require to change buf_write_note
> to handle an Error pointer. Considerable easier than the alternative
> above, but it's still a lot of code just for the benefit of the callers
> of fd_write_vmcore.
>
> This patch presents an easier solution that benefits all fd_write_vmcore
> callers:
>
> - instead of returning -1 on error, return -errno. All existing callers
> already checks for ret < 0 so there is no need to change the caller's
> logic too much. This also allows the retrieval of the errno.
>
> - all callers were updated to use error_setg_errno instead of just
> errno_setg. Now that fd_write_vmcore can return an errno, let's update
> all callers so they can benefit from a more detailed error message.
>
> This is the same dump-guest-memory example with this patch applied:
>
> (qemu) dump-guest-memory /home/yasmin/mnt/test.dump
> dump: failed to save memory: No space left on device
> (qemu)
>
> This example illustrates an error of fd_write_vmcore when called
> from write_data. All other callers will benefit from better
> error messages as well.
>
> Reported-by: yilzh...@redhat.com
> Cc: Jose Ricardo Ziviani 
> Signed-off-by: Yasmin Beatriz 
> Signed-off-by: Daniel Henrique Barboza 

lgtm,

Reviewed-by: Marc-André Lureau 


> ---
>  dump.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index 7b13baa413..171ff8a3b8 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -107,7 +107,7 @@ static int fd_write_vmcore(const void *buf, size_t size, 
> void *opaque)
>
>  written_size = qemu_write_full(s->fd, buf, size);
>  if (written_size != size) {
> -return -1;
> +return -errno;
>  }
>
>  return 0;
> @@ -140,7 +140,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
>
>  ret = fd_write_vmcore(_header, sizeof(elf_header), s);
>  if (ret < 0) {
> -error_setg(errp, "dump: failed to write elf header");
> +error_setg_errno(errp, -ret, "dump: failed to write elf header");
>  }
>  }
>
> @@ -171,7 +171,7 @@ static void write_elf32_header(DumpState *s, Error **errp)
>
>  ret = fd_write_vmcore(_header, sizeof(elf_header), s);
>  if (ret < 0) {
> -error_setg(errp, "dump: failed to write elf header");
> +error_setg_errno(errp, -ret, "dump: failed to write elf header");
>  }
>  }
>
> @@ -194,7 +194,8 @@ static void write_elf64_load(DumpState *s, MemoryMapping 
> *memory_mapping,
>
>  ret = fd_write_vmcore(, sizeof(Elf64_Phdr), s);
>  if (ret < 0) {
> -error_setg(errp, "dump: failed to write program header table");
> +error_setg_errno(errp, -ret,
> + "dump: failed to write program header table");
>  }
>  }
>
> @@ -217,7 +218,8 @@ static void write_elf32_load(DumpState *s, MemoryMapping 
> *memory_mapping,
>
>  ret = fd_write_vmcore(, sizeof(Elf32_Phdr), s);
>  if (ret < 0) {
> -error_setg(errp, "dump: failed to write program header table");
> +error_setg_errno(errp, -ret,
> + "dump: failed to write program header table");
>  }
>  }
>
> @@ -237,7 +239,8 @@ static void write_elf64_note(DumpState *s, Error **errp)
>
>  ret = fd_write_vmcore(, sizeof(Elf64_Phdr), s);
>  if (ret < 0) {
> -error_setg(errp, "dump: failed to write program header table");
> +error_setg_errno(errp, -ret,
> + "dump: failed to write program header table");
>  }
>  }
>
> @@ -302,7 +305,8 @@ 

Re: [Qemu-devel] [PATCH v4 5/9] sockets: move fd_is_socket() into common sockets code

[Marc-Andre Lureau]

On Mon, Feb 5, 2018 at 4:24 PM, Daniel P. Berrangé  wrote:
> From: "Daniel P. Berrange" 
>
> The fd_is_socket() helper method is useful in a few places, so put it in
> the common sockets code. Make the code more compact while moving it.
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Marc-André Lureau 

> ---
>  include/qemu/sockets.h|  1 +
>  io/channel-util.c | 13 
>  tests/.gitignore  |  1 +
>  tests/Makefile.include|  3 ++
>  tests/test-util-sockets.c | 77 
> +++
>  util/qemu-sockets.c   |  8 +
>  6 files changed, 90 insertions(+), 13 deletions(-)
>  create mode 100644 tests/test-util-sockets.c
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 8889bcb1ec..88777590dd 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -12,6 +12,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #include "qapi-types.h"
>
>  /* misc helpers */
> +bool fd_is_socket(int fd);
>  int qemu_socket(int domain, int type, int protocol);
>  int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
>  int socket_set_cork(int fd, int v);
> diff --git a/io/channel-util.c b/io/channel-util.c
> index 0fb4bd0837..423d79845a 100644
> --- a/io/channel-util.c
> +++ b/io/channel-util.c
> @@ -24,19 +24,6 @@
>  #include "io/channel-socket.h"
>
>
> -static bool fd_is_socket(int fd)
> -{
> -int optval;
> -socklen_t optlen;
> -optlen = sizeof(optval);
> -return qemu_getsockopt(fd,
> -   SOL_SOCKET,
> -   SO_TYPE,
> -   (char *),
> -   ) == 0;
> -}
> -
> -
>  QIOChannel *qio_channel_new_fd(int fd,
> Error **errp)
>  {
> diff --git a/tests/.gitignore b/tests/.gitignore
> index e5c744b7ed..e2e3f71db7 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -87,6 +87,7 @@ test-thread-pool
>  test-throttle
>  test-timed-average
>  test-uuid
> +test-util-sockets
>  test-visitor-serialization
>  test-vmstate
>  test-write-threshold
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 7bfdabeb6d..c2b51ef93c 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -136,6 +136,7 @@ ifneq (,$(findstring qemu-ga,$(TOOLS)))
>  check-unit-$(CONFIG_LINUX) += tests/test-qga$(EXESUF)
>  endif
>  check-unit-y += tests/test-timed-average$(EXESUF)
> +check-unit-y += tests/test-util-sockets$(EXESUF)
>  check-unit-y += tests/test-io-task$(EXESUF)
>  check-unit-y += tests/test-io-channel-socket$(EXESUF)
>  check-unit-y += tests/test-io-channel-file$(EXESUF)
> @@ -706,6 +707,8 @@ tests/test-crypto-tlscredsx509$(EXESUF): 
> tests/test-crypto-tlscredsx509.o \
>  tests/test-crypto-tlssession.o-cflags := $(TASN1_CFLAGS)
>  tests/test-crypto-tlssession$(EXESUF): tests/test-crypto-tlssession.o \
> tests/crypto-tls-x509-helpers.o tests/pkix_asn1_tab.o 
> $(test-crypto-obj-y)
> +tests/test-util-sockets$(EXESUF): tests/test-util-sockets.o \
> +   tests/socket-helpers.o $(test-util-obj-y)
>  tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y)
>  tests/test-io-channel-socket$(EXESUF): tests/test-io-channel-socket.o \
>  tests/io-channel-helpers.o tests/socket-helpers.o $(test-io-obj-y)
> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
> new file mode 100644
> index 00..65190e0530
> --- /dev/null
> +++ b/tests/test-util-sockets.c
> @@ -0,0 +1,77 @@
> +/*
> + * Tests for util/qemu-sockets.c
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this library; if not, see .
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/sockets.h"
> +#include "qapi/error.h"
> +#include "socket-helpers.h"
> +
> +static void test_fd_is_socket_bad(void)
> +{
> +char *tmp = g_strdup("qemu-test-util-sockets-XX");
> +int fd = mkstemp(tmp);
> +if (fd != 0) {
> +unlink(tmp);
> +}
> +g_free(tmp);
> +
> +g_assert(fd >= 0);
> +
> +g_assert(!fd_is_socket(fd));
> +close(fd);
> +}
> +
> +static void test_fd_is_socket_good(void)
> +{
> +int fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> +
> +g_assert(fd >= 0);
> 

Re: [Qemu-devel] [PATCH RFC 14/21] qapi: Generate in source order

[Marc-Andre Lureau]

On Mon, Feb 5, 2018 at 3:33 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Marc-Andre Lureau <mlur...@redhat.com> writes:
>
>> On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster <arm...@redhat.com> wrote:
>>> The generators' conversion to visitors (merge commit 9e72681d16)
>>> changed the processing order of entities from source order to
>>> alphabetical order.  The next commit needs source order, so change it
>>> back.
>>>
>>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>>> ---
>>>  scripts/qapi/common.py   |   4 +-
>>>  tests/qapi-schema/comments.out   |   2 +-
>>>  tests/qapi-schema/doc-bad-section.out|   4 +-
>>>  tests/qapi-schema/doc-good.out   |  32 ++--
>>>  tests/qapi-schema/empty.out  |   2 +-
>>>  tests/qapi-schema/event-case.out |   2 +-
>>>  tests/qapi-schema/ident-with-escape.out  |   6 +-
>>>  tests/qapi-schema/include-relpath.out|   2 +-
>>>  tests/qapi-schema/include-repetition.out |   2 +-
>>>  tests/qapi-schema/include-simple.out |   2 +-
>>>  tests/qapi-schema/indented-expr.out  |   2 +-
>>>  tests/qapi-schema/qapi-schema-test.out   | 320 
>>> +++
>>>  12 files changed, 191 insertions(+), 189 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index d5b93e7381..3b97bf8702 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -1471,6 +1471,7 @@ class QAPISchema(object):
>>>  parser = QAPISchemaParser(open(fname, 'r'))
>>>  exprs = check_exprs(parser.exprs)
>>>  self.docs = parser.docs
>>> +self._entity_list = []
>>>  self._entity_dict = {}
>>>  self._predefining = True
>>>  self._def_predefineds()
>>> @@ -1482,6 +1483,7 @@ class QAPISchema(object):
>>>  # Only the predefined types are allowed to not have info
>>>  assert ent.info or self._predefining
>>>  assert ent.name not in self._entity_dict
>>> +self._entity_list.append(ent)
>>>  self._entity_dict[ent.name] = ent
>>
>> Why not use the OrderedDict instead?
>
> Fair question!  However, the next patch will create anonymous entities,
> which get added only to ._entity_list, not _entity_dict.

I see, something we could improve or clarify, but that's ok to me as is:
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>


>
> [...]

Re: [Qemu-devel] [PATCH RFC 18/21] qapi/common: Fix guardname() for funny filenames

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> guardname() fails to return a valid C identifier for arguments
> containing anything but [A-Za-z0-9_.-'].  Fix that.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 7c78d9..7d497b5b17 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1860,7 +1860,7 @@ def mcgen(code, **kwds):
>
>
>  def guardname(filename):
> -return c_name(filename, protect=False).upper()
> +return re.sub(r'[^A-Za-z0-9_]', '_', filename).upper()
>
>
>  def guardstart(name):
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 17/21] qapi/types qapi/visit: Generate built-in stuff into separate files

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Linking code from multiple separate QAPI schemata into the same
> program is possible, but involves some weirdness around built-in
> types:
>
> * We generate code for built-in types into .c only with option
>   --builtins.  The user is responsible to generate code for exactly
>   one QAPI schema per program with --builtins.
>
> * We generate code for them it into .h regardless of --builtins,
>   guarded by #ifndef QAPI_VISIT_BUILTIN.  Because the code for
>   built-in types is exactly the same in all of them, including any
>   combination of these headers works.
>
> Replace this contraption by something more conventional: generate code
> for built-in types into their very own files: qapi-builtin-types.c,
> qapi-builtin-visit.c, qapi-builtin-types.h, qapi-builtin-visit.h, but
> only with --builtins.  Obey --output-dir, but ignore --prefix for
> them.
>
> Make qapi-types.h include qapi-builtin-types.h.  With multiple
> schemata you now have multiple qapi-types.[ch], but only one
> qapi-builtin-types.[ch].  Same for qapi-visit.[ch] and
> qapi-builtin-visit.[ch].
>
> Bonus: if all you need is built-in stuff, you can include a much
> smaller header.  To be exploited shortly.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  Makefile   | 13 +---
>  Makefile.objs  |  1 +
>  scripts/qapi/common.py | 18 +--
>  scripts/qapi/types.py  | 82 ++--
>  scripts/qapi/visit.py  | 84 
> --
>  5 files changed, 111 insertions(+), 87 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index e02f0c13ef..f9b7900330 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -88,10 +88,13 @@ endif
>  include $(SRC_PATH)/rules.mak
>
>  GENERATED_FILES = qemu-version.h config-host.h qemu-options.def
> -GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h qapi-event.h
> -GENERATED_FILES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c
> -GENERATED_FILES += qmp-introspect.h
> -GENERATED_FILES += qmp-introspect.c
> +GENERATED_FILES += qmp-commands.h qmp-marshal.c
> +GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
> +GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
> +GENERATED_FILES += qapi-visit.h qapi-visit.c
> +GENERATED_FILES += qapi-event.h qapi-event.c
> +GENERATED_FILES += qmp-introspect.c qmp-introspect.h
>  GENERATED_FILES += qapi.texi
>
>  GENERATED_FILES += trace/generated-tcg-tracers.h
> @@ -514,7 +517,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/qapi/common.json \
> $(SRC_PATH)/qapi/transaction.json \
> $(SRC_PATH)/qapi/ui.json
>
> +qapi-builtin-types.c qapi-builtin-types.h \
>  qapi-types.c qapi-types.h \
> +qapi-builtin-visit.c qapi-builtin-visit.h \
>  qapi-visit.c qapi-visit.h \
>  qmp-commands.h qmp-marshal.c \
>  qapi-event.c qapi-event.h \
> diff --git a/Makefile.objs b/Makefile.objs
> index 323ef12384..f16cca06e7 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,6 +2,7 @@
>  # Common libraries for tools and emulators
>  stub-obj-y = stubs/ crypto/
>  util-obj-y = util/ qobject/ qapi/
> +util-obj-y += qapi-builtin-types.o qapi-builtin-visit.o
>  util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
>
>  chardev-obj-y = chardev/
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index f4e9ebbb53..7c78d9 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1527,11 +1527,10 @@ class QAPISchema(object):
>
>  def _def_builtin_type(self, name, json_type, c_type):
>  self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
> -# TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
> -# qapi-types.h from a single .c, all arrays of builtins must be
> -# declared in the first file whether or not they are used.  Nicer
> -# would be to use lazy instantiation, while figuring out how to
> -# avoid compilation issues with multiple qapi-types.h.
> +# Instantiating only the arrays that are actually used would
> +# be nice, but we can't as long as their generated code
> +# (qapi-builtin-types.[ch]) may be shared by some other
> +# schema.
>  self._make_array_type(name, None)
>
>  def _def_predefineds(self):
> @@ -1985,14 +1984,15 @@ class QAPIGen(object):
>  return ''
>
>  def write(self, output_dir, fname):
> -if output_dir:
> +pathname = os.path.join(output_dir, fname)
> +dir = os.path.dirname(pathname)
> +if dir:
>  try:
> -os.makedirs(output_dir)
> +os.makedirs(dir)
>  except os.error as e:
>  if e.errno != errno.EEXIST:
>  

Re: [Qemu-devel] [PATCH RFC 19/21] qapi/types: Generate separate .h, .c for each module

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Our qapi-schema.json is composed of modules connected by include
> directives, but the generated code is monolithic all the same: one
> qapi-types.h with all the types, one qapi-visit.h with all the
> visitors, and so forth.  These monolithic headers get included all
> over the place.  In my "build everything" tree, adding a QAPI type
> recompiles about 4500 out of 4800 objects.
>
> Nobody would write such monolithic headers by hand.  It stands to
> reason that one shouldn't generate them, either.
>
> Split up generated qapi-types.h to mirror the schema's modular
> structure: one header per module.  Name the main module's header
> qapi-types.h, and sub-module D/B.json's header D/qapi-types-B.h.
>
> Mirror the schema's includes in the headers, so that qapi-types.h gets
> you everything exactly as before.  If you need less, you can include
> one or more of the sub-module headers.  To be exploited shortly.
>
> Split up qapi-types.c similarly.
>
> Signed-off-by: Markus Armbruster 

Most of the necessary split work is done in previous patch, this enables it:

Reviewed-by: Marc-André Lureau 


> ---
>  Makefile  | 30 ++
>  Makefile.objs | 18 +-
>  scripts/qapi/types.py | 18 --
>  3 files changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f9b7900330..f1b68dca9b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -91,6 +91,21 @@ GENERATED_FILES = qemu-version.h config-host.h 
> qemu-options.def
>  GENERATED_FILES += qmp-commands.h qmp-marshal.c
>  GENERATED_FILES += qapi-builtin-types.h qapi-builtin-types.c
>  GENERATED_FILES += qapi-types.h qapi-types.c
> +GENERATED_FILES += qapi/qapi-types-block-core.h qapi/qapi-types-block-core.c
> +GENERATED_FILES += qapi/qapi-types-block.h qapi/qapi-types-block.c
> +GENERATED_FILES += qapi/qapi-types-char.h qapi/qapi-types-char.c
> +GENERATED_FILES += qapi/qapi-types-common.h qapi/qapi-types-common.c
> +GENERATED_FILES += qapi/qapi-types-crypto.h qapi/qapi-types-crypto.c
> +GENERATED_FILES += qapi/qapi-types-introspect.h qapi/qapi-types-introspect.c
> +GENERATED_FILES += qapi/qapi-types-migration.h qapi/qapi-types-migration.c
> +GENERATED_FILES += qapi/qapi-types-net.h qapi/qapi-types-net.c
> +GENERATED_FILES += qapi/qapi-types-rocker.h qapi/qapi-types-rocker.c
> +GENERATED_FILES += qapi/qapi-types-run-state.h qapi/qapi-types-run-state.c
> +GENERATED_FILES += qapi/qapi-types-sockets.h qapi/qapi-types-sockets.c
> +GENERATED_FILES += qapi/qapi-types-tpm.h qapi/qapi-types-tpm.c
> +GENERATED_FILES += qapi/qapi-types-trace.h qapi/qapi-types-trace.c
> +GENERATED_FILES += qapi/qapi-types-transaction.h 
> qapi/qapi-types-transaction.c
> +GENERATED_FILES += qapi/qapi-types-ui.h qapi/qapi-types-ui.c
>  GENERATED_FILES += qapi-builtin-visit.h qapi-builtin-visit.c
>  GENERATED_FILES += qapi-visit.h qapi-visit.c
>  GENERATED_FILES += qapi-event.h qapi-event.c
> @@ -519,6 +534,21 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/qapi/common.json \
>
>  qapi-builtin-types.c qapi-builtin-types.h \
>  qapi-types.c qapi-types.h \
> +qapi/qapi-types-block-core.c qapi/qapi-types-block-core.h \
> +qapi/qapi-types-block.c qapi/qapi-types-block.h \
> +qapi/qapi-types-char.c qapi/qapi-types-char.h \
> +qapi/qapi-types-common.c qapi/qapi-types-common.h \
> +qapi/qapi-types-crypto.c qapi/qapi-types-crypto.h \
> +qapi/qapi-types-introspect.c qapi/qapi-types-introspect.h \
> +qapi/qapi-types-migration.c qapi/qapi-types-migration.h \
> +qapi/qapi-types-net.c qapi/qapi-types-net.h \
> +qapi/qapi-types-rocker.c qapi/qapi-types-rocker.h \
> +qapi/qapi-types-run-state.c qapi/qapi-types-run-state.h \
> +qapi/qapi-types-sockets.c qapi/qapi-types-sockets.h \
> +qapi/qapi-types-tpm.c qapi/qapi-types-tpm.h \
> +qapi/qapi-types-trace.c qapi/qapi-types-trace.h \
> +qapi/qapi-types-transaction.c qapi/qapi-types-transaction.h \
> +qapi/qapi-types-ui.c qapi/qapi-types-ui.h \
>  qapi-builtin-visit.c qapi-builtin-visit.h \
>  qapi-visit.c qapi-visit.h \
>  qmp-commands.h qmp-marshal.c \
> diff --git a/Makefile.objs b/Makefile.objs
> index f16cca06e7..e7411a2658 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -3,7 +3,23 @@
>  stub-obj-y = stubs/ crypto/
>  util-obj-y = util/ qobject/ qapi/
>  util-obj-y += qapi-builtin-types.o qapi-builtin-visit.o
> -util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
> +util-obj-y += qapi-types.o
> +util-obj-y += qapi/qapi-types-block-core.o
> +util-obj-y += qapi/qapi-types-block.o
> +util-obj-y += qapi/qapi-types-char.o
> +util-obj-y += qapi/qapi-types-common.o
> +util-obj-y += qapi/qapi-types-crypto.o
> +util-obj-y += qapi/qapi-types-introspect.o
> +util-obj-y += qapi/qapi-types-migration.o
> +util-obj-y += qapi/qapi-types-net.o
> +util-obj-y += qapi/qapi-types-rocker.o
> +util-obj-y += qapi/qapi-types-run-state.o

Re: [Qemu-devel] [PATCH RFC 15/21] qapi: Record 'include' directives in intermediate representation

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> The include directive permits modular QAPI schemata, but the generated
> code is monolithic all the same.  To permit generating modular code,
> the front end needs to pass more information on inclusions to the back
> ends.  The commit before last added the necessary information to the
> parse tree.  This commit adds it to the intermediate representation
> and its QAPISchemaVisitor.  A later commit will use this to to
> generate modular code.
>
> New entity QAPISchemaInclude represents inclusions.  Call new visitor
> method visit_include() for it, so visitors can see the sub-modules a
> module includes.
>
> New QAPISchemaEntity attribute @module names the entity's source file.
> Call new visitor method visit_module() when it changes during a visit,
> so visitors can keep track of the module being visited.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py   | 44 
> 
>  tests/qapi-schema/comments.out   |  1 +
>  tests/qapi-schema/doc-bad-section.out|  1 +
>  tests/qapi-schema/doc-good.out   |  1 +
>  tests/qapi-schema/event-case.out |  1 +
>  tests/qapi-schema/ident-with-escape.out  |  1 +
>  tests/qapi-schema/include-relpath.out|  5 
>  tests/qapi-schema/include-repetition.out | 10 
>  tests/qapi-schema/include-simple.out |  3 +++
>  tests/qapi-schema/indented-expr.out  |  1 +
>  tests/qapi-schema/qapi-schema-test.out   |  1 +
>  tests/qapi-schema/test-qapi.py   |  7 +
>  12 files changed, 71 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 3b97bf8702..f4e9ebbb53 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -981,8 +981,9 @@ def check_exprs(exprs):
>
>  class QAPISchemaEntity(object):
>  def __init__(self, name, info, doc):
> -assert isinstance(name, str)
> +assert name is None or isinstance(name, str)
>  self.name = name
> +self.module = None
>  # For explicitly defined entities, info points to the (explicit)
>  # definition.  For builtins (and their arrays), info is None.
>  # For implicitly defined entities, info points to a place that
> @@ -1011,10 +1012,16 @@ class QAPISchemaVisitor(object):
>  def visit_end(self):
>  pass
>
> +def visit_module(self, fname):
> +pass
> +
>  def visit_needed(self, entity):
>  # Default to visiting everything
>  return True
>
> +def visit_include(self, fname, info):
> +pass
> +
>  def visit_builtin_type(self, name, info, json_type):
>  pass
>
> @@ -1041,6 +1048,16 @@ class QAPISchemaVisitor(object):
>  pass
>
>
> +class QAPISchemaInclude(QAPISchemaEntity):
> +
> +def __init__(self, fname, info):
> +QAPISchemaEntity.__init__(self, None, info, None)
> +self.fname = fname
> +
> +def visit(self, visitor):
> +visitor.visit_include(self.fname, self.info)
> +
> +
>  class QAPISchemaType(QAPISchemaEntity):
>  # Return the C type for common use.
>  # For the types we commonly box, this is a pointer type.
> @@ -1468,6 +1485,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
>
>  class QAPISchema(object):
>  def __init__(self, fname):
> +self._fname = fname
>  parser = QAPISchemaParser(open(fname, 'r'))
>  exprs = check_exprs(parser.exprs)
>  self.docs = parser.docs
> @@ -1475,16 +1493,19 @@ class QAPISchema(object):
>  self._entity_dict = {}
>  self._predefining = True
>  self._def_predefineds()
> -self._predefining = False
>  self._def_exprs(exprs)
>  self.check()
>
>  def _def_entity(self, ent):
>  # Only the predefined types are allowed to not have info
>  assert ent.info or self._predefining
> -assert ent.name not in self._entity_dict
> +assert ent.name is None or ent.name not in self._entity_dict
>  self._entity_list.append(ent)
> -self._entity_dict[ent.name] = ent
> +if ent.name is not None:
> +self._entity_dict[ent.name] = ent
> +if ent.info:
> +ent.module = os.path.relpath(ent.info['file'],
> + os.path.dirname(self._fname))
>
>  def lookup_entity(self, name, typ=None):
>  ent = self._entity_dict.get(name)
> @@ -1495,6 +1516,15 @@ class QAPISchema(object):
>  def lookup_type(self, name):
>  return self.lookup_entity(name, QAPISchemaType)
>
> +def _def_include(self, expr, info, doc):
> +include = expr['include']
> +assert doc is None
> +main_info = info
> +while main_info['parent']:
> +main_info = main_info['parent']
> +fname = 

Re: [Qemu-devel] [PATCH RFC 04/21] qapi: Reduce use of global variables in generators some

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> In preparation of the next commit, which will turn the generators into
> modules.  These global variables will become local to main() then.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-commands.py   |  9 +
>  scripts/qapi-event.py  | 15 +++
>  scripts/qapi-introspect.py |  7 ---
>  scripts/qapi-types.py  | 17 +
>  scripts/qapi-visit.py  | 17 +
>  5 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 4be7dbc482..d229537659 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -207,7 +207,7 @@ def gen_register_command(name, success_response):
>  return ret
>
>
> -def gen_registry(registry):
> +def gen_registry(registry, prefix):
>  ret = mcgen('''
>
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
> @@ -224,7 +224,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
>
>
>  class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> -def __init__(self):
> +def __init__(self, prefix):
> +self._prefix = prefix
>  self.decl = None
>  self.defn = None
>  self._regy = None
> @@ -237,7 +238,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>  self._visited_ret_types = set()
>
>  def visit_end(self):
> -self.defn += gen_registry(self._regy)
> +self.defn += gen_registry(self._regy, self._prefix)
>  self._regy = None
>  self._visited_ret_types = None
>
> @@ -289,7 +290,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>
>  schema = QAPISchema(input_file)
> -vis = QAPISchemaGenCommandVisitor()
> +vis = QAPISchemaGenCommandVisitor(prefix)
>  schema.visit(vis)
>  genc.body(vis.defn)
>  genh.body(vis.decl)
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index da3de17c76..1af21b580a 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -58,7 +58,7 @@ def gen_param_var(typ):
>  return ret
>
>
> -def gen_event_send(name, arg_type, boxed):
> +def gen_event_send(name, arg_type, boxed, event_enum_name):
>  # FIXME: Our declaration of local variables (and of 'errp' in the
>  # parameter list) can collide with exploded members of the event's
>  # data type passed in as parameters.  If this collision ever hits in
> @@ -149,7 +149,8 @@ out:
>
>
>  class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
> -def __init__(self):
> +def __init__(self, prefix):
> +self._enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>  self.decl = None
>  self.defn = None
>  self._event_names = None
> @@ -160,13 +161,13 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>  self._event_names = []
>
>  def visit_end(self):
> -self.decl += gen_enum(event_enum_name, self._event_names)
> -self.defn += gen_enum_lookup(event_enum_name, self._event_names)
> +self.decl += gen_enum(self._enum_name, self._event_names)
> +self.defn += gen_enum_lookup(self._enum_name, self._event_names)
>  self._event_names = None
>
>  def visit_event(self, name, info, arg_type, boxed):
>  self.decl += gen_event_send_decl(name, arg_type, boxed)
> -self.defn += gen_event_send(name, arg_type, boxed)
> +self.defn += gen_event_send(name, arg_type, boxed, self._enum_name)
>  self._event_names.append(name)
>
>
> @@ -199,10 +200,8 @@ genh.body(mcgen('''
>  ''',
>  prefix=prefix))
>
> -event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
> -
>  schema = QAPISchema(input_file)
> -vis = QAPISchemaGenEventVisitor()
> +vis = QAPISchemaGenEventVisitor(prefix)
>  schema.visit(vis)
>  genc.body(vis.defn)
>  genh.body(vis.decl)
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index c654f8fa94..8d4e3c1c3a 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -41,7 +41,8 @@ def to_c_string(string):
>
>
>  class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
> -def __init__(self, unmask):
> +def __init__(self, prefix, unmask):
> +self._prefix = prefix
>  self._unmask = unmask
>  self.defn = None
>  self.decl = None
> @@ -65,7 +66,7 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>  # generate C
>  # TODO can generate awfully long lines
>  jsons.extend(self._jsons)
> -name = c_name(prefix, protect=False) + 'qmp_schema_json'
> +name = c_name(self._prefix, protect=False) + 'qmp_schema_json'
>  self.decl = mcgen('''
>  extern const char %(c_name)s[];
>  ''',
> @@ -192,7 +193,7 @@ genc.body(mcgen('''
>   

Re: [Qemu-devel] [PATCH RFC 13/21] qapi: Record 'include' directives in parse tree

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> The parse tree is a list of expressions.  Except include expressions
> currently get replaced by the included file's parse tree.
>
> Instead of throwing away the include expression, keep it with the file
> name expanded so you don't have to track the including file's
> directory to make sense of it.
>
> A future commit will put this include expression to use.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py | 21 +
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 7a327bfe9f..d5b93e7381 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -286,8 +286,11 @@ class QAPISchemaParser(object):
>  if not isinstance(include, str):
>  raise QAPISemError(info,
> "Value of 'include' must be a string")
> -exprs_include = self._include(include, info,
> -  os.path.dirname(self.fname),
> +incl_fname = os.path.join(os.path.dirname(self.fname),
> +  include)
> +self.exprs.append({'expr': {'include': incl_fname},
> +   'info': info})
> +exprs_include = self._include(include, info, incl_fname,
>previously_included)
>  if exprs_include:
>  self.exprs.extend(exprs_include.exprs)
> @@ -322,8 +325,7 @@ class QAPISchemaParser(object):
>  "Documentation for '%s' is not followed by the definition"
>  % doc.symbol)
>
> -def _include(self, include, info, base_dir, previously_included):
> -incl_fname = os.path.join(base_dir, include)
> +def _include(self, include, info, incl_fname, previously_included):
>  incl_abs_fname = os.path.abspath(incl_fname)
>  # catch inclusion cycle
>  inf = info
> @@ -889,6 +891,9 @@ def check_exprs(exprs):
>  info = expr_elem['info']
>  doc = expr_elem.get('doc')
>
> +if 'include' in expr:
> +continue
> +
>  if not doc and doc_required:
>  raise QAPISemError(info,
> "Expression missing documentation comment")
> @@ -927,6 +932,9 @@ def check_exprs(exprs):
>
>  # Try again for hidden UnionKind enum
>  for expr_elem in exprs:
> +if 'include' in expr:
> +continue
> +
>  expr = expr_elem['expr']
>  if 'union' in expr and not discriminator_find_enum_define(expr):
>  name = '%sKind' % expr['union']
> @@ -939,6 +947,9 @@ def check_exprs(exprs):
>
>  # Validate that exprs make sense
>  for expr_elem in exprs:
> +if 'include' in expr:
> +continue
> +
>  expr = expr_elem['expr']
>  info = expr_elem['info']
>  doc = expr_elem.get('doc')
> @@ -1663,6 +1674,8 @@ class QAPISchema(object):
>  self._def_command(expr, info, doc)
>  elif 'event' in expr:
>  self._def_event(expr, info, doc)
> +elif 'include' in expr:
> +pass
>  else:
>  assert False
>
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 03/21] qapi: New classes QAPIGenC, QAPIGenH, QAPIGenDoc

[Marc-Andre Lureau]

Hi

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> These classes encapsulate accumulating and writing output.
>
> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
> rather shallow: most of the output accumulation is not converted.
> Left for later.
>
> The indentation machinery uses a single global variable indent_level,
> even though we generally interleave creation of a .c and its .h.  It
> should become instance variable of QAPIGenC.  Also left for later.
>
> Documentation generation isn't converted, and QAPIGenDoc isn't used.
> This will change shortly.
>
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi-commands.py   | 27 ++---
>  scripts/qapi-event.py  | 26 +++--
>  scripts/qapi-introspect.py | 22 ++-
>  scripts/qapi-types.py  | 26 +++--
>  scripts/qapi-visit.py  | 26 +++--
>  scripts/qapi.py| 96 
> ++
>  6 files changed, 122 insertions(+), 101 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index a861ac52e7..4be7dbc482 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -260,12 +260,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP commands
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -'qmp-marshal.c', 'qmp-commands.h',
> -blurb, __doc__)
> -
> -fdef.write(mcgen('''
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/module.h"
> @@ -279,21 +277,24 @@ fdef.write(mcgen('''
>  #include "%(prefix)sqmp-commands.h"
>
>  ''',
> - prefix=prefix))
> +prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "%(prefix)sqapi-types.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/dispatch.h"
>
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> +prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenCommandVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenCommandVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +genc.write(output_dir, prefix + 'qmp-marshal.c')
> +if do_h:
> +genh.write(output_dir, prefix + 'qmp-commands.h')
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b1d611c5ea..da3de17c76 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -176,11 +176,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP events
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -'qapi-event.c', 'qapi-event.h',
> -blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "%(prefix)sqapi-event.h"
> @@ -190,22 +189,25 @@ fdef.write(mcgen('''
>  #include "qapi/qmp-event.h"
>
>  ''',
> - prefix=prefix))
> +prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "qapi/util.h"
>  #include "qapi/qmp/qdict.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> -  prefix=prefix))
> +prefix=prefix))
>
>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenEventVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenEventVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +genc.write(output_dir, prefix + 'qapi-event.c')
> +if do_h:
> +genh.write(output_dir, prefix + 'qapi-event.h')
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index bd9253a172..c654f8fa94 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -181,21 +181,23 @@ blurb = '''
>   * QAPI/QMP schema introspection
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -'qmp-introspect.c', 'qmp-introspect.h',
> -blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "%(prefix)sqmp-introspect.h"
>
>  ''',
> - prefix=prefix))
> +prefix=prefix))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
> -schema.visit(gen)
> -fdef.write(gen.defn)
> 

Re: [Qemu-devel] [PATCH RFC 02/21] qapi: Generate up-to-date copyright notice

[Marc-Andre Lureau]

Hi

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Each generator carries a copyright notice for the generator itself,
> and another one for the files it generates.  Only the former have been
> updated along the way, the latter have not, and are all out of date.
>
> Fix by copying the generator's copyright notice to the generated files
> instead.

That makes sense, but we loose the Author lines in the generated
files. Not a big deal I guess, but worth to point out, no?

>
> Signed-off-by: Markus Armbruster 

Other than that,
Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-commands.py   | 34 +++---
>  scripts/qapi-event.py  | 32 ++--
>  scripts/qapi-introspect.py | 25 -
>  scripts/qapi-types.py  | 32 ++--
>  scripts/qapi-visit.py  | 34 +++---
>  scripts/qapi.py|  7 +--
>  6 files changed, 75 insertions(+), 89 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 25ac52503a..a861ac52e7 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -1,16 +1,17 @@
> -#
> -# QAPI command marshaller generator
> -#
> -# Copyright IBM, Corp. 2011
> -# Copyright (C) 2014-2016 Red Hat, Inc.
> -#
> -# Authors:
> -#  Anthony Liguori 
> -#  Michael Roth
> -#  Markus Armbruster 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.
> -# See the COPYING file in the top-level directory.
> +"""
> +QAPI command marshaller generator
> +
> +Copyright IBM, Corp. 2011
> +Copyright (C) 2014-2018 Red Hat, Inc.
> +
> +Authors:
> + Anthony Liguori 
> + Michael Roth 
> + Markus Armbruster 
> +
> +This work is licensed under the terms of the GNU GPL, version 2.
> +See the COPYING file in the top-level directory.
> +"""
>
>  from qapi import *
>
> @@ -257,16 +258,11 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>
>  blurb = '''
>   * Schema-defined QAPI/QMP commands
> - *
> - * Copyright IBM, Corp. 2011
> - *
> - * Authors:
> - *  Anthony Liguori   
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>  'qmp-marshal.c', 'qmp-commands.h',
> -blurb)
> +blurb, __doc__)
>
>  fdef.write(mcgen('''
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 31faedc689..b1d611c5ea 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -1,15 +1,16 @@
> -#
> -# QAPI event generator
> -#
> -# Copyright (c) 2014 Wenchao Xia
> -# Copyright (c) 2015-2016 Red Hat Inc.
> -#
> -# Authors:
> -#  Wenchao Xia 
> -#  Markus Armbruster 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.
> -# See the COPYING file in the top-level directory.
> +"""
> +QAPI event generator
> +
> +Copyright (c) 2014 Wenchao Xia
> +Copyright (c) 2015-2018 Red Hat Inc.
> +
> +Authors:
> + Wenchao Xia 
> + Markus Armbruster 
> +
> +This work is licensed under the terms of the GNU GPL, version 2.
> +See the COPYING file in the top-level directory.
> +"""
>
>  from qapi import *
>
> @@ -173,16 +174,11 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>
>  blurb = '''
>   * Schema-defined QAPI/QMP events
> - *
> - * Copyright (c) 2014 Wenchao Xia
> - *
> - * Authors:
> - *  Wenchao Xia   
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>  'qapi-event.c', 'qapi-event.h',
> -blurb)
> +blurb, __doc__)
>
>  fdef.write(mcgen('''
>  #include "qemu/osdep.h"
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 83da2bdb94..bd9253a172 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -1,13 +1,14 @@
> -#
> -# QAPI introspection generator
> -#
> -# Copyright (C) 2015-2016 Red Hat, Inc.
> -#
> -# Authors:
> -#  Markus Armbruster 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.
> -# See the COPYING file in the top-level directory.
> +"""
> +QAPI introspection generator
> +
> +Copyright (C) 2015-2018 Red Hat, Inc.
> +
> +Authors:
> + Markus Armbruster 
> +
> +This work is licensed under the terms of the GNU GPL, version 2.
> +See the COPYING file in the top-level directory.
> +"""
>
>  from qapi import *
>
> @@ -178,13 +179,11 @@ for o, a in opts:
>
>  blurb = '''
>   * QAPI/QMP schema introspection
> - *
> - * Copyright (C) 2015 Red Hat, Inc.
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>

Re: [Qemu-devel] [PATCH RFC 06/21] qapi-gen: New common driver for code and doc generators

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Whenever qapi-schema.json changes, we run six programs eleven times to
> update eleven files.  This is silly.  Replace the six programs by a
> single program that spits out all eleven files.
>

Now we will need documentation update.

Also greping for the renamed files:

git grep -E 
'qapi-commands|qapi2texi|qapi-event.py|qapi-intros|qapi-types.py|qapi-visit.py'
docs/devel/qapi-code-gen.txt:=== scripts/qapi-types.py ===
docs/devel/qapi-code-gen.txt:$ python scripts/qapi-types.py
--output-dir="qapi-generated" \
docs/devel/qapi-code-gen.txt:=== scripts/qapi-visit.py ===
docs/devel/qapi-code-gen.txt:$ python scripts/qapi-visit.py
--output-dir="qapi-generated"
docs/devel/qapi-code-gen.txt:=== scripts/qapi-commands.py ===
docs/devel/qapi-code-gen.txt:generated by
qapi-visit.py are used to
docs/devel/qapi-code-gen.txt:$ python scripts/qapi-commands.py
--output-dir="qapi-generated"
docs/devel/qapi-code-gen.txt:=== scripts/qapi-event.py ===
docs/devel/qapi-code-gen.txt:$ python scripts/qapi-event.py
--output-dir="qapi-generated"
docs/devel/qapi-code-gen.txt:=== scripts/qapi-introspect.py ===
docs/devel/qapi-code-gen.txt:$ python scripts/qapi-introspect.py
--output-dir="qapi-generated"
monitor.c: * qapi-introspect.py's output actually conforms to the schema.
qapi-schema.json:# Documentation generated with qapi2texi.py is in
source order, with

> Signed-off-by: Markus Armbruster 
> ---
>  Makefile   | 86 
> ++
>  scripts/qapi-gen.py| 41 +++
>  scripts/qapi/__init__.py   |  0
>  scripts/{qapi-commands.py => qapi/commands.py} | 23 ++
>  scripts/{qapi.py => qapi/common.py}|  0
>  scripts/{qapi2texi.py => qapi/doc.py}  | 29 ++--
>  scripts/{qapi-event.py => qapi/events.py}  | 23 ++
>  scripts/{qapi-introspect.py => qapi/introspect.py} | 32 ++--
>  scripts/{qapi-types.py => qapi/types.py}   | 34 ++---
>  scripts/{qapi-visit.py => qapi/visit.py}   | 34 ++---
>  tests/Makefile.include | 56 +++---
>  tests/qapi-schema/test-qapi.py |  2 +-
>  12 files changed, 140 insertions(+), 220 deletions(-)
>  create mode 100755 scripts/qapi-gen.py
>  create mode 100644 scripts/qapi/__init__.py
>  rename scripts/{qapi-commands.py => qapi/commands.py} (94%)
>  rename scripts/{qapi.py => qapi/common.py} (100%)
>  rename scripts/{qapi2texi.py => qapi/doc.py} (92%)
>  mode change 100755 => 100644
>  rename scripts/{qapi-event.py => qapi/events.py} (92%)
>  rename scripts/{qapi-introspect.py => qapi/introspect.py} (90%)
>  rename scripts/{qapi-types.py => qapi/types.py} (90%)
>  rename scripts/{qapi-visit.py => qapi/visit.py} (92%)
>
> diff --git a/Makefile b/Makefile
> index af31e8981f..e02f0c13ef 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -92,6 +92,7 @@ GENERATED_FILES += qmp-commands.h qapi-types.h qapi-visit.h 
> qapi-event.h
>  GENERATED_FILES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-event.c
>  GENERATED_FILES += qmp-introspect.h
>  GENERATED_FILES += qmp-introspect.c
> +GENERATED_FILES += qapi.texi
>
>  GENERATED_FILES += trace/generated-tcg-tracers.h
>
> @@ -477,25 +478,26 @@ qemu-ga$(EXESUF): QEMU_CFLAGS += -I qga/qapi-generated
>  qemu-keymap$(EXESUF): LIBS += $(XKBCOMMON_LIBS)
>  qemu-keymap$(EXESUF): QEMU_CFLAGS += $(XKBCOMMON_CFLAGS)
>
> -gen-out-type = $(subst .,-,$(suffix $@))
> +qapi-py = $(SRC_PATH)/scripts/qapi/commands.py \
> +$(SRC_PATH)/scripts/qapi/events.py \
> +$(SRC_PATH)/scripts/qapi/introspect.py \
> +$(SRC_PATH)/scripts/qapi/types.py \
> +$(SRC_PATH)/scripts/qapi/visit.py \
> +$(SRC_PATH)/scripts/qapi/common.py \
> +$(SRC_PATH)/scripts/qapi/doc.py \
> +$(SRC_PATH)/scripts/ordereddict.py \
> +$(SRC_PATH)/scripts/qapi-gen.py
>
> -qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
> -
> -qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> -   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
> -   $(gen-out-type) -o qga/qapi-generated -p "qga-" $<, \
> -   "GEN","$@")
> -qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> -   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py \
> -   $(gen-out-type) -o qga/qapi-generated -p "qga-" $<, \
> -   "GEN","$@")
> -qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-marshal.c :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py 
> $(qapi-py)
> -   $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py \
> 

Re: [Qemu-devel] [PATCH RFC 16/21] qapi/types qapi/visit: Make visitors use QAPIGen more

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> The conversion is rather shallow so far: most of the output
> accumulation is not converted.  Take the next step: convert output
> accumulation in QAPISchemaGenTypeVisitor and
> QAPISchemaGenVisitVisitor.  Helper functions outside these classes are
> not converted.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/types.py | 83 
> +++
>  scripts/qapi/visit.py | 83 
> +++
>  2 files changed, 74 insertions(+), 92 deletions(-)
>
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index b2095120e0..eeffcbf32c 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -168,35 +168,44 @@ void qapi_free_%(c_name)s(%(c_name)s *obj)
>
>
>  class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> -def __init__(self, opt_builtins):
> +def __init__(self, opt_builtins, prefix):
>  self._opt_builtins = opt_builtins
> -self.decl = None
> -self.defn = None
> -self._fwdecl = None
> -self._btin = None
> +self._prefix = prefix
> +blurb = ' * Schema-defined QAPI types'
> +self._genc = QAPIGenC(blurb, __doc__)
> +self._genh = QAPIGenH(blurb, __doc__)
> +self._genc.preamble(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/dealloc-visitor.h"
> +#include "%(prefix)sqapi-types.h"
> +#include "%(prefix)sqapi-visit.h"
> +''',
> +  prefix=prefix))
> +self._genh.preamble(mcgen('''
> +#include "qapi/util.h"
> +'''))
> +self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
> +
> +def write(self, output_dir):
> +self._genc.write(output_dir, self._prefix + 'qapi-types.c')
> +self._genh.write(output_dir, self._prefix + 'qapi-types.h')
>
>  def visit_begin(self, schema):
>  # gen_object() is recursive, ensure it doesn't visit the empty type
>  objects_seen.add(schema.the_empty_object_type.name)
> -self.decl = ''
> -self.defn = ''
> -self._fwdecl = ''
> -self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
>
>  def visit_end(self):
> -self.decl = self._fwdecl + self.decl
> -self._fwdecl = None
>  # To avoid header dependency hell, we always generate
>  # declarations for built-in types in our header files and
>  # simply guard them.  See also opt_builtins (command line
>  # option -b).
>  self._btin += guardend('QAPI_TYPES_BUILTIN')
> -self.decl = self._btin + self.decl
> +self._genh.preamble(self._btin)
>  self._btin = None
>
>  def _gen_type_cleanup(self, name):
> -self.decl += gen_type_cleanup_decl(name)
> -self.defn += gen_type_cleanup(name)
> +self._genh.body(gen_type_cleanup_decl(name))
> +self._genc.body(gen_type_cleanup(name))
>
>  def visit_enum_type(self, name, info, values, prefix):
>  # Special case for our lone builtin enum type
> @@ -204,10 +213,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>  if not info:
>  self._btin += gen_enum(name, values, prefix)
>  if self._opt_builtins:
> -self.defn += gen_enum_lookup(name, values, prefix)
> +self._genc.body(gen_enum_lookup(name, values, prefix))
>  else:
> -self._fwdecl += gen_enum(name, values, prefix)
> -self.defn += gen_enum_lookup(name, values, prefix)
> +self._genh.preamble(gen_enum(name, values, prefix))
> +self._genc.body(gen_enum_lookup(name, values, prefix))
>
>  def visit_array_type(self, name, info, element_type):
>  if isinstance(element_type, QAPISchemaBuiltinType):
> @@ -215,20 +224,20 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>  self._btin += gen_array(name, element_type)
>  self._btin += gen_type_cleanup_decl(name)
>  if self._opt_builtins:
> -self.defn += gen_type_cleanup(name)
> +self._genc.body(gen_type_cleanup(name))
>  else:
> -self._fwdecl += gen_fwd_object_or_array(name)
> -self.decl += gen_array(name, element_type)
> +self._genh.preamble(gen_fwd_object_or_array(name))
> +self._genh.body(gen_array(name, element_type))
>  self._gen_type_cleanup(name)
>
>  def visit_object_type(self, name, info, base, members, variants):
>  # Nothing to do for the special empty builtin
>  if name == 'q_empty':
>  return
> -self._fwdecl += gen_fwd_object_or_array(name)
> -self.decl += gen_object(name, base, members, variants)
> +self._genh.preamble(gen_fwd_object_or_array(name))
> +

Re: [Qemu-devel] [PATCH RFC 01/21] qapi: Streamline boilerplate comment generation

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Every generator has separate boilerplate for .h and .c, and their
> differences are boring.  All of them repeat the license note.
>
> Reduce the repetition as follows.  Move common text like the license
> note to common open_output(), next to the existintg common text there.
> For each generator, replace the two separate descriptions by a single
> one.
>
> While there, emit an "automatically generated" note into generated
> documentation, too.
>
> Signed-off-by: Markus Armbruster 

Looks good,
Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-commands.py| 26 +++---
>  scripts/qapi-event.py   | 26 +++---
>  scripts/qapi-introspect.py  | 21 ++---
>  scripts/qapi-types.py   | 26 +++---
>  scripts/qapi-visit.py   | 26 +++---
>  scripts/qapi.py | 31 ++-
>  scripts/qapi2texi.py|  3 ++-
>  tests/qapi-schema/doc-good.texi |  3 ++-
>  8 files changed, 36 insertions(+), 126 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 26c56c5062..25ac52503a 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -255,38 +255,18 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>
>  (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
>
> -c_comment = '''
> -/*
> - * schema-defined QMP->QAPI command dispatch
> +blurb = '''
> + * Schema-defined QAPI/QMP commands
>   *
>   * Copyright IBM, Corp. 2011
>   *
>   * Authors:
>   *  Anthony Liguori   
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
> -'''
> -h_comment = '''
> -/*
> - * schema-defined QAPI function prototypes
> - *
> - * Copyright IBM, Corp. 2011
> - *
> - * Authors:
> - *  Anthony Liguori   
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>  'qmp-marshal.c', 'qmp-commands.h',
> -c_comment, h_comment)
> +blurb)
>
>  fdef.write(mcgen('''
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 9d7134658d..31faedc689 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -171,38 +171,18 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>
>  (input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
>
> -c_comment = '''
> -/*
> - * schema-defined QAPI event functions
> +blurb = '''
> + * Schema-defined QAPI/QMP events
>   *
>   * Copyright (c) 2014 Wenchao Xia
>   *
>   * Authors:
>   *  Wenchao Xia   
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
> -'''
> -h_comment = '''
> -/*
> - * schema-defined QAPI event functions
> - *
> - * Copyright (c) 2014 Wenchao Xia
> - *
> - * Authors:
> - *  Wenchao Xia  
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>  'qapi-event.c', 'qapi-event.h',
> -c_comment, h_comment)
> +blurb)
>
>  fdef.write(mcgen('''
>  #include "qemu/osdep.h"
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 032bcea491..83da2bdb94 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -176,32 +176,15 @@ for o, a in opts:
>  if o in ('-u', '--unmask-non-abi-names'):
>  opt_unmask = True
>
> -c_comment = '''
> -/*
> +blurb = '''
>   * QAPI/QMP schema introspection
>   *
>   * Copyright (C) 2015 Red Hat, Inc.
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
> -'''
> -h_comment = '''
> -/*
> - * QAPI/QMP schema introspection
> - *
> - * Copyright (C) 2015 Red Hat, Inc.
> - *
> - * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> - * See the COPYING.LIB file in the top-level directory.
> - *
> - */
>  '''
>
>  (fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>  'qmp-introspect.c', 'qmp-introspect.h',
> -c_comment, h_comment)
> +blurb)
>
>  

Re: [Qemu-devel] [PATCH RFC 07/21] qapi: Move parse_command_line() next to its only use

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-gen.py| 52 +++-
>  scripts/qapi/common.py | 54 
> --
>  2 files changed, 51 insertions(+), 55 deletions(-)
>
> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index 575c938a1b..6302fd0d55 100755
> --- a/scripts/qapi-gen.py
> +++ b/scripts/qapi-gen.py
> @@ -4,8 +4,10 @@
>  # This work is licensed under the terms of the GNU GPL, version 2 or later.
>  # See the COPYING file in the top-level directory.
>
> +import getopt
> +import re
>  import sys
> -from qapi.common import parse_command_line, QAPISchema
> +from qapi.common import QAPISchema
>  from qapi.types import gen_types
>  from qapi.visit import gen_visit
>  from qapi.commands import gen_commands
> @@ -14,6 +16,54 @@ from qapi.introspect import gen_introspect
>  from qapi.doc import gen_doc
>
>
> +def parse_command_line(extra_options='', extra_long_options=[]):
> +
> +try:
> +opts, args = getopt.gnu_getopt(sys.argv[1:],
> +   'chp:o:' + extra_options,
> +   ['source', 'header', 'prefix=',
> +'output-dir='] + extra_long_options)
> +except getopt.GetoptError as err:
> +print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
> +sys.exit(1)
> +
> +output_dir = ''
> +prefix = ''
> +do_c = False
> +do_h = False
> +extra_opts = []
> +
> +for oa in opts:
> +o, a = oa
> +if o in ('-p', '--prefix'):
> +match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', a)
> +if match.end() != len(a):
> +print >>sys.stderr, \
> +"%s: 'funny character '%s' in argument of --prefix" \
> +% (sys.argv[0], a[match.end()])
> +sys.exit(1)
> +prefix = a
> +elif o in ('-o', '--output-dir'):
> +output_dir = a + '/'
> +elif o in ('-c', '--source'):
> +do_c = True
> +elif o in ('-h', '--header'):
> +do_h = True
> +else:
> +extra_opts.append(oa)
> +
> +if not do_c and not do_h:
> +do_c = True
> +do_h = True
> +
> +if len(args) != 1:
> +print >>sys.stderr, "%s: need exactly one argument" % sys.argv[0]
> +sys.exit(1)
> +fname = args[0]
> +
> +return (fname, output_dir, do_c, do_h, prefix, extra_opts)
> +
> +
>  def main(argv):
>  (input_file, output_dir, do_c, do_h, prefix, opts) = \
>  parse_command_line('bu', ['builtins', 'unmask-non-abi-names'])
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index d73ef618e2..cfa2671ca3 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -12,7 +12,6 @@
>  # See the COPYING file in the top-level directory.
>
>  import errno
> -import getopt
>  import os
>  import re
>  import string
> @@ -1917,59 +1916,6 @@ def build_params(arg_type, boxed, extra):
>
>
>  #
> -# Common command line parsing
> -#
> -
> -
> -def parse_command_line(extra_options='', extra_long_options=[]):
> -
> -try:
> -opts, args = getopt.gnu_getopt(sys.argv[1:],
> -   'chp:o:' + extra_options,
> -   ['source', 'header', 'prefix=',
> -'output-dir='] + extra_long_options)
> -except getopt.GetoptError as err:
> -print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
> -sys.exit(1)
> -
> -output_dir = ''
> -prefix = ''
> -do_c = False
> -do_h = False
> -extra_opts = []
> -
> -for oa in opts:
> -o, a = oa
> -if o in ('-p', '--prefix'):
> -match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', a)
> -if match.end() != len(a):
> -print >>sys.stderr, \
> -"%s: 'funny character '%s' in argument of --prefix" \
> -% (sys.argv[0], a[match.end()])
> -sys.exit(1)
> -prefix = a
> -elif o in ('-o', '--output-dir'):
> -output_dir = a + '/'
> -elif o in ('-c', '--source'):
> -do_c = True
> -elif o in ('-h', '--header'):
> -do_h = True
> -else:
> -extra_opts.append(oa)
> -
> -if not do_c and not do_h:
> -do_c = True
> -do_h = True
> -
> -if len(args) != 1:
> -print >>sys.stderr, "%s: need exactly one argument" % sys.argv[0]
> -sys.exit(1)
> -fname = args[0]
> -
> -return (fname, output_dir, do_c, do_h, prefix, extra_opts)
> -
> -
> -#
>  # Accumulate and write output
>  #
>
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 20/21] Include less of qapi-types.h

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> In my "build everything" tree, a change to the types in
> qapi-schema.json triggers a recompile of about 4500 out of 4800
> objects.
>
> The previous commit split up the generated qapi-types.h.  Replace
> includes of qapi-types.h (i.e. all types) by includes of parts where
> possible.
>
> To illustrate the benefits: adding a type to qapi/migration.json now
> recompiles some 2300 instead of 4500 objects.  The next commit will
> improve it further.
>
> Signed-off-by: Markus Armbruster 

for the record, what was your methodology?
Reviewed-by: Marc-André Lureau 



> ---
>  crypto/cipherpriv.h  | 2 +-
>  hw/block/block.c | 1 +
>  hw/block/hd-geometry.c   | 1 +
>  hw/net/rocker/rocker_fp.c| 2 +-
>  include/block/block.h| 2 +-
>  include/block/dirty-bitmap.h | 2 +-
>  include/chardev/char.h   | 1 +
>  include/crypto/cipher.h  | 2 +-
>  include/crypto/hash.h| 2 +-
>  include/crypto/hmac.h| 2 +-
>  include/crypto/secret.h  | 1 +
>  include/crypto/tlscreds.h| 1 +
>  include/hw/block/block.h | 2 +-
>  include/hw/block/fdc.h   | 2 +-
>  include/hw/ppc/spapr_drc.h   | 1 +
>  include/hw/qdev-properties.h | 1 +
>  include/io/dns-resolver.h| 1 +
>  include/migration/colo.h | 2 +-
>  include/migration/failover.h | 2 +-
>  include/migration/global_state.h | 1 +
>  include/monitor/monitor.h| 1 +
>  include/net/filter.h | 1 +
>  include/net/net.h| 2 +-
>  include/qapi/error.h | 2 +-
>  include/qapi/qmp/qobject.h   | 2 +-
>  include/qapi/visitor.h   | 2 +-
>  include/qemu/sockets.h   | 2 +-
>  include/qemu/throttle.h  | 2 +-
>  include/qom/cpu.h| 1 +
>  include/qom/object.h | 2 +-
>  include/sysemu/dump.h| 2 ++
>  include/sysemu/hostmem.h | 1 +
>  include/sysemu/replay.h  | 1 +
>  include/sysemu/sysemu.h  | 1 +
>  include/sysemu/tpm.h | 1 +
>  include/sysemu/watchdog.h| 2 +-
>  include/ui/input.h   | 2 +-
>  migration/migration.h| 1 +
>  migration/ram.h  | 2 +-
>  net/tap_int.h| 2 +-
>  replication.h| 1 +
>  ui/vnc.h | 1 +
>  42 files changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/crypto/cipherpriv.h b/crypto/cipherpriv.h
> index 77da4c2f32..0823239f41 100644
> --- a/crypto/cipherpriv.h
> +++ b/crypto/cipherpriv.h
> @@ -15,7 +15,7 @@
>  #ifndef QCRYPTO_CIPHERPRIV_H
>  #define QCRYPTO_CIPHERPRIV_H
>
> -#include "qapi-types.h"
> +#include "qapi/qapi-types-crypto.h"
>
>  typedef struct QCryptoCipherDriver QCryptoCipherDriver;
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index b0269c857f..b91e2b6d7e 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -12,6 +12,7 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/block/block.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-types-block.h"
>  #include "qemu/error-report.h"
>
>  void blkconf_serial(BlockConf *conf, char **serial)
> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
> index 57ad5012a7..79384a2b0a 100644
> --- a/hw/block/hd-geometry.c
> +++ b/hw/block/hd-geometry.c
> @@ -32,6 +32,7 @@
>
>  #include "qemu/osdep.h"
>  #include "sysemu/block-backend.h"
> +#include "qapi/qapi-types-block.h"
>  #include "qemu/bswap.h"
>  #include "hw/block/block.h"
>  #include "trace.h"
> diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
> index 4b3c9847db..27b17c890f 100644
> --- a/hw/net/rocker/rocker_fp.c
> +++ b/hw/net/rocker/rocker_fp.c
> @@ -16,7 +16,7 @@
>
>  #include "qemu/osdep.h"
>  #include "net/clients.h"
> -
> +#include "qapi/qapi-types-rocker.h"
>  #include "rocker.h"
>  #include "rocker_hw.h"
>  #include "rocker_fp.h"
> diff --git a/include/block/block.h b/include/block/block.h
> index ae1517f32d..70b90cd767 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -2,7 +2,7 @@
>  #define BLOCK_H
>
>  #include "block/aio.h"
> -#include "qapi-types.h"
> +#include "qapi/qapi-types-block-core.h"
>  #include "qemu/iov.h"
>  #include "qemu/coroutine.h"
>  #include "block/accounting.h"
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 3da8486ab1..1454be358d 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -2,7 +2,7 @@
>  #define BLOCK_DIRTY_BITMAP_H
>
>  #include "qemu-common.h"
> -#include "qapi-types.h"
> +#include "qapi/qapi-types-block-core.h"
>  #include "qemu/hbitmap.h"
>
>  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index a381dc3df8..ebf1e0ba04 100644
> --- a/include/chardev/char.h
> +++ 

Re: [Qemu-devel] [PATCH RFC 14/21] qapi: Generate in source order

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> The generators' conversion to visitors (merge commit 9e72681d16)
> changed the processing order of entities from source order to
> alphabetical order.  The next commit needs source order, so change it
> back.
>
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/common.py   |   4 +-
>  tests/qapi-schema/comments.out   |   2 +-
>  tests/qapi-schema/doc-bad-section.out|   4 +-
>  tests/qapi-schema/doc-good.out   |  32 ++--
>  tests/qapi-schema/empty.out  |   2 +-
>  tests/qapi-schema/event-case.out |   2 +-
>  tests/qapi-schema/ident-with-escape.out  |   6 +-
>  tests/qapi-schema/include-relpath.out|   2 +-
>  tests/qapi-schema/include-repetition.out |   2 +-
>  tests/qapi-schema/include-simple.out |   2 +-
>  tests/qapi-schema/indented-expr.out  |   2 +-
>  tests/qapi-schema/qapi-schema-test.out   | 320 
> +++
>  12 files changed, 191 insertions(+), 189 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index d5b93e7381..3b97bf8702 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1471,6 +1471,7 @@ class QAPISchema(object):
>  parser = QAPISchemaParser(open(fname, 'r'))
>  exprs = check_exprs(parser.exprs)
>  self.docs = parser.docs
> +self._entity_list = []
>  self._entity_dict = {}
>  self._predefining = True
>  self._def_predefineds()
> @@ -1482,6 +1483,7 @@ class QAPISchema(object):
>  # Only the predefined types are allowed to not have info
>  assert ent.info or self._predefining
>  assert ent.name not in self._entity_dict
> +self._entity_list.append(ent)
>  self._entity_dict[ent.name] = ent

Why not use the OrderedDict instead?

>
>  def lookup_entity(self, name, typ=None):
> @@ -1685,7 +1687,7 @@ class QAPISchema(object):
>
>  def visit(self, visitor):
>  visitor.visit_begin(self)
> -for (name, entity) in sorted(self._entity_dict.items()):
> +for entity in self._entity_list:
>  if visitor.visit_needed(entity):
>  entity.visit(visitor)
>  visitor.visit_end()
> diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
> index 17e652535c..0261ddf202 100644
> --- a/tests/qapi-schema/comments.out
> +++ b/tests/qapi-schema/comments.out
> @@ -1,4 +1,4 @@
> +object q_empty
>  enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
>  prefix QTYPE
>  enum Status ['good', 'bad', 'ugly']
> -object q_empty
> diff --git a/tests/qapi-schema/doc-bad-section.out 
> b/tests/qapi-schema/doc-bad-section.out
> index 089bde1381..23bf8c71ab 100644
> --- a/tests/qapi-schema/doc-bad-section.out
> +++ b/tests/qapi-schema/doc-bad-section.out
> @@ -1,7 +1,7 @@
> -enum Enum ['one', 'two']
> +object q_empty
>  enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
>  prefix QTYPE
> -object q_empty
> +enum Enum ['one', 'two']
>  doc symbol=Enum
>  body=
>  == Produces *invalid* texinfo
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 1d2c250527..0c07301f07 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -1,35 +1,35 @@
> +object q_empty
> +enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
> +prefix QTYPE
> +enum Enum ['one', 'two']
>  object Base
>  member base1: Enum optional=False
> -enum Enum ['one', 'two']
> +object Variant1
> +member var1: str optional=False
> +object Variant2
>  object Object
>  base Base
>  tag base1
>  case one: Variant1
>  case two: Variant2
> -enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
> -prefix QTYPE
> +object q_obj_Variant1-wrapper
> +member data: Variant1 optional=False
> +object q_obj_Variant2-wrapper
> +member data: Variant2 optional=False
> +enum SugaredUnionKind ['one', 'two']
>  object SugaredUnion
>  member type: SugaredUnionKind optional=False
>  tag type
>  case one: q_obj_Variant1-wrapper
>  case two: q_obj_Variant2-wrapper
> -enum SugaredUnionKind ['one', 'two']
> -object Variant1
> -member var1: str optional=False
> -object Variant2
> -command cmd q_obj_cmd-arg -> Object
> -   gen=True success_response=True boxed=False
> -command cmd-boxed Object -> None
> -   gen=True success_response=True boxed=True
> -object q_empty
> -object q_obj_Variant1-wrapper
> -member data: Variant1 optional=False
> -object q_obj_Variant2-wrapper
> -member data: Variant2 optional=False
>  object q_obj_cmd-arg
>  member arg1: int optional=False
>  member arg2: str optional=True
>  member arg3: bool optional=False
> +command cmd q_obj_cmd-arg -> Object
> +   gen=True success_response=True boxed=False
> +command cmd-boxed 

Re: [Qemu-devel] [PATCH RFC 09/21] qapi: Don't absolutize include file name in error messages

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Error messages print absolute filenames of included files even gave a
> relative one on the command line:
>
>  PYTHONPATH=scripts python -B tests/qapi-schema/test-qapi.py 
> tests/qapi-schema/include-cycle.json
> In file included from tests/qapi-schema/include-cycle.json:1:
> In file included from 
> /work/armbru/qemu/tests/qapi-schema/include-cycle-b.json:1:
> /work/armbru/qemu/tests/qapi-schema/include-cycle-c.json:1: Inclusion 
> loop for include-cycle.json
>
> Improve this to
>
> In file included from tests/qapi-schema/include-cycle.json:1:
> In file included from tests/qapi-schema/include-cycle-b.json:1:
> tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for 
> include-cycle.json
>
> Signed-off-by: Markus Armbruster 

most of the necessary refactoring/split is done in previous patch,

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py| 12 ++--
>  tests/qapi-schema/include-no-file.err |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index be0fcc548a..6c6962a364 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -255,9 +255,8 @@ class QAPIDoc(object):
>  class QAPISchemaParser(object):
>
>  def __init__(self, fp, previously_included=[], incl_info=None):
> -abs_fname = os.path.abspath(fp.name)
>  self.fname = fp.name
> -previously_included.append(abs_fname)
> +previously_included.append(os.path.abspath(fp.name))
>  self.incl_info = incl_info
>  self.src = fp.read()
>  if self.src == '' or self.src[-1] != '\n':
> @@ -288,7 +287,7 @@ class QAPISchemaParser(object):
>  if not isinstance(include, str):
>  raise QAPISemError(info,
> "Value of 'include' must be a string")
> -self._include(include, info, os.path.dirname(abs_fname),
> +self._include(include, info, os.path.dirname(self.fname),
>previously_included)
>  elif "pragma" in expr:
>  self.reject_expr_doc(cur_doc)
> @@ -321,7 +320,8 @@ class QAPISchemaParser(object):
>  % doc.symbol)
>
>  def _include(self, include, info, base_dir, previously_included):
> -incl_abs_fname = os.path.join(base_dir, include)
> +incl_fname = os.path.join(base_dir, include)
> +incl_abs_fname = os.path.abspath(incl_fname)
>  # catch inclusion cycle
>  inf = info
>  while inf:
> @@ -333,9 +333,9 @@ class QAPISchemaParser(object):
>  if incl_abs_fname in previously_included:
>  return
>  try:
> -fobj = open(incl_abs_fname, 'r')
> +fobj = open(incl_fname, 'r')
>  except IOError as e:
> -raise QAPISemError(info, '%s: %s' % (e.strerror, include))
> +raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
>  exprs_include = QAPISchemaParser(fobj, previously_included, info)
>  self.exprs.extend(exprs_include.exprs)
>  self.docs.extend(exprs_include.docs)
> diff --git a/tests/qapi-schema/include-no-file.err 
> b/tests/qapi-schema/include-no-file.err
> index d5b9b22d85..e42bcf4bc1 100644
> --- a/tests/qapi-schema/include-no-file.err
> +++ b/tests/qapi-schema/include-no-file.err
> @@ -1 +1 @@
> -tests/qapi-schema/include-no-file.json:1: No such file or directory: 
> include-no-file-sub.json
> +tests/qapi-schema/include-no-file.json:1: No such file or directory: 
> tests/qapi-schema/include-no-file-sub.json
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 05/21] qapi: Turn generators into modules

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> The next commit will introduce a common driver program for all
> generators.  The generators need to be modules for that.  qapi2texi.py
> already is.  Make the other generators follow suit.
>
> The changes are actually trivial.  Obvious in the diffs once you view
> them with whitespace changes ignored.
>
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-commands.py   | 43 ++--
>  scripts/qapi-event.py  | 43 ++--
>  scripts/qapi-introspect.py | 54 ++--
>  scripts/qapi-types.py  | 56 ++---
>  scripts/qapi-visit.py  | 62 
> +-
>  5 files changed, 143 insertions(+), 115 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index d229537659..331b58670e 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -255,16 +255,17 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>  self._regy += gen_register_command(name, success_response)
>
>
> -(input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
> +def main(argv):
> +(input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
>
> -blurb = '''
> +blurb = '''
>   * Schema-defined QAPI/QMP commands
>  '''
>
> -genc = QAPIGenC(blurb, __doc__)
> -genh = QAPIGenH(blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -genc.body(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/module.h"
> @@ -278,24 +279,28 @@ genc.body(mcgen('''
>  #include "%(prefix)sqmp-commands.h"
>
>  ''',
> -prefix=prefix))
> +prefix=prefix))
>
> -genh.body(mcgen('''
> +genh.body(mcgen('''
>  #include "%(prefix)sqapi-types.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/dispatch.h"
>
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> -
> -schema = QAPISchema(input_file)
> -vis = QAPISchemaGenCommandVisitor(prefix)
> -schema.visit(vis)
> -genc.body(vis.defn)
> -genh.body(vis.decl)
> -
> -if do_c:
> -genc.write(output_dir, prefix + 'qmp-marshal.c')
> -if do_h:
> -genh.write(output_dir, prefix + 'qmp-commands.h')
> +prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> +
> +schema = QAPISchema(input_file)
> +vis = QAPISchemaGenCommandVisitor(prefix)
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
> +
> +if do_c:
> +genc.write(output_dir, prefix + 'qmp-marshal.c')
> +if do_h:
> +genh.write(output_dir, prefix + 'qmp-commands.h')
> +
> +
> +if __name__ == '__main__':
> +main(sys.argv)
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 1af21b580a..5b33c694d4 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -171,16 +171,17 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>  self._event_names.append(name)
>
>
> -(input_file, output_dir, do_c, do_h, prefix, dummy) = parse_command_line()
> +def main(argv):
> +(input_file, output_dir, do_c, do_h, prefix, dummy) = 
> parse_command_line()
>
> -blurb = '''
> +blurb = '''
>   * Schema-defined QAPI/QMP events
>  '''
>
> -genc = QAPIGenC(blurb, __doc__)
> -genh = QAPIGenH(blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -genc.body(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "%(prefix)sqapi-event.h"
> @@ -190,23 +191,27 @@ genc.body(mcgen('''
>  #include "qapi/qmp-event.h"
>
>  ''',
> -prefix=prefix))
> +prefix=prefix))
>
> -genh.body(mcgen('''
> +genh.body(mcgen('''
>  #include "qapi/util.h"
>  #include "qapi/qmp/qdict.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> -prefix=prefix))
> -
> -schema = QAPISchema(input_file)
> -vis = QAPISchemaGenEventVisitor(prefix)
> -schema.visit(vis)
> -genc.body(vis.defn)
> -genh.body(vis.decl)
> -
> -if do_c:
> -genc.write(output_dir, prefix + 'qapi-event.c')
> -if do_h:
> -genh.write(output_dir, prefix + 'qapi-event.h')
> +prefix=prefix))
> +
> +schema = QAPISchema(input_file)
> +vis = QAPISchemaGenEventVisitor(prefix)
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
> +
> +if do_c:
> +genc.write(output_dir, prefix + 'qapi-event.c')
> +if do_h:
> +genh.write(output_dir, prefix + 'qapi-event.h')
> +
> +
> +if __name__ == '__main__':
> +main(sys.argv)
> diff --git a/scripts/qapi-introspect.py 

Re: [Qemu-devel] [PATCH RFC 12/21] qapi: Concentrate QAPISchemaParser.exprs updates in .__init__()

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Signed-off-by: Markus Armbruster 

It's not obvious the motivation behind this change (beside behind more
elegant), but
Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index d334e1db5a..7a327bfe9f 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -286,8 +286,12 @@ class QAPISchemaParser(object):
>  if not isinstance(include, str):
>  raise QAPISemError(info,
> "Value of 'include' must be a string")
> -self._include(include, info, os.path.dirname(self.fname),
> -  previously_included)
> +exprs_include = self._include(include, info,
> +  os.path.dirname(self.fname),
> +  previously_included)
> +if exprs_include:
> +self.exprs.extend(exprs_include.exprs)
> +self.docs.extend(exprs_include.docs)
>  elif "pragma" in expr:
>  self.reject_expr_doc(cur_doc)
>  if len(expr) != 1:
> @@ -330,14 +334,13 @@ class QAPISchemaParser(object):
>
>  # skip multiple include of the same file
>  if incl_abs_fname in previously_included:
> -return
> +return None
> +
>  try:
>  fobj = open(incl_fname, 'r')
>  except IOError as e:
>  raise QAPISemError(info, '%s: %s' % (e.strerror, incl_fname))
> -exprs_include = QAPISchemaParser(fobj, previously_included, info)
> -self.exprs.extend(exprs_include.exprs)
> -self.docs.extend(exprs_include.docs)
> +return QAPISchemaParser(fobj, previously_included, info)
>
>  def _pragma(self, name, value, info):
>  global doc_required, returns_whitelist, name_case_whitelist
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 11/21] qapi: Lift error reporting from QAPISchema.__init__() to callers

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi-gen.py|  8 ++--
>  scripts/qapi/common.py | 23 +--
>  tests/qapi-schema/test-qapi.py |  8 +++-
>  3 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py
> index 6302fd0d55..ba82ca92cc 100755
> --- a/scripts/qapi-gen.py
> +++ b/scripts/qapi-gen.py
> @@ -7,7 +7,7 @@
>  import getopt
>  import re
>  import sys
> -from qapi.common import QAPISchema
> +from qapi.common import QAPIError, QAPISchema
>  from qapi.types import gen_types
>  from qapi.visit import gen_visit
>  from qapi.commands import gen_commands
> @@ -77,7 +77,11 @@ def main(argv):
>  if o in ('-u', '--unmask-non-abi-names'):
>  opt_unmask = True
>
> -schema = QAPISchema(input_file)
> +try:
> +schema = QAPISchema(input_file)
> +except QAPIError as err:
> +print >>sys.stderr, err
> +exit(1)
>
>  gen_types(schema, output_dir, prefix, opt_builtins)
>  gen_visit(schema, output_dir, prefix, opt_builtins)
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 78e960d07c..d334e1db5a 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -15,7 +15,6 @@ import errno
>  import os
>  import re
>  import string
> -import sys
>  from ordereddict import OrderedDict
>
>  builtin_types = {
> @@ -1455,19 +1454,15 @@ class QAPISchemaEvent(QAPISchemaEntity):
>
>  class QAPISchema(object):
>  def __init__(self, fname):
> -try:
> -parser = QAPISchemaParser(open(fname, 'r'))
> -exprs = check_exprs(parser.exprs)
> -self.docs = parser.docs
> -self._entity_dict = {}
> -self._predefining = True
> -self._def_predefineds()
> -self._predefining = False
> -self._def_exprs(exprs)
> -self.check()
> -except QAPIError as err:
> -print >>sys.stderr, err
> -exit(1)
> +parser = QAPISchemaParser(open(fname, 'r'))
> +exprs = check_exprs(parser.exprs)
> +self.docs = parser.docs
> +self._entity_dict = {}
> +self._predefining = True
> +self._def_predefineds()
> +self._predefining = False
> +self._def_exprs(exprs)
> +self.check()
>
>  def _def_entity(self, ent):
>  # Only the predefined types are allowed to not have info
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 7772d09919..d6bb8ec6a4 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -53,7 +53,13 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>  for v in variants.variants:
>  print 'case %s: %s' % (v.name, v.type.name)
>
> -schema = QAPISchema(sys.argv[1])
> +
> +try:
> +schema = QAPISchema(sys.argv[1])
> +except QAPIError as err:
> +print >>sys.stderr, err
> +exit(1)
> +
>  schema.visit(QAPISchemaTestVisitor())
>
>  for doc in schema.docs:
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH RFC 10/21] qapi/common: Eliminate QAPISchema.exprs

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster  wrote:
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  scripts/qapi/common.py | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 6c6962a364..78e960d07c 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -1457,13 +1457,13 @@ class QAPISchema(object):
>  def __init__(self, fname):
>  try:
>  parser = QAPISchemaParser(open(fname, 'r'))
> -self.exprs = check_exprs(parser.exprs)
> +exprs = check_exprs(parser.exprs)
>  self.docs = parser.docs
>  self._entity_dict = {}
>  self._predefining = True
>  self._def_predefineds()
>  self._predefining = False
> -self._def_exprs()
> +self._def_exprs(exprs)
>  self.check()
>  except QAPIError as err:
>  print >>sys.stderr, err
> @@ -1648,8 +1648,8 @@ class QAPISchema(object):
>  name, info, doc, 'arg', self._make_members(data, info))
>  self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed))
>
> -def _def_exprs(self):
> -for expr_elem in self.exprs:
> +def _def_exprs(self, exprs):
> +for expr_elem in exprs:
>  expr = expr_elem['expr']
>  info = expr_elem['info']
>  doc = expr_elem.get('doc')
> --
> 2.13.6
>

Re: [Qemu-devel] [PATCH 3/4] MAINTAINERS: add pointer to tpm-next repository

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 3:44 PM, Stefan Berger
 wrote:
> Signed-off-by: Stefan Berger 

Reviewed-by: Marc-André Lureau 


> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f8deaf6..d352d16 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1593,6 +1593,7 @@ F: include/hw/acpi/tpm.h
>  F: include/sysemu/tpm*
>  F: qapi/tpm.json
>  F: backends/tpm.c
> +T: git git://github.com/stefanberger/qemu-tpm.git tpm-next
>
>  Checkpatch
>  S: Odd Fixes
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH 1/4] tpm: Split off tpm_crb_reset function

[Marc-Andre Lureau]

On Fri, Feb 2, 2018 at 3:44 PM, Stefan Berger
 wrote:
> Split off the tpm_crb_reset function part from tpm_crb_realize
> that we need to run every time the machine resets.
>
> Also register our reset function with the system since TYPE_DEVICE
> seems to not get a reset otherwise.
>
> Signed-off-by: Stefan Berger 

Reviewed-by: Marc-André Lureau 


>
> ---
>  v1->v2: register reset function with qemu_register_reset since
>  TYPE_DEVICE seems to not get a reset otherwise
> ---
>  hw/tpm/tpm_crb.c | 48 
>  1 file changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> index 687d255..b5b8256 100644
> --- a/hw/tpm/tpm_crb.c
> +++ b/hw/tpm/tpm_crb.c
> @@ -26,6 +26,7 @@
>  #include "hw/acpi/tpm.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/tpm_backend.h"
> +#include "sysemu/reset.h"
>  #include "tpm_int.h"
>  #include "tpm_util.h"
>
> @@ -210,29 +211,10 @@ static Property tpm_crb_properties[] = {
>  DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static void tpm_crb_realize(DeviceState *dev, Error **errp)
> +static void tpm_crb_reset(void *dev)
>  {
>  CRBState *s = CRB(dev);
>
> -if (!tpm_find()) {
> -error_setg(errp, "at most one TPM device is permitted");
> -return;
> -}
> -if (!s->tpmbe) {
> -error_setg(errp, "'tpmdev' property is required");
> -return;
> -}
> -
> -memory_region_init_io(>mmio, OBJECT(s), _crb_memory_ops, s,
> -"tpm-crb-mmio", sizeof(s->regs));
> -memory_region_init_ram(>cmdmem, OBJECT(s),
> -"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> -
> -memory_region_add_subregion(get_system_memory(),
> -TPM_CRB_ADDR_BASE, >mmio);
> -memory_region_add_subregion(get_system_memory(),
> -TPM_CRB_ADDR_BASE + sizeof(s->regs), >cmdmem);
> -
>  tpm_backend_reset(s->tpmbe);
>
>  ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> @@ -267,6 +249,32 @@ static void tpm_crb_realize(DeviceState *dev, Error 
> **errp)
>  tpm_backend_startup_tpm(s->tpmbe, s->be_buffer_size);
>  }
>
> +static void tpm_crb_realize(DeviceState *dev, Error **errp)
> +{
> +CRBState *s = CRB(dev);
> +
> +if (!tpm_find()) {
> +error_setg(errp, "at most one TPM device is permitted");
> +return;
> +}
> +if (!s->tpmbe) {
> +error_setg(errp, "'tpmdev' property is required");
> +return;
> +}
> +
> +memory_region_init_io(>mmio, OBJECT(s), _crb_memory_ops, s,
> +"tpm-crb-mmio", sizeof(s->regs));
> +memory_region_init_ram(>cmdmem, OBJECT(s),
> +"tpm-crb-cmd", CRB_CTRL_CMD_SIZE, errp);
> +
> +memory_region_add_subregion(get_system_memory(),
> +TPM_CRB_ADDR_BASE, >mmio);
> +memory_region_add_subregion(get_system_memory(),
> +TPM_CRB_ADDR_BASE + sizeof(s->regs), >cmdmem);
> +
> +qemu_register_reset(tpm_crb_reset, dev);
> +}
> +
>  static void tpm_crb_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH 2/2] tpm: wrap stX_be_p in tpm_cmd_set_XYZ functions

[Marc-Andre Lureau]

Hi

On Fri, Feb 2, 2018 at 12:23 AM, Stefan Berger
 wrote:
> Wrap the calls to stl_be_p and stw_be_p in tpm_cmd_set_XYZ functions
> that are similar to existing getters.

why not,

Reviewed-by: Marc-André Lureau 


>
> Signed-off-by: Stefan Berger 
> ---
>  hw/tpm/tpm_util.c |  6 +++---
>  hw/tpm/tpm_util.h | 15 +++
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/tpm/tpm_util.c b/hw/tpm/tpm_util.c
> index 8abde59..2de52a0 100644
> --- a/hw/tpm/tpm_util.c
> +++ b/hw/tpm/tpm_util.c
> @@ -106,9 +106,9 @@ const PropertyInfo qdev_prop_tpm = {
>  void tpm_util_write_fatal_error_response(uint8_t *out, uint32_t out_len)
>  {
>  if (out_len >= sizeof(struct tpm_resp_hdr)) {
> -stw_be_p(out, TPM_TAG_RSP_COMMAND);
> -stl_be_p(out + 2, sizeof(struct tpm_resp_hdr));
> -stl_be_p(out + 6, TPM_FAIL);
> +tpm_cmd_set_tag(out, TPM_TAG_RSP_COMMAND);
> +tpm_cmd_set_size(out, sizeof(struct tpm_resp_hdr));
> +tpm_cmd_set_error(out, TPM_FAIL);
>  }
>  }
>
> diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
> index f003d15..f397ac2 100644
> --- a/hw/tpm/tpm_util.h
> +++ b/hw/tpm/tpm_util.h
> @@ -36,11 +36,21 @@ static inline uint16_t tpm_cmd_get_tag(const void *b)
>  return lduw_be_p(b);
>  }
>
> +static inline void tpm_cmd_set_tag(void *b, uint16_t tag)
> +{
> +stw_be_p(b, tag);
> +}
> +
>  static inline uint32_t tpm_cmd_get_size(const void *b)
>  {
>  return ldl_be_p(b + 2);
>  }
>
> +static inline void tpm_cmd_set_size(void *b, uint32_t size)
> +{
> +stl_be_p(b + 2, size);
> +}
> +
>  static inline uint32_t tpm_cmd_get_ordinal(const void *b)
>  {
>  return ldl_be_p(b + 6);
> @@ -51,6 +61,11 @@ static inline uint32_t tpm_cmd_get_errcode(const void *b)
>  return ldl_be_p(b + 6);
>  }
>
> +static inline void tpm_cmd_set_error(void *b, uint32_t error)
> +{
> +stl_be_p(b + 6, error);
> +}
> +
>  int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>   size_t *buffersize);
>
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH] tpm: fix alignment issues

[Marc-Andre Lureau]

Hi

On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger
 wrote:
> On 01/29/2018 07:32 AM, Marc-André Lureau wrote:
>>
>> The new tpm-crb-test fails on sparc host:
>>
>> TEST: tests/tpm-crb-test... (pid=230409)
>>/i386/tpm-crb/test:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02S29cea50247fe1efa59ee885a26d51a85
>> (pid=230423)
>> FAIL: tests/tpm-crb-test
>>
>> and generates a new clang sanitizer runtime warning:
>>
>> /home/petmay01/linaro/qemu-for-merges/hw/tpm/tpm_util.h:36:24: runtime
>> error: load of misaligned address 0x7fdc24c2 for type 'const
>> uint32_t' (aka 'const unsigned int'), which requires 4 byte alignment
>> 0x7fdc24c2: note: pointer points here
>> 
>>
>> The sparc architecture does not allow misaligned loads and will
>> segfault if you try them.  For example, this function:
>>
>> static inline uint32_t tpm_cmd_get_size(const void *b)
>> {
>>  return be32_to_cpu(*(const uint32_t *)(b + 2));
>> }
>>
>> Should read,
>>  return ldl_be_p(b + 2);
>>
>> As a general rule you can't take an arbitrary pointer into a byte
>> buffer and try to interpret it as a structure or a pointer to a
>> larger-than-bytesize-data simply by casting the pointer.
>>
>> Use this clean up as an opportunity to remove unnecessary temporary
>> buffers and casts.
>>
>> Reported-by: Peter Maydell 
>> Signed-off-by: Marc-André Lureau 
>> ---
>>   hw/tpm/tpm_util.h| 17 ++-
>>   hw/tpm/tpm_emulator.c| 14 -
>>   hw/tpm/tpm_passthrough.c |  6 ++--
>>   hw/tpm/tpm_util.c| 75
>> ++--
>>   4 files changed, 58 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
>> index 19b28474ae..c562140e52 100644
>> --- a/hw/tpm/tpm_util.h
>> +++ b/hw/tpm/tpm_util.h
>> @@ -31,9 +31,24 @@ bool tpm_util_is_selftest(const uint8_t *in, uint32_t
>> in_len);
>>
>>   int tpm_util_test_tpmdev(int tpm_fd, TPMVersion *tpm_version);
>>
>> +static inline uint16_t tpm_cmd_get_tag(const void *b)
>> +{
>> +return lduw_be_p(b);;
>> +}
>> +
>>   static inline uint32_t tpm_cmd_get_size(const void *b)
>>   {
>> -return be32_to_cpu(*(const uint32_t *)(b + 2));
>> +return ldl_be_p(b + 2);;
>
>
> Why are there these two ';;' ?
>

obvious typo (repeated..)

>
>> +}
>> +
>> +static inline uint32_t tpm_cmd_get_ordinal(const void *b)
>> +{
>> +return ldl_be_p(b + 6);;
>> +}
>> +
>> +static inline uint32_t tpm_cmd_get_errcode(const void *b)
>> +{
>> +return ldl_be_p(b + 6);;
>>   }
>>
>>   int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>> index 35c78de5a9..a34a18ac7a 100644
>> --- a/hw/tpm/tpm_emulator.c
>> +++ b/hw/tpm/tpm_emulator.c
>> @@ -120,7 +120,6 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>> *tpm_emu,
>>   {
>>   ssize_t ret;
>>   bool is_selftest = false;
>> -const struct tpm_resp_hdr *hdr = NULL;
>>
>>   if (selftest_done) {
>>   *selftest_done = false;
>> @@ -132,22 +131,21 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>> *tpm_emu,
>>   return -1;
>>   }
>>
>> -ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> sizeof(*hdr),
>> -   err);
>> +ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> +  sizeof(struct tpm_resp_hdr), err);
>>   if (ret != 0) {
>>   return -1;
>>   }
>>
>> -hdr = (struct tpm_resp_hdr *)out;
>> -out += sizeof(*hdr);
>> -ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>> -   be32_to_cpu(hdr->len) - sizeof(*hdr) ,
>> err);
>> +ret = qio_channel_read_all(tpm_emu->data_ioc,
>> +  (char *)out + sizeof(struct tpm_resp_hdr),
>> +  tpm_cmd_get_size(out) - sizeof(struct tpm_resp_hdr), err);
>>   if (ret != 0) {
>>   return -1;
>>   }
>>
>>   if (is_selftest) {
>> -*selftest_done = (be32_to_cpu(hdr->errcode) == 0);
>> +*selftest_done = tpm_cmd_get_errcode(out) == 0;
>>   }
>>
>>   return 0;
>> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
>> index 29142f38bb..537e11a3f9 100644
>> --- a/hw/tpm/tpm_passthrough.c
>> +++ b/hw/tpm/tpm_passthrough.c
>> @@ -87,7 +87,6 @@ static int tpm_passthrough_unix_tx_bufs(TPMPassthruState
>> *tpm_pt,
>>   {
>>   ssize_t ret;
>>   bool is_selftest;
>> -const struct tpm_resp_hdr *hdr;
>>
>>   /* FIXME: protect shared variables or use other sync mechanism */
>>   tpm_pt->tpm_op_canceled = false;
>> @@ -116,15 +115,14 @@ static int
>> tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
>>strerror(errno), errno);
>>   }
>>   } else if (ret < sizeof(struct tpm_resp_hdr) ||
>> -   be32_to_cpu(((struct tpm_resp_hdr *)out)->len) != ret) {
>> +   

Re: [Qemu-devel] [PATCH] tpm: fix alignment issues

[Marc-Andre Lureau]

Hi

On Mon, Jan 29, 2018 at 6:57 PM, Stefan Berger
<stef...@linux.vnet.ibm.com> wrote:
> On 01/29/2018 12:51 PM, Marc-Andre Lureau wrote:
>>
>> Hi
>>
>> On Mon, Jan 29, 2018 at 6:41 PM, Stefan Berger
>> <stef...@linux.vnet.ibm.com> wrote:
>>>
>>> On 01/29/2018 07:32 AM, Marc-André Lureau wrote:
>>>>
>>>> The new tpm-crb-test fails on sparc host:
>>>>
>>>> TEST: tests/tpm-crb-test... (pid=230409)
>>>> /i386/tpm-crb/test:
>>>> Broken pipe
>>>> FAIL
>>>> GTester: last random seed: R02S29cea50247fe1efa59ee885a26d51a85
>>>> (pid=230423)
>>>> FAIL: tests/tpm-crb-test
>>>>
>>>> and generates a new clang sanitizer runtime warning:
>>>>
>>>> /home/petmay01/linaro/qemu-for-merges/hw/tpm/tpm_util.h:36:24: runtime
>>>> error: load of misaligned address 0x7fdc24c2 for type 'const
>>>> uint32_t' (aka 'const unsigned int'), which requires 4 byte alignment
>>>> 0x7fdc24c2: note: pointer points here
>>>> 
>>>>
>>>> The sparc architecture does not allow misaligned loads and will
>>>> segfault if you try them.  For example, this function:
>>>>
>>>> static inline uint32_t tpm_cmd_get_size(const void *b)
>>>> {
>>>>   return be32_to_cpu(*(const uint32_t *)(b + 2));
>>>> }
>>>>
>>>> Should read,
>>>>   return ldl_be_p(b + 2);
>>>>
>>>> As a general rule you can't take an arbitrary pointer into a byte
>>>> buffer and try to interpret it as a structure or a pointer to a
>>>> larger-than-bytesize-data simply by casting the pointer.
>>>>
>>>> Use this clean up as an opportunity to remove unnecessary temporary
>>>> buffers and casts.
>>>>
>>>> Reported-by: Peter Maydell <peter.mayd...@linaro.org>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>>>> ---
>>>>hw/tpm/tpm_util.h| 17 ++-
>>>>hw/tpm/tpm_emulator.c| 14 -
>>>>hw/tpm/tpm_passthrough.c |  6 ++--
>>>>hw/tpm/tpm_util.c| 75
>>>> ++--
>>>>4 files changed, 58 insertions(+), 54 deletions(-)
>>>>
>>>> diff --git a/hw/tpm/tpm_util.h b/hw/tpm/tpm_util.h
>>>> index 19b28474ae..c562140e52 100644
>>>> --- a/hw/tpm/tpm_util.h
>>>> +++ b/hw/tpm/tpm_util.h
>>>> @@ -31,9 +31,24 @@ bool tpm_util_is_selftest(const uint8_t *in, uint32_t
>>>> in_len);
>>>>
>>>>int tpm_util_test_tpmdev(int tpm_fd, TPMVersion *tpm_version);
>>>>
>>>> +static inline uint16_t tpm_cmd_get_tag(const void *b)
>>>> +{
>>>> +return lduw_be_p(b);;
>>>> +}
>>>> +
>>>>static inline uint32_t tpm_cmd_get_size(const void *b)
>>>>{
>>>> -return be32_to_cpu(*(const uint32_t *)(b + 2));
>>>> +return ldl_be_p(b + 2);;
>>>
>>>
>>> Why are there these two ';;' ?
>>>
>> obvious typo (repeated..)
>>
>>>> +}
>>>> +
>>>> +static inline uint32_t tpm_cmd_get_ordinal(const void *b)
>>>> +{
>>>> +return ldl_be_p(b + 6);;
>>>> +}
>>>> +
>>>> +static inline uint32_t tpm_cmd_get_errcode(const void *b)
>>>> +{
>>>> +return ldl_be_p(b + 6);;
>>>>}
>>>>
>>>>int tpm_util_get_buffer_size(int tpm_fd, TPMVersion tpm_version,
>>>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>>>> index 35c78de5a9..a34a18ac7a 100644
>>>> --- a/hw/tpm/tpm_emulator.c
>>>> +++ b/hw/tpm/tpm_emulator.c
>>>> @@ -120,7 +120,6 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>>>> *tpm_emu,
>>>>{
>>>>ssize_t ret;
>>>>bool is_selftest = false;
>>>> -const struct tpm_resp_hdr *hdr = NULL;
>>>>
>>>>if (selftest_done) {
>>>>*selftest_done = false;
>>>> @@ -132,22 +131,21 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator
>>>> *tpm_emu,
>>>>return -1;
>>>>}
>>>>
>>>> -ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
>>>> sizeo

Re: [Qemu-devel] [PATCH v3] tpm: add CRB device

[Marc-Andre Lureau]

Hi,

Stash this to fix test leaks:

diff --git a/tests/tpm-crb-test.c b/tests/tpm-crb-test.c
index de78a28844..8bf1507e00 100644
--- a/tests/tpm-crb-test.c
+++ b/tests/tpm-crb-test.c
@@ -81,6 +81,7 @@ static void *emu_tpm_thread(void *data)

 g_free(s->tpm_msg);
 s->tpm_msg = NULL;
+object_unref(OBJECT(s->tpm_ioc));
 return NULL;
 }

@@ -180,6 +181,8 @@ static void *emu_ctrl_thread(void *data)
 }
 }

+object_unref(OBJECT(ioc));
+object_unref(OBJECT(lioc));
 return NULL;
 }

Re: [Qemu-devel] [PATCH] char-pty: avoid assertion warning

[Marc-Andre Lureau]

Hi

On Fri, Jan 26, 2018 at 12:40 AM, Peng Hao  wrote:
> g_source_unref(s->open_source) in pty_chr_timer may trigger a assertion like 
> this:
> g_source_unref: assertion 'source != NULL' failed.
> pty_chr_update_read_handler_locked-->pty_chr_state(chr, 0) may be called
> in pty_chr_timer, pty_chr_state(chr, 0) will call 
> g_source_unref(s->open_source)
> and set s->open_source=NULL.

Peter already sent "[PATCH] chardev: fix incorrect unref of source",
queued by Paolo,
thanks

>
> Signed-off-by: Peng Hao 
> ---
>  chardev/char-pty.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 89315e6..da0f286 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -58,8 +58,10 @@ static gboolean pty_chr_timer(gpointer opaque)
>
>  qemu_mutex_lock(>chr_write_lock);
>  s->timer_src = NULL;
> -g_source_unref(s->open_source);
> -s->open_source = NULL;
> +if (s->open_source) {
> +g_source_unref(s->open_source);
> +s->open_source = NULL;
> +}
>  if (!s->connected) {
>  /* Next poll ... */
>  pty_chr_update_read_handler_locked(chr);
> --
> 1.8.3.1
>

Re: [Qemu-devel] [PATCH v10 2/4] fw_cfg: do DMA read operation

[Marc-Andre Lureau]

Hi

On Wed, Jan 24, 2018 at 4:25 AM, Peter Xu  wrote:
> On Tue, Jan 23, 2018 at 05:40:39PM +0100, Marc-André Lureau wrote:
>> Modify fw_cfg_read_blob() to use DMA if the device supports it.
>> Return errors, because the operation may fail.
>>
>> The DMA operation is expected to run synchronously with today qemu,
>> but the specification states that it may become async, so we run
>> "control" field check in a loop for eventual changes.
>>
>> We may want to switch all the *buf addresses to use only kmalloc'ed
>> buffers (instead of using stack/image addresses with dma=false).
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  drivers/firmware/qemu_fw_cfg.c | 131 
>> ++---
>>  1 file changed, 111 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
>> index 740df0df2260..686f0e839858 100644
>> --- a/drivers/firmware/qemu_fw_cfg.c
>> +++ b/drivers/firmware/qemu_fw_cfg.c
>> @@ -33,6 +33,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  MODULE_AUTHOR("Gabriel L. Somlo ");
>>  MODULE_DESCRIPTION("QEMU fw_cfg sysfs support");
>> @@ -43,12 +44,22 @@ MODULE_LICENSE("GPL");
>>  #define FW_CFG_ID 0x01
>>  #define FW_CFG_FILE_DIR   0x19
>>
>> +#define FW_CFG_VERSION_DMA 0x02
>> +#define FW_CFG_DMA_CTL_ERROR   0x01
>> +#define FW_CFG_DMA_CTL_READ0x02
>> +#define FW_CFG_DMA_CTL_SKIP0x04
>> +#define FW_CFG_DMA_CTL_SELECT  0x08
>> +#define FW_CFG_DMA_CTL_WRITE   0x10
>> +
>>  /* size in bytes of fw_cfg signature */
>>  #define FW_CFG_SIG_SIZE 4
>>
>>  /* fw_cfg "file name" is up to 56 characters (including terminating nul) */
>>  #define FW_CFG_MAX_FILE_PATH 56
>>
>> +/* fw_cfg revision attribute, in /sys/firmware/qemu_fw_cfg top-level dir. */
>> +static u32 fw_cfg_rev;
>> +
>>  /* fw_cfg file directory entry type */
>>  struct fw_cfg_file {
>>   u32 size;
>> @@ -57,6 +68,12 @@ struct fw_cfg_file {
>>   char name[FW_CFG_MAX_FILE_PATH];
>>  };
>>
>> +struct fw_cfg_dma {
>> + u32 control;
>> + u32 length;
>> + u64 address;
>> +} __packed;
>> +
>>  /* fw_cfg device i/o register addresses */
>>  static bool fw_cfg_is_mmio;
>>  static phys_addr_t fw_cfg_p_base;
>> @@ -75,12 +92,68 @@ static inline u16 fw_cfg_sel_endianness(u16 key)
>>   return fw_cfg_is_mmio ? cpu_to_be16(key) : cpu_to_le16(key);
>>  }
>>
>> +static inline bool fw_cfg_dma_enabled(void)
>> +{
>> + return fw_cfg_rev & FW_CFG_VERSION_DMA && fw_cfg_reg_dma;
>> +}
>> +
>> +/* qemu fw_cfg device is sync today, but spec says it may become async */
>> +static void fw_cfg_wait_for_control(struct fw_cfg_dma *d)
>> +{
>> + do {
>> + u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
>> +
>> + if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
>> + return;
>> +
>> + usleep_range(50, 100);
>> + } while (true);
>> +}
>> +
>> +static ssize_t fw_cfg_dma_transfer(struct device *dev,
>> + void *address, u32 length, u32 control)
>> +{
>> + phys_addr_t dma;
>> + struct fw_cfg_dma *d = NULL;
>> + ssize_t ret = length;
>> +
>> + d = kmalloc(sizeof(*d), GFP_KERNEL);
>> + if (!d) {
>> + ret = -ENOMEM;
>> + goto end;
>> + }
>> +
>> + *d = (struct fw_cfg_dma) {
>> + .address = cpu_to_be64(virt_to_phys(address)),
>> + .length = cpu_to_be32(length),
>> + .control = cpu_to_be32(control)
>> + };
>> +
>> + dma = virt_to_phys(d);
>> +
>> + iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
>> + iowrite32be(dma, fw_cfg_reg_dma + 4);
>
> We can do it with iowrite64be(virt_to_phys(d)) too?  In all cases I
> think it's good enough and no worth for a repost.

That would not build on 32 bit, but we could have a #ifdef
CONFIG_64BIT (untested).

>
> For the DMA transfer part:
>
> Acked-by: Peter Xu 

thanks

Re: [Qemu-devel] [PATCH v1 2/2] make: fix help message reference to bogus V=0 variable

[Marc-Andre Lureau]

On Tue, Jan 23, 2018 at 5:47 PM, Daniel P. Berrange  wrote:
> The make rules for building QEMU are mostly silent by default. They can
> be made verbose by setting the variable V=1. The default state does not
> however correspond to a V=0 setting - $(V) must be undefined / empty to
> get the default quiet build.
>
> Signed-off-by: Daniel P. Berrange 


(with that, qemu build-sys differs from automake & kernel at least)

Reviewed-by: Marc-André Lureau 


> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index c263190b8d..554ba69ced 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -940,4 +940,5 @@ ifdef QEMU_GA_MSI_ENABLED
>  endif
> @echo  ''
>  endif
> -   @echo  '  $(MAKE) V=0|1 [targets] 0 => quiet build (default), 1 => 
> verbose build'
> +   @echo  '  $(MAKE) [targets]  (quiet build, default)'
> +   @echo  '  $(MAKE) V=1 [targets]  (verbose build)'
> --
> 2.14.3
>

Re: [Qemu-devel] [PATCH v1 1/2] Revert "build-sys: silence make by default or V=0"

[Marc-Andre Lureau]

On Tue, Jan 23, 2018 at 5:47 PM, Daniel P. Berrange  wrote:
> This reverts commit 42a77f1ce4934b243df003f95bda88530631387a.
>
> The primary intention of this change was to silence messages
> like
>
>   make[1]: '/home/berrange/src/virt/qemu/capstone/libcapstone.a' is up to 
> date.
>
> which we get when calling make recursively with explicit
> targets.
>
> The problem is that this change affected every make target,
> not merely the targets that triggered these "is up to date"
> messages. As a result any targets that were not invoking
> commands via "$(call quiet-command ...)" suddenly become
> silent. This is particularly bad for "make install" which
> now appears todo nothing.
>
> Rather than go through every make rule and try to identify
> places where we now need to explicitly print a message to
> show work taking place, just revert the change.
>
> To address the original problem of silencing "is up to date"
> messages, we simply add --quiet to the SUBDIR_MAKEVARS
> variable, so it only affects us on recursive make calls.
>
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Marc-André Lureau 


> ---
>  Makefile  | 2 +-
>  rules.mak | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f26ef1b1df..c263190b8d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -280,7 +280,7 @@ else
>  DOCS=
>  endif
>
> -SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
> +SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory --quiet) 
> BUILD_DIR=$(BUILD_DIR)
>  SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
>  SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d, $(TARGET_DIRS))
>
> diff --git a/rules.mak b/rules.mak
> index 5fb4951561..6e943335f3 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -131,8 +131,6 @@ modules:
>  # If called with only a single argument, will print nothing in quiet mode.
>  quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, 
> @$1))
>
> -MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
> -
>  # cc-option
>  # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
>
> --
> 2.14.3
>

Re: [Qemu-devel] [PULL 10/51] build-sys: silence make by default or V=0

[Marc-Andre Lureau]

Hi

On Tue, Jan 23, 2018 at 4:38 PM, Daniel P. Berrange  wrote:
> On Tue, Jan 16, 2018 at 03:16:52PM +0100, Paolo Bonzini wrote:
>> From: Marc-André Lureau 
>>
>> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu 
>> specific).
>>
>> Use --quiet to silence make 'is up to date' message.
>>
>> Signed-off-by: Marc-André Lureau 
>> Tested-by: Eric Blake 
>> Reviewed-by: Paolo Bonzini 
>> Message-Id: <20180104160523.22995-3-marcandre.lur...@redhat.com>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  Makefile  | 2 +-
>>  rules.mak | 2 ++
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> After applying it when you run 'make install' absolutely nothing is
> displayed, but it none the less does work. This is very misleading
> to devs who thing nothing is being installed...

Right, you would need V=1 now

> Either this needs reverting, or we need to re-write the 'install' target
> so that it generates messages of whats being installed. Perhaps something
> like this
>

Make sense to me, could you send a former patch for review?

thanks

> diff --git a/Makefile b/Makefile
> index f26ef1b1df..8ef195a0df 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -697,28 +697,33 @@ ifneq ($(TOOLS),)
>  endif
>  ifneq ($(CONFIG_MODULES),)
> $(INSTALL_DIR) "$(DESTDIR)$(qemu_moddir)"
> +   $(call quiet-command,\
> for s in $(modules-m:.mo=$(DSOSUF)); do \
> t="$(DESTDIR)$(qemu_moddir)/$$(echo $$s | tr / -)"; \
> $(INSTALL_LIB) $$s "$$t"; \
> test -z "$(STRIP)" || $(STRIP) "$$t"; \
> -   done
> +   done, "INSTALL", "$(modules-m)")
>  endif
>  ifneq ($(HELPERS-y),)
> $(call install-prog,$(HELPERS-y),$(DESTDIR)$(libexecdir))
>  endif
>  ifneq ($(BLOBS),)
> +   $(call quiet-command,\
> set -e; for x in $(BLOBS); do \
> $(INSTALL_DATA) $(SRC_PATH)/pc-bios/$$x 
> "$(DESTDIR)$(qemu_datadir)"; \
> -   done
> +   done, "INSTALL", "$(BLOBS)")
>  endif
>  ifeq ($(CONFIG_GTK),y)
> $(MAKE) -C po $@
>  endif
> $(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)/keymaps"
> +   $(call quiet-command,\
> set -e; for x in $(KEYMAPS); do \
> $(INSTALL_DATA) $(SRC_PATH)/pc-bios/keymaps/$$x 
> "$(DESTDIR)$(qemu_datadir)/keymaps"; \
> -   done
> -   $(INSTALL_DATA) $(BUILD_DIR)/trace-events-all 
> "$(DESTDIR)$(qemu_datadir)/trace-events-all"
> +   done, "INSTALL", "$(KEYMAPS)")
> +   $(call quiet-command,\
> +   $(INSTALL_DATA) $(BUILD_DIR)/trace-events-all 
> "$(DESTDIR)$(qemu_datadir)/trace-events-all",\
> +   "INSTALL", "trace-events-all")
> for d in $(TARGET_DIRS); do \
> $(MAKE) $(SUBDIR_MAKEFLAGS) TARGET_DIR=$$d/ -C $$d $@ || exit 1 ; \
>  done
> diff --git a/rules.mak b/rules.mak
> index 5fb4951561..cd669833bf 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -147,7 +147,8 @@ set-vpath = $(if $1,$(foreach 
> PATTERN,$(VPATH_SUFFIXES),$(eval vpath $(PATTERN)
>  # install-prog list, dir
>  define install-prog
> $(INSTALL_DIR) "$2"
> -   $(INSTALL_PROG) $1 "$2"
> +   $(call quiet-command,\
> +   $(INSTALL_PROG) $1 "$2", "INSTALL", "$1")
> $(if $(STRIP),$(STRIP) $(foreach T,$1,"$2/$(notdir $T)"),)
>  endef
>
>
>
>
>>
>> diff --git a/Makefile b/Makefile
>> index d86ecd2..1671db3 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -277,7 +277,7 @@ else
>>  DOCS=
>>  endif
>>
>> -SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory) BUILD_DIR=$(BUILD_DIR)
>> +SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
>>  SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
>>  SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d, $(TARGET_DIRS))
>>
>> diff --git a/rules.mak b/rules.mak
>> index 6e94333..5fb4951 100644
>> --- a/rules.mak
>> +++ b/rules.mak
>> @@ -131,6 +131,8 @@ modules:
>>  # If called with only a single argument, will print nothing in quiet mode.
>>  quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, 
>> @$1))
>>
>> +MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
>> +
>>  # cc-option
>>  # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
>>
>
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-Andre Lureau]

Hi

On Sun, Jan 21, 2018 at 11:01 PM, Stefan Berger
<stef...@linux.vnet.ibm.com> wrote:
> On 01/21/2018 02:24 PM, Marc-Andre Lureau wrote:
>>
>> Hi
>>
>> On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
>> <stef...@linux.vnet.ibm.com> wrote:
>>>
>>> On 01/20/2018 07:54 AM, Philippe Mathieu-Daudé wrote:
>>>>
>>>> On 01/19/2018 11:11 AM, Marc-André Lureau wrote:
>>>>>
>>>>> tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
>>>>> Interface as defined in TCG PC Client Platform TPM Profile (PTP)
>>>>> Specification Family “2.0” Level 00 Revision 01.03 v22.
>>>>>
>>>>> The PTP allows device implementation to switch between TIS and CRB
>>>>> model at run time, but given that CRB is a simpler device to
>>>>> implement, I chose to implement it as a different device.
>>>>>
>>>>> The device doesn't implement other locality than 0 for now (my laptop
>>>>> TPM doesn't either, so I assume this isn't so bad)
>>>>>
>>>>> The command/reply memory region is statically allocated after the CRB
>>>>> registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
>>>>> wonder if the BIOS could or should allocate it instead, or what size
>>>>> to use, again this seems to fit well expectations)
>>>>>
>>>>> The PTP doesn't specify a particular bus to put the device. So I added
>>>>> it on the system bus directly, so it could hopefully be used easily on
>>>>> a different platform than x86. Currently, it fails to init on piix,
>>>>> because error_on_sysbus_device() check. The check may be changed in a
>>>>> near future, see discussion on the qemu-devel ML.
>>>>>
>>>>> Tested with some success with Linux upstream and Windows 10, seabios &
>>>>> modified ovmf. The device is recognized and correctly transmit
>>>>> command/response with passthrough & emu. However, we are missing PPI
>>>>> ACPI part atm.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>>>>> Signed-off-by: Stefan Berger <stef...@linux.vnet.ibm.com>
>>>>> ---
>>>>>qapi/tpm.json  |   5 +-
>>>>>include/hw/acpi/tpm.h  |  72 
>>>>>include/sysemu/tpm.h   |   3 +
>>>>>hw/i386/acpi-build.c   |  34 +++-
>>>>>hw/tpm/tpm_crb.c   | 327
>>>>> +
>>>>>default-configs/i386-softmmu.mak   |   1 +
>>>>>default-configs/x86_64-softmmu.mak |   1 +
>>>>>hw/tpm/Makefile.objs   |   1 +
>>>>>8 files changed, 434 insertions(+), 10 deletions(-)
>>>>>create mode 100644 hw/tpm/tpm_crb.c
>>>>>
>>>>> diff --git a/qapi/tpm.json b/qapi/tpm.json
>>>>> index 7093f268fb..d50deef5e9 100644
>>>>> --- a/qapi/tpm.json
>>>>> +++ b/qapi/tpm.json
>>>>> @@ -11,10 +11,11 @@
>>>>># An enumeration of TPM models
>>>>>#
>>>>># @tpm-tis: TPM TIS model
>>>>> +# @tpm-crb: TPM CRB model (since 2.12)
>>>>>#
>>>>># Since: 1.5
>>>>>##
>>>>> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
>>>>> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
>>>>>  ##
>>>>># @query-tpm-models:
>>>>> @@ -28,7 +29,7 @@
>>>>># Example:
>>>>>#
>>>>># -> { "execute": "query-tpm-models" }
>>>>> -# <- { "return": [ "tpm-tis" ] }
>>>>> +# <- { "return": [ "tpm-tis", "tpm-crb" ] }
>>>>>#
>>>>>##
>>>>>{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
>>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>>>> index 6d516c6a7f..b0048515fa 100644
>>>>> --- a/include/hw/acpi/tpm.h
>>>>> +++ b/include/hw/acpi/tpm.h
>>>>> @@ -16,11 +16,82 @@
>>>>>#ifndef HW_ACPI_TPM_H
>>>>>#define HW_ACPI_TPM_H
>>>>>+#include "qemu/osdep.h"
>>>>> +
&g

Re: [Qemu-devel] [PATCH v2 4/5] tpm: add CRB device

[Marc-Andre Lureau]

Hi

On Sun, Jan 21, 2018 at 6:46 AM, Stefan Berger
 wrote:
> On 01/20/2018 07:54 AM, Philippe Mathieu-Daudé wrote:
>>
>> On 01/19/2018 11:11 AM, Marc-André Lureau wrote:
>>>
>>> tpm_crb is a device for TPM 2.0 Command Response Buffer (CRB)
>>> Interface as defined in TCG PC Client Platform TPM Profile (PTP)
>>> Specification Family “2.0” Level 00 Revision 01.03 v22.
>>>
>>> The PTP allows device implementation to switch between TIS and CRB
>>> model at run time, but given that CRB is a simpler device to
>>> implement, I chose to implement it as a different device.
>>>
>>> The device doesn't implement other locality than 0 for now (my laptop
>>> TPM doesn't either, so I assume this isn't so bad)
>>>
>>> The command/reply memory region is statically allocated after the CRB
>>> registers address TPM_CRB_ADDR_BASE + sizeof(struct crb_regs) (I
>>> wonder if the BIOS could or should allocate it instead, or what size
>>> to use, again this seems to fit well expectations)
>>>
>>> The PTP doesn't specify a particular bus to put the device. So I added
>>> it on the system bus directly, so it could hopefully be used easily on
>>> a different platform than x86. Currently, it fails to init on piix,
>>> because error_on_sysbus_device() check. The check may be changed in a
>>> near future, see discussion on the qemu-devel ML.
>>>
>>> Tested with some success with Linux upstream and Windows 10, seabios &
>>> modified ovmf. The device is recognized and correctly transmit
>>> command/response with passthrough & emu. However, we are missing PPI
>>> ACPI part atm.
>>>
>>> Signed-off-by: Marc-André Lureau 
>>> Signed-off-by: Stefan Berger 
>>> ---
>>>   qapi/tpm.json  |   5 +-
>>>   include/hw/acpi/tpm.h  |  72 
>>>   include/sysemu/tpm.h   |   3 +
>>>   hw/i386/acpi-build.c   |  34 +++-
>>>   hw/tpm/tpm_crb.c   | 327
>>> +
>>>   default-configs/i386-softmmu.mak   |   1 +
>>>   default-configs/x86_64-softmmu.mak |   1 +
>>>   hw/tpm/Makefile.objs   |   1 +
>>>   8 files changed, 434 insertions(+), 10 deletions(-)
>>>   create mode 100644 hw/tpm/tpm_crb.c
>>>
>>> diff --git a/qapi/tpm.json b/qapi/tpm.json
>>> index 7093f268fb..d50deef5e9 100644
>>> --- a/qapi/tpm.json
>>> +++ b/qapi/tpm.json
>>> @@ -11,10 +11,11 @@
>>>   # An enumeration of TPM models
>>>   #
>>>   # @tpm-tis: TPM TIS model
>>> +# @tpm-crb: TPM CRB model (since 2.12)
>>>   #
>>>   # Since: 1.5
>>>   ##
>>> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
>>> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb' ] }
>>> ##
>>>   # @query-tpm-models:
>>> @@ -28,7 +29,7 @@
>>>   # Example:
>>>   #
>>>   # -> { "execute": "query-tpm-models" }
>>> -# <- { "return": [ "tpm-tis" ] }
>>> +# <- { "return": [ "tpm-tis", "tpm-crb" ] }
>>>   #
>>>   ##
>>>   { 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>> index 6d516c6a7f..b0048515fa 100644
>>> --- a/include/hw/acpi/tpm.h
>>> +++ b/include/hw/acpi/tpm.h
>>> @@ -16,11 +16,82 @@
>>>   #ifndef HW_ACPI_TPM_H
>>>   #define HW_ACPI_TPM_H
>>>   +#include "qemu/osdep.h"
>>> +
>>>   #define TPM_TIS_ADDR_BASE   0xFED4
>>>   #define TPM_TIS_ADDR_SIZE   0x5000
>>> #define TPM_TIS_IRQ 5
>>>   +struct crb_regs {
>>> +union {
>>> +uint32_t reg;
>>> +struct {
>>> +unsigned tpm_established:1;
>>> +unsigned loc_assigned:1;
>>> +unsigned active_locality:3;
>>> +unsigned reserved:2;
>>> +unsigned tpm_reg_valid_sts:1;
>>> +} bits;
>>
>> I suppose this is little-endian layout, so this won't work on big-endian
>> hosts.
>>
>> Can you add a qtest?
>>
>>> +} loc_state;
>>> +uint32_t reserved1;
>>> +uint32_t loc_ctrl;
>>> +union {
>>> +uint32_t reg;
>>> +struct {
>>> +unsigned granted:1;
>>> +unsigned been_seized:1;
>>> +} bits;
>>
>>
>> This is unclear where you expect those bits (right/left aligned).
>>
>> Can you add an unnamed field to be more explicit?
>>
>> i.e. without using struct if left alignment expected:
>>
>> unsigned granted:1, been_seized:1, :30;
>
>
>
> I got rid of all the bitfields and this patch here makes it work on a ppc64
> big endian and also x86_64 host:
>
> https://github.com/stefanberger/qemu-tpm/commit/28fc07f0d9314168986190effd6d72d9fd3972dd
>

Thank you Stefan! I am all for squashing this fix to the patch. You
should then add your signed-off to the commit.


> Regards,
> Stefan
>
>
>
>>
>>> +} loc_sts;
>>> +uint8_t reserved2[32];
>>> +union {
>>> +uint64_t reg;
>>> +struct {
>>> +unsigned type:4;
>>> +unsigned version:4;
>>> +unsigned cap_locality:1;
>>> +  

Re: [Qemu-devel] [PULL 0/1] Dump patches

[Marc-Andre Lureau]

On Fri, Jan 19, 2018 at 5:21 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 19 January 2018 at 16:08, Marc-Andre Lureau <mlur...@redhat.com> wrote:
>> Hi
>>
>> On Fri, Jan 19, 2018 at 4:29 PM, Peter Maydell <peter.mayd...@linaro.org> 
>> wrote:
>>> On 19 January 2018 at 15:24, Peter Maydell <peter.mayd...@linaro.org> wrote:
>>>> On 19 January 2018 at 14:35, Marc-Andre Lureau <mlur...@redhat.com> wrote:
>>>>> I have not found how to translate a python 'buffer' to a bytes string
>>>>> in 2.6.
>>>>
>>>> A local python expert suggests that "bytes(buffer)" should work.
>>>
>>> ...and that it ought to work if handed a memoryview too, so do we
>>> just want
>>>
>>>self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))
>>>
>>
>> Yes, I tested on 2.6 and 3.6, it works. I'll send a new patch.
>
> Testing 2.7 as well would probably not be a bad plan. Apparently
> this works because:
>  * on python 3, bytes(memoryview) and memoryview.to_bytes() are the same
>  * gdb gives us a memoryview only if python 3
>  * so we don't have to handle the case of getting a memoryview and
>being python 2 (in which case we would need to use memoryview.to_bytes()
>as bytes() would do the wrong thing)

I just tested with 2.7 as well, works fine too.

Updated patch sent.

Re: [Qemu-devel] [PULL 0/1] Dump patches

[Marc-Andre Lureau]

Hi

On Fri, Jan 19, 2018 at 4:29 PM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 19 January 2018 at 15:24, Peter Maydell <peter.mayd...@linaro.org> wrote:
>> On 19 January 2018 at 14:35, Marc-Andre Lureau <mlur...@redhat.com> wrote:
>>> I have not found how to translate a python 'buffer' to a bytes string
>>> in 2.6.
>>
>> A local python expert suggests that "bytes(buffer)" should work.
>
> ...and that it ought to work if handed a memoryview too, so do we
> just want
>
>self.elf.add_vmcoreinfo_note(bytes(vmcoreinfo))
>

Yes, I tested on 2.6 and 3.6, it works. I'll send a new patch.

Thanks!

Re: [Qemu-devel] [PATCH v2 0/5] tpm: CRB device and cleanups

[Marc-Andre Lureau]

On Fri, Jan 19, 2018 at 3:36 PM,   wrote:
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c: In function 'tpm_crb_mmio_read':
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:126:13: error: format '%lx' expects 
> argument of type 'long unsigned int', but argument 2 has type 'hwaddr {aka 
> long long unsigned int}' [-Werror=format=]
>  DPRINTF("CRB read 0x%lx:%s len:%u\n",
>  ^
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:48:20: note: in definition of macro 
> 'DPRINTF'
>  printf(fmt, ## __VA_ARGS__);\
> ^~~
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c: In function 'tpm_crb_mmio_write':
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:146:13: error: format '%lx' expects 
> argument of type 'long unsigned int', but argument 2 has type 'hwaddr {aka 
> long long unsigned int}' [-Werror=format=]
>  DPRINTF("CRB write 0x%lx:%s len:%u val:%lu\n",
>  ^
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:48:20: note: in definition of macro 
> 'DPRINTF'
>  printf(fmt, ## __VA_ARGS__);\
> ^~~
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:146:13: error: format '%lu' expects 
> argument of type 'long unsigned int', but argument 5 has type 'uint64_t {aka 
> long long unsigned int}' [-Werror=format=]
>  DPRINTF("CRB write 0x%lx:%s len:%u val:%lu\n",
>  ^
> /tmp/qemu-test/src/hw/tpm/tpm_crb.c:48:20: note: in definition of macro 
> 'DPRINTF'
>  printf(fmt, ## __VA_ARGS__);\
> ^~~

This will fix it:
Stefan feel free to squash it if no further changes required

--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -123,7 +123,7 @@ static uint64_t tpm_crb_mmio_read(void *opaque, hwaddr addr,
   unsigned size)
 {
 CRBState *s = CRB(opaque);
-DPRINTF("CRB read 0x%lx:%s len:%u\n",
+DPRINTF("CRB read 0x" TARGET_FMT_plx ":%s len:%u\n",
 addr, addr_desc(addr), size);
 void *regs = (void *)>regs + addr;

@@ -143,7 +143,7 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
uint64_t val, unsigned size)
 {
 CRBState *s = CRB(opaque);
-DPRINTF("CRB write 0x%lx:%s len:%u val:%lu\n",
+DPRINTF("CRB write 0x" TARGET_FMT_plx ":%s len:%u val:%" PRIu64 "\n",
 addr, addr_desc(addr), size, val);

 switch (addr) {

Re: [Qemu-devel] [PULL 0/1] Dump patches

[Marc-Andre Lureau]

Hi

On Fri, Jan 19, 2018 at 3:29 PM, Peter Maydell  wrote:
> On 17 January 2018 at 15:02, Eric Blake  wrote:
>> On 01/17/2018 08:47 AM, Marc-André Lureau wrote:
>>> The following changes since commit 8e5dc9ba49743b46d955ec7dacb04e42ae7ada7c:
>>>
>>>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180116' into 
>>> staging (2018-01-16 17:36:39 +)
>>>
>>> are available in the Git repository at:
>>>
>>>   https://github.com/elmarco/qemu.git tags/dump-pull-request
>>>
>>> for you to fetch changes up to 48fb74965a8d8f2916da30d9c5b9945df25142af:
>>>
>>>   dump-guest-memory.py: fix python 2 support (2018-01-17 15:47:14 +0100)
>>>
>>
>> The commit says it works with python 2.7, but we still require support
>> for python 2.6.  Is this pull request premature?
>
> So should I apply this, or not?

I have not found how to translate a python 'buffer' to a bytes string
in 2.6. For now, I think we should go with this patch, it's already an
improvement..

Thanks

Re: [Qemu-devel] [PATCH v3] chardev/char-socket: add POLLHUP handler

[Marc-Andre Lureau]

Hi

On Fri, Jan 19, 2018 at 11:47 AM, Klim Kireev  wrote:
> The following behavior was observed for QEMU configured by libvirt
> to use guest agent as usual for the guests without virtio-serial
> driver (Windows or the guest remaining in BIOS stage).
>
> In QEMU on first connect to listen character device socket
> the listen socket is removed from poll just after the accept().
> virtio_serial_guest_ready() returns 0 and the descriptor
> of the connected Unix socket is removed from poll and it will
> not be present in poll() until the guest will initialize the driver
> and change the state of the serial to "guest connected".
>
> In libvirt connect() to guest agent is performed on restart and
> is run under VM state lock. Connect() is blocking and can
> wait forever.
> In this case libvirt can not perform ANY operation on that VM.
>
> The bug can be easily reproduced this way:
>
> Terminal 1:
> qemu-system-x86_64 -m 512 -device pci-serial,chardev=serial1 -chardev 
> socket,id=serial1,path=/tmp/console.sock,server,nowait
> (virtio-serial and isa-serial also fit)
>
> Terminal 2:
> minicom -D unix\#/tmp/console.sock
> (type something and press enter)
> C-a x (to exit)
>
> Do 3 times:
> minicom -D unix\#/tmp/console.sock
> C-a x
>
> It needs 4 connections, because the first one is accepted by QEMU, then two 
> are queued by
> the kernel, and the 4th blocks.
>
> The problem is that QEMU doesn't add a read watcher after succesful read
> until the guest device wants to acquire recieved data, so
> I propose to install a separate pullhup watcher regardless of
> whether the device waits for data or not.
>
> Signed-off-by: Klim Kireev 
> ---
> Changelog:
> v2: Remove timer as a redundant feature
>
> v3: Remove read call and return G_SOURCE_REMOVE
>
>  chardev/char-socket.c | 25 -
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 77cdf487eb..83fa7b70f0 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -42,6 +42,7 @@ typedef struct {
>  QIOChannel *ioc; /* Client I/O channel */
>  QIOChannelSocket *sioc; /* Client master channel */
>  QIONetListener *listener;
> +guint hup_tag;
>  QCryptoTLSCreds *tls_creds;
>  int connected;
>  int max_size;
> @@ -352,6 +353,11 @@ static void tcp_chr_free_connection(Chardev *chr)
>  s->read_msgfds_num = 0;
>  }
>
> +if (s->hup_tag != 0) {
> +g_source_remove(s->hup_tag);
> +s->hup_tag = 0;
> +}
> +
>  tcp_set_msgfds(chr, NULL, 0);
>  remove_fd_in_watch(chr);
>  object_unref(OBJECT(s->sioc));
> @@ -455,6 +461,15 @@ static gboolean tcp_chr_read(QIOChannel *chan, 
> GIOCondition cond, void *opaque)
>  return TRUE;
>  }
>
> +static gboolean tcp_chr_hup(QIOChannel *channel,
> +   GIOCondition cond,
> +   void *opaque)
> +{
> +Chardev *chr = CHARDEV(opaque);
> +tcp_chr_disconnect(chr);
> +return G_SOURCE_REMOVE;

The source is removed, it should clear s->hup_tag to 0

> +}
> +
>  static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>  {
>  SocketChardev *s = SOCKET_CHARDEV(chr);
> @@ -528,6 +543,10 @@ static void tcp_chr_connect(void *opaque)
> tcp_chr_read,
> chr, chr->gcontext);
>  }
> +if (s->hup_tag == 0) {
> +s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
> +   tcp_chr_hup, chr, NULL);

Why not use chr->gcontext ?


> +}
>  qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>  }
>
> @@ -546,7 +565,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> tcp_chr_read, chr,
> chr->gcontext);
>  }
> -}
> +if (s->hup_tag == 0) {
> +s->hup_tag = qio_channel_add_watch(s->ioc, G_IO_HUP,
> +   tcp_chr_hup, chr, NULL);
> +}

Why not set the handler on tcp_chr_connect() ?

thanks

> + }
>
>  typedef struct {
>  Chardev *chr;
> --
> 2.14.3
>

Re: [Qemu-devel] [PATCH] fw_cfg: don't use DMA mapping for fw_cfg device

[Marc-Andre Lureau]

Hi

On Mon, Jan 15, 2018 at 9:55 AM, Peter Xu  wrote:
> fw_cfg device does not need IOMMU protection, so use physical addresses
> always.  That's how QEMU implements fw_cfg.  Otherwise we'll see call
> traces during boot when vIOMMU is enabled in guest:
>
> [1.018306] [ cut here ]
> [1.018314] WARNING: CPU: 1 PID: 1 at drivers/firmware/qemu_fw_cfg.c:152 
> fw_cfg_dma_transfer+0x399/0x500
> [1.018315] fw_cfg_dma_transfer: failed to map fw_cfg_dma
> [1.018316] Modules linked in:
> [1.018320] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 
> 3.10.0-827.el7.x86_64 #1
> [1.018321] Hardware name: Red Hat KVM, BIOS 1.11.0-1.el7 04/01/2014
> [1.018322] Call Trace:
> [1.018330]  [] dump_stack+0x19/0x1b
> [1.018334]  [] __warn+0xd8/0x100
> [1.018336]  [] warn_slowpath_fmt+0x5f/0x80
> [1.018338]  [] fw_cfg_dma_transfer+0x399/0x500
> [1.018340]  [] fw_cfg_read_blob+0xac/0x1c0
> [1.018342]  [] fw_cfg_register_dir_entries+0x80/0x450
> [1.018344]  [] fw_cfg_sysfs_probe+0x212/0x3f0
> [1.018347]  [] platform_drv_probe+0x42/0x110
> [1.018350]  [] driver_probe_device+0xc2/0x3e0
> [1.018352]  [] __driver_attach+0x93/0xa0
> [1.018354]  [] ? __device_attach+0x40/0x40
> [1.018359]  [] bus_for_each_dev+0x73/0xc0
> [1.018362]  [] driver_attach+0x1e/0x20
> [1.018364]  [] bus_add_driver+0x200/0x2d0
> [1.018366]  [] ? firmware_map_add_early+0x58/0x58
> [1.018368]  [] driver_register+0x64/0xf0
> [1.018370]  [] __platform_driver_register+0x4a/0x50
> [1.018372]  [] fw_cfg_sysfs_init+0x34/0x61
> [1.018376]  [] do_one_initcall+0xb8/0x230
> [1.018379]  [] kernel_init_freeable+0x17a/0x219
> [1.018381]  [] ? initcall_blacklist+0xb0/0xb0
> [1.018383]  [] ? rest_init+0x80/0x80
> [1.018385]  [] kernel_init+0xe/0xf0
> [1.018388]  [] ret_from_fork+0x58/0x90
> [1.018390]  [] ? rest_init+0x80/0x80
> [1.018392] ---[ end trace d00a5b71608a8f59 ]---
>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1533367
> Fixes: e90cb816599b ("fw_cfg: do DMA read operation", 2017-11-28)
> CC: Marc-André Lureau 
> CC: Michael S. Tsirkin 
> Signed-off-by: Peter Xu 
> --
>
> This is based on tree:
>   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/log/?h=vhost
>
> Please review, thanks.
>
> Signed-off-by: Peter Xu 

The DMA business is confusing, sadly I didn't get much clue what I was
supposed to do. What I can say:

Tested-by: Marc-André Lureau 

Should the series be removed from Michael tree and I squash your fix &
send a v10?

Fwiw, "fw_cfg: write vmcoreinfo details" should also be fixed to
allocate memory (unless your approach fixes that?)

thanks

> ---
>  drivers/firmware/qemu_fw_cfg.c | 37 -
>  1 file changed, 8 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 6eb5d8f43c3e..62a44bc81a88 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -106,11 +106,12 @@ static inline bool fw_cfg_dma_enabled(void)
>  }
>
>  /* qemu fw_cfg device is sync today, but spec says it may become async */
> -static void fw_cfg_wait_for_control(struct fw_cfg_dma *d, dma_addr_t dma)
> +static void fw_cfg_wait_for_control(struct fw_cfg_dma *d)
>  {
> do {
> -   dma_sync_single_for_cpu(dev, dma, sizeof(*d), 
> DMA_FROM_DEVICE);
> -   if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> +   u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
> +
> +   if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
> return;
>
> usleep_range(50, 100);
> @@ -119,21 +120,9 @@ static void fw_cfg_wait_for_control(struct fw_cfg_dma 
> *d, dma_addr_t dma)
>
>  static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>  {
> -   dma_addr_t dma_addr = 0;
> struct fw_cfg_dma *d = NULL;
> -   dma_addr_t dma;
> ssize_t ret = length;
> -   enum dma_data_direction dir =
> -   (control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> -   (control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> -
> -   if (address && length) {
> -   dma_addr = dma_map_single(dev, address, length, dir);
> -   if (dma_mapping_error(NULL, dma_addr)) {
> -   WARN(1, "%s: failed to map address\n", __func__);
> -   return -EFAULT;
> -   }
> -   }
> +   phys_addr_t dma;
>
> d = kmalloc(sizeof(*d), GFP_KERNEL);
> if (!d) {
> @@ -142,34 +131,24 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 
> length, u32 control)
> }
>
> *d = (struct fw_cfg_dma) {
> -   .address = cpu_to_be64(dma_addr),
> +