Signed-off-by: NEdgar E. Iglesias <edgar.iglesias@xilinx.com>

* remotes/qmp-unstable/queue/qmp: (38 commits)
  Revert "qapi: Clean up superfluous null check in qapi_dealloc_type_str()"
  qapi: Document optional arguments' backwards compatibility
  qmp: use valid JSON in transaction example
  qmp: Don't use error_is_set() to suppress additional errors
  dump: Drop pointless error_is_set(), DumpState member errp
  qemu-option: Clean up fragile use of error_is_set()
  qga: Drop superfluous error_is_set()
  qga: Clean up fragile use of error_is_set()
  qapi: Clean up fragile use of error_is_set()
  tests/qapi-schema: Drop superfluous error_is_set()
  qapi: Drop redundant, unclean error_is_set()
  hmp: Guard against misuse of hmp_handle_error()
  qga: Use return values instead of error_is_set(errp)
  error: Consistently name Error ** objects errp, and not err
  qmp: Consistently name Error ** objects errp, and not err
  qga: Consistently name Error ** objects errp, and not err
  qmp hmp: Consistently name Error * objects err, and not errp
  pci-assign: assigned_initfn(): set monitor error in common error handler
  pci-assign: propagate errors from assign_intx()
  pci-assign: propagate errors from assign_device()
  ...
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>

This reverts commit 25a70175.

Turns out the argument *can* be null: QEMU now segfaults if it
receives an invalid parameter via a qmp command instead of throwing an
error.

For example:
{ "execute": "blockdev-add",
     "arguments": { "options" : { "driver": "invalid-driver" } } }

CC: qemu-stable@nongnu.org
Signed-off-by: NPeter Lieven <pl@kamp.de>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NFam Zheng <famz@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Our example should use the correct quotes to match what someone
could actually pass over the wire.

* qmp-commands.hx: Use correct JSON quotes.
Signed-off-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Using error_is_set(errp) that way can sweep programming errors under
the carpet when we get called incorrectly with an error set.

encrypted_bdrv_it() does it, because there's no way to make
bdrv_iterate() break its loop.  Actually safe, because qmp_cont()
clears the error before the loop.  Clean it up anyway: replace
bdrv_iterate() by bdrv_next(), break the loop on error.

Replace both occurrences, for consistency.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

In qmp_dump_guest_memory(), the error must be clear on entry, and we
always bail out after setting it, directly or via dump_init().
Therefore, both error_is_set() are always false.  Drop them.

DumpState member errp is now write-only.  Drop it, too.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Using error_is_set(ERRP) to find out whether to bail out due to
previous error is either wrong, fragile, or unnecessarily opaque.
It's wrong when ERRP may be null, because errors go undetected when it
is.  It's fragile when proving ERRP non-null involves a non-local
argument.  Else, it's unnecessarily opaque (see commit 84d18f06).

The error_is_set(state->errp) in qemu_opts_from_qdict_1() is merely
fragile, because the callers never pass state argument with null
state->errp.

Make the code more robust and more obviously correct: test
*state->errp directly.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

acquire_privilege(), execute_async() and check_suspend_mode() do
nothing when called with an error set.  Callers shouldn't do that, and
no caller does.  Drop the superfluous tests.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Using error_is_set(ERRP) to find out whether a function failed is
either wrong, fragile, or unnecessarily opaque.  It's wrong when ERRP
may be null, because errors go undetected when it is.  It's fragile
when proving ERRP non-null involves a non-local argument.  Else, it's
unnecessarily opaque (see commit 84d18f06).

The error_is_set(errp) in the guest agent command handler functions
are merely fragile, because all chall chains (do_qmp_dispatch() via
the generated marshalling functions) pass a non-null errp argument.

Make the code more robust and more obviously correct: receive the
error in a local variable, then propagate it through the parameter.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Using error_is_set(ERRP) to find out whether a function failed is
either wrong, fragile, or unnecessarily opaque.  It's wrong when ERRP
may be null, because errors go undetected when it is.  It's fragile
when proving ERRP non-null involves a non-local argument.  Else, it's
unnecessarily opaque (see commit 84d18f06).

The error_is_set(errp) in do_qmp_dispatch() is merely fragile, because
the caller never passes a null errp argument.

Make the code more robust and more obviously correct: receive the
error in a local variable, then propagate it through the parameter.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

visit_type_TestStruct() does nothing when called with an error set.
Callers shouldn't do that, and no caller does.  Drop the superfluous
test.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

do_qmp_dispatch()'s test for qmp_dispatch_check_obj() failure examines
both the return value and the error object.  The latter part is
unclean; it works only when do_qmp_dispatch()'s caller passes a
non-null errp argument.  That's the case, but it's not locally
obvious.  Unclean.

Cleanup would be easy enough, but since the unclean code is also
redundant, let's just drop it.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Null errp argument makes no sense.  Assert it's not null, to make this
explicit, and guard against misuse.  All current callers pass non-null
errp.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Using error_is_set(errp) to check whether a function call failed is
fragile: it breaks when errp is null.  ga_get_fd_handle() and
guest_file_handle_add() don't return a useful value when they fail,
but that's just stupid.  Fix that, and check them instead.  As far
as I can tell, errp can't be null there, but this is more robust and
more obviously correct.
Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NMarkus Armbruster <armbru@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Among the callers, only assigned_initfn() should set the  monitor's stored
error. Other callers may run in contexts where the monitor's stored error
makes no sense. For example:

assigned_dev_pci_write_config()
  assigned_dev_update_msix()
    assign_intx()
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Also, change the return type to "void"; the function is static (with a
sole caller) and the negative errno values are not distinguished from each
other.
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

The return type is also changed from "int" to "void", because it was used
in a success vs. failure sense only (the caller didn't distinguish error
codes from each other, and even assigned_dev_register_msix_mmio() masked
mmap()'s errno values with a common -EFAULT).
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

assigned_initfn()
  get_real_device()
    read()
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Propagate any errors while adding PCI capabilities to
assigned_device_pci_cap_init(). We'll continue the propagation upwards
when assigned_device_pci_cap_init() becomes a leaf itself (when none of
its callees will report errors internally any longer when detecting and
returning them).
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

... and rebase pci_add_capability() to it.
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Rename check_irqchip_in_kernel() to verify_irqchip_in_kernel(), so that
the name reflects our expectation better. Rather than returning a bool,
make it do nothing or set an Error.
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

get_real_id() has two thin wrappers (and no other callers),
get_real_vendor_id() and get_real_device_id(); it's easiest to convert
them in one fell swoop.
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

This allows us to report the entire error with one error_report() call,
easing future error propagation.
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Propagate any errors in monitor fd handling up to get_real_device(), and
report them there. We'll continue the propagation upwards when
get_real_device() becomes a leaf itself (when none of its callees will
report errors internally any longer when detecting and returning an
error).
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
eviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

and rebase monitor_handle_fd_param() to it. (Note that this will slightly
change the behavior when the qemu_parse_fd() branch is selected and it
fails: we now report (and in case of QMP, set) the error immediately,
rather than allowing the caller to set its own error message (if any)).
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

qemu_parse_fd() used to handle at least the following strings incorrectly:
o "-2":         simply let through
o "2147483648": returned as LONG_MAX==INT_MAX on ILP32 (with ERANGE
                ignored); implementation-defined behavior on LP64
Signed-off-by: NLaszlo Ersek <lersek@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

strtosz_suffix() might return negative error, this patch fixes
the error handling.

This patch also changes to handle error in the if statement
rather than handle success specially, this will make this use
of strtosz_suffix consistent with all other uses.
Signed-off-by: NAmos Kong <akong@redhat.com>
Reviewed-by: NMichael S. Tsirkin <mst@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

The primitive uses JSON syntax, and include paths are relative to the file using the directive:

  { 'include': 'path/to/file.json' }
Signed-off-by: NLluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Use an explicit input file on the command-line instead of reading from standard
input.

It also outputs the proper file name when there's an error.
Signed-off-by: NLluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>

Signed-off-by: NLluís Vilanova <vilanova@ac.upc.edu>
Reviewed-by: NEric Blake <eblake@redhat.com>
Reviewed-by: NMarkus Armbruster <armbru@redhat.com>
Signed-off-by: NLuiz Capitulino <lcapitulino@redhat.com>