Re: [PATCH 1/3] Use _abort instead of separate assert()

[Markus Armbruster]

Alexander Bulekov  writes:

> On 200313 1805, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>
>
>> index 1a99277d60..aa9eee6ebf 100644
>> --- a/tests/qtest/fuzz/qos_fuzz.c
>> +++ b/tests/qtest/fuzz/qos_fuzz.c
>> @@ -57,8 +57,7 @@ static void qos_set_machines_devices_available(void)
>>  QList *lst;
>>  Error *err = NULL;
> Can this err declaration be removed? Don't think it's used anywhere
> else.

Will do.

>> -qmp_marshal_query_machines(NULL, , );
>> -assert(!err);
>> +qmp_marshal_query_machines(NULL, , _abort);
>>  lst = qobject_to(QList, response);
>>  apply_to_qlist(lst, true);
>>  
>> @@ -70,8 +69,7 @@ static void qos_set_machines_devices_available(void)
>>  qdict_put_bool(args, "abstract", true);
>>  qdict_put_obj(req, "arguments", (QObject *) args);
>>  
>> -qmp_marshal_qom_list_types(args, , );
>> -assert(!err);
>> +qmp_marshal_qom_list_types(args, , _abort);
>>  lst = qobject_to(QList, response);
>>  apply_to_qlist(lst, false);
>>  qobject_unref(response);
>> -- 
>> 2.21.1
>> 
> Thanks!
>
> Acked-by: Alexander Bulekov 

Thanks!

Re: [PATCH 2/9] qapi/misc: Move add_client command with chardev code

[Marc-André Lureau]

Hi

On Fri, Mar 13, 2020 at 7:42 PM Philippe Mathieu-Daudé
 wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé 

Without looking at the rest of the series, I fail to see the
improvement, quite the opposite. A bit of context?

> ---
>  qapi/char.json | 32 
>  qapi/misc.json | 32 
>  monitor/qmp-cmds.c |  1 +
>  3 files changed, 33 insertions(+), 32 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index 6907b2bfdb..8b7baf11eb 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -572,3 +572,35 @@
>  { 'event': 'VSERPORT_CHANGE',
>'data': { 'id': 'str',
>  'open': 'bool' } }
> +
> +##
> +# @add_client:
> +#
> +# Allow client connections for VNC, Spice and socket based
> +# character devices to be passed in to QEMU via SCM_RIGHTS.
> +#
> +# @protocol: protocol name. Valid names are "vnc", "spice" or the
> +#name of a character device (eg. from -chardev id=)
> +#
> +# @fdname: file descriptor name previously passed via 'getfd' command
> +#
> +# @skipauth: whether to skip authentication. Only applies
> +#to "vnc" and "spice" protocols
> +#
> +# @tls: whether to perform TLS. Only applies to the "spice"
> +#   protocol
> +#
> +# Returns: nothing on success.
> +#
> +# Since: 0.14.0
> +#
> +# Example:
> +#
> +# -> { "execute": "add_client", "arguments": { "protocol": "vnc",
> +#  "fdname": "myclient" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'add_client',
> +  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> +'*tls': 'bool' } }
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c18fe681fb..e84e6823e9 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -39,38 +39,6 @@
>  { 'enum': 'LostTickPolicy',
>'data': ['discard', 'delay', 'slew' ] }
>
> -##
> -# @add_client:
> -#
> -# Allow client connections for VNC, Spice and socket based
> -# character devices to be passed in to QEMU via SCM_RIGHTS.
> -#
> -# @protocol: protocol name. Valid names are "vnc", "spice" or the
> -#name of a character device (eg. from -chardev id=)
> -#
> -# @fdname: file descriptor name previously passed via 'getfd' command
> -#
> -# @skipauth: whether to skip authentication. Only applies
> -#to "vnc" and "spice" protocols
> -#
> -# @tls: whether to perform TLS. Only applies to the "spice"
> -#   protocol
> -#
> -# Returns: nothing on success.
> -#
> -# Since: 0.14.0
> -#
> -# Example:
> -#
> -# -> { "execute": "add_client", "arguments": { "protocol": "vnc",
> -#  "fdname": "myclient" } }
> -# <- { "return": {} }
> -#
> -##
> -{ 'command': 'add_client',
> -  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> -'*tls': 'bool' } }
> -
>  ##
>  # @NameInfo:
>  #
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 864cbfa32e..67d95b4af7 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -31,6 +31,7 @@
>  #include "sysemu/block-backend.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-block.h"
> +#include "qapi/qapi-commands-char.h"
>  #include "qapi/qapi-commands-control.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qapi-commands-misc.h"
> --
> 2.21.1
>

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[BALATON Zoltan]

On Fri, 13 Mar 2020, BALATON Zoltan wrote:

The pci_ide_create_devs() function takes a hd_table parameter but all
callers just pass what ide_drive_get() returns so we can do it locally
simplifying callers and removing hd_table parameter.

Signed-off-by: BALATON Zoltan 
---
hw/alpha/dp264.c  | 13 +++--
hw/i386/pc_piix.c |  9 +
hw/ide/pci.c  |  5 +++--
hw/isa/piix4.c| 10 ++
hw/mips/mips_fulong2e.c   |  4 +---
hw/mips/mips_malta.c  |  2 +-
hw/sparc64/sun4u.c|  6 +-
include/hw/ide/pci.h  |  2 +-
include/hw/southbridge/piix.h |  3 +--
9 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 0f58b1b668..e23172f177 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -15,7 +15,6 @@
#include "qemu/error-report.h"
#include "sysemu/sysemu.h"
#include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
#include "hw/ide/pci.h"
#include "hw/timer/i8254.h"
#include "hw/isa/superio.h"
@@ -56,6 +55,7 @@ static void clipper_init(MachineState *machine)
const char *initrd_filename = machine->initrd_filename;
AlphaCPU *cpus[4];
PCIBus *pci_bus;
+PCIDevice *pci_dev;
ISABus *isa_bus;
qemu_irq rtc_irq;
long size, i;
@@ -98,15 +98,8 @@ static void clipper_init(MachineState *machine)
isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);

/* IDE disk setup.  */
-{
-DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-PCIDevice *pci_dev;
-
-ide_drive_get(hd, ARRAY_SIZE(hd));
-
-pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
-pci_ide_create_devs(pci_dev, hd);
-}
+pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
+pci_ide_create_devs(pci_dev);


Additionally, I think it may also make sense to move pci_ide_create_devs 
call into the realize methods of these IDE controllers so boards do not 
need to do it explicitely. These calls always follow the creation of the 
device immediately so could just be done internally in IDE device and 
simplify it further. I can attempt to prepare additional patches for that 
but first I'd like to hear if anyone has anything against that to avoid 
doing useless work.


Regards,
BALATON Zoltan


/* Load PALcode.  Given that this is not "real" cpu palcode,
   but one explicitly written for the emulation, we might as
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b363a69e2e..a7a696d0c8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -84,7 +84,6 @@ static void pc_init1(MachineState *machine,
int piix3_devfn = -1;
qemu_irq smi_irq;
GSIState *gsi_state;
-DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
BusState *idebus[MAX_IDE_BUS];
ISADevice *rtc_state;
MemoryRegion *ram_memory;
@@ -238,20 +237,22 @@ static void pc_init1(MachineState *machine,

pc_nic_init(pcmc, isa_bus, pci_bus);

-ide_drive_get(hd, ARRAY_SIZE(hd));
if (pcmc->pci_enabled) {
PCIDevice *dev;

dev = pci_create_simple(pci_bus, piix3_devfn + 1,
xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
-pci_ide_create_devs(dev, hd);
+pci_ide_create_devs(dev);
idebus[0] = qdev_get_child_bus(>qdev, "ide.0");
idebus[1] = qdev_get_child_bus(>qdev, "ide.1");
pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
}
#ifdef CONFIG_IDE_ISA
-else {
+else {
+DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
int i;
+
+ide_drive_get(hd, ARRAY_SIZE(hd));
for (i = 0; i < MAX_IDE_BUS; i++) {
ISADevice *dev;
char busname[] = "ide.0";
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e0c84392e2..03d6eef592 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -476,14 +476,15 @@ const VMStateDescription vmstate_ide_pci = {
}
};

-/* hd_table must contain 4 block drivers */
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+void pci_ide_create_devs(PCIDevice *dev)
{
PCIIDEState *d = PCI_IDE(dev);
+DriveInfo *hd_table[MAX_IDE_BUS * MAX_IDE_DEVS];
static const int bus[4]  = { 0, 0, 1, 1 };
static const int unit[4] = { 0, 1, 0, 1 };
int i;

+ide_drive_get(hd_table, ARRAY_SIZE(hd_table));
for (i = 0; i < 4; i++) {
if (hd_table[i]) {
ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0ab4787658..13fa1660c3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -241,11 +241,8 @@ static void piix4_register_types(void)

type_init(piix4_register_types)

-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-  I2CBus **smbus, size_t ide_buses)
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
{
-size_t ide_drives = ide_buses * MAX_IDE_DEVS;
-DriveInfo **hd;
PCIDevice *pci;
DeviceState *dev;

@@ -257,10 +254,7 @@ DeviceState *piix4_create(PCIBus 

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Eric Blake]

On 3/13/20 4:54 PM, Markus Armbruster wrote:



I append my hacked up version of auto-propagated-errp.cocci.  It
produces the same patch as yours for the complete tree.



// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
//



//
// Usage example:
// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
//  --macro-file scripts/cocci-macro-file.h --in-place \
//  --no-show-diff --max-width 80 FILES...
//
// Note: --max-width 80 is needed because coccinelle default is less
// than 80, and without this parameter coccinelle may reindent some
// lines which fit into 80 characters but not to coccinelle default,
// which in turn produces extra patch hunks for no reason.


Do we really need this note?  And/or should we update other Coccinelle 
script examples to mention --max-width?




// Switch unusual Error ** parameter names to errp
// (this is necessary to use ERRP_AUTO_PROPAGATE).
//
// Disable optional_qualifier to skip functions with
// "Error *const *errp" parameter.
//
// Skip functions with "assert(_errp && *_errp)" statement, because
// that signals unusual semantics, and the parameter name may well
// serve a purpose. (like nbd_iter_channel_error()).
//
// Skip util/error.c to not touch, for example, error_propagate() and
// error_propagate_prepend().
@ depends on !(file in "util/error.c") disable optional_qualifier@


The comments are definitely helpful.


identifier fn;
identifier _errp != errp;
@@

  fn(...,
-   Error **_errp
+   Error **errp
 ,...)
  {
(
  ... when != assert(_errp && *_errp)
&
  <...
-_errp
+errp
  ...>
)
  }

// Add invocation of ERRP_AUTO_PROPAGATE to errp-functions where
// necessary
//
// Note, that without "when any" the final "..." does not mach
// something matched by previous pattern, i.e. the rule will not match
// double error_prepend in control flow like in
// vfio_set_irq_signaling().


How likely are we that this comment might go stale over time?  But I'm 
not opposed to it.



//
// Note, "exists" says that we want apply rule even if it matches not
// on all possible control flows (otherwise, it will not match


s/matches not on/does not match on/


// standard pattern when error_propagate() call is in if branch).
@ disable optional_qualifier exists@
identifier fn, local_err;
symbol errp;
@@

  fn(..., Error **errp, ...)
  {
+   ERRP_AUTO_PROPAGATE();
 ...  when != ERRP_AUTO_PROPAGATE();
(
(
 error_append_hint(errp, ...);
|
 error_prepend(errp, ...);
|
 error_vprepend(errp, ...);
)
 ... when any
|
 Error *local_err = NULL;
 ...
(
 error_propagate_prepend(errp, local_err, ...);
|
 error_propagate(errp, local_err);
)
 ...
)
  }


// Match functions with propagation of local error to errp.
// We want to refer these functions in several following rules, but I
// don't know a proper way to inherit a function, not just its name
// (to not match another functions with same name in following rules).
// Not-proper way is as follows: rename errp parameter in functions
// header and match it in following rules. Rename it back after all
// transformations.
//
// The simplest case of propagation scheme is single definition of
// local_err with at most one error_propagate_prepend or
// error_propagate on each control-flow. Still, we want to match more
// complex schemes too. We'll warn them with help of further rules.


We'll warn for those with the help of further rules.


@rule1 disable optional_qualifier exists@
identifier fn, local_err;
symbol errp;
@@

  fn(..., Error **
-errp
+
 , ...)
  {
  ...
  Error *local_err = NULL;
  ...
(
  error_propagate_prepend(errp, local_err, ...);
|
  error_propagate(errp, local_err);
)
  ...
  }


// Warn several Error * definitions.


// Warn when there are several Error * definitions.



@check1 disable optional_qualifier exists@
identifier fn, _errp, local_err, local_err2;
position p1, p2;
@@

  fn(..., Error **_errp, ...)
  {
  ...
  Error *local_err = NULL;@p1
  ... when any
  Error *local_err2 = NULL;@p2
  ... when any
  }

@ script:python @
fn << check1.fn;
p1 << check1.p1;
p2 << check1.p2;
@@

print('Warning: function {} has several definitions of '
   'Error * local variable: at {}:{} and then at {}:{}'.format(
   fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))

// Warn several propagations in control flow.


// Warn when several propagations are in the control flow.


@check2 disable optional_qualifier exists@
identifier fn, _errp;
position p1, p2;
@@

  fn(..., Error **_errp, ...)
  {
  ...
(
  error_propagate_prepend(_errp, ...);@p1
|
  error_propagate(_errp, ...);@p1
)
  ...
(
  error_propagate_prepend(_errp, ...);@p2
|
  error_propagate(_errp, ...);@p2
)
  ... when any
  }

@ script:python @
fn << check2.fn;
p1 << check2.p1;
p2 << check2.p2;
@@

print('Warning: function {} propagates to errp several times in '
   'one control flow: at {}:{} 

Re: [PATCH 5/5] block/io: auto-no-fallback for write-zeroes

[Eric Blake]

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:

NBD driver may has max_pwrite_zeroes but doesn't has
max_pwrite_zeroes_no_fallback limit. This means, that (when
BDRV_REQ_NO_FALLBACK is supported) it is beneficial to try send request
with BDRV_REQ_NO_FALLBACK instead of splitting the request accordingly
to max_pwrite_zeroes.

If failed, fallback to old behavior.


Grammar:

When BDRV_REQ_NO_FALLBACK is supported, the NBD driver supports a larger 
request size.  Add code to try large zero requests with a NO_FALLBACK 
request prior to having to split a request into chunks according to 
max_pwrite_zeroes.




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/io.c | 30 ++
  1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/block/io.c b/block/io.c
index c64566b4cf..48d71b0883 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1752,17 +1752,28 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  int head = 0;
  int tail = 0;
  
-int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?

-bs->bl.max_pwrite_zeroes_no_fallback :
-bs->bl.max_pwrite_zeroes, INT_MAX);
+int max_write_zeroes;
  int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
  bs->bl.request_alignment);
  int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
+bool auto_no_fallback;
  
  if (!drv) {

  return -ENOMEDIUM;
  }
  
+if (!(flags & BDRV_REQ_NO_FALLBACK) &&

+(bs->supported_zero_flags & BDRV_REQ_NO_FALLBACK) &&
+bs->bl.max_pwrite_zeroes &&
+bs->bl.max_pwrite_zeroes < bytes &&
+(bs->bl.max_pwrite_zeroes < bs->bl.max_pwrite_zeroes_no_fallback ||
+ bs->bl.max_pwrite_zeroes_no_fallback == 0))


Why are we letting max_pwrite_zeroes_no_fallback ever be 0?  It might be 
more convenient if it is always guaranteed to be >= max_pwrite_zeroes by 
the block layer.



+{
+assert(drv->bdrv_co_pwrite_zeroes);
+flags |= BDRV_REQ_NO_FALLBACK;
+auto_no_fallback = true;
+}
+
  if ((flags & ~bs->supported_zero_flags) & BDRV_REQ_NO_FALLBACK) {
  return -ENOTSUP;
  }
@@ -1770,7 +1781,11 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  assert(alignment % bs->bl.request_alignment == 0);
  head = offset % alignment;
  tail = (offset + bytes) % alignment;
-max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
+max_write_zeroes =
+QEMU_ALIGN_DOWN(MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
+ bs->bl.max_pwrite_zeroes_no_fallback :
+ bs->bl.max_pwrite_zeroes, INT_MAX),
+alignment);
  assert(max_write_zeroes >= bs->bl.request_alignment);
  
  while (bytes > 0 && !ret) {

@@ -1801,6 +1816,13 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  if (drv->bdrv_co_pwrite_zeroes) {
  ret = drv->bdrv_co_pwrite_zeroes(bs, offset, num,
   flags & 
bs->supported_zero_flags);
+if (ret == -ENOTSUP && auto_no_fallback) {
+flags &= ~BDRV_REQ_NO_FALLBACK;
+max_write_zeroes =
+QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_pwrite_zeroes,
+ INT_MAX), alignment);
+continue;
+}
  if (ret != -ENOTSUP && (flags & BDRV_REQ_FUA) &&
  !(bs->supported_zero_flags & BDRV_REQ_FUA)) {
  need_flush = true;



Otherwise makes sense.

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

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 13.03.2020 18:42, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> 12.03.2020 19:36, Markus Armbruster wrote:
 I may have a second look tomorrow with fresher eyes, but let's get this
 out now as is.

 Vladimir Sementsov-Ogievskiy  writes:

> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
> does corresponding changes in code (look for details in
> include/qapi/error.h)
>
> Usage example:
> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>--macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>--max-width 80 FILES...
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>
> Cc: Eric Blake 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Greg Kurz 
> Cc: Christian Schoenebeck 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Stefan Hajnoczi 
> Cc: "Philippe Mathieu-Daudé" 
> Cc: Laszlo Ersek 
> Cc: Gerd Hoffmann 
> Cc: Stefan Berger 
> Cc: Markus Armbruster 
> Cc: Michael Roth 
> Cc: qemu-de...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
>
>scripts/coccinelle/auto-propagated-errp.cocci | 327 ++
>include/qapi/error.h  |   3 +
>MAINTAINERS   |   1 +
>3 files changed, 331 insertions(+)
>create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>
> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
> b/scripts/coccinelle/auto-propagated-errp.cocci
> new file mode 100644
> index 00..7dac2dcfa4
> --- /dev/null
> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
> @@ -0,0 +1,327 @@
> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
> +//
> +// Copyright (c) 2020 Virtuozzo International GmbH.
> +//
> +// 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 program.  If not, see
> +// .
> +//
> +// Usage example:
> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
> +//  --macro-file scripts/cocci-macro-file.h --in-place \
> +//  --no-show-diff --max-width 80 FILES...
> +//
> +// Note: --max-width 80 is needed because coccinelle default is less
> +// than 80, and without this parameter coccinelle may reindent some
> +// lines which fit into 80 characters but not to coccinelle default,
> +// which in turn produces extra patch hunks for no reason.

 This is about unwanted reformatting of parameter lists due to the ___
 chaining hack.  --max-width 80 makes that less likely, but not
 impossible.

 We can search for unwanted reformatting of parameter lists.  I think
 grepping diffs for '^\+.*Error \*\*' should do the trick.  For the whole
 tree, I get one false positive (not a parameter list), and one hit:

   @@ -388,8 +388,10 @@ static void object_post_init_with_type(O
}
}

   -void object_apply_global_props(Object *obj, const GPtrArray *props, 
 Error **errp)
   +void object_apply_global_props(Object *obj, const GPtrArray *props,
   +   Error **errp)
{
   +ERRP_AUTO_PROPAGATE();
int i;

if (!props) {

 Reformatting, but not unwanted.
>>>
>>> Yes, I saw it. This line is 81 character length, so it's OK to fix it in 
>>> one hunk with
>>> ERRP_AUTO_PROPAGATE addition even for non-automatic patch.
>>
>> Agree.
>>

 The --max-width 80 hack is good enough for me.

 It does result in slightly long transformed lines, e.g. this one in
 replication.c:

   @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS
s->mode = REPLICATION_MODE_PRIMARY;
top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
if (top_id) {
   -error_setg(_err, "The primary side does not 
 support option top-id");
   +error_setg(errp, "The primary side does not support 
 option top-id");
goto fail;

Re: [PATCH 4/5] block/io: fix bdrv_co_do_pwrite_zeroes head calculation

[Eric Blake]

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:

It's wrong to update head using num in this place, as num may be
reduced during the iteration, and we'll have wrong head value on next
iteration.

Instead update head at iteration end.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/io.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)


Offhand, I don't see how this fixes any bug
/me reads on



diff --git a/block/io.c b/block/io.c
index 75fd5600c2..c64566b4cf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1785,7 +1785,6 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
   * convenience, limit this request to max_transfer even if
   * we don't need to fall back to writes.  */
  num = MIN(MIN(bytes, max_transfer), alignment - head);
-head = (head + num) % alignment;
  assert(num < max_write_zeroes);


Here, we've asserted that if head was non-zero, num was already smaller 
than max_write_zeroes.  The rest of the loop does indeed have code that 
appears like it can reduce num, but that code is guarded:


/* limit request size */
if (num > max_write_zeroes) {
num = max_write_zeroes;
}
...
if (ret == -ENOTSUP && !(flags & BDRV_REQ_NO_FALLBACK)) {
/* Fall back to bounce buffer if write zeroes is unsupported */
BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;

if ((flags & BDRV_REQ_FUA) &&
!(bs->supported_write_flags & BDRV_REQ_FUA)) {
/* No need for bdrv_driver_pwrite() to do a fallback
 * flush on each chunk; use just one at the end */
write_flags &= ~BDRV_REQ_FUA;
need_flush = true;
}
num = MIN(num, max_transfer);

Oh. Now I see.  If max_write_zeroes is > max_transfer, but we fall back 
to a bounce buffer, it is indeed possible that a misaligned request that 
forces fallbacks to writes may indeed require more than one write to get 
to the point where it is then using a buffer aligned to max_write_zeroes.


Do we have an iotest provoking this, or is it theoretical?  With an 
iotest, this one is material for 5.0 even if the rest of the series 
misses soft freeze.



  } else if (tail && num > alignment) {
  /* Shorten the request to the last aligned sector.  */
@@ -1844,6 +1843,9 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  
  offset += num;

  bytes -= num;
+if (head) {
+head = offset % alignment;
+}


Reviewed-by: Eric Blake 

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

Re: [PATCH v6 1/4] qcow2: introduce compression type feature

[Eric Blake]

On 3/12/20 4:22 AM, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to qcow2 allowing the use different compression methods for
image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Adding of the compression type breaks a number of tests because now the
compression type is reported on image creation and there are some changes
in the qcow2 header in size and offsets.

The tests are fixed in the following ways:
 * filter out compression_type for all the tests


Presumably this filter is optional, and we will not use it on the 
specific new tests that prove zstd compression works - but that should 
be later in the series, so for this patch it is okay.



 * fix header size, feature table size and backing file offset
   affected tests: 031, 036, 061, 080
   header_size +=8: 1 byte compression type
7 bytes padding
   feature_table += 48: incompatible feture compression type


feature


   backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)


feature

(interesting that you have two different changed spellings ;)


 * add "compression type" for test output matching when it isn't filtered
   affected tests: 049, 060, 061, 065, 144, 182, 242, 255


Or maybe the comment above should be changed to "many tests" rather than 
"all the tests".




Signed-off-by: Denis Plotnikov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json |  22 +-
  block/qcow2.h|  20 -
  include/block/block_int.h|   1 +
  block/qcow2.c| 121 +++
  tests/qemu-iotests/031.out   |  14 ++--
  tests/qemu-iotests/036.out   |   4 +-
  tests/qemu-iotests/049.out   | 102 +-
  tests/qemu-iotests/060.out   |   1 +
  tests/qemu-iotests/061.out   |  34 +
  tests/qemu-iotests/065   |  28 ---
  tests/qemu-iotests/080   |   2 +-
  tests/qemu-iotests/144.out   |   4 +-
  tests/qemu-iotests/182.out   |   2 +-
  tests/qemu-iotests/242.out   |   5 ++
  tests/qemu-iotests/255.out   |   8 +-
  tests/qemu-iotests/common.filter |   3 +-
  16 files changed, 275 insertions(+), 96 deletions(-)




+++ b/block/qcow2.h
@@ -146,8 +146,16 @@ typedef struct QCowHeader {
  
  uint32_t refcount_order;

  uint32_t header_length;
+
+/* Additional fields */
+uint8_t  compression_type;
+
+/* header must be a multiple of 8 */
+uint8_t  padding[7];


Why two spaces after uint8_t (twice)?



@@ -369,6 +380,13 @@ typedef struct BDRVQcow2State {
  
  bool metadata_preallocation_checked;

  bool metadata_preallocation;
+/*
+ * Compression type used for the image. Default: 0 - ZLIB
+ * The image compression type is set on image creation.
+ * The only way to change the compression type is to convert the image
+ * with the desired compression type set


Missing trailing '.'.  Maybe someday we can get 'qemu-img amend' to also 
adjust the compression type in-place; if that's something we think we 
might do, then this could be better worded as "For now, the only way to 
change...".



+++ b/block/qcow2.c
@@ -1242,6 +1242,48 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
  return ret;
  }
  
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)



+
+static int qcow2_compression_type_from_format(const char *ct)
+{
+if (g_str_equal(ct, "zlib")) {
+return QCOW2_COMPRESSION_TYPE_ZLIB;
+} else {
+return -EINVAL;
+}


Why are you open-coding this?

qapi_enum_parse(_lookup, ct, -1, errp)

should do what you use this for, and automatically updates itself when 
you add zstd to the qapi enum later.




@@ -3401,6 +3493,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
  .refcount_table_offset  = cpu_to_be64(cluster_size),
  .refcount_table_clusters= cpu_to_be32(1),
  .refcount_order = cpu_to_be32(refcount_order),
+/* don't deal with endians since compression_type is 1 byte long */


endianness


+.compression_type   = compression_type,
  .header_length  = cpu_to_be32(sizeof(*header)),
  };
  
@@ -5516,6 +5631,12 @@ static QemuOptsList qcow2_create_opts = {

  .help = "Width of a reference count entry in bits",
  .def_value_str = 

[PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[BALATON Zoltan]

The pci_ide_create_devs() function takes a hd_table parameter but all
callers just pass what ide_drive_get() returns so we can do it locally
simplifying callers and removing hd_table parameter.

Signed-off-by: BALATON Zoltan 
---
 hw/alpha/dp264.c  | 13 +++--
 hw/i386/pc_piix.c |  9 +
 hw/ide/pci.c  |  5 +++--
 hw/isa/piix4.c| 10 ++
 hw/mips/mips_fulong2e.c   |  4 +---
 hw/mips/mips_malta.c  |  2 +-
 hw/sparc64/sun4u.c|  6 +-
 include/hw/ide/pci.h  |  2 +-
 include/hw/southbridge/piix.h |  3 +--
 9 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 0f58b1b668..e23172f177 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -15,7 +15,6 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/timer/i8254.h"
 #include "hw/isa/superio.h"
@@ -56,6 +55,7 @@ static void clipper_init(MachineState *machine)
 const char *initrd_filename = machine->initrd_filename;
 AlphaCPU *cpus[4];
 PCIBus *pci_bus;
+PCIDevice *pci_dev;
 ISABus *isa_bus;
 qemu_irq rtc_irq;
 long size, i;
@@ -98,15 +98,8 @@ static void clipper_init(MachineState *machine)
 isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
 
 /* IDE disk setup.  */
-{
-DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-PCIDevice *pci_dev;
-
-ide_drive_get(hd, ARRAY_SIZE(hd));
-
-pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
-pci_ide_create_devs(pci_dev, hd);
-}
+pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
+pci_ide_create_devs(pci_dev);
 
 /* Load PALcode.  Given that this is not "real" cpu palcode,
but one explicitly written for the emulation, we might as
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b363a69e2e..a7a696d0c8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -84,7 +84,6 @@ static void pc_init1(MachineState *machine,
 int piix3_devfn = -1;
 qemu_irq smi_irq;
 GSIState *gsi_state;
-DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
 BusState *idebus[MAX_IDE_BUS];
 ISADevice *rtc_state;
 MemoryRegion *ram_memory;
@@ -238,20 +237,22 @@ static void pc_init1(MachineState *machine,
 
 pc_nic_init(pcmc, isa_bus, pci_bus);
 
-ide_drive_get(hd, ARRAY_SIZE(hd));
 if (pcmc->pci_enabled) {
 PCIDevice *dev;
 
 dev = pci_create_simple(pci_bus, piix3_devfn + 1,
 xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
-pci_ide_create_devs(dev, hd);
+pci_ide_create_devs(dev);
 idebus[0] = qdev_get_child_bus(>qdev, "ide.0");
 idebus[1] = qdev_get_child_bus(>qdev, "ide.1");
 pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
 }
 #ifdef CONFIG_IDE_ISA
-else {
+else {
+DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
 int i;
+
+ide_drive_get(hd, ARRAY_SIZE(hd));
 for (i = 0; i < MAX_IDE_BUS; i++) {
 ISADevice *dev;
 char busname[] = "ide.0";
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e0c84392e2..03d6eef592 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -476,14 +476,15 @@ const VMStateDescription vmstate_ide_pci = {
 }
 };
 
-/* hd_table must contain 4 block drivers */
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+void pci_ide_create_devs(PCIDevice *dev)
 {
 PCIIDEState *d = PCI_IDE(dev);
+DriveInfo *hd_table[MAX_IDE_BUS * MAX_IDE_DEVS];
 static const int bus[4]  = { 0, 0, 1, 1 };
 static const int unit[4] = { 0, 1, 0, 1 };
 int i;
 
+ide_drive_get(hd_table, ARRAY_SIZE(hd_table));
 for (i = 0; i < 4; i++) {
 if (hd_table[i]) {
 ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0ab4787658..13fa1660c3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -241,11 +241,8 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-  I2CBus **smbus, size_t ide_buses)
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
-size_t ide_drives = ide_buses * MAX_IDE_DEVS;
-DriveInfo **hd;
 PCIDevice *pci;
 DeviceState *dev;
 
@@ -257,10 +254,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
**isa_bus,
 }
 
 pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
-hd = g_new(DriveInfo *, ide_drives);
-ide_drive_get(hd, ide_drives);
-pci_ide_create_devs(pci, hd);
-g_free(hd);
+pci_ide_create_devs(pci);
 
 pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
 if (smbus) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 3690b76061..508dc57737 100644
--- 

[PATCH 3/8] hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h

[BALATON Zoltan]

After previous patches we don't need hw/pci/pci.h any more in
hw/ide.h. Some files depended on implicit inclusion by this header
which are also fixed up here.

Signed-off-by: BALATON Zoltan 
---
 hw/ide/ahci_internal.h| 1 +
 include/hw/ide.h  | 1 -
 include/hw/ide/pci.h  | 1 +
 include/hw/misc/macio/macio.h | 1 +
 4 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 73424516da..bab0459774 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -27,6 +27,7 @@
 #include "hw/ide/ahci.h"
 #include "hw/ide/internal.h"
 #include "hw/sysbus.h"
+#include "hw/pci/pci.h"
 
 #define AHCI_MEM_BAR_SIZE 0x1000
 #define AHCI_MAX_PORTS32
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 21bd8f23f1..d52c211f32 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -2,7 +2,6 @@
 #define HW_IDE_H
 
 #include "hw/isa/isa.h"
-#include "hw/pci/pci.h"
 #include "exec/memory.h"
 
 #define MAX_IDE_DEVS   2
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a9f2c33e68..98ffa7dfcd 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -2,6 +2,7 @@
 #define HW_IDE_PCI_H
 
 #include "hw/ide/internal.h"
+#include "hw/pci/pci.h"
 
 #define BM_STATUS_DMAING 0x01
 #define BM_STATUS_ERROR  0x02
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 070a694eb5..87335a991c 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -27,6 +27,7 @@
 #define MACIO_H
 
 #include "hw/char/escc.h"
+#include "hw/pci/pci.h"
 #include "hw/ide/internal.h"
 #include "hw/intc/heathrow_pic.h"
 #include "hw/misc/macio/cuda.h"
-- 
2.21.1

[PATCH 7/8] hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h

[BALATON Zoltan]

We can move it next to the MAX_IDE_BUS define now that less files use
it.

Signed-off-by: BALATON Zoltan 
---
 include/hw/ide.h  | 2 --
 include/hw/ide/internal.h | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/ide.h b/include/hw/ide.h
index d52c211f32..c5ce5da4f4 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -4,8 +4,6 @@
 #include "hw/isa/isa.h"
 #include "exec/memory.h"
 
-#define MAX_IDE_DEVS   2
-
 /* ide-isa.c */
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
 DriveInfo *hd0, DriveInfo *hd1);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1a49d35959..fd3cae4acf 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -28,6 +28,7 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
 #define MAX_IDE_BUS 2
+#define MAX_IDE_DEVS 2
 
 /* Bits of HD_STATUS */
 #define ERR_STAT   0x01
-- 
2.21.1

[PATCH 8/8] hw/ide: Remove unneeded inclusion of hw/ide.h

[BALATON Zoltan]

After previous clean ups we can drop direct inclusion of hw/ide.h from
several places.

Signed-off-by: BALATON Zoltan 
---
 hw/hppa/hppa_sys.h  | 1 -
 hw/hppa/machine.c   | 1 -
 hw/i386/pc_piix.c   | 1 -
 hw/isa/piix4.c  | 1 -
 hw/mips/mips_fulong2e.c | 1 -
 hw/ppc/mac_newworld.c   | 1 -
 hw/ppc/mac_oldworld.c   | 1 -
 hw/ppc/prep.c   | 1 -
 8 files changed, 8 deletions(-)

diff --git a/hw/hppa/hppa_sys.h b/hw/hppa/hppa_sys.h
index 4d08501464..0b18271cc9 100644
--- a/hw/hppa/hppa_sys.h
+++ b/hw/hppa/hppa_sys.h
@@ -5,7 +5,6 @@
 
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
-#include "hw/ide.h"
 #include "hw/boards.h"
 #include "hw/intc/i8259.h"
 
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 1f9a390f99..d1fbf781c5 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -13,7 +13,6 @@
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/timer/i8254.h"
 #include "hw/char/serial.h"
 #include "hw/net/lasi_82596.h"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a7a696d0c8..ac78b07451 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -38,7 +38,6 @@
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
 #include "net/net.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 13fa1660c3..136a763e3f 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -34,7 +34,6 @@
 #include "hw/dma/i8257.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 508dc57737..ab6634d538 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -36,7 +36,6 @@
 #include "audio/audio.h"
 #include "qemu/log.h"
 #include "hw/loader.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "elf.h"
 #include "hw/isa/vt82c686.h"
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index daa1523feb..dfe5aeab0a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -62,7 +62,6 @@
 #include "hw/char/escc.h"
 #include "hw/misc/macio/macio.h"
 #include "hw/ppc/openpic.h"
-#include "hw/ide.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 2478748c78..4c1b1f35a1 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -41,7 +41,6 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/char/escc.h"
 #include "hw/misc/macio/macio.h"
-#include "hw/ide.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index e1b1549e58..52bded7e5e 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -37,7 +37,6 @@
 #include "hw/boards.h"
 #include "qemu/error-report.h"
 #include "qemu/log.h"
-#include "hw/ide.h"
 #include "hw/irq.h"
 #include "hw/loader.h"
 #include "hw/rtc/mc146818rtc.h"
-- 
2.21.1

[PATCH 2/8] hw/ide: Get rid of piix4_init function

[BALATON Zoltan]

This removes pci_piix4_ide_init() function similar to clean up done to
other ide devices.

Signed-off-by: BALATON Zoltan 
---
 hw/ide/piix.c| 12 +---
 hw/isa/piix4.c   |  5 -
 include/hw/ide.h |  1 -
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 8bcd6b72c2..3b2de4c312 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
 }
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
-PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-PCIDevice *dev;
-
-dev = pci_create_simple(bus, devfn, "piix4-ide");
-pci_ide_create_devs(dev, hd_table);
-return dev;
-}
-
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
 .class_init= piix3_ide_class_init,
 };
 
+/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
 static void piix4_ide_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 7edec5e149..0ab4787658 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -35,6 +35,7 @@
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
@@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
**isa_bus,
 *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
 }
 
+pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
 hd = g_new(DriveInfo *, ide_drives);
 ide_drive_get(hd, ide_drives);
-pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
+pci_ide_create_devs(pci, hd);
 g_free(hd);
+
 pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
 if (smbus) {
 *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 883bbaeb9b..21bd8f23f1 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, 
int isairq,
 DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
 
 /* ide-mmio.c */
-- 
2.21.1

[PATCH 0/8] Misc hw/ide legacy clean up

[BALATON Zoltan]

These are some clean ups to remove more legacy init functions and
lessen dependence on include/hw/ide.h with some simplifications in
board code. There should be no functional change.

BALATON Zoltan (8):
  hw/ide: Get rid of piix3_init functions
  hw/ide: Get rid of piix4_init function
  hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h
  hw/ide: Move MAX_IDE_BUS define to one header
  hw/ide/pci.c: Coding style update to fix checkpatch errors
  hw/ide: Do ide_drive_get() within pci_ide_create_devs()
  hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h
  hw/ide: Remove unneeded inclusion of hw/ide.h

 hw/alpha/dp264.c  | 15 +++
 hw/hppa/hppa_sys.h|  1 -
 hw/hppa/machine.c |  3 ---
 hw/i386/pc_piix.c | 20 +---
 hw/ide/ahci_internal.h|  1 +
 hw/ide/pci.c  | 10 ++
 hw/ide/piix.c | 31 +--
 hw/isa/piix4.c| 14 +-
 hw/mips/mips_fulong2e.c   |  6 +-
 hw/mips/mips_malta.c  |  6 ++
 hw/mips/mips_r4k.c|  4 +---
 hw/ppc/mac_newworld.c |  2 --
 hw/ppc/mac_oldworld.c |  2 --
 hw/ppc/prep.c |  3 ---
 hw/sparc64/sun4u.c|  7 +--
 include/hw/ide.h  |  6 --
 include/hw/ide/internal.h |  3 +++
 include/hw/ide/pci.h  |  3 ++-
 include/hw/misc/macio/macio.h |  1 +
 include/hw/southbridge/piix.h |  3 +--
 20 files changed, 37 insertions(+), 104 deletions(-)

-- 
2.21.1

[PATCH 5/8] hw/ide/pci.c: Coding style update to fix checkpatch errors

[BALATON Zoltan]

Spaces are required around a + operator and if statements should have
braces even for single line. Also make it simpler by reversing the
condition instead of breaking the loop.

Signed-off-by: BALATON Zoltan 
---
 hw/ide/pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4fc76c5225..e0c84392e2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -485,9 +485,9 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo 
**hd_table)
 int i;
 
 for (i = 0; i < 4; i++) {
-if (hd_table[i] == NULL)
-continue;
-ide_create_drive(d->bus+bus[i], unit[i], hd_table[i]);
+if (hd_table[i]) {
+ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
+}
 }
 }
 
-- 
2.21.1

[PATCH 1/8] hw/ide: Get rid of piix3_init functions

[BALATON Zoltan]

This removes pci_piix3_ide_init() and pci_piix3_xen_ide_init()
functions similar to clean up done to other ide devices.

Signed-off-by: BALATON Zoltan 
---
 hw/i386/pc_piix.c | 10 +-
 hw/ide/pci.c  |  1 +
 hw/ide/piix.c | 21 +
 include/hw/ide.h  |  2 --
 4 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e2d98243bc..c399398739 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -39,6 +39,7 @@
 #include "hw/usb.h"
 #include "net/net.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
 #include "hw/kvm/clock.h"
@@ -242,11 +243,10 @@ static void pc_init1(MachineState *machine,
 ide_drive_get(hd, ARRAY_SIZE(hd));
 if (pcmc->pci_enabled) {
 PCIDevice *dev;
-if (xen_enabled()) {
-dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
-} else {
-dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
-}
+
+dev = pci_create_simple(pci_bus, piix3_devfn + 1,
+xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+pci_ide_create_devs(dev, hd);
 idebus[0] = qdev_get_child_bus(>qdev, "ide.0");
 idebus[1] = qdev_get_child_bus(>qdev, "ide.1");
 pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 1a6a287e76..4fc76c5225 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -476,6 +476,7 @@ const VMStateDescription vmstate_ide_pci = {
 }
 };
 
+/* hd_table must contain 4 block drivers */
 void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
 {
 PCIIDEState *d = PCI_IDE(dev);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bc575b4d70..8bcd6b72c2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -197,15 +197,6 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
 return 0;
 }
 
-PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-PCIDevice *dev;
-
-dev = pci_create_simple(bus, devfn, "piix3-ide-xen");
-pci_ide_create_devs(dev, hd_table);
-return dev;
-}
-
 static void pci_piix_ide_exitfn(PCIDevice *dev)
 {
 PCIIDEState *d = PCI_IDE(dev);
@@ -217,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
 }
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
-PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-PCIDevice *dev;
-
-dev = pci_create_simple(bus, devfn, "piix3-ide");
-pci_ide_create_devs(dev, hd_table);
-return dev;
-}
-
 /* hd_table must contain 4 block drivers */
 /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
@@ -239,6 +219,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo 
**hd_table, int devfn)
 return dev;
 }
 
+/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index dea0ecf5be..883bbaeb9b 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,8 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, 
int isairq,
 DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
devfn);
-PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
 
-- 
2.21.1

[PATCH 4/8] hw/ide: Move MAX_IDE_BUS define to one header

[BALATON Zoltan]

There are several definitions of MAX_IDE_BUS in different boards (some
of them unused) with the same value. Move it to include/hw/ide/internal.h
to have it in a central place.

Signed-off-by: BALATON Zoltan 
---
 hw/alpha/dp264.c  | 2 --
 hw/hppa/machine.c | 2 --
 hw/i386/pc_piix.c | 2 --
 hw/mips/mips_fulong2e.c   | 1 -
 hw/mips/mips_malta.c  | 4 +---
 hw/mips/mips_r4k.c| 4 +---
 hw/ppc/mac_newworld.c | 1 -
 hw/ppc/mac_oldworld.c | 1 -
 hw/ppc/prep.c | 2 --
 hw/sparc64/sun4u.c| 1 -
 include/hw/ide/internal.h | 2 ++
 11 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 27595767e5..0f58b1b668 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -24,8 +24,6 @@
 #include "qemu/cutils.h"
 #include "net/net.h"
 
-#define MAX_IDE_BUS 2
-
 static uint64_t cpu_alpha_superpage_to_phys(void *opaque, uint64_t addr)
 {
 if (((addr >> 41) & 3) == 2) {
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 9175f4b790..1f9a390f99 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -24,8 +24,6 @@
 #include "qemu/log.h"
 #include "net/net.h"
 
-#define MAX_IDE_BUS 2
-
 static ISABus *hppa_isa_bus(void)
 {
 ISABus *isa_bus;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c399398739..b363a69e2e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -63,8 +63,6 @@
 #include "sysemu/numa.h"
 #include "hw/mem/nvdimm.h"
 
-#define MAX_IDE_BUS 2
-
 #ifdef CONFIG_IDE_ISA
 static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 639ba2a091..3690b76061 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -55,7 +55,6 @@
 
 /* fulong 2e has a 512k flash: Winbond W39L040AP70Z */
 #define BIOS_SIZE   (512 * KiB)
-#define MAX_IDE_BUS 2
 
 /*
  * PMON is not part of qemu and released with BSD license, anyone
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index d380f73d7b..6f51e33e7b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -40,7 +40,7 @@
 #include "sysemu/arch_init.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
-#include "hw/ide.h"
+#include "hw/ide/internal.h"
 #include "hw/irq.h"
 #include "hw/loader.h"
 #include "elf.h"
@@ -68,8 +68,6 @@
 
 #define FLASH_SIZE  0x40
 
-#define MAX_IDE_BUS 2
-
 typedef struct {
 MemoryRegion iomem;
 MemoryRegion iomem_lo; /* 0 - 0x900 */
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index ad8b75e286..2e5372bda0 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -25,7 +25,7 @@
 #include "hw/block/flash.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
-#include "hw/ide.h"
+#include "hw/ide/internal.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "hw/rtc/mc146818rtc.h"
@@ -37,8 +37,6 @@
 #include "sysemu/runstate.h"
 #include "qemu/error-report.h"
 
-#define MAX_IDE_BUS 2
-
 static const int ide_iobase[2] = { 0x1f0, 0x170 };
 static const int ide_iobase2[2] = { 0x3f6, 0x376 };
 static const int ide_irq[2] = { 14, 15 };
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b8189bf7a4..daa1523feb 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -75,7 +75,6 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-#define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf510
 #define TBFREQ (100UL * 1000UL * 1000UL)
 #define CLOCKFREQ (900UL * 1000UL * 1000UL)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 440c406eb4..2478748c78 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -51,7 +51,6 @@
 #include "kvm_ppc.h"
 #include "exec/address-spaces.h"
 
-#define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf510
 #define TBFREQ 1660UL
 #define CLOCKFREQ 26600UL
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 111cc80867..e1b1549e58 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -56,8 +56,6 @@
 /* SMP is not enabled, for now */
 #define MAX_CPUS 1
 
-#define MAX_IDE_BUS 2
-
 #define CFG_ADDR 0xf510
 
 #define KERNEL_LOAD_ADDR 0x0100
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d33e84f831..74acfd39b3 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -66,7 +66,6 @@
 #define PBM_PCI_IO_BASE  (PBM_SPECIAL_BASE + 0x0200ULL)
 #define PROM_FILENAME"openbios-sparc64"
 #define NVRAM_SIZE   0x2000
-#define MAX_IDE_BUS  2
 #define BIOS_CFG_IOPORT  0x510
 #define FW_CFG_SPARC64_WIDTH (FW_CFG_ARCH_LOCAL + 0x00)
 #define FW_CFG_SPARC64_HEIGHT (FW_CFG_ARCH_LOCAL + 0x01)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1bc1fc73e5..1a49d35959 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -27,6 +27,8 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define TYPE_IDE_BUS "IDE"
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
+#define MAX_IDE_BUS 2
+
 

Re: [PATCH 0/9] user-mode: Prune build dependencies (part 2)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200313184153.11275-1-phi...@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  LINKvhost-user-input
qapi/qapi-commands-char.o: In function `qmp_marshal_add_client':
/tmp/qemu-test/build/qapi/qapi-commands-char.c:404: undefined reference to 
`qmp_add_client'
collect2: error: ld returned 1 exit status
make: *** [qemu-storage-daemon] Error 1
make: *** Waiting for unfinished jobs
  GEN x86_64-softmmu/config-target.h
  GEN x86_64-softmmu/hmp-commands.h
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=79fd3dced38c4a66beb52a21af5ece9a', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 
'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
'/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-eu33v9p1/src/docker-src.2020-03-13-17.16.06.18800:/var/tmp/qemu:z,ro',
 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=79fd3dced38c4a66beb52a21af5ece9a
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-eu33v9p1/src'
make: *** [docker-run-test-quick@centos7] Error 2

real3m28.886s
user0m8.592s


The full log is available at
http://patchew.org/logs/20200313184153.11275-1-phi...@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH 0/9] user-mode: Prune build dependencies (part 2)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200313184153.11275-1-phi...@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  LINKqemu-img
/usr/bin/ld: qapi/qapi-commands-char.o: in function `qmp_marshal_add_client':
/tmp/qemu-test/build/qapi/qapi-commands-char.c:404: undefined reference to 
`qmp_add_client'
clang-8: error: linker command failed with exit code 1 (use -v to see 
invocation)
make: *** [/tmp/qemu-test/src/rules.mak:124: qemu-storage-daemon] Error 1
make: *** Waiting for unfinished jobs
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in 
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
'--label', 'com.qemu.instance.uuid=51e14086532c419b93bbc70aaa6062a4', '-u', 
'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 
'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', 
'-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
'/var/tmp/patchew-tester-tmp-_4qqozfd/src/docker-src.2020-03-13-17.10.10.8098:/var/tmp/qemu:z,ro',
 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit 
status 2.
filter=--filter=label=com.qemu.instance.uuid=51e14086532c419b93bbc70aaa6062a4
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_4qqozfd/src'
make: *** [docker-run-test-debug@fedora] Error 2

real4m17.682s
user0m8.190s


The full log is available at
http://patchew.org/logs/20200313184153.11275-1-phi...@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [PATCH 3/5] block: add max_pwrite_zeroes_no_fallback to BlockLimits

[Eric Blake]

On 3/2/20 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:

NBD spec is updated, so that max_block doesn't relate to


Maybe: The NBD spec was recently updated to clarify that max_block...


NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which mirrors Qemu
flag BDRV_REQ_NO_FALLBACK). To drop the restriction we need new
max_pwrite_zeroes_no_fallback.


It feels odd to have two different pwrite_zeroes limits in the block 
layer, but I can live with it if other block layer gurus are also okay 
with it.




Default value of new max_pwrite_zeroes_no_fallback is zero and it means
no-restriction, so we are automatically done by this commit. Note that


Why not have the default value be set to the existing value of the 
normal pwrite_zeroes limit, rather than 0?



nbd and blkdebug are the only drivers which in the same time define
max_pwrite_zeroes limit and support BDRV_REQ_NO_FALLBACK, so we need to
update only blkdebug.


Grammar:

The default value for the new max_pwrite_zeroes_no_fallback is zero, 
meaning no restriction, which covers all drivers not touched by this 
commit.  Note that nbd and blkdebug are the only drivers which have a 
max_pwrite_zeroes limit while supporting BDRV_REQ_NO_FALLBACK, so we 
only need to update blkdebug.


Except that I think there IS still a limit in current NBD: you can't 
request anything larger than 32 bits (whereas some other drivers may 
allow a full 63-bit request, as well as future NBD usage when we finally 
add 64-bit extensions to the protocol).  So I think this patch is 
incomplete; it should be updating the nbd code to set the proper limit.


(I still need to post v2 of my patches for bdrv_co_make_zero support, 
which is a case where knowing if there is a 32-bit limit when using 
BDRV_REQ_NO_FALLBACK for fast zeroing is important).




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block_int.h | 8 
  block/blkdebug.c  | 7 ++-
  block/io.c| 4 +++-
  3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f9fd5e20e..c167e887c6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -618,6 +618,14 @@ typedef struct BlockLimits {
   * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
  int32_t max_pwrite_zeroes;
  
+/*

+ * Maximum number of bytes that can zeroized at once if flag


zeroed


+ * BDRV_REQ_NO_FALLBACK specified (since it is signed, it must be < 2G, if
+ * set).


Why must it be a signed 32-bit number?  Why not let it be a 64-bit number?


Must be multiple of pwrite_zeroes_alignment. May be 0 if no
+ * inherent 32-bit limit.
+ */
+int32_t max_pwrite_zeroes_no_fallback;
+
  /* Optimal alignment for write zeroes requests in bytes. A power
   * of 2 is best but not mandatory.  Must be a multiple of
   * bl.request_alignment, and must be less than max_pwrite_zeroes
diff --git a/block/blkdebug.c b/block/blkdebug.c
index af44aa973f..7627fbcb3b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -692,7 +692,11 @@ static int coroutine_fn 
blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
  }
  assert(QEMU_IS_ALIGNED(offset, align));
  assert(QEMU_IS_ALIGNED(bytes, align));
-if (bs->bl.max_pwrite_zeroes) {
+if ((flags & BDRV_REQ_NO_FALLBACK) &&
+bs->bl.max_pwrite_zeroes_no_fallback)
+{
+assert(bytes <= bs->bl.max_pwrite_zeroes_no_fallback);
+} else if (bs->bl.max_pwrite_zeroes) {
  assert(bytes <= bs->bl.max_pwrite_zeroes);
  }
  
@@ -977,6 +981,7 @@ static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp)

  }
  if (s->max_write_zero) {
  bs->bl.max_pwrite_zeroes = s->max_write_zero;
+bs->bl.max_pwrite_zeroes_no_fallback = s->max_write_zero;


Ah, so you DO default it to max_pwwrite_zeroes instead of to 0; the 
commit message does not quite match the code.



  }
  if (s->opt_discard) {
  bs->bl.pdiscard_alignment = s->opt_discard;
diff --git a/block/io.c b/block/io.c
index 7e4cb74cf4..75fd5600c2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1752,7 +1752,9 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  int head = 0;
  int tail = 0;
  
-int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);

+int max_write_zeroes = MIN_NON_ZERO((flags & BDRV_REQ_NO_FALLBACK) ?
+bs->bl.max_pwrite_zeroes_no_fallback :
+bs->bl.max_pwrite_zeroes, INT_MAX);


I'd still like to get rid of this INT_MAX clamping.  If we can blank the 
entire image in one call, even when it is larger than 4G, then it is 
worth making that exposed to the user.  (Even in NBD, we might decide to 
add an extension that allows NBD_CMD_WRITE_ZEROES with a new flag and 
with offset/length == 0/0, as an official way to make the entire image 
zero, whereas it is 

Re: [PATCH 0/7] via-ide: fixes and improvements

[BALATON Zoltan]

On Fri, 13 Mar 2020, John Snow wrote:

On 3/13/20 4:24 AM, Mark Cave-Ayland wrote:

Following on from the earlier thread "Implement "non 100% native mode"
in via-ide", here is an updated patchset based upon the test cases
sent to me off-list.

The VIA IDE controller is similar to early versions of the PIIX
controller in that the primary and secondary IDE channels are hardwired
to IRQs 14 and 15 respectively. Guest OSs typically handle this by
either switching the controller to legacy mode, or using native mode and
using a combination of PCI device/vendor ID and/or checking various
registers in PCI configuration space to detect this condition and apply
a special fixed IRQ 14/15 routing.

This patchset effectively updates the VIA IDE PCI device to follow the
behaviour in the datasheet in two ways: fixing some PCI configuration
space register defaults and behaviours, and always using legacy IRQ 14/15
routing, and once applied allows all our known test images to boot
correctly.

Signed-off-by: Mark Cave-Ayland 


BALATON Zoltan (2):
  ide/via: Get rid of via_ide_init()
  pci: Honour wmask when resetting PCI_INTERRUPT_LINE

Mark Cave-Ayland (5):
  via-ide: move registration of VMStateDescription to DeviceClass
  via-ide: ensure that PCI_INTERRUPT_LINE is hard-wired to its default
value
  via-ide: initialise IDE controller in legacy mode
  via-ide: allow guests to write to PCI_CLASS_PROG
  via-ide: always use legacy IRQ 14/15 routing

 hw/ide/via.c| 21 +
 hw/mips/mips_fulong2e.c |  5 -
 hw/pci/pci.c|  5 -
 include/hw/ide.h|  1 -
 4 files changed, 13 insertions(+), 19 deletions(-)



Does this supersede everything else so far?


Yes, this includes all needed changes from my series (two patches directly 
and other changes split up in smaller commits) so none of my previous 
series is needed just this series.



(Except the two cmd646
related series, four patches total, which are already staged)


Yes those are not included here and independent changes that should stay. 
Your tree seemed to have the commits twice though at least on web 
interface of github.


I've also done some more clean ups that I'm polishing now and will submit 
soon but those are unrelated and a different series on top of this and the 
cmd646 clean up.


Regards,
BALATON Zoltan

Re: [PATCH 1/7] via-ide: move registration of VMStateDescription to DeviceClass

[Philippe Mathieu-Daudé]

On 3/13/20 9:24 AM, Mark Cave-Ayland wrote:

Signed-off-by: Mark Cave-Ayland 
---
  hw/ide/via.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 096de8dba0..84f0efff94 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -190,8 +190,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
  bmdma_setup_bar(d);
  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
  
-vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);

-
  for (i = 0; i < 2; i++) {
  ide_bus_new(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
  ide_init2(>bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
@@ -227,6 +225,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
*data)
  PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
  
  dc->reset = via_ide_reset;

+dc->vmsd = _ide_pci;
  k->realize = via_ide_realize;
  k->exit = via_ide_exitfn;
  k->vendor_id = PCI_VENDOR_ID_VIA;



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 3/3] xen-block: Use one Error * variable instead of two

[Philippe Mathieu-Daudé]

On 3/13/20 6:05 PM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
---
  hw/block/xen-block.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3885464513..7b3b6dee97 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -998,14 +998,13 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
  XenBlockVdev *vdev = >props.vdev;
  XenBlockDrive *drive = blockdev->drive;
  XenBlockIOThread *iothread = blockdev->iothread;
+Error *local_err = NULL;
  
  trace_xen_block_device_destroy(vdev->number);
  
  object_unparent(OBJECT(xendev));
  
  if (iothread) {

-Error *local_err = NULL;
-
  xen_block_iothread_destroy(iothread, _err);
  if (local_err) {
  error_propagate_prepend(errp, local_err,
@@ -1015,8 +1014,6 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
  }
  
  if (drive) {

-Error *local_err = NULL;
-
  xen_block_drive_destroy(drive, _err);
  if (local_err) {
  error_propagate_prepend(errp, local_err,



Reviewed-by: Philippe Mathieu-Daudé 

[PATCH 9/9] qapi/misc: Restrict device memory commands to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json   | 131 +++
 qapi/misc.json  | 132 
 include/hw/mem/memory-device.h  |   1 +
 include/hw/virtio/virtio-pmem.h |   2 +-
 4 files changed, 133 insertions(+), 133 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 33b259dbd0..17ccebda14 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1476,3 +1476,134 @@
 #
 ##
 { 'command': 'query-pci', 'returns': ['PciInfo'] }
+
+##
+# @MemoryInfo:
+#
+# Actual memory information in bytes.
+#
+# @base-memory: size of "base" memory specified with command line
+#   option -m.
+#
+# @plugged-memory: size of memory that can be hot-unplugged. This field
+#  is omitted if target doesn't support memory hotplug
+#  (i.e. CONFIG_MEM_DEVICE not defined at build time).
+#
+# Since: 2.11.0
+##
+{ 'struct': 'MemoryInfo',
+  'data'  : { 'base-memory': 'size', '*plugged-memory': 'size' } }
+
+##
+# @query-memory-size-summary:
+#
+# Return the amount of initially allocated and present hotpluggable (if
+# enabled) memory in bytes.
+#
+# Example:
+#
+# -> { "execute": "query-memory-size-summary" }
+# <- { "return": { "base-memory": 4294967296, "plugged-memory": 0 } }
+#
+# Since: 2.11.0
+##
+{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
+
+##
+# @PCDIMMDeviceInfo:
+#
+# PCDIMMDevice state information
+#
+# @id: device's ID
+#
+# @addr: physical address, where device is mapped
+#
+# @size: size of memory that the device provides
+#
+# @slot: slot number at which device is plugged in
+#
+# @node: NUMA node number where device is plugged in
+#
+# @memdev: memory backend linked with device
+#
+# @hotplugged: true if device was hotplugged
+#
+# @hotpluggable: true if device if could be added/removed while machine is 
running
+#
+# Since: 2.1
+##
+{ 'struct': 'PCDIMMDeviceInfo',
+  'data': { '*id': 'str',
+'addr': 'int',
+'size': 'int',
+'slot': 'int',
+'node': 'int',
+'memdev': 'str',
+'hotplugged': 'bool',
+'hotpluggable': 'bool'
+  }
+}
+
+##
+# @VirtioPMEMDeviceInfo:
+#
+# VirtioPMEM state information
+#
+# @id: device's ID
+#
+# @memaddr: physical address in memory, where device is mapped
+#
+# @size: size of memory that the device provides
+#
+# @memdev: memory backend linked with device
+#
+# Since: 4.1
+##
+{ 'struct': 'VirtioPMEMDeviceInfo',
+  'data': { '*id': 'str',
+'memaddr': 'size',
+'size': 'size',
+'memdev': 'str'
+  }
+}
+
+##
+# @MemoryDeviceInfo:
+#
+# Union containing information about a memory device
+#
+# nvdimm is included since 2.12. virtio-pmem is included since 4.1.
+#
+# Since: 2.1
+##
+{ 'union': 'MemoryDeviceInfo',
+  'data': { 'dimm': 'PCDIMMDeviceInfo',
+'nvdimm': 'PCDIMMDeviceInfo',
+'virtio-pmem': 'VirtioPMEMDeviceInfo'
+  }
+}
+
+##
+# @query-memory-devices:
+#
+# Lists available memory devices and their state
+#
+# Since: 2.1
+#
+# Example:
+#
+# -> { "execute": "query-memory-devices" }
+# <- { "return": [ { "data":
+#   { "addr": 5368709120,
+# "hotpluggable": true,
+# "hotplugged": true,
+# "id": "d1",
+# "memdev": "/objects/memX",
+# "node": 0,
+# "size": 1073741824,
+# "slot": 0},
+#"type": "dimm"
+#  } ] }
+#
+##
+{ 'command': 'query-memory-devices', 'returns': ['MemoryDeviceInfo'] }
diff --git a/qapi/misc.json b/qapi/misc.json
index 699a533e6c..48a0c5410b 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -486,39 +486,6 @@
 ##
 { 'command': 'closefd', 'data': {'fdname': 'str'} }
 
-##
-# @MemoryInfo:
-#
-# Actual memory information in bytes.
-#
-# @base-memory: size of "base" memory specified with command line
-#   option -m.
-#
-# @plugged-memory: size of memory that can be hot-unplugged. This field
-#  is omitted if target doesn't support memory hotplug
-#  (i.e. CONFIG_MEM_DEVICE not defined at build time).
-#
-# Since: 2.11.0
-##
-{ 'struct': 'MemoryInfo',
-  'data'  : { 'base-memory': 'size', '*plugged-memory': 'size' } }
-
-##
-# @query-memory-size-summary:
-#
-# Return the amount of initially allocated and present hotpluggable (if
-# enabled) memory in bytes.
-#
-# Example:
-#
-# -> { "execute": "query-memory-size-summary" }
-# <- { "return": { "base-memory": 4294967296, "plugged-memory": 0 } }
-#
-# Since: 2.11.0
-##
-{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
-
-
 ##
 # @AddfdInfo:
 #
@@ -756,105 +723,6 @@
  'returns': ['CommandLineOptionInfo'],
  'allow-preconfig': true }
 
-##
-# @PCDIMMDeviceInfo:
-#
-# PCDIMMDevice state information
-#
-# @id: 

[PATCH 7/9] qapi/misc: Restrict ACPI commands to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json| 154 +++
 qapi/misc.json   | 154 ---
 include/hw/acpi/acpi_dev_interface.h |   2 +-
 hw/acpi/core.c   |   2 +-
 hw/acpi/cpu.c|   2 +-
 hw/acpi/memory_hotplug.c |   2 +-
 6 files changed, 158 insertions(+), 158 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 1a2a4b0d48..f77ee63730 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1018,3 +1018,157 @@
 ##
 { 'event': 'BALLOON_CHANGE',
   'data': { 'actual': 'int' } }
+
+##
+# @AcpiTableOptions:
+#
+# Specify an ACPI table on the command line to load.
+#
+# At most one of @file and @data can be specified. The list of files specified
+# by any one of them is loaded and concatenated in order. If both are omitted,
+# @data is implied.
+#
+# Other fields / optargs can be used to override fields of the generic ACPI
+# table header; refer to the ACPI specification 5.0, section 5.2.6 System
+# Description Table Header. If a header field is not overridden, then the
+# corresponding value from the concatenated blob is used (in case of @file), or
+# it is filled in with a hard-coded value (in case of @data).
+#
+# String fields are copied into the matching ACPI member from lowest address
+# upwards, and silently truncated / NUL-padded to length.
+#
+# @sig: table signature / identifier (4 bytes)
+#
+# @rev: table revision number (dependent on signature, 1 byte)
+#
+# @oem_id: OEM identifier (6 bytes)
+#
+# @oem_table_id: OEM table identifier (8 bytes)
+#
+# @oem_rev: OEM-supplied revision number (4 bytes)
+#
+# @asl_compiler_id: identifier of the utility that created the table
+#   (4 bytes)
+#
+# @asl_compiler_rev: revision number of the utility that created the
+#table (4 bytes)
+#
+# @file: colon (:) separated list of pathnames to load and
+#concatenate as table data. The resultant binary blob is expected to
+#have an ACPI table header. At least one file is required. This field
+#excludes @data.
+#
+# @data: colon (:) separated list of pathnames to load and
+#concatenate as table data. The resultant binary blob must not have an
+#ACPI table header. At least one file is required. This field excludes
+#@file.
+#
+# Since: 1.5
+##
+{ 'struct': 'AcpiTableOptions',
+  'data': {
+'*sig':   'str',
+'*rev':   'uint8',
+'*oem_id':'str',
+'*oem_table_id':  'str',
+'*oem_rev':   'uint32',
+'*asl_compiler_id':   'str',
+'*asl_compiler_rev':  'uint32',
+'*file':  'str',
+'*data':  'str' }}
+
+##
+# @MEM_UNPLUG_ERROR:
+#
+# Emitted when memory hot unplug error occurs.
+#
+# @device: device name
+#
+# @msg: Informative message
+#
+# Since: 2.4
+#
+# Example:
+#
+# <- { "event": "MEM_UNPLUG_ERROR"
+#  "data": { "device": "dimm1",
+#"msg": "acpi: device unplug for unsupported device"
+#  },
+#  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'MEM_UNPLUG_ERROR',
+  'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @ACPISlotType:
+#
+# @DIMM: memory slot
+# @CPU: logical CPU slot (since 2.7)
+##
+{ 'enum': 'ACPISlotType', 'data': [ 'DIMM', 'CPU' ] }
+
+##
+# @ACPIOSTInfo:
+#
+# OSPM Status Indication for a device
+# For description of possible values of @source and @status fields
+# see "_OST (OSPM Status Indication)" chapter of ACPI5.0 spec.
+#
+# @device: device ID associated with slot
+#
+# @slot: slot ID, unique per slot of a given @slot-type
+#
+# @slot-type: type of the slot
+#
+# @source: an integer containing the source event
+#
+# @status: an integer containing the status code
+#
+# Since: 2.1
+##
+{ 'struct': 'ACPIOSTInfo',
+  'data'  : { '*device': 'str',
+  'slot': 'str',
+  'slot-type': 'ACPISlotType',
+  'source': 'int',
+  'status': 'int' } }
+
+##
+# @query-acpi-ospm-status:
+#
+# Return a list of ACPIOSTInfo for devices that support status
+# reporting via ACPI _OST method.
+#
+# Since: 2.1
+#
+# Example:
+#
+# -> { "execute": "query-acpi-ospm-status" }
+# <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM", 
"source": 1, "status": 0},
+#  { "slot": "1", "slot-type": "DIMM", "source": 0, "status": 
0},
+#  { "slot": "2", "slot-type": "DIMM", "source": 0, "status": 
0},
+#  { "slot": "3", "slot-type": "DIMM", "source": 0, "status": 
0}
+#]}
+#
+##
+{ 'command': 'query-acpi-ospm-status', 'returns': ['ACPIOSTInfo'] }
+
+##
+# @ACPI_DEVICE_OST:
+#
+# Emitted when guest executes ACPI _OST method.
+#
+# @info: OSPM Status Indication
+#
+# Since: 2.1
+#
+# Example:
+#
+# <- { "event": "ACPI_DEVICE_OST",
+#  "data": { "device": "d1", "slot": "0",
+#

[PATCH 6/9] qapi/misc: Restrict query-vm-generation-id command to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json | 20 
 qapi/misc.json| 21 -
 hw/acpi/vmgenid.c |  2 +-
 stubs/vmgenid.c   |  2 +-
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index c096efbea3..1a2a4b0d48 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -415,6 +415,26 @@
 ##
 { 'command': 'query-target', 'returns': 'TargetInfo' }
 
+##
+# @GuidInfo:
+#
+# GUID information.
+#
+# @guid: the globally unique identifier
+#
+# Since: 2.9
+##
+{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
+
+##
+# @query-vm-generation-id:
+#
+# Show Virtual Machine Generation ID
+#
+# Since: 2.9
+##
+{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
+
 ##
 # @LostTickPolicy:
 #
diff --git a/qapi/misc.json b/qapi/misc.json
index 18cd0719f3..8e0902caf4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1351,24 +1351,3 @@
 #
 ##
 { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
-
-##
-# @GuidInfo:
-#
-# GUID information.
-#
-# @guid: the globally unique identifier
-#
-# Since: 2.9
-##
-{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
-
-##
-# @query-vm-generation-id:
-#
-# Show Virtual Machine Generation ID
-#
-# Since: 2.9
-##
-{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
-
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 2df7623d74..2b26bacaf8 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qemu/module.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
index 568e42b064..bfad656c6c 100644
--- a/stubs/vmgenid.c
+++ b/stubs/vmgenid.c
@@ -1,6 +1,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/qmp/qerror.h"
 
 GuidInfo *qmp_query_vm_generation_id(Error **errp)
-- 
2.21.1

[PATCH 8/9] qapi/misc: Restrict PCI commands to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json | 304 ++
 qapi/misc.json| 304 --
 hw/pci/pci-stub.c |   2 +-
 hw/pci/pci.c  |   2 +-
 4 files changed, 306 insertions(+), 306 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index f77ee63730..33b259dbd0 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1172,3 +1172,307 @@
 ##
 { 'event': 'ACPI_DEVICE_OST',
  'data': { 'info': 'ACPIOSTInfo' } }
+
+##
+# @PciMemoryRange:
+#
+# A PCI device memory region
+#
+# @base: the starting address (guest physical)
+#
+# @limit: the ending address (guest physical)
+#
+# Since: 0.14.0
+##
+{ 'struct': 'PciMemoryRange', 'data': {'base': 'int', 'limit': 'int'} }
+
+##
+# @PciMemoryRegion:
+#
+# Information about a PCI device I/O region.
+#
+# @bar: the index of the Base Address Register for this region
+#
+# @type: - 'io' if the region is a PIO region
+#- 'memory' if the region is a MMIO region
+#
+# @size: memory size
+#
+# @prefetch: if @type is 'memory', true if the memory is prefetchable
+#
+# @mem_type_64: if @type is 'memory', true if the BAR is 64-bit
+#
+# Since: 0.14.0
+##
+{ 'struct': 'PciMemoryRegion',
+  'data': {'bar': 'int', 'type': 'str', 'address': 'int', 'size': 'int',
+   '*prefetch': 'bool', '*mem_type_64': 'bool' } }
+
+##
+# @PciBusInfo:
+#
+# Information about a bus of a PCI Bridge device
+#
+# @number: primary bus interface number.  This should be the number of the
+#  bus the device resides on.
+#
+# @secondary: secondary bus interface number.  This is the number of the
+# main bus for the bridge
+#
+# @subordinate: This is the highest number bus that resides below the
+#   bridge.
+#
+# @io_range: The PIO range for all devices on this bridge
+#
+# @memory_range: The MMIO range for all devices on this bridge
+#
+# @prefetchable_range: The range of prefetchable MMIO for all devices on
+#  this bridge
+#
+# Since: 2.4
+##
+{ 'struct': 'PciBusInfo',
+  'data': {'number': 'int', 'secondary': 'int', 'subordinate': 'int',
+   'io_range': 'PciMemoryRange',
+   'memory_range': 'PciMemoryRange',
+   'prefetchable_range': 'PciMemoryRange' } }
+
+##
+# @PciBridgeInfo:
+#
+# Information about a PCI Bridge device
+#
+# @bus: information about the bus the device resides on
+#
+# @devices: a list of @PciDeviceInfo for each device on this bridge
+#
+# Since: 0.14.0
+##
+{ 'struct': 'PciBridgeInfo',
+  'data': {'bus': 'PciBusInfo', '*devices': ['PciDeviceInfo']} }
+
+##
+# @PciDeviceClass:
+#
+# Information about the Class of a PCI device
+#
+# @desc: a string description of the device's class
+#
+# @class: the class code of the device
+#
+# Since: 2.4
+##
+{ 'struct': 'PciDeviceClass',
+  'data': {'*desc': 'str', 'class': 'int'} }
+
+##
+# @PciDeviceId:
+#
+# Information about the Id of a PCI device
+#
+# @device: the PCI device id
+#
+# @vendor: the PCI vendor id
+#
+# @subsystem: the PCI subsystem id (since 3.1)
+#
+# @subsystem-vendor: the PCI subsystem vendor id (since 3.1)
+#
+# Since: 2.4
+##
+{ 'struct': 'PciDeviceId',
+  'data': {'device': 'int', 'vendor': 'int', '*subsystem': 'int',
+'*subsystem-vendor': 'int'} }
+
+##
+# @PciDeviceInfo:
+#
+# Information about a PCI device
+#
+# @bus: the bus number of the device
+#
+# @slot: the slot the device is located in
+#
+# @function: the function of the slot used by the device
+#
+# @class_info: the class of the device
+#
+# @id: the PCI device id
+#
+# @irq: if an IRQ is assigned to the device, the IRQ number
+#
+# @qdev_id: the device name of the PCI device
+#
+# @pci_bridge: if the device is a PCI bridge, the bridge information
+#
+# @regions: a list of the PCI I/O regions associated with the device
+#
+# Notes: the contents of @class_info.desc are not stable and should only be
+#treated as informational.
+#
+# Since: 0.14.0
+##
+{ 'struct': 'PciDeviceInfo',
+  'data': {'bus': 'int', 'slot': 'int', 'function': 'int',
+   'class_info': 'PciDeviceClass', 'id': 'PciDeviceId',
+   '*irq': 'int', 'qdev_id': 'str', '*pci_bridge': 'PciBridgeInfo',
+   'regions': ['PciMemoryRegion']} }
+
+##
+# @PciInfo:
+#
+# Information about a PCI bus
+#
+# @bus: the bus index
+#
+# @devices: a list of devices on this bus
+#
+# Since: 0.14.0
+##
+{ 'struct': 'PciInfo', 'data': {'bus': 'int', 'devices': ['PciDeviceInfo']} }
+
+##
+# @query-pci:
+#
+# Return information about the PCI bus topology of the guest.
+#
+# Returns: a list of @PciInfo for each PCI bus. Each bus is
+#  represented by a json-object, which has a key with a json-array of
+#  all PCI devices attached to it. Each device is represented by a
+#  json-object.
+#
+# Since: 0.14.0
+#
+# Example:
+#
+# -> { "execute": "query-pci" }
+# <- { "return": [
+#  {
+# "bus": 0,
+# "devices": [
+# 

[PATCH 5/9] qapi/misc: Move query-uuid command with block code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/block-core.json | 30 ++
 qapi/misc.json   | 30 --
 block/iscsi.c|  2 +-
 stubs/uuid.c |  2 +-
 4 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 91586fb1fb..5c3fa6c5d0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5415,3 +5415,33 @@
 { 'command': 'blockdev-snapshot-delete-internal-sync',
   'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
   'returns': 'SnapshotInfo' }
+
+##
+# @UuidInfo:
+#
+# Guest UUID information (Universally Unique Identifier).
+#
+# @UUID: the UUID of the guest
+#
+# Since: 0.14.0
+#
+# Notes: If no UUID was specified for the guest, a null UUID is returned.
+##
+{ 'struct': 'UuidInfo', 'data': {'UUID': 'str'} }
+
+##
+# @query-uuid:
+#
+# Query the guest UUID information.
+#
+# Returns: The @UuidInfo for the guest
+#
+# Since: 0.14.0
+#
+# Example:
+#
+# -> { "execute": "query-uuid" }
+# <- { "return": { "UUID": "550e8400-e29b-41d4-a716-44665544" } }
+#
+##
+{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
diff --git a/qapi/misc.json b/qapi/misc.json
index 74cbe253f2..18cd0719f3 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -65,36 +65,6 @@
 ##
 { 'command': 'query-kvm', 'returns': 'KvmInfo' }
 
-##
-# @UuidInfo:
-#
-# Guest UUID information (Universally Unique Identifier).
-#
-# @UUID: the UUID of the guest
-#
-# Since: 0.14.0
-#
-# Notes: If no UUID was specified for the guest, a null UUID is returned.
-##
-{ 'struct': 'UuidInfo', 'data': {'UUID': 'str'} }
-
-##
-# @query-uuid:
-#
-# Query the guest UUID information.
-#
-# Returns: The @UuidInfo for the guest
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "query-uuid" }
-# <- { "return": { "UUID": "550e8400-e29b-41d4-a716-44665544" } }
-#
-##
-{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
-
 ##
 # @IOThreadInfo:
 #
diff --git a/block/iscsi.c b/block/iscsi.c
index 682abd8e09..68ed5cf3f8 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -42,7 +42,7 @@
 #include "qemu/uuid.h"
 #include "sysemu/replay.h"
 #include "qapi/error.h"
-#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-block-core.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
 #include "crypto/secret.h"
diff --git a/stubs/uuid.c b/stubs/uuid.c
index 67f182fa3a..9ef75fdae4 100644
--- a/stubs/uuid.c
+++ b/stubs/uuid.c
@@ -1,5 +1,5 @@
 #include "qemu/osdep.h"
-#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-block-core.h"
 #include "qemu/uuid.h"
 
 UuidInfo *qmp_query_uuid(Error **errp)
-- 
2.21.1

[PATCH 1/9] target/i386: Restrict X86CPUFeatureWord to X86 targets

[Philippe Mathieu-Daudé]

Move out x86-specific structures from generic machine code.

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine-target.json   | 45 ++
 qapi/machine.json  | 42 ---
 target/i386/cpu.c  |  2 +-
 target/i386/machine-stub.c | 22 +++
 target/i386/Makefile.objs  |  3 ++-
 5 files changed, 70 insertions(+), 44 deletions(-)
 create mode 100644 target/i386/machine-stub.c

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index f2c82949d8..fb7a4b7850 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -3,6 +3,51 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or later.
 # See the COPYING file in the top-level directory.
 
+##
+# @X86CPURegister32:
+#
+# A X86 32-bit register
+#
+# Since: 1.5
+##
+{ 'enum': 'X86CPURegister32',
+  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ],
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @X86CPUFeatureWordInfo:
+#
+# Information about a X86 CPU feature word
+#
+# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word
+#
+# @cpuid-input-ecx: Input ECX value for CPUID instruction for that
+#   feature word
+#
+# @cpuid-register: Output register containing the feature bits
+#
+# @features: value of output register, containing the feature bits
+#
+# Since: 1.5
+##
+{ 'struct': 'X86CPUFeatureWordInfo',
+  'data': { 'cpuid-input-eax': 'int',
+'*cpuid-input-ecx': 'int',
+'cpuid-register': 'X86CPURegister32',
+'features': 'int' },
+  'if': 'defined(TARGET_I386)' }
+
+##
+# @DummyForceArrays:
+#
+# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
+#
+# Since: 2.5
+##
+{ 'struct': 'DummyForceArrays',
+  'data': { 'unused': ['X86CPUFeatureWordInfo'] },
+  'if': 'defined(TARGET_I386)' }
+
 ##
 # @CpuModelInfo:
 #
diff --git a/qapi/machine.json b/qapi/machine.json
index 6c11e3cf3a..de05730704 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -505,48 +505,6 @@
'dst': 'uint16',
'val': 'uint8' }}
 
-##
-# @X86CPURegister32:
-#
-# A X86 32-bit register
-#
-# Since: 1.5
-##
-{ 'enum': 'X86CPURegister32',
-  'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] }
-
-##
-# @X86CPUFeatureWordInfo:
-#
-# Information about a X86 CPU feature word
-#
-# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word
-#
-# @cpuid-input-ecx: Input ECX value for CPUID instruction for that
-#   feature word
-#
-# @cpuid-register: Output register containing the feature bits
-#
-# @features: value of output register, containing the feature bits
-#
-# Since: 1.5
-##
-{ 'struct': 'X86CPUFeatureWordInfo',
-  'data': { 'cpuid-input-eax': 'int',
-'*cpuid-input-ecx': 'int',
-'cpuid-register': 'X86CPURegister32',
-'features': 'int' } }
-
-##
-# @DummyForceArrays:
-#
-# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
-#
-# Since: 2.5
-##
-{ 'struct': 'DummyForceArrays',
-  'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
-
 ##
 # @NumaCpuOptions:
 #
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a84553e50c..0753fe4935 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -37,7 +37,7 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qapi/error.h"
-#include "qapi/qapi-visit-machine.h"
+#include "qapi/qapi-visit-machine-target.h"
 #include "qapi/qapi-visit-run-state.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
diff --git a/target/i386/machine-stub.c b/target/i386/machine-stub.c
new file mode 100644
index 00..cb301af057
--- /dev/null
+++ b/target/i386/machine-stub.c
@@ -0,0 +1,22 @@
+/*
+ * QAPI x86 CPU features stub
+ *
+ * Copyright (c) 2020 Red Hat, Inc.
+ *
+ * Author:
+ *   Philippe Mathieu-Daudé 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-visit-machine-target.h"
+
+void visit_type_X86CPUFeatureWordInfoList(Visitor *v, const char *name,
+  X86CPUFeatureWordInfoList **obj,
+  Error **errp)
+{
+}
diff --git a/target/i386/Makefile.objs b/target/i386/Makefile.objs
index 48e0c28434..1cdfc9f50c 100644
--- a/target/i386/Makefile.objs
+++ b/target/i386/Makefile.objs
@@ -17,6 +17,7 @@ obj-$(CONFIG_HAX) += hax-all.o hax-mem.o hax-posix.o
 endif
 obj-$(CONFIG_HVF) += hvf/
 obj-$(CONFIG_WHPX) += whpx-all.o
-endif
+endif # CONFIG_SOFTMMU
 obj-$(CONFIG_SEV) += sev.o
 obj-$(call lnot,$(CONFIG_SEV)) += sev-stub.o
+obj-$(call lnot,$(CONFIG_SOFTMMU)) += machine-stub.o
-- 
2.21.1

[PATCH 2/9] qapi/misc: Move add_client command with chardev code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/char.json | 32 
 qapi/misc.json | 32 
 monitor/qmp-cmds.c |  1 +
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/qapi/char.json b/qapi/char.json
index 6907b2bfdb..8b7baf11eb 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -572,3 +572,35 @@
 { 'event': 'VSERPORT_CHANGE',
   'data': { 'id': 'str',
 'open': 'bool' } }
+
+##
+# @add_client:
+#
+# Allow client connections for VNC, Spice and socket based
+# character devices to be passed in to QEMU via SCM_RIGHTS.
+#
+# @protocol: protocol name. Valid names are "vnc", "spice" or the
+#name of a character device (eg. from -chardev id=)
+#
+# @fdname: file descriptor name previously passed via 'getfd' command
+#
+# @skipauth: whether to skip authentication. Only applies
+#to "vnc" and "spice" protocols
+#
+# @tls: whether to perform TLS. Only applies to the "spice"
+#   protocol
+#
+# Returns: nothing on success.
+#
+# Since: 0.14.0
+#
+# Example:
+#
+# -> { "execute": "add_client", "arguments": { "protocol": "vnc",
+#  "fdname": "myclient" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'add_client',
+  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
+'*tls': 'bool' } }
diff --git a/qapi/misc.json b/qapi/misc.json
index c18fe681fb..e84e6823e9 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -39,38 +39,6 @@
 { 'enum': 'LostTickPolicy',
   'data': ['discard', 'delay', 'slew' ] }
 
-##
-# @add_client:
-#
-# Allow client connections for VNC, Spice and socket based
-# character devices to be passed in to QEMU via SCM_RIGHTS.
-#
-# @protocol: protocol name. Valid names are "vnc", "spice" or the
-#name of a character device (eg. from -chardev id=)
-#
-# @fdname: file descriptor name previously passed via 'getfd' command
-#
-# @skipauth: whether to skip authentication. Only applies
-#to "vnc" and "spice" protocols
-#
-# @tls: whether to perform TLS. Only applies to the "spice"
-#   protocol
-#
-# Returns: nothing on success.
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "add_client", "arguments": { "protocol": "vnc",
-#  "fdname": "myclient" } }
-# <- { "return": {} }
-#
-##
-{ 'command': 'add_client',
-  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
-'*tls': 'bool' } }
-
 ##
 # @NameInfo:
 #
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 864cbfa32e..67d95b4af7 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -31,6 +31,7 @@
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-block.h"
+#include "qapi/qapi-commands-char.h"
 #include "qapi/qapi-commands-control.h"
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc.h"
-- 
2.21.1

[PATCH 0/9] user-mode: Prune build dependencies (part 2)

[Philippe Mathieu-Daudé]

This is the second part of a series reducing user-mode
dependencies. By stripping out unused code, the build
and testing time is reduced (as is space used by objects).

Part 2:
- Extract code not related to user-mode from qapi/misc.json

Based-on: <20200313183652.10258-1-phi...@redhat.com>

Philippe Mathieu-Daudé (9):
  target/i386: Restrict X86CPUFeatureWord to X86 targets
  qapi/misc: Move add_client command with chardev code
  qapi/misc: Restrict LostTickPolicy enum to machine code
  qapi/misc: Restrict balloon-related commands to machine code
  qapi/misc: Move query-uuid command with block code
  qapi/misc: Restrict query-vm-generation-id command to machine code
  qapi/misc: Restrict ACPI commands to machine code
  qapi/misc: Restrict PCI commands to machine code
  qapi/misc: Restrict device memory commands to machine code

 qapi/block-core.json |  30 +
 qapi/char.json   |  32 ++
 qapi/machine-target.json |  45 ++
 qapi/machine.json| 766 --
 qapi/misc.json   | 788 ---
 include/hw/acpi/acpi_dev_interface.h |   2 +-
 include/hw/mem/memory-device.h   |   1 +
 include/hw/rtc/mc146818rtc.h |   2 +-
 include/hw/virtio/virtio-pmem.h  |   2 +-
 include/sysemu/balloon.h |   2 +-
 balloon.c|   2 +-
 block/iscsi.c|   2 +-
 hw/acpi/core.c   |   2 +-
 hw/acpi/cpu.c|   2 +-
 hw/acpi/memory_hotplug.c |   2 +-
 hw/acpi/vmgenid.c|   2 +-
 hw/core/qdev-properties.c|   1 +
 hw/i386/kvm/i8254.c  |   2 +-
 hw/pci/pci-stub.c|   2 +-
 hw/pci/pci.c |   2 +-
 hw/virtio/virtio-balloon.c   |   2 +-
 monitor/hmp-cmds.c   |   1 +
 monitor/qmp-cmds.c   |   1 +
 stubs/uuid.c |   2 +-
 stubs/vmgenid.c  |   2 +-
 target/i386/cpu.c|   2 +-
 target/i386/machine-stub.c   |  22 +
 target/i386/Makefile.objs|   3 +-
 28 files changed, 876 insertions(+), 848 deletions(-)
 create mode 100644 target/i386/machine-stub.c

-- 
2.21.1

[PATCH 4/9] qapi/misc: Restrict balloon-related commands to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json  | 83 ++
 qapi/misc.json | 83 --
 include/sysemu/balloon.h   |  2 +-
 balloon.c  |  2 +-
 hw/virtio/virtio-balloon.c |  2 +-
 monitor/hmp-cmds.c |  1 +
 6 files changed, 87 insertions(+), 86 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 07ffc07ba2..c096efbea3 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -915,3 +915,86 @@
   'data': 'NumaOptions',
   'allow-preconfig': true
 }
+
+##
+# @balloon:
+#
+# Request the balloon driver to change its balloon size.
+#
+# @value: the target size of the balloon in bytes
+#
+# Returns: - Nothing on success
+#  - If the balloon driver is enabled but not functional because the 
KVM
+#kernel module cannot support it, KvmMissingCap
+#  - If no balloon device is present, DeviceNotActive
+#
+# Notes: This command just issues a request to the guest.  When it returns,
+#the balloon size may not have changed.  A guest can change the balloon
+#size independent of this command.
+#
+# Since: 0.14.0
+#
+# Example:
+#
+# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'balloon', 'data': {'value': 'int'} }
+
+##
+# @BalloonInfo:
+#
+# Information about the guest balloon device.
+#
+# @actual: the number of bytes the balloon currently contains
+#
+# Since: 0.14.0
+#
+##
+{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
+
+##
+# @query-balloon:
+#
+# Return information about the balloon device.
+#
+# Returns: - @BalloonInfo on success
+#  - If the balloon driver is enabled but not functional because the 
KVM
+#kernel module cannot support it, KvmMissingCap
+#  - If no balloon device is present, DeviceNotActive
+#
+# Since: 0.14.0
+#
+# Example:
+#
+# -> { "execute": "query-balloon" }
+# <- { "return": {
+#  "actual": 1073741824,
+#   }
+#}
+#
+##
+{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
+
+##
+# @BALLOON_CHANGE:
+#
+# Emitted when the guest changes the actual BALLOON level. This value is
+# equivalent to the @actual field return by the 'query-balloon' command
+#
+# @actual: actual level of the guest memory balloon in bytes
+#
+# Note: this event is rate-limited.
+#
+# Since: 1.2
+#
+# Example:
+#
+# <- { "event": "BALLOON_CHANGE",
+#  "data": { "actual": 944766976 },
+#  "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
+#
+##
+{ 'event': 'BALLOON_CHANGE',
+  'data': { 'actual': 'int' } }
diff --git a/qapi/misc.json b/qapi/misc.json
index f827bdc8f6..74cbe253f2 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -154,63 +154,6 @@
 { 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
   'allow-preconfig': true }
 
-##
-# @BalloonInfo:
-#
-# Information about the guest balloon device.
-#
-# @actual: the number of bytes the balloon currently contains
-#
-# Since: 0.14.0
-#
-##
-{ 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
-
-##
-# @query-balloon:
-#
-# Return information about the balloon device.
-#
-# Returns: - @BalloonInfo on success
-#  - If the balloon driver is enabled but not functional because the 
KVM
-#kernel module cannot support it, KvmMissingCap
-#  - If no balloon device is present, DeviceNotActive
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "query-balloon" }
-# <- { "return": {
-#  "actual": 1073741824,
-#   }
-#}
-#
-##
-{ 'command': 'query-balloon', 'returns': 'BalloonInfo' }
-
-##
-# @BALLOON_CHANGE:
-#
-# Emitted when the guest changes the actual BALLOON level. This value is
-# equivalent to the @actual field return by the 'query-balloon' command
-#
-# @actual: actual level of the guest memory balloon in bytes
-#
-# Note: this event is rate-limited.
-#
-# Since: 1.2
-#
-# Example:
-#
-# <- { "event": "BALLOON_CHANGE",
-#  "data": { "actual": 944766976 },
-#  "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
-#
-##
-{ 'event': 'BALLOON_CHANGE',
-  'data': { 'actual': 'int' } }
-
 ##
 # @PciMemoryRange:
 #
@@ -719,32 +662,6 @@
 ##
 { 'command': 'inject-nmi' }
 
-##
-# @balloon:
-#
-# Request the balloon driver to change its balloon size.
-#
-# @value: the target size of the balloon in bytes
-#
-# Returns: - Nothing on success
-#  - If the balloon driver is enabled but not functional because the 
KVM
-#kernel module cannot support it, KvmMissingCap
-#  - If no balloon device is present, DeviceNotActive
-#
-# Notes: This command just issues a request to the guest.  When it returns,
-#the balloon size may not have changed.  A guest can change the balloon
-#size independent of this command.
-#
-# Since: 0.14.0
-#
-# Example:
-#
-# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
-# <- { "return": {} }
-#
-##
-{ 

[PATCH 3/9] qapi/misc: Restrict LostTickPolicy enum to machine code

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json| 32 
 qapi/misc.json   | 32 
 include/hw/rtc/mc146818rtc.h |  2 +-
 hw/core/qdev-properties.c|  1 +
 hw/i386/kvm/i8254.c  |  2 +-
 5 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index de05730704..07ffc07ba2 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -415,6 +415,38 @@
 ##
 { 'command': 'query-target', 'returns': 'TargetInfo' }
 
+##
+# @LostTickPolicy:
+#
+# Policy for handling lost ticks in timer devices.  Ticks end up getting
+# lost when, for example, the guest is paused.
+#
+# @discard: throw away the missed ticks and continue with future injection
+#   normally.  The guest OS will see the timer jump ahead by a
+#   potentially quite significant amount all at once, as if the
+#   intervening chunk of time had simply not existed; needless to
+#   say, such a sudden jump can easily confuse a guest OS which is
+#   not specifically prepared to deal with it.  Assuming the guest
+#   OS can deal correctly with the time jump, the time in the guest
+#   and in the host should now match.
+#
+# @delay: continue to deliver ticks at the normal rate.  The guest OS will
+# not notice anything is amiss, as from its point of view time will
+# have continued to flow normally.  The time in the guest should now
+# be behind the time in the host by exactly the amount of time during
+# which ticks have been missed.
+#
+# @slew: deliver ticks at a higher rate to catch up with the missed ticks.
+#The guest OS will not notice anything is amiss, as from its point
+#of view time will have continued to flow normally.  Once the timer
+#has managed to catch up with all the missing ticks, the time in
+#the guest and in the host should match.
+#
+# Since: 2.0
+##
+{ 'enum': 'LostTickPolicy',
+  'data': ['discard', 'delay', 'slew' ] }
+
 ##
 # @NumaOptionsType:
 #
diff --git a/qapi/misc.json b/qapi/misc.json
index e84e6823e9..f827bdc8f6 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -7,38 +7,6 @@
 
 { 'include': 'common.json' }
 
-##
-# @LostTickPolicy:
-#
-# Policy for handling lost ticks in timer devices.  Ticks end up getting
-# lost when, for example, the guest is paused.
-#
-# @discard: throw away the missed ticks and continue with future injection
-#   normally.  The guest OS will see the timer jump ahead by a
-#   potentially quite significant amount all at once, as if the
-#   intervening chunk of time had simply not existed; needless to
-#   say, such a sudden jump can easily confuse a guest OS which is
-#   not specifically prepared to deal with it.  Assuming the guest
-#   OS can deal correctly with the time jump, the time in the guest
-#   and in the host should now match.
-#
-# @delay: continue to deliver ticks at the normal rate.  The guest OS will
-# not notice anything is amiss, as from its point of view time will
-# have continued to flow normally.  The time in the guest should now
-# be behind the time in the host by exactly the amount of time during
-# which ticks have been missed.
-#
-# @slew: deliver ticks at a higher rate to catch up with the missed ticks.
-#The guest OS will not notice anything is amiss, as from its point
-#of view time will have continued to flow normally.  Once the timer
-#has managed to catch up with all the missing ticks, the time in
-#the guest and in the host should match.
-#
-# Since: 2.0
-##
-{ 'enum': 'LostTickPolicy',
-  'data': ['discard', 'delay', 'slew' ] }
-
 ##
 # @NameInfo:
 #
diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
index 10c93a096a..58a7748c66 100644
--- a/include/hw/rtc/mc146818rtc.h
+++ b/include/hw/rtc/mc146818rtc.h
@@ -9,7 +9,7 @@
 #ifndef HW_RTC_MC146818RTC_H
 #define HW_RTC_MC146818RTC_H
 
-#include "qapi/qapi-types-misc.h"
+#include "qapi/qapi-types-machine.h"
 #include "qemu/queue.h"
 #include "qemu/timer.h"
 #include "hw/isa/isa.h"
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 2047114fca..59380ed761 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -4,6 +4,7 @@
 #include "qapi/error.h"
 #include "hw/pci/pci.h"
 #include "qapi/qapi-types-block.h"
+#include "qapi/qapi-types-machine.h"
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ctype.h"
diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
index 876f5aa6fa..22ba6149b5 100644
--- a/hw/i386/kvm/i8254.c
+++ b/hw/i386/kvm/i8254.c
@@ -25,7 +25,7 @@
 
 #include "qemu/osdep.h"
 #include 
-#include "qapi/qapi-types-misc.h"
+#include "qapi/qapi-types-machine.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/timer.h"
-- 

Re: [PATCH v4 7/7] qemu-img: Deprecate use of -b without -F

[Eric Blake]

On 3/12/20 2:28 PM, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot, although
these days tools like libvirt are aware of the issue enough to prevent
the worst effects).  However, if our probing algorithm ever changes,
or if other tools like libvirt determine a different probe result than
we do, then subsequent use of that backing file under a different
format will present corrupted data to the guest.  Start a deprecation
clock so that future qemu-img can refuse to create unsafe backing
chains that would rely on probing.  The warnings are intentionally
emitted from the block layer rather than qemu-img (thus, all paths
into image creation or rewriting perform the check).

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.  While touching it, expand it to
cover all of the various warnings enabled by this patch.  iotest 290
also shows a change to qcow messages; note that the fact that we now
make a probed format of 'raw' explicit now results in a double
warning, but no one should be creating new qcow images so it is not
worth cleaning up.

Signed-off-by: Eric Blake 
---
  docs/system/deprecated.rst | 19 +++


Squash this in per Kashyap's v3 review comments:

diff --git i/docs/system/deprecated.rst w/docs/system/deprecated.rst
index b541d52c7dc0..54a50c45e190 100644
--- i/docs/system/deprecated.rst
+++ w/docs/system/deprecated.rst
@@ -388,18 +388,19 @@ qemu-img backing file without format (since 5.0.0)
 The use of ``qemu-img create``, ``qemu-img rebase``, or ``qemu-img
 convert`` to create or modify an image that depends on a backing file
 now recommends that an explicit backing format be provided.  This is
-for safety: if qemu probes a different format than what you thought,
+for safety: if QEMU probes a different format than what you thought,
 the data presented to the guest will be corrupt; similarly, presenting
 a raw image to a guest allows a potential security exploit if a future
-probe sees a non-raw image based on guest writes.  To avoid the
-warning message, or even future refusal to create an unsafe image, you
-must pass ``-o backing_fmt=`` (or the shorthand ``-F`` during create)
-to specify the intended backing format.  You may use ``qemu-img rebase
--u`` to retroactively add a backing format to an existing image.
-However, be aware that there are already potential security risks to
-blindly using ``qemu-img info`` to probe the format of an untrusted
-backing image, when deciding what format to add into an existing
-image.
+probe sees a non-raw image based on guest writes.
+
+To avoid the warning message, or even future refusal to create an
+unsafe image, you must pass ``-o backing_fmt=`` (or the shorthand
+``-F`` during create) to specify the intended backing format.  You may
+use ``qemu-img rebase -u`` to retroactively add a backing format to an
+existing image.  However, be aware that there are already potential
+security risks to blindly using ``qemu-img info`` to probe the format
+of an untrusted backing image, when deciding what format to add into
+an existing image.

 ``qemu-img convert -n -o`` (since 4.2.0)
 

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

Re: [PATCH v3 4/4] qemu-img: Deprecate use of -b without -F

[Eric Blake]

On 3/9/20 10:31 AM, Kashyap Chamarthy wrote:

On Fri, Mar 06, 2020 at 04:51:21PM -0600, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on




+qemu-img backing file without format (since 5.0.0)
+''
+
+The use of ``qemu-img create``, ``qemu-img rebase``, ``qemu-img
+convert``, or ``qemu-img amend`` to create or modify an image that
+depends on a backing file now recommends that an explicit backing
+format be provided.  This is for safety: if qemu probes a different
+format than what you thought, the data presented to the guest will be
+corrupt; similarly, presenting a raw image to a guest allows a
+potential security exploit if a future probe sees a non-raw image
+based on guest writes.  To avoid the warning message, or even future
+refusal to create an unsafe image, you must pass ``-o backing_fmt=``
+(or the shorthand ``-F`` during create) to specify the intended
+backing format.  You may use ``qemu-img rebase -u`` to retroactively
+add a backing format to an existing image.  However, be aware that
+there are already potential security risks to blindly using ``qemu-img
+info`` to probe the format of an untrusted backing image, when
+deciding what format to add into an existing image.


Nit: s/qemu/QEMU/g/

Ultra Nit: should this paragraph be broken down into two?  Experience
tells people usually feel deterred read "substantial paragraphs" :-)


Shoot, I missed incorporating this comment during my v4 posting. It's 
now changed in my local tree, but I'll hold off on a v5 unless other 
review warrants it.


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

Re: [PATCH 3/3] xen-block: Use one Error * variable instead of two

[Eric Blake]

On 3/13/20 12:05 PM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
---
  hw/block/xen-block.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)


Reviewed-by: Eric Blake 

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

Re: [PATCH 2/3] hw/misc/ivshmem: Use one Error * variable instead of two

[Eric Blake]

On 3/13/20 12:05 PM, Markus Armbruster wrote:

Commit fe44dc9180 "migration: disallow migrate_add_blocker during
migration" accidentally added a second Error * variable.  Use the
first one instead.

Signed-off-by: Markus Armbruster 
---
  hw/misc/ivshmem.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)


Reviewed-by: Eric Blake 

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

Re: [PATCH 1/3] Use _abort instead of separate assert()

[Eric Blake]

On 3/13/20 12:37 PM, Alexander Bulekov wrote:

On 200313 1805, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 




index 1a99277d60..aa9eee6ebf 100644
--- a/tests/qtest/fuzz/qos_fuzz.c
+++ b/tests/qtest/fuzz/qos_fuzz.c
@@ -57,8 +57,7 @@ static void qos_set_machines_devices_available(void)
  QList *lst;
  Error *err = NULL;

Can this err declaration be removed? Don't think it's used anywhere
else.


Correct, it is not.  With that additional line gone,

Reviewed-by: Eric Blake 

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

Re: [PATCH 0/7] via-ide: fixes and improvements

[John Snow]

On 3/13/20 4:24 AM, Mark Cave-Ayland wrote:
> Following on from the earlier thread "Implement "non 100% native mode"
> in via-ide", here is an updated patchset based upon the test cases
> sent to me off-list.
> 
> The VIA IDE controller is similar to early versions of the PIIX
> controller in that the primary and secondary IDE channels are hardwired
> to IRQs 14 and 15 respectively. Guest OSs typically handle this by
> either switching the controller to legacy mode, or using native mode and
> using a combination of PCI device/vendor ID and/or checking various
> registers in PCI configuration space to detect this condition and apply
> a special fixed IRQ 14/15 routing.
> 
> This patchset effectively updates the VIA IDE PCI device to follow the
> behaviour in the datasheet in two ways: fixing some PCI configuration
> space register defaults and behaviours, and always using legacy IRQ 14/15
> routing, and once applied allows all our known test images to boot
> correctly.
> 
> Signed-off-by: Mark Cave-Ayland 
> 
> 
> BALATON Zoltan (2):
>   ide/via: Get rid of via_ide_init()
>   pci: Honour wmask when resetting PCI_INTERRUPT_LINE
> 
> Mark Cave-Ayland (5):
>   via-ide: move registration of VMStateDescription to DeviceClass
>   via-ide: ensure that PCI_INTERRUPT_LINE is hard-wired to its default
> value
>   via-ide: initialise IDE controller in legacy mode
>   via-ide: allow guests to write to PCI_CLASS_PROG
>   via-ide: always use legacy IRQ 14/15 routing
> 
>  hw/ide/via.c| 21 +
>  hw/mips/mips_fulong2e.c |  5 -
>  hw/pci/pci.c|  5 -
>  include/hw/ide.h|  1 -
>  4 files changed, 13 insertions(+), 19 deletions(-)
> 

Does this supersede everything else so far? (Except the two cmd646
related series, four patches total, which are already staged)

Re: [PATCH 1/3] Use _abort instead of separate assert()

[Alexander Bulekov]

On 200313 1805, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 


> index 1a99277d60..aa9eee6ebf 100644
> --- a/tests/qtest/fuzz/qos_fuzz.c
> +++ b/tests/qtest/fuzz/qos_fuzz.c
> @@ -57,8 +57,7 @@ static void qos_set_machines_devices_available(void)
>  QList *lst;
>  Error *err = NULL;
Can this err declaration be removed? Don't think it's used anywhere
else.

>  
> -qmp_marshal_query_machines(NULL, , );
> -assert(!err);
> +qmp_marshal_query_machines(NULL, , _abort);
>  lst = qobject_to(QList, response);
>  apply_to_qlist(lst, true);
>  
> @@ -70,8 +69,7 @@ static void qos_set_machines_devices_available(void)
>  qdict_put_bool(args, "abstract", true);
>  qdict_put_obj(req, "arguments", (QObject *) args);
>  
> -qmp_marshal_qom_list_types(args, , );
> -assert(!err);
> +qmp_marshal_qom_list_types(args, , _abort);
>  lst = qobject_to(QList, response);
>  apply_to_qlist(lst, false);
>  qobject_unref(response);
> -- 
> 2.21.1
> 
Thanks!

Acked-by: Alexander Bulekov 

Re: [PATCH 0/3] Minor error handling cleanups

[Peter Maydell]

On Fri, 13 Mar 2020 at 17:06, Markus Armbruster  wrote:
>
> Markus Armbruster (3):
>   Use _abort instead of separate assert()
>   hw/misc/ivshmem: Use one Error * variable instead of two
>   xen-block: Use one Error * variable instead of two
>
>  block/monitor/block-hmp-cmds.c | 4 +---
>  hw/block/xen-block.c   | 5 +
>  hw/misc/ivshmem.c  | 7 +++
>  target/arm/monitor.c   | 8 ++--
>  tests/qtest/fuzz/qos_fuzz.c| 6 ++
>  5 files changed, 9 insertions(+), 21 deletions(-)

series
Reviewed-by: Peter Maydell 

-- PMM

Re: [EXTERNAL][PATCH 0/7] via-ide: fixes and improvements

[BALATON Zoltan]

Hello,

On Fri, 13 Mar 2020, Aleksandar Markovic wrote:

Hi, Mark, could you just enumerate those test images, download
locations, etc. and whatever else is needed to reproduce the boot
processes in question - it would be useful not only for this patch
set, but for possible future work, wouldn't it?

Sorry in advance if that info in possibly in another message, and
was missed by me.


I've sent it to Mark off-list but here it is:

On Tue, 10 Mar 2020, BALATON Zoltan wrote:

I was testing fulong2e with this kernel:

http://distfiles.gentoo.org/experimental/mips/livecd/loongson-2007.1/

running it as

qemu-system-mips64el -M fulong2e -serial stdio -net none -vga none \
-trace enable="pci*" -kernel gentoo-loongson-2.6.22.6-20070902 \
-cdrom debian-8.11.0-mipsel-netinst.iso

adding the cdrom proabably does not really matter and I could not find 
corresponding gentoo iso so using a debian one, it's just useful to have 
something on the ide bus and also -cdrom adds it to second channel 
because if you test with something on first channel only it might work 
as that uses IRQ14 anyway.


On pegasos2 I've used:

qemu-system-ppc -M pegasos2 -net none -serial stdio \
-vga none -device ati-vga,romfile=VGABIOS-lgpl-latest.bin \
-cdrom morphos-3.13.iso

then enter "boot cd boot.img" at the firmware ok prompt.

And also the same pegasos2 command with

-cdrom debian-8.11.0-powerpc-netinst.iso

and enter "boot cd install/pegasos" then I usually select 3 for rescue 
mode which can get to a command prompt.


For more info on VGABIOS-lgpl-latest.bin and pegrom.bin needed for 
pegasos2 see Pegasos2 emulation subproject on my http://qmiga.osdn.io/ 
page.


The fulong2e probably still has some problem correctly emulating pci 
devices becuase without -net none -vga none the kernel panics, I have no 
interest trying to debug and fix that, I only using fulong2e to 
cross-check changes needed for pegasos2. It could also be that the kernel 
being experimental have some problems, I can't test if it works on real 
hardware.


Regards,
BALATON Zoltan

[PATCH 2/3] hw/misc/ivshmem: Use one Error * variable instead of two

[Markus Armbruster]

Commit fe44dc9180 "migration: disallow migrate_add_blocker during
migration" accidentally added a second Error * variable.  Use the
first one instead.

Signed-off-by: Markus Armbruster 
---
 hw/misc/ivshmem.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 1a0fad74e1..a8dc9b377d 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -832,7 +832,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
**errp)
 IVShmemState *s = IVSHMEM_COMMON(dev);
 Error *err = NULL;
 uint8_t *pci_conf;
-Error *local_err = NULL;
 
 /* IRQFD requires MSI */
 if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
@@ -899,9 +898,9 @@ static void ivshmem_common_realize(PCIDevice *dev, Error 
**errp)
 if (!ivshmem_is_master(s)) {
 error_setg(>migration_blocker,
"Migration is disabled when using feature 'peer mode' in 
device 'ivshmem'");
-migrate_add_blocker(s->migration_blocker, _err);
-if (local_err) {
-error_propagate(errp, local_err);
+migrate_add_blocker(s->migration_blocker, );
+if (err) {
+error_propagate(errp, err);
 error_free(s->migration_blocker);
 return;
 }
-- 
2.21.1

[PATCH 1/3] Use _abort instead of separate assert()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 block/monitor/block-hmp-cmds.c | 4 +---
 target/arm/monitor.c   | 8 ++--
 tests/qtest/fuzz/qos_fuzz.c| 6 ++
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index c3a6368dfc..4c8c375172 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -838,10 +838,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
 {
 BlockJobInfoList *list;
-Error *err = NULL;
 
-list = qmp_query_block_jobs();
-assert(!err);
+list = qmp_query_block_jobs(_abort);
 
 if (!list) {
 monitor_printf(mon, "No active jobs\n");
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index c2dc7908de..ea6598c412 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -206,9 +206,7 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
 return NULL;
 }
 } else {
-Error *err = NULL;
-arm_cpu_finalize_features(ARM_CPU(obj), );
-assert(err == NULL);
+arm_cpu_finalize_features(ARM_CPU(obj), _abort);
 }
 
 expansion_info = g_new0(CpuModelExpansionInfo, 1);
@@ -221,12 +219,10 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
 while ((name = cpu_model_advertised_features[i++]) != NULL) {
 ObjectProperty *prop = object_property_find(obj, name, NULL);
 if (prop) {
-Error *err = NULL;
 QObject *value;
 
 assert(prop->get);
-value = object_property_get_qobject(obj, name, );
-assert(!err);
+value = object_property_get_qobject(obj, name, _abort);
 
 qdict_put_obj(qdict_out, name, value);
 }
diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
index 1a99277d60..aa9eee6ebf 100644
--- a/tests/qtest/fuzz/qos_fuzz.c
+++ b/tests/qtest/fuzz/qos_fuzz.c
@@ -57,8 +57,7 @@ static void qos_set_machines_devices_available(void)
 QList *lst;
 Error *err = NULL;
 
-qmp_marshal_query_machines(NULL, , );
-assert(!err);
+qmp_marshal_query_machines(NULL, , _abort);
 lst = qobject_to(QList, response);
 apply_to_qlist(lst, true);
 
@@ -70,8 +69,7 @@ static void qos_set_machines_devices_available(void)
 qdict_put_bool(args, "abstract", true);
 qdict_put_obj(req, "arguments", (QObject *) args);
 
-qmp_marshal_qom_list_types(args, , );
-assert(!err);
+qmp_marshal_qom_list_types(args, , _abort);
 lst = qobject_to(QList, response);
 apply_to_qlist(lst, false);
 qobject_unref(response);
-- 
2.21.1

[PATCH 3/3] xen-block: Use one Error * variable instead of two

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 hw/block/xen-block.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3885464513..7b3b6dee97 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -998,14 +998,13 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
 XenBlockVdev *vdev = >props.vdev;
 XenBlockDrive *drive = blockdev->drive;
 XenBlockIOThread *iothread = blockdev->iothread;
+Error *local_err = NULL;
 
 trace_xen_block_device_destroy(vdev->number);
 
 object_unparent(OBJECT(xendev));
 
 if (iothread) {
-Error *local_err = NULL;
-
 xen_block_iothread_destroy(iothread, _err);
 if (local_err) {
 error_propagate_prepend(errp, local_err,
@@ -1015,8 +1014,6 @@ static void xen_block_device_destroy(XenBackendInstance 
*backend,
 }
 
 if (drive) {
-Error *local_err = NULL;
-
 xen_block_drive_destroy(drive, _err);
 if (local_err) {
 error_propagate_prepend(errp, local_err,
-- 
2.21.1

[PATCH 0/3] Minor error handling cleanups

[Markus Armbruster]

Markus Armbruster (3):
  Use _abort instead of separate assert()
  hw/misc/ivshmem: Use one Error * variable instead of two
  xen-block: Use one Error * variable instead of two

 block/monitor/block-hmp-cmds.c | 4 +---
 hw/block/xen-block.c   | 5 +
 hw/misc/ivshmem.c  | 7 +++
 target/arm/monitor.c   | 8 ++--
 tests/qtest/fuzz/qos_fuzz.c| 6 ++
 5 files changed, 9 insertions(+), 21 deletions(-)

-- 
2.21.1

Re: [EXTERNAL][PATCH 0/7] via-ide: fixes and improvements

[Aleksandar Markovic]

> From: Mark Cave-Ayland 
> This patchset effectively updates the VIA IDE PCI device to follow the
> behaviour in the datasheet in two ways: fixing some PCI configuration
> space register defaults and behaviours, and always using legacy IRQ 14/15
> routing, and once applied allows all our known test images to boot
> correctly.
> 

Hi, Mark, could you just enumerate those test images, download
locations, etc. and whatever else is needed to reproduce the boot
processes in question - it would be useful not only for this patch
set, but for possible future work, wouldn't it?

Sorry in advance if that info in possibly in another message, and
was missed by me.

Many thanks,
Aleksandar

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 18:42, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


12.03.2020 19:36, Markus Armbruster wrote:

I may have a second look tomorrow with fresher eyes, but let's get this
out now as is.

Vladimir Sementsov-Ogievskiy  writes:


Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)

Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
   --max-width 80 FILES...

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

   scripts/coccinelle/auto-propagated-errp.cocci | 327 ++
   include/qapi/error.h  |   3 +
   MAINTAINERS   |   1 +
   3 files changed, 331 insertions(+)
   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 00..7dac2dcfa4
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,327 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// 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 program.  If not, see
+// .
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place \
+//  --no-show-diff --max-width 80 FILES...
+//
+// Note: --max-width 80 is needed because coccinelle default is less
+// than 80, and without this parameter coccinelle may reindent some
+// lines which fit into 80 characters but not to coccinelle default,
+// which in turn produces extra patch hunks for no reason.


This is about unwanted reformatting of parameter lists due to the ___
chaining hack.  --max-width 80 makes that less likely, but not
impossible.

We can search for unwanted reformatting of parameter lists.  I think
grepping diffs for '^\+.*Error \*\*' should do the trick.  For the whole
tree, I get one false positive (not a parameter list), and one hit:

  @@ -388,8 +388,10 @@ static void object_post_init_with_type(O
   }
   }

  -void object_apply_global_props(Object *obj, const GPtrArray *props, 
Error **errp)
  +void object_apply_global_props(Object *obj, const GPtrArray *props,
  +   Error **errp)
   {
  +ERRP_AUTO_PROPAGATE();
   int i;

   if (!props) {

Reformatting, but not unwanted.


Yes, I saw it. This line is 81 character length, so it's OK to fix it in one 
hunk with
ERRP_AUTO_PROPAGATE addition even for non-automatic patch.


Agree.



The --max-width 80 hack is good enough for me.

It does result in slightly long transformed lines, e.g. this one in
replication.c:

  @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS
   s->mode = REPLICATION_MODE_PRIMARY;
   top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
   if (top_id) {
  -error_setg(_err, "The primary side does not support option 
top-id");
  +error_setg(errp, "The primary side does not support option 
top-id");
   goto fail;
   }
   } else if (!strcmp(mode, "secondary")) {

v8 did break this line (that's how I found it).  However, v9 still
shortens the line, just not below the target.  All your + lines look
quite unlikely to lengthen lines.  Let's not worry about this.


+// Switch unusual Error ** parameter names to errp
+// (this is necessary to use ERRP_AUTO_PROPAGATE).
+//
+// Disable optional_qualifier to skip functions with
+// "Error *const *errp" parameter.
+//
+// Skip functions with "assert(_errp && *_errp)" statement, because
+// that signals unusual semantics, and the parameter name may well
+// serve a purpose. (like nbd_iter_channel_error()).
+//
+// Skip util/error.c to 

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 12.03.2020 19:36, Markus Armbruster wrote:
>> I may have a second look tomorrow with fresher eyes, but let's get this
>> out now as is.
>>
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>>> Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
>>> does corresponding changes in code (look for details in
>>> include/qapi/error.h)
>>>
>>> Usage example:
>>> spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>>   --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
>>>   --max-width 80 FILES...
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>
>>> Cc: Eric Blake 
>>> Cc: Kevin Wolf 
>>> Cc: Max Reitz 
>>> Cc: Greg Kurz 
>>> Cc: Christian Schoenebeck 
>>> Cc: Stefano Stabellini 
>>> Cc: Anthony Perard 
>>> Cc: Paul Durrant 
>>> Cc: Stefan Hajnoczi 
>>> Cc: "Philippe Mathieu-Daudé" 
>>> Cc: Laszlo Ersek 
>>> Cc: Gerd Hoffmann 
>>> Cc: Stefan Berger 
>>> Cc: Markus Armbruster 
>>> Cc: Michael Roth 
>>> Cc: qemu-de...@nongnu.org
>>> Cc: qemu-block@nongnu.org
>>> Cc: xen-de...@lists.xenproject.org
>>>
>>>   scripts/coccinelle/auto-propagated-errp.cocci | 327 ++
>>>   include/qapi/error.h  |   3 +
>>>   MAINTAINERS   |   1 +
>>>   3 files changed, 331 insertions(+)
>>>   create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci
>>>
>>> diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
>>> b/scripts/coccinelle/auto-propagated-errp.cocci
>>> new file mode 100644
>>> index 00..7dac2dcfa4
>>> --- /dev/null
>>> +++ b/scripts/coccinelle/auto-propagated-errp.cocci
>>> @@ -0,0 +1,327 @@
>>> +// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
>>> +//
>>> +// Copyright (c) 2020 Virtuozzo International GmbH.
>>> +//
>>> +// 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 program.  If not, see
>>> +// .
>>> +//
>>> +// Usage example:
>>> +// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
>>> +//  --macro-file scripts/cocci-macro-file.h --in-place \
>>> +//  --no-show-diff --max-width 80 FILES...
>>> +//
>>> +// Note: --max-width 80 is needed because coccinelle default is less
>>> +// than 80, and without this parameter coccinelle may reindent some
>>> +// lines which fit into 80 characters but not to coccinelle default,
>>> +// which in turn produces extra patch hunks for no reason.
>>
>> This is about unwanted reformatting of parameter lists due to the ___
>> chaining hack.  --max-width 80 makes that less likely, but not
>> impossible.
>>
>> We can search for unwanted reformatting of parameter lists.  I think
>> grepping diffs for '^\+.*Error \*\*' should do the trick.  For the whole
>> tree, I get one false positive (not a parameter list), and one hit:
>>
>>  @@ -388,8 +388,10 @@ static void object_post_init_with_type(O
>>   }
>>   }
>>
>>  -void object_apply_global_props(Object *obj, const GPtrArray *props, 
>> Error **errp)
>>  +void object_apply_global_props(Object *obj, const GPtrArray *props,
>>  +   Error **errp)
>>   {
>>  +ERRP_AUTO_PROPAGATE();
>>   int i;
>>
>>   if (!props) {
>>
>> Reformatting, but not unwanted.
>
> Yes, I saw it. This line is 81 character length, so it's OK to fix it in one 
> hunk with
> ERRP_AUTO_PROPAGATE addition even for non-automatic patch.

Agree.

>>
>> The --max-width 80 hack is good enough for me.
>>
>> It does result in slightly long transformed lines, e.g. this one in
>> replication.c:
>>
>>  @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS
>>   s->mode = REPLICATION_MODE_PRIMARY;
>>   top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
>>   if (top_id) {
>>  -error_setg(_err, "The primary side does not support 
>> option top-id");
>>  +error_setg(errp, "The primary side does not support option 
>> top-id");
>>   goto fail;
>>   }
>>   } else if (!strcmp(mode, "secondary")) {
>>
>> v8 did break this line (that's how I found it).  However, v9 still
>> shortens the line, just not below the target.  All your + lines look
>> quite unlikely to lengthen lines.  Let's not worry about this.
>>
>>> +// Switch unusual Error ** parameter names to errp
>>> +// (this is 

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 13.03.2020 10:50, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy  writes:
>>
>> [...]
>>> +// Warn several Error * definitions.
>>> +@check1 disable optional_qualifier exists@
>>> +identifier fn = rule1.fn, local_err, local_err2;
>>> +@@
>>> +
>>> + fn(..., Error ** , ...)
>>> + {
>>> + ...
>>> + Error *local_err = NULL;
>>> + ... when any
>>> + Error *local_err2 = NULL;
>>> + ... when any
>>> + }
>>> +
>>> +@ script:python @
>>> +fn << check1.fn;
>>> +@@
>>> +
>>> +print('Warning: function {} has several definitions of '
>>> +  'Error * local variable'.format(fn))
>>
>> Printing the positions like you do in the next rule is useful when
>> examining these warnings.
>
> I decided that searching for Error * definition is simple, and better for
> user to search all definitions by hand (may be more than too).
>
> But understanding control flows is more complex thing and better to help
> user with line positions.
>
> But if you want, we can add them of course. Note, that for some reasons some
> times coccinelle instead of original filename prints something like 
> /tmp/...original-name...
> so it don't look nice and may be a bit misleading.

I noticed when I actually tried mu idea %-}

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 17:58, Markus Armbruster wrote:

I tried this script on the whole tree.  Observations:

* $ git-diff --shortstat \*.[ch]
333 files changed, 3480 insertions(+), 4586 deletions(-)

* Twelve functions have "several definitions of Error * local variable".

   Eight declare such a variable within a loop.  Reported because
   Coccinelle matches along control flow, not just along text.  Ignore.

   Remaining four:

   * ivshmem_common_realize()

 Two variables (messed up in commit fe44dc91807), should be replaced
 by one.

   * qmp_query_cpu_model_expansion() two times

 Three declarations in separate blocks; two should be replaced by
 _abort, one moved to the function block.

   * xen_block_device_destroy()

 Two declarations in seperate blocks; should be replaced by a single
 one.

   Separate manual cleanup patches, ideally applied before running
   Coccinelle to keep Coccinelle's changes as simple and safe as
   possible.  I'll post patches.  Only the one for
   xen_block_device_destroy() affects by this series.

* No function "propagates to errp several times"

   I tested the rule does detect this as advertized by feeding it an
   obvious example.  We're good.

* ERRP_AUTO_PROPAGATE() inserted 744 times, always right at the
   beginning of a function.

* As far as I can tell, all changed functions have ERRP_AUTO_PROPAGATE()
   inserted.  Good.

* Almost 1100 error propagations dropped:error_propagate() removed,
   error_propagate_prepend() replaced by just error_prepend().

* Four error_propagate() are transformed.  Two instances each in
   aspeed_soc_ast2600_realize() and aspeed_soc_realize().  Pattern:

  {
 +ERRP_AUTO_PROPAGATE();
  ...
 -Error *err = NULL, *local_err = NULL;
 +Error *local_err = NULL;
  ...

  object_property_set_T(...,
 -  );
 +  errp);
  object_property_set_T(...,
_err);
 -error_propagate(, local_err);
 -if (err) {
 -error_propagate(errp, err);
 +error_propagate(errp, local_err);
 +if (*errp) {
  return;
  }

   This is what error.h calls "Receive and accumulate multiple errors
   (first one wins)".

   Result:

 ERRP_AUTO_PROPAGATE();
 ...
 Error *local_err = NULL;
 ...

 object_property_set_T(..., errp);
 object_property_set_T(..., _err);
 error_propagate(errp, local_err);
 if (*errp) {
 return;
 }

   Could be done without the accumulation:

 ERRP_AUTO_PROPAGATE();
 ...

 object_property_set_T(..., errp);
 if (*errp) {
 return;
 }
 object_property_set_T(..., errp);
 if (*errp) {
 return;
 }

   I find this a bit easier to understand.  Matter of taste.  If we want
   to change to this, do it manually and separately.  I'd do it on top.

* Some 90 propagations remain.

   Some of them could use cleanup, e.g. file_memory_backend_set_pmem(),
   css_clear_io_interrupt().  Out of scope for this series.

   Some move errors around in unusual ways, e.g. in block/nbd.c.  Could
   use review.  Out of scope for this series.

   I spotted three that should be transformed, but aren't:

   - qcrypto_block_luks_store_key()

 I believe g_autoptr() confuses Coccinelle.  Undermines all our
 Coccinelle use, not just this patch.  I think we need to update
 scripts/cocci-macro-file.h for it.

   - armsse_realize()

 Something in this huge function confuses Coccinelle, but I don't
 know what exactly.  If I delete most of it, the error_propagate()
 transforms okay.  If I delete less, Coccinelle hangs.

   - apply_cpu_model()

 Gets transformed fine if I remove the #ifndef CONFIG_USER_ONLY.  I
 have no idea why the #if spooks Coccinelle here, but not elsewhere.

   None of these three affects this series.  No need to hold it back for
   further investigation.

* 30 error_free() and two warn_reportf_err() are transformed.  Patterns:

 -error_free(local_err);
 -local_err = NULL;
 +error_free_errp(errp);

   and

 -error_free(local_err);
 +error_free_errp(errp);

   and

 -warn_report_err(local_err);
 -local_err = NULL;
 +warn_report_errp(errp);

   Good.

* Many error_free(), error_reportf_err() and warn_reportf_err() remain.
   None of them have an argument of the form *errp.  Such arguments would
   have to be reviewed for possible interference with
   ERRP_AUTO_PROPAGATE().

* Almost 700 Error *err = NULL removed.  Almost 600 remain.

* Error usage in rdma.c is questionable / wrong.  Out of scope for this
   series.

As far as I can tell, your Coccinelle script is working as intended,
except 

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

I tried this script on the whole tree.  Observations:

* $ git-diff --shortstat \*.[ch]
   333 files changed, 3480 insertions(+), 4586 deletions(-)

* Twelve functions have "several definitions of Error * local variable".

  Eight declare such a variable within a loop.  Reported because
  Coccinelle matches along control flow, not just along text.  Ignore.

  Remaining four:

  * ivshmem_common_realize()

Two variables (messed up in commit fe44dc91807), should be replaced
by one.

  * qmp_query_cpu_model_expansion() two times

Three declarations in separate blocks; two should be replaced by
_abort, one moved to the function block.

  * xen_block_device_destroy()

Two declarations in seperate blocks; should be replaced by a single
one.

  Separate manual cleanup patches, ideally applied before running
  Coccinelle to keep Coccinelle's changes as simple and safe as
  possible.  I'll post patches.  Only the one for
  xen_block_device_destroy() affects by this series.

* No function "propagates to errp several times"

  I tested the rule does detect this as advertized by feeding it an
  obvious example.  We're good.

* ERRP_AUTO_PROPAGATE() inserted 744 times, always right at the
  beginning of a function.

* As far as I can tell, all changed functions have ERRP_AUTO_PROPAGATE()
  inserted.  Good.

* Almost 1100 error propagations dropped:error_propagate() removed,
  error_propagate_prepend() replaced by just error_prepend().

* Four error_propagate() are transformed.  Two instances each in
  aspeed_soc_ast2600_realize() and aspeed_soc_realize().  Pattern:

 {
+ERRP_AUTO_PROPAGATE();
 ...
-Error *err = NULL, *local_err = NULL;
+Error *local_err = NULL;
 ...

 object_property_set_T(..., 
-  );
+  errp);
 object_property_set_T(...,
   _err);
-error_propagate(, local_err);
-if (err) {
-error_propagate(errp, err);
+error_propagate(errp, local_err);
+if (*errp) {
 return;
 }

  This is what error.h calls "Receive and accumulate multiple errors
  (first one wins)".

  Result:

ERRP_AUTO_PROPAGATE();
...
Error *local_err = NULL;
...

object_property_set_T(..., errp);
object_property_set_T(..., _err);
error_propagate(errp, local_err);
if (*errp) {
return;
}

  Could be done without the accumulation:

ERRP_AUTO_PROPAGATE();
...

object_property_set_T(..., errp);
if (*errp) {
return;
}
object_property_set_T(..., errp);
if (*errp) {
return;
}

  I find this a bit easier to understand.  Matter of taste.  If we want
  to change to this, do it manually and separately.  I'd do it on top.

* Some 90 propagations remain.

  Some of them could use cleanup, e.g. file_memory_backend_set_pmem(),
  css_clear_io_interrupt().  Out of scope for this series.

  Some move errors around in unusual ways, e.g. in block/nbd.c.  Could
  use review.  Out of scope for this series.

  I spotted three that should be transformed, but aren't:

  - qcrypto_block_luks_store_key()

I believe g_autoptr() confuses Coccinelle.  Undermines all our
Coccinelle use, not just this patch.  I think we need to update
scripts/cocci-macro-file.h for it.

  - armsse_realize()

Something in this huge function confuses Coccinelle, but I don't
know what exactly.  If I delete most of it, the error_propagate()
transforms okay.  If I delete less, Coccinelle hangs.

  - apply_cpu_model()

Gets transformed fine if I remove the #ifndef CONFIG_USER_ONLY.  I
have no idea why the #if spooks Coccinelle here, but not elsewhere.

  None of these three affects this series.  No need to hold it back for
  further investigation.

* 30 error_free() and two warn_reportf_err() are transformed.  Patterns:

-error_free(local_err);
-local_err = NULL;
+error_free_errp(errp);

  and

-error_free(local_err);
+error_free_errp(errp);

  and

-warn_report_err(local_err);
-local_err = NULL;
+warn_report_errp(errp);

  Good.

* Many error_free(), error_reportf_err() and warn_reportf_err() remain.
  None of them have an argument of the form *errp.  Such arguments would
  have to be reviewed for possible interference with
  ERRP_AUTO_PROPAGATE().

* Almost 700 Error *err = NULL removed.  Almost 600 remain.

* Error usage in rdma.c is questionable / wrong.  Out of scope for this
  series.

As far as I can tell, your Coccinelle script is working as intended,
except for three missed error propagations noted above.  We can proceed
with this series regardless, if we want.  I'd prefer to integrate my

Re: [PATCH 0/7] via-ide: fixes and improvements

[BALATON Zoltan]

On Fri, 13 Mar 2020, Mark Cave-Ayland wrote:

Following on from the earlier thread "Implement "non 100% native mode"
in via-ide", here is an updated patchset based upon the test cases
sent to me off-list.

The VIA IDE controller is similar to early versions of the PIIX
controller in that the primary and secondary IDE channels are hardwired
to IRQs 14 and 15 respectively. Guest OSs typically handle this by
either switching the controller to legacy mode, or using native mode and
using a combination of PCI device/vendor ID and/or checking various
registers in PCI configuration space to detect this condition and apply
a special fixed IRQ 14/15 routing.

This patchset effectively updates the VIA IDE PCI device to follow the
behaviour in the datasheet in two ways: fixing some PCI configuration
space register defaults and behaviours, and always using legacy IRQ 14/15
routing, and once applied allows all our known test images to boot
correctly.


This supersedes my series and also works with the clients that I've also 
checked so for the series:


Tested-by: BALATON Zoltan 


Signed-off-by: Mark Cave-Ayland 


BALATON Zoltan (2):
 ide/via: Get rid of via_ide_init()
 pci: Honour wmask when resetting PCI_INTERRUPT_LINE

Mark Cave-Ayland (5):
 via-ide: move registration of VMStateDescription to DeviceClass
 via-ide: ensure that PCI_INTERRUPT_LINE is hard-wired to its default
   value
 via-ide: initialise IDE controller in legacy mode
 via-ide: allow guests to write to PCI_CLASS_PROG
 via-ide: always use legacy IRQ 14/15 routing

hw/ide/via.c| 21 +
hw/mips/mips_fulong2e.c |  5 -
hw/pci/pci.c|  5 -
include/hw/ide.h|  1 -
4 files changed, 13 insertions(+), 19 deletions(-)

[PATCH 2/3] python/qemu: Kill QEMU process if 'quit' doesn't work

[Kevin Wolf]

With a QEMU bug, it can happen that the QEMU process doesn't react to a
'quit' QMP command. If we got an exception during previous QMP
communication (e.g. iotests Timeout expiring), we could also be in an
inconsistent state where after sending 'quit' we immediately read an old
response and close the socket even though the 'quit' command wasn't
processed yet. Both cases would lead to a hanging test.

Fix this by waiting for the QEMU process to exit after sending 'quit'
with a timeout, and if it doesn't happen within three seconds, send
SIGKILL.

Signed-off-by: Kevin Wolf 
---
 python/qemu/machine.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 183d8f3d38..c837ee8723 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -358,6 +358,7 @@ class QEMUMachine(object):
 if not has_quit:
 self._qmp.cmd('quit')
 self._qmp.close()
+self._popen.wait(timeout=3)
 except:
 self._popen.kill()
 self._popen.wait()
-- 
2.20.1

[PATCH 3/3] iotests: Increase pause_wait() timeout

[Kevin Wolf]

Waiting for only 1 second proved to be too short on a loaded system,
resulting in false positives when testing pull requests. Increase the
timeout a bit to make this less likely.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b859c303a2..7bc4934cd2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -925,7 +925,7 @@ class QMPTestCase(unittest.TestCase):
 self.assert_qmp(event, 'data/type', 'mirror')
 
 def pause_wait(self, job_id='job0'):
-with Timeout(1, "Timeout waiting for job to pause"):
+with Timeout(3, "Timeout waiting for job to pause"):
 while True:
 result = self.vm.qmp('query-block-jobs')
 found = False
-- 
2.20.1

[PATCH 1/3] iotests.py: Enable faulthandler

[Kevin Wolf]

With this, you can send SIGABRT to a hanging test case and you'll get a
Python stack trace so you know where it was hanging.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 23043baa26..b859c303a2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,12 +30,15 @@ import logging
 import atexit
 import io
 from collections import OrderedDict
+import faulthandler
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
 assert sys.version_info >= (3,6)
 
+faulthandler.enable()
+
 # This will not work if arguments contain spaces but is necessary if we
 # want to support the override options that ./check supports.
 qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]
-- 
2.20.1

[PATCH 0/3] iotests: Fix intermittent 030 hang

[Kevin Wolf]

Peter ran into a 030 hang while testing a pull request. This turned out
to be two bugs in the test suite at once: First was the test failing
because a timeout was apparently too short, second was that the timeout
would actually cause the test to hang instead of failing. This series
should fix both.

Kevin Wolf (3):
  iotests.py: Enable faulthandler
  python/qemu: Kill QEMU process if 'quit' doesn't work
  iotests: Increase pause_wait() timeout

 python/qemu/machine.py| 1 +
 tests/qemu-iotests/iotests.py | 5 -
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 7/7] via-ide: always use legacy IRQ 14/15 routing

[Mark Cave-Ayland]

The existing code uses fixed PCI IRQ routing on IRQ 14 rather than legacy IRQ
14/15 routing as documented in the datasheet.

With the changes in this patchset guest OSs now correctly detect and configure
the VIA controller in legacy IRQ routing mode, allowing the incorrect fixed
PCI IRQ routing to be removed.

Note that this fixed legacy IRQ 14/15 routing is identical to similar behaviour
in the early PIIX IDE controllers.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 3c4d474e48..8de4945cc1 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -113,10 +113,7 @@ static void via_ide_set_irq(void *opaque, int n, int level)
 }
 
 level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
-n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
-if (n) {
-qemu_set_irq(isa_get_irq(NULL, n), level);
-}
+qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
 }
 
 static void via_ide_reset(DeviceState *dev)
-- 
2.20.1

[PATCH 5/7] via-ide: initialise IDE controller in legacy mode

[Mark Cave-Ayland]

According to both the VT82C686B and VT8231 datasheets the VIA Southbridge IDE
controller is initialised in legacy mode.

This allows Linux to correctly determine that legacy rather than PCI IRQ routing
should be used since the boot console text in the fulong2e test image changes 
from:

scsi0 : pata_via
scsi1 : pata_via
ata1: PATA max UDMA/100 cmd 0xbfd04050 ctl 0xbfd04062 \
  bmdma 0xbfd04040 irq 14
ata2: PATA max UDMA/100 cmd 0xbfd04058 ctl 0xbfd04066 \
  bmdma 0xbfd04048 irq 14

to:

scsi0 : pata_via
scsi1 : pata_via
ata1: PATA max UDMA/100 cmd 0xbfd001f0 ctl 0xbfd003f6 \
  bmdma 0xbfd04040 irq 14
ata2: PATA max UDMA/100 cmd 0xbfd00170 ctl 0xbfd00376 \
  bmdma 0xbfd04048 irq 15

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 8363bd4802..c8835de01b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -167,7 +167,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 uint8_t *pci_conf = dev->config;
 int i;
 
-pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
+pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
 pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
 dev->wmask[PCI_INTERRUPT_LINE] = 0;
 
-- 
2.20.1

[PATCH 4/7] via-ide: ensure that PCI_INTERRUPT_LINE is hard-wired to its default value

[Mark Cave-Ayland]

Some firmwares accidentally write to PCI_INTERRUPT_LINE on startup which has
no effect on real hardware since it is hard-wired to its default value, but
causes the guest OS to become confused trying to initialise IDE devices
when running under QEMU.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 3153be8862..8363bd4802 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -169,7 +169,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 
 pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
 pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
-dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
+dev->wmask[PCI_INTERRUPT_LINE] = 0;
 
 memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,
   >bus[0], "via-ide0-data", 8);
-- 
2.20.1

[PATCH 3/7] pci: Honour wmask when resetting PCI_INTERRUPT_LINE

[Mark Cave-Ayland]

From: BALATON Zoltan 

The pci_do_device_reset() function (called from pci_device_reset)
clears the PCI_INTERRUPT_LINE config reg of devices on the bus but did
this without taking wmask into account. We'll have a device model now
that needs to set a constant value for this reg and this patch allows
to do that without additional workaround in device emulation to
reverse the effect of this PCI bus reset function.

Suggested-by: Mark Cave-Ayland 
Signed-off-by: BALATON Zoltan 
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Mark Cave-Ayland 
---
 hw/pci/pci.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e1ed6677e1..b5bc842fac 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -302,8 +302,11 @@ static void pci_do_device_reset(PCIDevice *dev)
 pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
  pci_get_word(dev->wmask + PCI_STATUS) |
  pci_get_word(dev->w1cmask + PCI_STATUS));
+/* Some devices make bits of PCI_INTERRUPT_LINE read only */
+pci_byte_test_and_clear_mask(dev->config + PCI_INTERRUPT_LINE,
+  pci_get_word(dev->wmask + PCI_INTERRUPT_LINE) |
+  pci_get_word(dev->w1cmask + PCI_INTERRUPT_LINE));
 dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
-dev->config[PCI_INTERRUPT_LINE] = 0x0;
 for (r = 0; r < PCI_NUM_REGIONS; ++r) {
 PCIIORegion *region = >io_regions[r];
 if (!region->size) {
-- 
2.20.1

[PATCH 1/7] via-ide: move registration of VMStateDescription to DeviceClass

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 096de8dba0..84f0efff94 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -190,8 +190,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 bmdma_setup_bar(d);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
 
-vmstate_register(VMSTATE_IF(dev), 0, _ide_pci, d);
-
 for (i = 0; i < 2; i++) {
 ide_bus_new(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
 ide_init2(>bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
@@ -227,6 +225,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
*data)
 PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
 dc->reset = via_ide_reset;
+dc->vmsd = _ide_pci;
 k->realize = via_ide_realize;
 k->exit = via_ide_exitfn;
 k->vendor_id = PCI_VENDOR_ID_VIA;
-- 
2.20.1

[PATCH 0/7] via-ide: fixes and improvements

[Mark Cave-Ayland]

Following on from the earlier thread "Implement "non 100% native mode"
in via-ide", here is an updated patchset based upon the test cases
sent to me off-list.

The VIA IDE controller is similar to early versions of the PIIX
controller in that the primary and secondary IDE channels are hardwired
to IRQs 14 and 15 respectively. Guest OSs typically handle this by
either switching the controller to legacy mode, or using native mode and
using a combination of PCI device/vendor ID and/or checking various
registers in PCI configuration space to detect this condition and apply
a special fixed IRQ 14/15 routing.

This patchset effectively updates the VIA IDE PCI device to follow the
behaviour in the datasheet in two ways: fixing some PCI configuration
space register defaults and behaviours, and always using legacy IRQ 14/15
routing, and once applied allows all our known test images to boot
correctly.

Signed-off-by: Mark Cave-Ayland 


BALATON Zoltan (2):
  ide/via: Get rid of via_ide_init()
  pci: Honour wmask when resetting PCI_INTERRUPT_LINE

Mark Cave-Ayland (5):
  via-ide: move registration of VMStateDescription to DeviceClass
  via-ide: ensure that PCI_INTERRUPT_LINE is hard-wired to its default
value
  via-ide: initialise IDE controller in legacy mode
  via-ide: allow guests to write to PCI_CLASS_PROG
  via-ide: always use legacy IRQ 14/15 routing

 hw/ide/via.c| 21 +
 hw/mips/mips_fulong2e.c |  5 -
 hw/pci/pci.c|  5 -
 include/hw/ide.h|  1 -
 4 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH 2/7] ide/via: Get rid of via_ide_init()

[Mark Cave-Ayland]

From: BALATON Zoltan 

Follow example of CMD646 and remove via_ide_init function and do it
directly in board code instead.

Signed-off-by: BALATON Zoltan 
Reviewed-by: Mark Cave-Ayland 
---
 hw/ide/via.c| 8 
 hw/mips/mips_fulong2e.c | 5 -
 include/hw/ide.h| 1 -
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 84f0efff94..3153be8862 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -211,14 +211,6 @@ static void via_ide_exitfn(PCIDevice *dev)
 }
 }
 
-void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-PCIDevice *dev;
-
-dev = pci_create_simple(bus, devfn, "via-ide");
-pci_ide_create_devs(dev, hd_table);
-}
-
 static void via_ide_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 4727b1d3a4..639ba2a091 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -37,6 +37,7 @@
 #include "qemu/log.h"
 #include "hw/loader.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "elf.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/rtc/mc146818rtc.h"
@@ -239,6 +240,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
 qemu_irq *i8259;
 ISABus *isa_bus;
 DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+PCIDevice *dev;
 
 isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
 if (!isa_bus) {
@@ -256,8 +258,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int 
slot, qemu_irq intc,
 /* Super I/O */
 isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
+dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
 ide_drive_get(hd, ARRAY_SIZE(hd));
-via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
+pci_ide_create_devs(dev, hd);
 
 pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
 pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 28d8a06439..575c099b5b 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -18,7 +18,6 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo 
**hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
-void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 
 /* ide-mmio.c */
 void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
-- 
2.20.1

[PATCH 6/7] via-ide: allow guests to write to PCI_CLASS_PROG

[Mark Cave-Ayland]

MorphOS writes to PCI_CLASS_PROG during IDE initialisation to place the
controller in native mode, but thinks the initialisation has failed
because the native mode bits aren't set when reading the register back.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index c8835de01b..3c4d474e48 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -170,6 +170,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy mode */
 pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
 dev->wmask[PCI_INTERRUPT_LINE] = 0;
+dev->wmask[PCI_CLASS_PROG] = 5;
 
 memory_region_init_io(>data_bar[0], OBJECT(d), _ide_data_le_ops,
   >bus[0], "via-ide0-data", 8);
-- 
2.20.1

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

13.03.2020 10:50, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:

[...]

+// Warn several Error * definitions.
+@check1 disable optional_qualifier exists@
+identifier fn = rule1.fn, local_err, local_err2;
+@@
+
+ fn(..., Error ** , ...)
+ {
+ ...
+ Error *local_err = NULL;
+ ... when any
+ Error *local_err2 = NULL;
+ ... when any
+ }
+
+@ script:python @
+fn << check1.fn;
+@@
+
+print('Warning: function {} has several definitions of '
+  'Error * local variable'.format(fn))


Printing the positions like you do in the next rule is useful when
examining these warnings.


I decided that searching for Error * definition is simple, and better for
user to search all definitions by hand (may be more than too).

But understanding control flows is more complex thing and better to help
user with line positions.

But if you want, we can add them of course. Note, that for some reasons some
times coccinelle instead of original filename prints something like 
/tmp/...original-name...
so it don't look nice and may be a bit misleading.




+
+// Warn several propagations in control flow.
+@check2 disable optional_qualifier exists@
+identifier fn = rule1.fn;
+symbol errp;
+position p1, p2;
+@@
+
+ fn(..., Error ** , ...)
+ {
+ ...
+(
+ error_propagate_prepend(errp, ...);@p1
+|
+ error_propagate(errp, ...);@p1
+)
+ ...
+(
+ error_propagate_prepend(errp, ...);@p2
+|
+ error_propagate(errp, ...);@p2
+)
+ ... when any
+ }
+
+@ script:python @
+fn << check2.fn;
+p1 << check2.p1;
+p2 << check2.p2;
+@@
+
+print('Warning: function {} propagates to errp several times in '
+  'one control flow: at {}:{} and then at {}:{}'.format(
+  fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))

[...]




--
Best regards,
Vladimir

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

[...]
> +// Warn several Error * definitions.
> +@check1 disable optional_qualifier exists@
> +identifier fn = rule1.fn, local_err, local_err2;
> +@@
> +
> + fn(..., Error ** , ...)
> + {
> + ...
> + Error *local_err = NULL;
> + ... when any
> + Error *local_err2 = NULL;
> + ... when any
> + }
> +
> +@ script:python @
> +fn << check1.fn;
> +@@
> +
> +print('Warning: function {} has several definitions of '
> +  'Error * local variable'.format(fn))

Printing the positions like you do in the next rule is useful when
examining these warnings.

> +
> +// Warn several propagations in control flow.
> +@check2 disable optional_qualifier exists@
> +identifier fn = rule1.fn;
> +symbol errp;
> +position p1, p2;
> +@@
> +
> + fn(..., Error ** , ...)
> + {
> + ...
> +(
> + error_propagate_prepend(errp, ...);@p1
> +|
> + error_propagate(errp, ...);@p1
> +)
> + ...
> +(
> + error_propagate_prepend(errp, ...);@p2
> +|
> + error_propagate(errp, ...);@p2
> +)
> + ... when any
> + }
> +
> +@ script:python @
> +fn << check2.fn;
> +p1 << check2.p1;
> +p2 << check2.p2;
> +@@
> +
> +print('Warning: function {} propagates to errp several times in '
> +  'one control flow: at {}:{} and then at {}:{}'.format(
> +  fn, p1[0].file, p1[0].line, p2[0].file, p2[0].line))
[...]

Re: [PATCH v4 00/10] Further bitmaps improvements

[Vladimir Sementsov-Ogievskiy]

12.03.2020 23:41, John Snow wrote:



On 3/12/20 1:59 AM, Vladimir Sementsov-Ogievskiy wrote:

11.03.2020 20:03, John Snow wrote:



On 3/11/20 9:58 AM, Vladimir Sementsov-Ogievskiy wrote:

11.03.2020 12:55, Max Reitz wrote:

On 11.03.20 07:17, Vladimir Sementsov-Ogievskiy wrote:

10.03.2020 20:17, Max Reitz wrote:

On 06.03.20 08:45, Vladimir Sementsov-Ogievskiy wrote:

26.02.2020 16:13, Max Reitz wrote:

On 05.02.20 12:20, Vladimir Sementsov-Ogievskiy wrote:

Hi!

The main feature here is improvement of _next_dirty_area API,
which
I'm
going to use then for backup / block-copy.

Somehow, I thought that it was merged, but seems I even forgot to
send
v4.


The changes from v3 look good to me, but I’d prefer a review from
Eric
on patch 8.



Hi!

Could you take it now, or do you prefer me to resend?j


I understand that you agreed to drop the comment above
bd_extent_array_convert_to_be(), then do the
“s/further call/so further calls/” replacement, and finally
replace the
whole four lines Eric has quoted by “(this ensures that after a
failure,
no further extents can accidentally change the bounds of the last
extent
in the array)”?



Yes, all true.


Hm, I could take it then, but on second thought, John is the maintainer
for 8/10 patches, and Eric is for the other two...  So I’m not sure
whether I’m even the right person to do so.



Hmm, true. Let's wait for John?




I am *VERY* behind on my email, and this patch series is sitting in my
to-review folder. However, if it's ready to go and reviewed, I'm willing
to merge it, test it, and give it a quick look-over and get you on
your way.



It would be great, if it is convenient for you. Thanks!
All patches are reviewed now by Max or Eric, so, I'd be very glad if
this get in 5.0.





Thanks, applied to my bitmaps tree:

https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git



Thank you!


--
Best regards,
Vladimir

Re: [PATCH] block/io: fix bdrv_co_do_copy_on_readv

[Vladimir Sementsov-Ogievskiy]

13.03.2020 2:09, John Snow wrote:



On 3/12/20 4:19 AM, Vladimir Sementsov-Ogievskiy wrote:

Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up
buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end
anyway.

But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on
part of original qiov, defined by qiov_offset and bytes. So we must not
touch qiov behind qiov_offset+bytes bound. Fix it.



For the purposes of the stable branch commit log, how does the bug
manifest? Are there known cases? What's the impact?

(Do we have tests?)


Sorry, nothing of these things. I just saw it while working with this code.




Cc: qemu-sta...@nongnu.org # v4.2
Fixes: 1143ec5ebf4
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/io.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 7e4cb74cf4..aba67f66b9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1399,7 +1399,7 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
  if (!(flags & BDRV_REQ_PREFETCH)) {
  qemu_iovec_from_buf(qiov, qiov_offset + progress,
  bounce_buffer + skip_bytes,
-pnum - skip_bytes);
+MIN(pnum - skip_bytes, bytes - progress));
  }
  } else if (!(flags & BDRV_REQ_PREFETCH)) {
  /* Read directly into the destination */


Even if I don't understand the bug, the tighter bound seems provably
correct anyway, so...

Reviewed-by: John Snow 



Thanks!

--
Best regards,
Vladimir

Re: [PATCH v9 00/10] error: auto propagated local_err part I

[Vladimir Sementsov-Ogievskiy]

12.03.2020 17:24, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


v9
01: A lot of rewordings [thanks to Eric]
 Still, keep all r-b marks, assuming that they are mostly about macro 
definition
02: significant changes are:
 1. Do not match double propagation pattern in ERRP_AUTO_PROPAGATE-adding 
rule
 2. Introduce errp->->errp scheme to match only functions matched by 
rule1
in rules inherited from rule1
 3. Add rules to warn about unusual patterns

 Also, add line to MAINTAINERS to keep error related coccinelle scripts 
under
 Error section.
07: add Christian's r-b
09: add Eric's r-b
10: a bit of context in xen_block_iothread_create  and qmp_object_add()
 signature are changed. Patch change is obvious, so I keep Paul's r-b

v9 is available at
  https://src.openvz.org/scm/~vsementsov/qemu.git #tag 
up-auto-local-err-partI-v9


Did you forget to push the tag?


Seems I've pushed it to wrong remote. Done now.




v8 is available at
  https://src.openvz.org/scm/~vsementsov/qemu.git #tag 
up-auto-local-err-partI-v8

[...]




--
Best regards,
Vladimir

Re: [PATCH v9 02/10] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE()

[Vladimir Sementsov-Ogievskiy]

12.03.2020 19:36, Markus Armbruster wrote:

I may have a second look tomorrow with fresher eyes, but let's get this
out now as is.

Vladimir Sementsov-Ogievskiy  writes:


Script adds ERRP_AUTO_PROPAGATE macro invocation where appropriate and
does corresponding changes in code (look for details in
include/qapi/error.h)

Usage example:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
  --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff \
  --max-width 80 FILES...

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Cc: Eric Blake 
Cc: Kevin Wolf 
Cc: Max Reitz 
Cc: Greg Kurz 
Cc: Christian Schoenebeck 
Cc: Stefano Stabellini 
Cc: Anthony Perard 
Cc: Paul Durrant 
Cc: Stefan Hajnoczi 
Cc: "Philippe Mathieu-Daudé" 
Cc: Laszlo Ersek 
Cc: Gerd Hoffmann 
Cc: Stefan Berger 
Cc: Markus Armbruster 
Cc: Michael Roth 
Cc: qemu-de...@nongnu.org
Cc: qemu-block@nongnu.org
Cc: xen-de...@lists.xenproject.org

  scripts/coccinelle/auto-propagated-errp.cocci | 327 ++
  include/qapi/error.h  |   3 +
  MAINTAINERS   |   1 +
  3 files changed, 331 insertions(+)
  create mode 100644 scripts/coccinelle/auto-propagated-errp.cocci

diff --git a/scripts/coccinelle/auto-propagated-errp.cocci 
b/scripts/coccinelle/auto-propagated-errp.cocci
new file mode 100644
index 00..7dac2dcfa4
--- /dev/null
+++ b/scripts/coccinelle/auto-propagated-errp.cocci
@@ -0,0 +1,327 @@
+// Use ERRP_AUTO_PROPAGATE (see include/qapi/error.h)
+//
+// Copyright (c) 2020 Virtuozzo International GmbH.
+//
+// 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 program.  If not, see
+// .
+//
+// Usage example:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place \
+//  --no-show-diff --max-width 80 FILES...
+//
+// Note: --max-width 80 is needed because coccinelle default is less
+// than 80, and without this parameter coccinelle may reindent some
+// lines which fit into 80 characters but not to coccinelle default,
+// which in turn produces extra patch hunks for no reason.


This is about unwanted reformatting of parameter lists due to the ___
chaining hack.  --max-width 80 makes that less likely, but not
impossible.

We can search for unwanted reformatting of parameter lists.  I think
grepping diffs for '^\+.*Error \*\*' should do the trick.  For the whole
tree, I get one false positive (not a parameter list), and one hit:

 @@ -388,8 +388,10 @@ static void object_post_init_with_type(O
  }
  }

 -void object_apply_global_props(Object *obj, const GPtrArray *props, Error 
**errp)
 +void object_apply_global_props(Object *obj, const GPtrArray *props,
 +   Error **errp)
  {
 +ERRP_AUTO_PROPAGATE();
  int i;

  if (!props) {

Reformatting, but not unwanted.


Yes, I saw it. This line is 81 character length, so it's OK to fix it in one 
hunk with
ERRP_AUTO_PROPAGATE addition even for non-automatic patch.



The --max-width 80 hack is good enough for me.

It does result in slightly long transformed lines, e.g. this one in
replication.c:

 @@ -113,7 +113,7 @@ static int replication_open(BlockDriverS
  s->mode = REPLICATION_MODE_PRIMARY;
  top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
  if (top_id) {
 -error_setg(_err, "The primary side does not support option 
top-id");
 +error_setg(errp, "The primary side does not support option 
top-id");
  goto fail;
  }
  } else if (!strcmp(mode, "secondary")) {

v8 did break this line (that's how I found it).  However, v9 still
shortens the line, just not below the target.  All your + lines look
quite unlikely to lengthen lines.  Let's not worry about this.


+// Switch unusual Error ** parameter names to errp
+// (this is necessary to use ERRP_AUTO_PROPAGATE).
+//
+// Disable optional_qualifier to skip functions with
+// "Error *const *errp" parameter.
+//
+// Skip functions with "assert(_errp && *_errp)" statement, because
+// that signals unusual semantics, and the parameter name may well
+// serve a purpose. (like nbd_iter_channel_error()).
+//
+// Skip util/error.c to not touch, for example, error_propagate() and
+// error_propagate_prepend().
+@ depends on !(file in "util/error.c")