Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Thomas Huth]

On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>>> property
>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>>> added the
>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>>> property.
>>>
>>> Check for use_acpi_pci_hotplug before calling
>>> acpi_pcihp_device_[un]plug_cb().
[...]
>>> Reported-by: Thomas Huth 
>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>
>> Looks like this is a very old bug, isn't it?
>> Objections to merging this after the release?
> 
> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> let him decide if it is worth crying wolf :) It's very likely no-one but
> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> plugging AHCI devices :D

I'm fine if this gets included in 2.11 - it's quite unlikely that a user
tries hot-plug ahci on such an old machine type, I think. But we maybe
should include this in the 2.10.1 stable release, so I'm putting
qemu-stable on CC now.

Anyway, your patch seems to fix the issue for me, thanks!

Tested-by: Thomas Huth 

Re: [Qemu-block] [PATCH 07/10] io: add qio_channel_read/write_all

[Fam Zheng]

On Tue, 08/22 15:18, Paolo Bonzini wrote:
> It is pretty common to read a fixed-size buffer from a socket.  Add a
> function that does this, either with multiple reads (on blocking sockets)
> or by yielding if called from a coroutine.
> 
> Cc: Daniel P. Berrange 
> Signed-off-by: Paolo Bonzini 
> ---
>  include/io/channel.h | 36 ++-
>  io/channel.c | 54 
> 
>  2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index db9bb022a1..9cfb4d081f 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -299,7 +299,7 @@ ssize_t qio_channel_writev(QIOChannel *ioc,
> Error **errp);
>  
>  /**
> - * qio_channel_readv:
> + * qio_channel_read:
>   * @ioc: the channel object
>   * @buf: the memory region to read data into
>   * @buflen: the length of @buf
> @@ -315,6 +315,23 @@ ssize_t qio_channel_read(QIOChannel *ioc,
>   Error **errp);
>  
>  /**
> + * qio_channel_read_all:
> + * @ioc: the channel object
> + * @buf: the memory region to read data into
> + * @buflen: the number of bytes to @buf
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Reads @buflen bytes into @buf, possibly blocking or (if the
> + * channel is non-blocking) yielding from the current coroutine
> + * multiple times until the entire content is read.  Otherwise
> + * behaves as qio_channel_read().
> + */
> +ssize_t coroutine_fn qio_channel_read_all(QIOChannel *ioc,
> +  char *buf,
> +  size_t buflen,
> +  Error **errp);
> +
> +/**
>   * qio_channel_write:
>   * @ioc: the channel object
>   * @buf: the memory regions to send data from
> @@ -331,6 +348,23 @@ ssize_t qio_channel_write(QIOChannel *ioc,
>Error **errp);
>  
>  /**
> + * qio_channel_write_all:
> + * @ioc: the channel object
> + * @buf: the memory region to write data into
> + * @buflen: the number of bytes to @buf
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Writes @buflen bytes from @buf, possibly blocking or (if the
> + * channel is non-blocking) yielding from the current coroutine
> + * multiple times until the entire content is written.  Otherwise
> + * behaves as qio_channel_write().
> + */
> +ssize_t coroutine_fn qio_channel_write_all(QIOChannel *ioc,
> +   const char *buf,
> +   size_t buflen,
> +   Error **errp);
> +
> +/**
>   * qio_channel_set_blocking:
>   * @ioc: the channel object
>   * @enabled: the blocking flag state
> diff --git a/io/channel.c b/io/channel.c
> index 1cfb8b33a2..7ab3f4eede 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -113,6 +113,60 @@ ssize_t qio_channel_read(QIOChannel *ioc,
>  }
>  
>  
> +ssize_t qio_channel_read_all(QIOChannel *ioc,
> + char *buf,
> + size_t buflen,
> + Error **errp)
> +{
> +ssize_t total = 0;
> +while (buflen > 0) {
> +ssize_t n_read = qio_channel_read(ioc, buf, buflen, errp);
> +
> +if (n_read == QIO_CHANNEL_ERR_BLOCK) {
> +assert(ioc->ctx);
> +qio_channel_yield(ioc, G_IO_IN);
> +continue;
> +}
> +if (n_read < 0) {
> +return n_read;
> +}
> +
> +buf += n_read;
> +total += n_read;
> +buflen -= n_read;
> +}
> +
> +return total;
> +}
> +
> +
> +ssize_t qio_channel_write_all(QIOChannel *ioc,
> +  const char *buf,
> +  size_t buflen,
> +  Error **errp)
> +{
> +ssize_t total = 0;
> +while (buflen > 0) {
> +ssize_t n_written = qio_channel_write(ioc, buf, buflen, errp);
> +
> +if (n_written == QIO_CHANNEL_ERR_BLOCK) {
> +assert(ioc->ctx);
> +qio_channel_yield(ioc, G_IO_OUT);
> +continue;
> +}
> +if (n_written < 0) {
> +return n_written;
> +}
> +
> +buf += n_written;
> +total += n_written;
> +buflen -= n_written;
> +}
> +
> +return total;
> +}
> +
> +
>  ssize_t qio_channel_write(QIOChannel *ioc,
>const char *buf,
>size_t buflen,
> -- 
> 2.13.5
> 
> 

Are there any caller depending on short read/write? Is it okay to change all (or
most) qio_channel_{read,write}* functions to this "full read/write" behavior?

Fam

Re: [Qemu-block] [PATCH 09/10] scsi: add multipath support to qemu-pr-helper

[Fam Zheng]

On Tue, 08/22 15:18, Paolo Bonzini wrote:
> Proper support of persistent reservation for multipath devices requires
> communication with the multipath daemon, so that the reservation is
> registered and applied when a path comes up.  The device mapper
> utilities provide a library to do so; this patch makes qemu-pr-helper.c
> detect multipath devices and, when one is found, delegate the operation
> to libmpathpersist.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  Makefile  |   3 +
>  configure |  57 -
>  docs/pr-manager.rst   |  27 +
>  include/scsi/utils.h  |   6 +
>  scsi/qemu-pr-helper.c | 311 
> +-
>  scsi/utils.c  |  15 +++
>  6 files changed, 414 insertions(+), 5 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index bfd4f69ecd..f1acaad05b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -388,6 +388,9 @@ fsdev/virtfs-proxy-helper$(EXESUF): 
> fsdev/virtfs-proxy-helper.o fsdev/9p-marshal
>  fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
>  
>  scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
> $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
> +ifdef CONFIG_MPATH
> +scsi/qemu-pr-helper$(EXESUF): LIBS += -ludev -lmultipath -lmpathpersist
> +endif
>  
>  qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
>   $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
> $@,"GEN","$@")
> diff --git a/configure b/configure
> index 772aff18d6..d3c9371f7c 100755
> --- a/configure
> +++ b/configure
> @@ -286,6 +286,7 @@ pixman=""
>  sdl=""
>  sdlabi=""
>  virtfs=""
> +mpath=""

Whole patch: s/\/pr-helper/ ?

>  vnc="yes"
>  sparse="no"
>  vde=""
> @@ -948,6 +949,10 @@ for opt do
>;;
>--enable-virtfs) virtfs="yes"
>;;
> +  --disable-mpath) mpath="no"
> +  ;;
> +  --enable-mpath) mpath="yes"
> +  ;;
>--disable-vnc) vnc="no"
>;;
>--enable-vnc) vnc="yes"
> @@ -1491,6 +1496,7 @@ disabled with --disable-FEATURE, default is enabled if 
> available:
>vnc-png PNG compression for VNC server
>cocoa   Cocoa UI (Mac OS X only)
>virtfs  VirtFS
> +  mpath   Multipath persistent reservation passthrough
>xen xen backend driver support
>xen-pci-passthrough
>brlapi  BrlAPI (Braile)
> @@ -3336,6 +3342,29 @@ else
>  fi
>  
>  ##
> +# libmpathpersist probe
> +
> +if test "$mpath" != "no" ; then
> +  cat > $TMPC < +#include 
> +#include 
> +unsigned mpath_mx_alloc_len = 1024;
> +int logsink;
> +int main(void) {
> +struct udev *udev = udev_new();
> +mpath_lib_init(udev);
> +}
> +EOF
> +  if compile_prog "" "-ludev -lmultipath -lmpathpersist" ; then
> +mpathpersist=yes
> +  else
> +mpathpersist=no
> +  fi
> +else
> +  mpathpersist=no
> +fi
> +
> +##
>  # libcap probe
>  
>  if test "$cap" != "no" ; then
> @@ -5070,16 +5099,34 @@ if test "$want_tools" = "yes" ; then
>fi
>  fi
>  if test "$softmmu" = yes ; then
> -  if test "$virtfs" != no ; then
> -if test "$cap" = yes && test "$linux" = yes && test "$attr" = yes ; then
> +  if test "$linux" = yes; then
> +if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
>virtfs=yes
>tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
>  else
>if test "$virtfs" = yes; then
> -error_exit "VirtFS is supported only on Linux and requires libcap 
> devel and libattr devel"
> +error_exit "VirtFS requires libcap devel and libattr devel"
>fi
>virtfs=no
>  fi
> +if test "$mpath" != no && test "$mpathpersist" = yes ; then
> +  mpath=yes
> +  tools="$tools mpath/qemu-mpath-helper\$(EXESUF)"

scsi/qemu-pr-helper?

> +else
> +  if test "$mpath" = yes; then
> +error_exit "Multipath requires libmpathpersist devel"
> +  fi
> +  mpath=no
> +fi
> +  else
> +if test "$virtfs" = yes; then
> +  error_exit "VirtFS is supported only on Linux"
> +fi
> +virtfs=no
> +if test "$mpath" = yes; then
> +  error_exit "Multipath is supported only on Linux"
> +fi
> +mpath=no
>fi
>  fi
>  
> @@ -5326,6 +5373,7 @@ echo "Audio drivers $audio_drv_list"
>  echo "Block whitelist (rw) $block_drv_rw_whitelist"
>  echo "Block whitelist (ro) $block_drv_ro_whitelist"
>  echo "VirtFS support$virtfs"
> +echo "Multipath support $mpath"
>  echo "VNC support   $vnc"
>  if test "$vnc" = "yes" ; then
>  echo "VNC SASL support  $vnc_sasl"
> @@ -5773,6 +5821,9 @@ fi
>  if test "$virtfs" = "yes" ; then
>echo "CONFIG_VIRTFS=y" >> $config_host_mak
>  fi
> +if test "$mpath" = "yes" ; then
> +  echo "CONFIG_MPATH=y" >> $config_host_mak
> +fi
>  if test "$vhost_scsi" = "yes" ; then
>echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
>  fi

Fam

Re: [Qemu-block] [PATCH 10/10] scsi: add persistent reservation manager using qemu-pr-helper

[Fam Zheng]

On Tue, 08/22 15:18, Paolo Bonzini wrote:
> This adds a concrete subclass of pr-manager that talks to qemu-pr-helper.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  scsi/Makefile.objs   |   2 +-
>  scsi/pr-manager-helper.c | 288 
> +++
>  2 files changed, 289 insertions(+), 1 deletion(-)
>  create mode 100644 scsi/pr-manager-helper.c
> 
> diff --git a/scsi/Makefile.objs b/scsi/Makefile.objs
> index 5496d2ae6a..4d25e476cf 100644
> --- a/scsi/Makefile.objs
> +++ b/scsi/Makefile.objs
> @@ -1,3 +1,3 @@
>  block-obj-y += utils.o
>  
> -block-obj-$(CONFIG_LINUX) += pr-manager.o
> +block-obj-$(CONFIG_LINUX) += pr-manager.o pr-manager-helper.o
> diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
> new file mode 100644
> index 00..c9d9606696
> --- /dev/null
> +++ b/scsi/pr-manager-helper.c
> @@ -0,0 +1,288 @@
> +/*
> + * Persistent reservation manager that talks to qemu-mpath-helper

s/qemu-mpath-helper/qemu-pr-helper/

> + *
> + * Copyright (c) 2017 Red Hat, Inc.

Since I'm commenting on a header, just BTW I was educated that (c) stands for
"copyright" so this is actually a ubiquitous redundancy. :)

> + *
> + * Author: Paolo Bonzini 
> + *
> + * This code is licensed under the LGPL.

Interesting, I think the version of the license is required?

> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "scsi/constants.h"
> +#include "scsi/pr-manager.h"
> +#include "scsi/utils.h"
> +#include "io/channel.h"
> +#include "io/channel-socket.h"
> +#include "pr-helper.h"
> +
> +#include 
> +
> +#define PR_MAX_RECONNECT_ATTEMPTS 5
> +
> +#define TYPE_PR_MANAGER_HELPER "pr-manager-helper"
> +
> +#define PR_MANAGER_HELPER(obj) \
> + INTERFACE_CHECK(PRManagerHelper, (obj), \
> + TYPE_PR_MANAGER_HELPER)
> +
> +typedef struct PRManagerHelper {
> +/*  */
> +PRManager parent;
> +
> +char *path;
> +
> +QemuMutex lock;
> +QIOChannel *ioc;
> +} PRManagerHelper;
> +
> +/* Called with lock held.  */
> +static int pr_manager_helper_read(PRManagerHelper *pr_mgr,
> +  void *buf, int sz, Error **errp)
> +{
> +ssize_t r = qio_channel_read_all(pr_mgr->ioc, buf, sz, errp);
> +
> +if (r < 0) {
> +object_unref(OBJECT(pr_mgr->ioc));
> +pr_mgr->ioc = NULL;
> +return r;
> +}
> +
> +return r < 0 ? r : 0;
> +}
> +
> +/* Called with lock held.  */
> +static int pr_manager_helper_write(PRManagerHelper *pr_mgr,
> +   int fd,
> +   const void *buf, int sz, Error **errp)
> +{
> +size_t nfds = (fd != -1);
> +while (sz > 0) {
> +struct iovec iov;
> +ssize_t n_written;
> +
> +iov.iov_base = (void *)buf;
> +iov.iov_len = sz;
> +n_written = qio_channel_writev_full(QIO_CHANNEL(pr_mgr->ioc), , 
> 1,
> +nfds ?  : NULL, nfds, errp);
> +
> +if (n_written <= 0) {
> +assert(n_written != QIO_CHANNEL_ERR_BLOCK);
> +object_unref(OBJECT(pr_mgr->ioc));
> +pr_mgr->ioc = NULL;
> +return n_written;
> +}
> +
> +nfds = 0;
> +buf += n_written;
> +sz -= n_written;
> +}
> +
> +return 0;
> +}
> +
> +/* Called with lock held.  */
> +static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
> +Error **errp)
> +{
> +uint32_t flags;
> +
> +SocketAddress saddr = {
> +.type = SOCKET_ADDRESS_TYPE_UNIX,
> +.u.q_unix.path = g_strdup(pr_mgr->path)

Missing g_free()?

> +};
> +QIOChannelSocket *sioc = qio_channel_socket_new();
> +Error *local_err = NULL;
> +
> +qio_channel_set_name(QIO_CHANNEL(sioc), "pr-manager-helper");
> +qio_channel_socket_connect_sync(sioc,
> +,
> +_err);
> +if (local_err) {
> +object_unref(OBJECT(sioc));
> +error_propagate(errp, local_err);
> +return -ENOTCONN;
> +}
> +
> +qio_channel_set_delay(QIO_CHANNEL(sioc), false);
> +pr_mgr->ioc = QIO_CHANNEL(sioc);
> +
> +/* A simple feature negotation protocol, even though there is
> + * no optional feature right now.
> + */
> +if (pr_manager_helper_read(pr_mgr, , sizeof(flags), errp) < 0) {

Not returning the return value of pr_manager_helper_read()?.

> +return -EINVAL;
> +}
> +
> +flags = 0;
> +if (pr_manager_helper_write(pr_mgr, -1, , sizeof(flags), errp) < 
> 0) {
> +return -EINVAL;

Same here.

> +}
> +
> +return 0;
> +}
> +
> +static int pr_manager_helper_run(PRManager *p,
> + int fd, struct sg_io_hdr *io_hdr)
> +{
> +PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
> +
> +uint32_t len;
> +PRHelperResponse resp;
> +int ret;
> + 

Re: [Qemu-block] [PATCH 06/10] scsi, file-posix: add support for persistent reservation management

[Fam Zheng]

On Tue, 08/22 15:18, Paolo Bonzini wrote:
> diff --git a/docs/pr-manager.rst b/docs/pr-manager.rst

Is docs/interop/persistent-reservation-manager.rst better? (Move to interop/ and
de-abbreviate) ...

> new file mode 100644
> index 00..b6089fb57c
> --- /dev/null
> +++ b/docs/pr-manager.rst
> @@ -0,0 +1,51 @@
> +==
> +Persistent reservation managers
> +==
> +
> +SCSI persistent Reservations allow restricting access to block devices
> +to specific initiators in a shared storage setup.  When implementing
> +clustering of virtual machines, it is a common requirement for virtual
> +machines to send persistent reservation SCSI commands.  However,
> +the operating system restricts sending these commands to unprivileged
> +programs because incorrect usage can disrupt regular operation of the
> +storage fabric.
> +
> +For this reason, QEMU's SCSI passthrough devices, ``scsi-block``
> +and ``scsi-generic`` (both are only available on Linux) can delegate
> +implementation of persistent reservations to a separate object,
> +the "persistent reservation manager".  Only PERSISTENT RESERVE OUT and
> +PERSISTENT RESERVE IN commands are passed to the persistent reservation
> +manager object; other commands are processed by QEMU as usual.
> +
> +-
> +Defining a persistent reservation manager
> +-
> +
> +A persistent reservation manager is an instance of a subclass of the
> +"pr-manager" QOM class.

Or is this abstraction class the reason it is not under interop? Why not just
define the protocol?

> +
> +Right now only one subclass is defined, ``pr-manager-helper``, which
> +forwards the commands to an external privileged helper program
> +over Unix sockets.  The helper program only allows sending persistent
> +reservation commands to devices for which QEMU has a file descriptor,
> +so that QEMU will not be able to effect persistent reservations
> +unless it has access to both the socket and the device.
> +
> +``pr-manager-helper`` has a single string property, ``path``, which
> +accepts the path to the helper program's Unix socket.  For example,
> +the following command line defines a ``pr-manager-helper`` object and
> +attaches it to a SCSI passthrough device::
> +
> +  $ qemu-system-x86_64
> +  -device virtio-scsi \
> +  -object 
> pr-manager-helper,id=helper0,path=/var/run/qemu-pr-helper.sock
> +  -drive 
> if=none,id=hd,driver=raw,file.filename=/dev/sdb,file.pr-manager=helper0
> +  -device scsi-block,drive=hd
> +
> +Alternatively, using ``-blockdev``::
> +
> +  $ qemu-system-x86_64
> +  -device virtio-scsi \
> +  -object 
> pr-manager-helper,id=helper0,path=/var/run/qemu-pr-helper.sock
> +  -blockdev 
> node-name=hd,driver=raw,file.driver=host_device,file.filename=/dev/sdb,file.pr-manager=helper0
> +  -device scsi-block,drive=hd

Fam

Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Philippe Mathieu-Daudé]

On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:

On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:

9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
'use_acpi_pci_hotplug' for pc-1.7 and older machines.
c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
property.

Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().

If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.

The following valgrind Trace equivs:

   qdev_device_add( "ich9-ahci" )
-> device_set_realized()
 -> hotplug_handler_plug()
  -> piix4_device_plug_cb()
   -> acpi_pcihp_device_plug_cb()
-> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
-> object_unparent()
  <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"

$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604==at 0x609AB0: object_unparent (object.c:445)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)

Reported-by: Thomas Huth 
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 


Looks like this is a very old bug, isn't it?
Objections to merging this after the release?


Yes, I'm also inclined to delay it so we can release 2.10, I tagged 
"2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll 
let him decide if it is worth crying wolf :) It's very likely no-one but 
him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot 
plugging AHCI devices :D





---
  hw/acpi/piix4.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..d4df209a2e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler 
*hotplug_dev,
  dev, errp);
  }
  } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-if (!xen_enabled()) {
+if (s->use_acpi_pci_hotplug) {
  acpi_pcihp_device_plug_cb(hotplug_dev, >acpi_pci_hotplug, dev,
errp);
  }
@@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler 
*hotplug_dev,
  acpi_memory_unplug_request_cb(hotplug_dev, >acpi_memory_hotplug,
dev, errp);
  } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-if (!xen_enabled()) {
+if (s->use_acpi_pci_hotplug) {
  acpi_pcihp_device_unplug_cb(hotplug_dev, 

Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Michael S. Tsirkin]

On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
> 
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
> 
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
> 
> The following valgrind Trace equivs:
> 
>   qdev_device_add( "ich9-ahci" )
>-> device_set_realized()
> -> hotplug_handler_plug()
>  -> piix4_device_plug_cb()
>   -> acpi_pcihp_device_plug_cb()
>-> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
>-> object_unparent()
>  <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
> 
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604==at 0x609AB0: object_unparent (object.c:445)
> ==6604==by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604==by 0x364311: monitor_read (monitor.c:3905)
> ==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
> free'd
> ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==  Block was alloc'd at
> ==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> 
> Reported-by: Thomas Huth 
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé 

Looks like this is a very old bug, isn't it?
Objections to merging this after the release?

> ---
>  hw/acpi/piix4.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  dev, errp);
>  }
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -if (!xen_enabled()) {
> +if (s->use_acpi_pci_hotplug) {
>  acpi_pcihp_device_plug_cb(hotplug_dev, >acpi_pci_hotplug, dev,
>errp);
>  }
> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler 
> *hotplug_dev,
>  acpi_memory_unplug_request_cb(hotplug_dev, >acpi_memory_hotplug,
>dev, errp);
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -if (!xen_enabled()) {
> +if (s->use_acpi_pci_hotplug) {
>  acpi_pcihp_device_unplug_cb(hotplug_dev, >acpi_pci_hotplug, 
> dev,
>  errp);
>  }
> -- 
> 2.14.1

Re: [Qemu-block] [Qemu-devel] Use after free problem somewhere in ahci.c or ich.c code

[John Snow]

On 08/22/2017 05:02 PM, Philippe Mathieu-Daudé wrote:
> On 08/22/2017 03:39 PM, John Snow wrote:
>> On 08/22/2017 02:15 PM, Thomas Huth wrote:
>>>
>>> Looks like there is a use-after-free problem somewhere in
>>> the ahci.c or ich.c code when trying to add the ich9-ahci
>>> on a old PC machine. Using valgrind, I get:
>>>
> 
> those old PC don't support AHCI hotplug, so realize() fails then
> unparent() is called.
> 
>> I'll look; it looks like it works okay for pc-i440fx-2.9 as well as 2.0
>> and 1.7.
>>
>> 1.6 appears to be the most modern board that has issues, as well as 1.4
>> and the pc-1.2 board you specify.
> 
> commit 9e047b982452 "piix4: add acpi pci hotplug support"
> 
> "Add support for acpi pci hotplug using the new infrastructure.
> PIIX4 legacy interface is maintained as is for machine types 1.7 and
> older."
> 
> I see piix4_pm_init() disabling use_acpi_pci_hotplug if xen_enabled(),
> later when piix4_device_plug_cb() is called for TYPE_PCI_DEVICE it
> checks xen_enabled() instead of checking use_acpi_pci_hotplug.
> Same happens in piix4_device_unplug_request_cb(), not sure it can be
> reached although.
> 
> My guess is changing !xen_enabled() -> s->use_acpi_pci_hotplug fixes
> this issue, but I'm not sure this is the safest way to fix it.
> 
> I'll send a patch.
> 
> Regards,
> 
> Phil.

Beat me to it! I'll review, thanks.

[Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Philippe Mathieu-Daudé]

9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
'use_acpi_pci_hotplug' for pc-1.7 and older machines.
c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
property.

Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().

If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.

The following valgrind Trace equivs:

  qdev_device_add( "ich9-ahci" )
   -> device_set_realized()
-> hotplug_handler_plug()
 -> piix4_device_plug_cb()
  -> acpi_pcihp_device_plug_cb()
   -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
   -> object_unparent()
 <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"

$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604==at 0x609AB0: object_unparent (object.c:445)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)

Reported-by: Thomas Huth 
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/acpi/piix4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..d4df209a2e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler 
*hotplug_dev,
 dev, errp);
 }
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-if (!xen_enabled()) {
+if (s->use_acpi_pci_hotplug) {
 acpi_pcihp_device_plug_cb(hotplug_dev, >acpi_pci_hotplug, dev,
   errp);
 }
@@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler 
*hotplug_dev,
 acpi_memory_unplug_request_cb(hotplug_dev, >acpi_memory_hotplug,
   dev, errp);
 } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-if (!xen_enabled()) {
+if (s->use_acpi_pci_hotplug) {
 acpi_pcihp_device_unplug_cb(hotplug_dev, >acpi_pci_hotplug, dev,
 errp);
 }
-- 
2.14.1

Re: [Qemu-block] [Qemu-devel] Use after free problem somewhere in ahci.c or ich.c code

[Philippe Mathieu-Daudé]

On 08/22/2017 03:39 PM, John Snow wrote:

On 08/22/2017 02:15 PM, Thomas Huth wrote:


Looks like there is a use-after-free problem somewhere in
the ahci.c or ich.c code when trying to add the ich9-ahci
on a old PC machine. Using valgrind, I get:



those old PC don't support AHCI hotplug, so realize() fails then 
unparent() is called.



I'll look; it looks like it works okay for pc-i440fx-2.9 as well as 2.0
and 1.7.

1.6 appears to be the most modern board that has issues, as well as 1.4
and the pc-1.2 board you specify.


commit 9e047b982452 "piix4: add acpi pci hotplug support"

"Add support for acpi pci hotplug using the new infrastructure.
PIIX4 legacy interface is maintained as is for machine types 1.7 and older."

I see piix4_pm_init() disabling use_acpi_pci_hotplug if xen_enabled(),
later when piix4_device_plug_cb() is called for TYPE_PCI_DEVICE it 
checks xen_enabled() instead of checking use_acpi_pci_hotplug.
Same happens in piix4_device_unplug_request_cb(), not sure it can be 
reached although.


My guess is changing !xen_enabled() -> s->use_acpi_pci_hotplug fixes 
this issue, but I'm not sure this is the safest way to fix it.


I'll send a patch.

Regards,

Phil.

[Qemu-block] Persistent bitmaps for non-qcow2 formats

[John Snow]

Well, we knew we'd want this sooner or later. I've got some pings
downstream over whether or not we support persistent bitmaps for
non-qcow2 formats.

Currently: no, we don't.

We tried two different ideas for storage agnostic bitmap persistence:


(1) Using qcow2 as a storage container format (similar to VM save
states) for information not associated with that particular drive.

This didn't go over so well; we decided it was too messy logistically to
manage data in any meaningful sense in a file that didn't share any
semantic relationship to the qcow2 in question.

Well, "We" decided is more like "Kevin and Max" decided. They are right,
though.


(2) Using a new proto-wrapper format that Fam Zheng designed that serves
as a simple top-layer redirect for metadata that allows us to associate
raw bitmap data with an arbitrary backing image.

This also didn't go over so well, the desire for a "new format" was
pretty thin, even if it was only a pseudo-format.


We'd still like to be able to use bitmaps with non-qcow2 formats
however, and people are going to keep asking. So here's a third thought:


(3) Add either a new flag that turns qcow2's backing file into a full
R/W backing file, or add a new extension to qcow2 entirely (bypassing
the traditional backing file mechanism to avoid confusion for older
tooling) that adds a new read-write backing file field.

This RW backing file field will be used for all reads AND writes; the
qcow2 in question becomes a metadata container on top of the BDS chain.
We can re-use Vladimir's bitmap persistence extension to save bitmaps in
a qcow2 shell.

The qcow2 becomes effectively a metadata cache for a new (essentially)
filter node that handles features such as bitmaps. This could also be
used to provide allocation map data for RAW files and other goodies down
the road.

Hopefully this achieves our desire to not create new formats AND our
desire to concentrate features (and debugging, testing, etc) into qcow2,
while allowing users to "have bitmaps with raw files."

Of course, in this scenario, users now have two files: a qcow2 wrapper
and the actual raw file in question; but regardless of how we were going
to solve this, a raw file necessitates an external file of some sort,
else we give up the idea that it was a raw file.


Sound good?

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system

[John Snow]

On 08/08/2017 04:00 PM, Eric Blake wrote:
> On 08/08/2017 01:32 PM, John Snow wrote:
>> Out with the old, in with the new.
>>
>> Signed-off-by: John Snow 
>> ---
> 
>>  hw/ide/piix.c | 11 
>>  hw/ide/trace-events   | 33 
>>  hw/ide/via.c  | 10 +++-
> 
> Hmm - should we tweak scripts/git.orderfile to prioritize trace-events
> over .c files? Then again, right now it prioritizes all .c files before
> anything that didn't match, so that things like trace-events will at
> least avoid falling in the middle of a patch if you use the project's
> orderfile.
> 
>> +++ b/hw/ide/cmd646.c
>> @@ -32,6 +32,7 @@
>>  #include "sysemu/dma.h"
>>  
>>  #include "hw/ide/pci.h"
>> +#include "trace.h"
>>  
>>  /* CMD646 specific */
>>  #define CFR 0x50
>> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>  val = 0xff;
>>  break;
>>  }
>> -#ifdef DEBUG_IDE
>> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val);
>> -#endif
> 
> Yay for killing code prone to bitrot.
> 
>> +++ b/hw/ide/core.c
> 
>> @@ -2054,18 +2044,18 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>  }
> 
>>  hob = 0;
>> -switch(addr) {
>> +switch(reg_num) {
> 
> Worth fixing the style to put space after switch while touching this?
> 
>> +++ b/hw/ide/trace-events
>> @@ -0,0 +1,33 @@
>> +# See docs/devel/tracing.txt for syntax documentation.
>> +
>> +# hw/ide/core.c
> 
>> +
>> +# hw/ide/pci.c
>> +bmdma_reset(void) ""
> 
> An empty trace? Do all the backends support it?
> 

Not the first instance of this, so I'm assuming yes.

Re: [Qemu-block] [Qemu-devel] Use after free problem somewhere in ahci.c or ich.c code

[John Snow]

On 08/22/2017 02:15 PM, Thomas Huth wrote:
> 
>  Hi!
> 
> Looks like there is a use-after-free problem somewhere in
> the ahci.c or ich.c code when trying to add the ich9-ahci
> on a old PC machine. Using valgrind, I get:
> 

I'll look; it looks like it works okay for pc-i440fx-2.9 as well as 2.0
and 1.7.

1.6 appears to be the most modern board that has issues, as well as 1.4
and the pc-1.2 board you specify.

> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> ==6604== Memcheck, a memory error detector
> ==6604== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
> ==6604== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
> ==6604== Command: x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> ==6604== 
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604==at 0x609AB0: object_unparent (object.c:445)
> ==6604==by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604==by 0x364311: monitor_read (monitor.c:3905)
> ==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
> free'd
> ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==  Block was alloc'd at
> ==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== 
> ==6604== Invalid read of size 8
> ==6604==at 0x60A350: object_finalize_child_property (object.c:1395)
> ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604==by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604==by 0x364311: monitor_read (monitor.c:3905)
> ==6604==  Address 0x15fc5428 is 30,296 bytes inside a block of size 36,288 
> free'd
> ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==  Block was alloc'd 

[Qemu-block] Use after free problem somewhere in ahci.c or ich.c code

[Thomas Huth]

 Hi!

Looks like there is a use-after-free problem somewhere in
the ahci.c or ich.c code when trying to add the ich9-ahci
on a old PC machine. Using valgrind, I get:

$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
==6604== Memcheck, a memory error detector
==6604== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==6604== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==6604== Command: x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
==6604== 
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604==at 0x609AB0: object_unparent (object.c:445)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== 
==6604== Invalid read of size 8
==6604==at 0x60A350: object_finalize_child_property (object.c:1395)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x4C4478: device_unparent (qdev.c:1095)
==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604==by 0x364311: monitor_read (monitor.c:3905)
==6604==  Address 0x15fc5428 is 30,296 bytes inside a block of size 36,288 
free'd
==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604==by 0x608DCD: property_set_bool (object.c:1886)
==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
==6604==  Block was alloc'd at
==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604==by 0x50094F: ahci_realize (ahci.c:1468)
==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
==6604==by 0x608DCD: property_set_bool 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 06/54] qapi: introduce qapi_enum_lookup()

[John Snow]

On 08/22/2017 09:22 AM, Marc-André Lureau wrote:
> This will help with the introduction of a new structure to handle
> enum lookup.
> 

Procedurally for the sake of review, it's a little odd to introduce the
function, deploy it, and then change it and update all callers.

content-wise, I'm really glad to have a function like this that we can
use universally.

ACK (6, 12)

Re: [Qemu-block] [Qemu-devel] [PATCH v7 0/4] Add shrink image for qcow2

[Max Reitz]

On 2017-08-22 01:31, John Snow wrote:
> 
> 
> On 08/17/2017 05:15 AM, Pavel Butsykin wrote:
>> This patch add shrinking of the image file for qcow2. As a result, this 
>> allows
>> us to reduce the virtual image size and free up space on the disk without
>> copying the image. Image can be fragmented and shrink is done by punching 
>> holes
>> in the image file.
>>
>> # ./qemu-img create -f qcow2 image.qcow2 4G
>> Formatting 'image.qcow2', fmt=qcow2 size=4294967296 encryption=off 
>> cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>
>> # ./qemu-io -c "write -P 0x22 0 1G" image.qcow2
>> wrote 1073741824/1073741824 bytes at offset 0
>> 1 GiB, 1 ops; 0:00:04.59 (222.886 MiB/sec and 0.2177 ops/sec)
>>
>> # ./qemu-img resize image.qcow2 512M
>> warning: qemu-img: Shrinking an image will delete all data beyond the 
>> shrunken image's end. Before performing such an operation, make sure there 
>> is no important data there.
>> error: qemu-img: Use the --shrink option to perform a shrink operation.
>>
>> # ./qemu-img resize --shrink image.qcow2 128M
>> Image resized.
>>
>> # ./qemu-img info image.qcow2
>> image: image.qcow2
>> file format: qcow2
>> virtual size: 128M (134217728 bytes)
>> disk size: 128M
>> cluster_size: 65536
>> Format specific information:
>> compat: 1.1
>> lazy refcounts: false
>> refcount bits: 16
>> corrupt: false
>>
>> # du -h image.qcow2
>> 129Mimage.qcow2
>>
> 
> It looks sane to me, and already has a full set of R-Bs from Max. Are we
> waiting for Kevin?

We were waiting for Kevin to handle 2.10 patches so he could go into PTO
and for me to come back from PTO. ;-)

I'm still sifting through my inbox... When I'm done, I'll take a look,
but I have no idea how long that might last.
(Since 2.10 isn't out yet, there might be more pressing issues still...?
I don't quite now yet, to be honest...)

> It looks like in Vladimir's series we sidestepped the problem of what
> happens if we resize with persistent bitmaps: we deny the operation,
> regardless of if we are shrinking, growing, or have bitmaps that are
> read-only, frozen, or what-have-you.
> 
> It shouldn't be too hard to add soon, but this is fine for now.
> (I think for adding it we just need to make sure the bitmaps aren't
> frozen and are not read-only, and the implementation bitmap structure
> can already handle truncation in either direction just fine.)
> 
> Anyway;
> 
> Reviewed-by: John Snow 

Thanks, in any case. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 08/10] scsi: build qemu-pr-helper

[Paolo Bonzini]

On 22/08/2017 16:34, Marc-André Lureau wrote:
> Could this be handled by udisk? It seems at first the problem is not
> specific to qemu.

Yes, possibly.  In practice, everybody else who uses persistent
reservations seems to run as root. :)

>> +static void usage(const char *name)
>> +{
>> +(printf) (
> Why '(printf)' ? I noticed qemu-nbd use this too.
> 

It lets you use #ifdef inside the argument list.

>>
>> +/* Stash one file descriptor per request.  */
>> +if (nfds) {
>> +for (i = 0; i < nfds; i++) {
>> +if (client->fd == -1) {
>> +client->fd = fds[i++];
>> +} else {
>> +close(fds[i]);
> 
> Isn't this a condition to close the client?

Yes, though I still have to go through the descriptors and close them.

Paolo

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Markus Armbruster]

Igor Mammedov  writes:

> On Tue, 22 Aug 2017 15:50:14 +0200
> Markus Armbruster  wrote:
>
>> Igor Mammedov  writes:
>> 
>> > On Mon,  7 Aug 2017 16:45:16 +0200
>> > Markus Armbruster  wrote:
>> >  
>> >> Sizes and addresses should use QAPI type 'size' (uint64_t).
>> >> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
>> >> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
>> >> 
>> >> Change these PCDIMMDeviceInfo members to 'size'.
>> >> 
>> >> query-memory-devices now reports sizes and addresses above 2^63-1
>> >> correctly instead of their (negative) two's complement.
>> >> 
>> >> HMP's "info memory-devices" already reported them correctly, because
>> >> it printed the signed integers with PRIx64 and PRIu32.  
>> > s/signed/unsigned/  
>> 
>> Before this patch: signed.  Afterwards: unsigned.  Would
>> 
>>HMP's "info memory-devices" already reported them correctly, because
>>it printed the signed (before the patch) integers with PRIx64 and
>>PRIu32.
>> 
>> be clearer?
> yes, that's more clear

Okay, I'll update my commit messages.  Thanks!

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 04/56] char: Make ringbuf-read size unsigned in QAPI/QMP

[Marc-André Lureau]

Hi

On Tue, Aug 22, 2017 at 3:00 PM, Markus Armbruster  wrote:
> Marc-André Lureau  writes:
>
>> Hi
>>
>> On Mon, Aug 7, 2017 at 4:45 PM, Markus Armbruster  wrote:
>>> Sizes should use QAPI type 'size' (uint64_t).  ringbuf-read parameter
>>> @size is 'int' (int64_t).  qmp_ringbuf_read() rejects negative values,
>>> then implicitly converts to size_t.
>>>
>>> Change the parameter to 'size' and drop the check for negative values.
>>>
>>> ringbuf-read now accepts size values between 2^63 and 2^64-1.  It
>>> accepts negative values as before, because that's how the QObject
>>> input visitor works for backward compatibility.
>>>
>>
>> Negative values over json will be implicitly converted to positive
>> values with this change, right? Or are they rejected earlier?
>
> Yes.  For details, see my reply to Juan's review of PATCH 15.
>
>> If so that is a change of behaviour that I am not sure is worth doing
>> now (without explicit protocol break), but I don't mind.

So before this change:

(QEMU) ringbuf-read device=foo size=-1
{"error": {"class": "GenericError", "desc": "size must be greater than zero"}}

after:

(QEMU) ringbuf-read device=foo size=-1
{"return": ""}

Is this really what we want?

-- 
Marc-André Lureau

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Igor Mammedov]

On Tue, 22 Aug 2017 15:50:14 +0200
Markus Armbruster  wrote:

> Igor Mammedov  writes:
> 
> > On Mon,  7 Aug 2017 16:45:16 +0200
> > Markus Armbruster  wrote:
> >  
> >> Sizes and addresses should use QAPI type 'size' (uint64_t).
> >> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
> >> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
> >> 
> >> Change these PCDIMMDeviceInfo members to 'size'.
> >> 
> >> query-memory-devices now reports sizes and addresses above 2^63-1
> >> correctly instead of their (negative) two's complement.
> >> 
> >> HMP's "info memory-devices" already reported them correctly, because
> >> it printed the signed integers with PRIx64 and PRIu32.  
> > s/signed/unsigned/  
> 
> Before this patch: signed.  Afterwards: unsigned.  Would
> 
>HMP's "info memory-devices" already reported them correctly, because
>it printed the signed (before the patch) integers with PRIx64 and
>PRIu32.
> 
> be clearer?
yes, that's more clear

Thanks.

> 
> >> Signed-off-by: Markus Armbruster   
> > Reviewed-by: Igor Mammedov   
> 
> Thanks!

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 04/56] char: Make ringbuf-read size unsigned in QAPI/QMP

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Tue, Aug 22, 2017 at 3:00 PM, Markus Armbruster  wrote:
>> Marc-André Lureau  writes:
>>
>>> Hi
>>>
>>> On Mon, Aug 7, 2017 at 4:45 PM, Markus Armbruster  wrote:
 Sizes should use QAPI type 'size' (uint64_t).  ringbuf-read parameter
 @size is 'int' (int64_t).  qmp_ringbuf_read() rejects negative values,
 then implicitly converts to size_t.

 Change the parameter to 'size' and drop the check for negative values.

 ringbuf-read now accepts size values between 2^63 and 2^64-1.  It
 accepts negative values as before, because that's how the QObject
 input visitor works for backward compatibility.

>>>
>>> Negative values over json will be implicitly converted to positive
>>> values with this change, right? Or are they rejected earlier?
>>
>> Yes.  For details, see my reply to Juan's review of PATCH 15.
>>
>>> If so that is a change of behaviour that I am not sure is worth doing
>>> now (without explicit protocol break), but I don't mind.
>
> So before this change:
>
> (QEMU) ringbuf-read device=foo size=-1
> {"error": {"class": "GenericError", "desc": "size must be greater than zero"}}
>
> after:
>
> (QEMU) ringbuf-read device=foo size=-1
> {"return": ""}
>
> Is this really what we want?

Yes, because it's what all the other commands do with byte counts, and
because it's the price of admission for "size": 18446744073709551615 to
work.

We could split QAPI type size into legacy-size (accepts negative for
backward compatibility, do not use in new code) and size (rejects
negative, do use in new code and for cases that previously rejected
negative manually).  Trades consistency for tightness.

This series picked consistency.

Re: [Qemu-block] [PATCH] nbd-client: avoid spurious qio_channel_yield() re-entry

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefa...@redhat.com) wrote:
> The following scenario leads to an assertion failure in
> qio_channel_yield():
> 
> 1. Request coroutine calls qio_channel_yield() successfully when sending
>would block on the socket.  It is now yielded.
> 2. nbd_read_reply_entry() calls nbd_recv_coroutines_enter_all() because
>nbd_receive_reply() failed.
> 3. Request coroutine is entered and returns from qio_channel_yield().
>Note that the socket fd handler has not fired yet so
>ioc->write_coroutine is still set.
> 4. Request coroutine attempts to send the request body with nbd_rwv()
>but the socket would still block.  qio_channel_yield() is called
>again and assert(!ioc->write_coroutine) is hit.
> 
> The problem is that nbd_read_reply_entry() does not distinguish between
> request coroutines that are waiting to receive a reply and those that
> are not.
> 
> This patch adds a per-request bool receiving flag so
> nbd_read_reply_entry() can avoid spurious aio_wake() calls.
> 
> Reported-by: Dr. David Alan Gilbert 
> Signed-off-by: Stefan Hajnoczi 

With that patch that assert does seem to go away; just leaving the 
other failure we're seeing.

Dave

> ---
> This should fix the issue that Dave is seeing but I'm concerned that
> there are more problems in nbd-client.c.  We don't have good
> abstractions for writing coroutine socket I/O code.  Something like Go's
> channels would avoid manual low-level coroutine calls.  There is
> currently no way to cancel qio_channel_yield() so requests doing I/O may
> remain in-flight indefinitely and nbd-client.c doesn't join them...
> 
>  block/nbd-client.h |  7 ++-
>  block/nbd-client.c | 35 ++-
>  2 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index 1935ffbcaa..b435754b82 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -17,6 +17,11 @@
>  
>  #define MAX_NBD_REQUESTS16
>  
> +typedef struct {
> +Coroutine *coroutine;
> +bool receiving; /* waiting for read_reply_co? */
> +} NBDClientRequest;
> +
>  typedef struct NBDClientSession {
>  QIOChannelSocket *sioc; /* The master data channel */
>  QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
> @@ -27,7 +32,7 @@ typedef struct NBDClientSession {
>  Coroutine *read_reply_co;
>  int in_flight;
>  
> -Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
> +NBDClientRequest requests[MAX_NBD_REQUESTS];
>  NBDReply reply;
>  bool quit;
>  } NBDClientSession;
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 422ecb4307..c2834f6b47 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -39,8 +39,10 @@ static void nbd_recv_coroutines_enter_all(NBDClientSession 
> *s)
>  int i;
>  
>  for (i = 0; i < MAX_NBD_REQUESTS; i++) {
> -if (s->recv_coroutine[i]) {
> -aio_co_wake(s->recv_coroutine[i]);
> +NBDClientRequest *req = >requests[i];
> +
> +if (req->coroutine && req->receiving) {
> +aio_co_wake(req->coroutine);
>  }
>  }
>  }
> @@ -88,28 +90,28 @@ static coroutine_fn void nbd_read_reply_entry(void 
> *opaque)
>   * one coroutine is called until the reply finishes.
>   */
>  i = HANDLE_TO_INDEX(s, s->reply.handle);
> -if (i >= MAX_NBD_REQUESTS || !s->recv_coroutine[i]) {
> +if (i >= MAX_NBD_REQUESTS ||
> +!s->requests[i].coroutine ||
> +!s->requests[i].receiving) {
>  break;
>  }
>  
> -/* We're woken up by the recv_coroutine itself.  Note that there
> +/* We're woken up again by the request itself.  Note that there
>   * is no race between yielding and reentering read_reply_co.  This
>   * is because:
>   *
> - * - if recv_coroutine[i] runs on the same AioContext, it is only
> + * - if the request runs on the same AioContext, it is only
>   *   entered after we yield
>   *
> - * - if recv_coroutine[i] runs on a different AioContext, reentering
> + * - if the request runs on a different AioContext, reentering
>   *   read_reply_co happens through a bottom half, which can only
>   *   run after we yield.
>   */
> -aio_co_wake(s->recv_coroutine[i]);
> +aio_co_wake(s->requests[i].coroutine);
>  qemu_coroutine_yield();
>  }
>  
> -if (ret < 0) {
> -s->quit = true;
> -}
> +s->quit = true;
>  nbd_recv_coroutines_enter_all(s);
>  s->read_reply_co = NULL;
>  }
> @@ -128,14 +130,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
>  s->in_flight++;
>  
>  for (i = 0; i < MAX_NBD_REQUESTS; i++) {
> -if (s->recv_coroutine[i] == NULL) {
> -s->recv_coroutine[i] = qemu_coroutine_self();
> +if (s->requests[i].coroutine == 

Re: [Qemu-block] [Qemu-devel] [PATCH 08/10] scsi: build qemu-pr-helper

[Marc-André Lureau]

Hi

On Tue, Aug 22, 2017 at 3:18 PM, Paolo Bonzini  wrote:
> Introduce a privileged helper to run persistent reservation commands.
> This lets virtual machines send persistent reservations without using
> CAP_SYS_RAWIO or out-of-tree patches.  The helper uses Unix permissions
> and SCM_RIGHTS to restrict access to processes that can access its socket
> and prove that they have an open file descriptor for a raw SCSI device.
>

Could this be handled by udisk? It seems at first the problem is not
specific to qemu.

(I was also wondering if there was any overlap with the stratis
daemon, https://stratis-storage.github.io/, but after 5 minutes
research, probably not, stratis is higher level)

> The next patch will also correct the usage of persistent reservations
> with multipath devices.
>
> It would also be possible to support for Linux's IOC_PR_* ioctls in
> the future, to support NVMe devices.  For now, however, only SCSI is
> supported.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  Makefile   |   9 +-
>  configure  |   2 +-
>  docs/interop/pr-helper.rst |  78 ++
>  docs/pr-manager.rst|  33 +++
>  scsi/pr-helper.h   |  13 +
>  scsi/qemu-pr-helper.c  | 657 
> +
>  6 files changed, 790 insertions(+), 2 deletions(-)
>  create mode 100644 docs/interop/pr-helper.rst
>  create mode 100644 scsi/pr-helper.h
>  create mode 100644 scsi/qemu-pr-helper.c
>
> diff --git a/Makefile b/Makefile
> index 97a58a0f4e..bfd4f69ecd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -209,6 +209,9 @@ ifdef BUILD_DOCS
>  DOCS=qemu-doc.html qemu-doc.txt qemu.1 qemu-img.1 qemu-nbd.8 qemu-ga.8
>  DOCS+=docs/interop/qemu-qmp-ref.html docs/interop/qemu-qmp-ref.txt 
> docs/interop/qemu-qmp-ref.7
>  DOCS+=docs/interop/qemu-ga-ref.html docs/interop/qemu-ga-ref.txt 
> docs/interop/qemu-ga-ref.7
> +ifdef CONFIG_LINUX
> +DOCS+=scsi/qemu-pr-helper.8
> +endif
>  ifdef CONFIG_VIRTFS
>  DOCS+=fsdev/virtfs-proxy-helper.1
>  endif
> @@ -384,6 +387,8 @@ qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o 
> $(COMMON_LDADDS)
>  fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
> fsdev/9p-marshal.o fsdev/9p-iov-marshal.o $(COMMON_LDADDS)
>  fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
>
> +scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
> $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
> +
>  qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
> $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
> $@,"GEN","$@")
>
> @@ -491,7 +496,7 @@ clean:
> rm -f *.msi
> find . \( -name '*.so' -o -name '*.dll' -o -name '*.mo' -o -name 
> '*.[oda]' \) -type f -exec rm {} +
> rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* 
> *.pod *~ */*~
> -   rm -f fsdev/*.pod
> +   rm -f fsdev/*.pod scsi/*.pod
> rm -f qemu-img-cmds.h
> rm -f ui/shader/*-vert.h ui/shader/*-frag.h
> @# May not be present in GENERATED_FILES
> @@ -590,6 +595,7 @@ endif
>  ifdef CONFIG_VIRTFS
> $(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
> $(INSTALL_DATA) fsdev/virtfs-proxy-helper.1 "$(DESTDIR)$(mandir)/man1"
> +   $(INSTALL_DATA) scsi/qemu-pr-helper.8 "$(DESTDIR)$(mandir)/man8"

ifdef CONFIG_LINUX?

>  endif
>
>  install-datadir:
> @@ -718,6 +724,7 @@ qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi 
> qemu-monitor-info.texi
>  qemu.1: qemu-option-trace.texi
>  qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi
>  fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
> +scsi/qemu-pr-helper.8: scsi/qemu-pr-helper.texi
>  qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi
>  qemu-ga.8: qemu-ga.texi
>
> diff --git a/configure b/configure
> index dd73cce62f..772aff18d6 100755
> --- a/configure
> +++ b/configure
> @@ -6545,7 +6545,7 @@ fi
>
>  # build tree in object directory in case the source is not in the current 
> directory
>  DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos 
> tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests"
> -DIRS="$DIRS docs docs/interop fsdev"
> +DIRS="$DIRS docs docs/interop fsdev scsi"
>  DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
>  DIRS="$DIRS roms/seabios roms/vgabios"
>  DIRS="$DIRS qapi-generated"
> diff --git a/docs/interop/pr-helper.rst b/docs/interop/pr-helper.rst
> new file mode 100644
> index 00..765174c31f
> --- /dev/null
> +++ b/docs/interop/pr-helper.rst
> @@ -0,0 +1,78 @@
> +..
> +
> +==
> +Persistent reservation helper protocol
> +==
> +
> +QEMU's SCSI passthrough devices, ``scsi-block`` and ``scsi-generic``,
> +can delegate implementation of persistent reservations to an external
> +(and typically privilege) program.  Persistent Reservations allow
> +restricting access to block devices to specific 

Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Manos Pitsidianakis]

On Tue, Aug 22, 2017 at 04:16:26PM +0200, Alberto Garcia wrote:

On Tue 22 Aug 2017 12:15:34 PM CEST, Manos Pitsidianakis wrote:

@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
*tgm)
 ThrottleGroupMember *token;
 int i;

+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+


Is this change needed in case throttle_configure_tgm() fails?


Yes, now all errors are before throttle_group_register_tgm(), therefore 
the unregister part in throttle_close() will not have valid data.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Alberto Garcia]

On Tue 22 Aug 2017 12:15:34 PM CEST, Manos Pitsidianakis wrote:
>  ##
> +# @BlockdevOptionsThrottle:
> +#
> +# Driver specific block device options for the throttle driver
> +#
> +# @throttle-group:   the name of the throttle-group object to use. It
> +#must already exist.
> +# @file: reference to or definition of the data source block 
> device
> +# Since: 2.11
> +##
> +{ 'struct': 'BlockdevOptionsThrottle',
> +  'data': { '*throttle-group': 'str',
> +'file' : 'BlockdevRef'
> + } }
> +##

I guess throttle-group is not optional here anymore ?

> @@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
> *tgm)
>  ThrottleGroupMember *token;
>  int i;
>  
> +if (!ts) {
> +/* Discard uninitialized tgm */
> +return;
> +}
> +

Is this change needed in case throttle_configure_tgm() fails?

> +static int throttle_configure_tgm(BlockDriverState *bs,
> +  ThrottleGroupMember *tgm,
> +  QDict *options, Error **errp)
> +{
> +int ret;
> +const char *group_name;
> +Error *local_err = NULL;
> +QemuOpts *opts = qemu_opts_create(_opts, NULL, 0, _abort);
> +
> +qemu_opts_absorb_qdict(opts, options, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fin;
> +}
> +
> +group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
> +if (!group_name) {
> +error_setg(errp, "Please specify a throttle group.");
> +ret = EINVAL;
> +goto fin;
> +} else if (!throttle_group_exists(group_name)) {
> +error_setg(errp, "Throttle group '%s' does not exist.", group_name);
> +ret = EINVAL;
> +goto fin;
> +}

It should be -EINVAL (negative value) in both cases.

Berto

Re: [Qemu-block] [RFC PATCH 13/56] pci: Make PCI addresses and sizes unsigned in QAPI/QMP

[Marcel Apfelbaum]

Hi Markus,

On 07/08/2017 17:45, Markus Armbruster wrote:

Sizes and addresses should use QAPI type 'size' (uint64_t).
PciMemoryRegion members @address and @size are 'int' (int64_t).
qmp_query_pci_regions() implicitly converts from pcibus_t,
i.e. uint64_t.

Change these PciMemoryRegion members to 'size'.

query-pci now reports sizes and addresses above 2^63-1 correctly
instead of their (negative) two's complement.

HMP's "info pci" already reported them correctly, because it
implicitly converted back to uint64_t.

Signed-off-by: Markus Armbruster 
---
  qapi-schema.json | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 6aa6be9..c8cceb9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2062,7 +2062,7 @@
  # Since: 0.14.0
  ##
  { 'struct': 'PciMemoryRegion',
-  'data': {'bar': 'int', 'type': 'str', 'address': 'int', 'size': 'int',
+  'data': {'bar': 'int', 'type': 'str', 'address': 'size', 'size': 'size',
 '*prefetch': 'bool', '*mem_type_64': 'bool' } }
  
  ##




Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] scsi: introduce sg_io_sense_from_errno

[Paolo Bonzini]

On 22/08/2017 15:45, Philippe Mathieu-Daudé wrote:
> Hi Paolo,
> 
> On 08/22/2017 10:18 AM, Paolo Bonzini wrote:
>> Move more knowledge of SG_IO out of hw/scsi/scsi-generic.c, for
>> reusability.
>>
>> Signed-off-by: Paolo Bonzini 
>> ---
>>   hw/scsi/scsi-generic.c | 40 +++-
>>   include/scsi/utils.h   |  3 +++
>>   scsi/utils.c   | 35 +++
>>   3 files changed, 45 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
>> index 7a8f500934..04c687ee76 100644
>> --- a/hw/scsi/scsi-generic.c
>> +++ b/hw/scsi/scsi-generic.c
>> @@ -81,6 +81,7 @@ static void scsi_free_request(SCSIRequest *req)
>>   static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
>>   {
>>   int status;
>> +SCSISense sense;
>> assert(r->req.aiocb == NULL);
>>   @@ -88,42 +89,15 @@ static void
>> scsi_command_complete_noio(SCSIGenericReq *r, int ret)
>>   scsi_req_cancel_complete(>req);
>>   goto done;
>>   }
>> -if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
>> -r->req.sense_len = r->io_header.sb_len_wr;
>> -}
>> -
>> -if (ret != 0) {
>> -switch (ret) {
>> -case -EDOM:
>> -status = TASK_SET_FULL;
>> -break;
>> -case -ENOMEM:
>> -status = CHECK_CONDITION;
>> -scsi_req_build_sense(>req, SENSE_CODE(TARGET_FAILURE));
>> -break;
>> -default:
>> -status = CHECK_CONDITION;
>> -scsi_req_build_sense(>req, SENSE_CODE(IO_ERROR));
>> -break;
>> -}
>> -} else {
>> -if (r->io_header.host_status == SG_ERR_DID_NO_CONNECT ||
>> -r->io_header.host_status == SG_ERR_DID_BUS_BUSY ||
>> -r->io_header.host_status == SG_ERR_DID_TIME_OUT ||
>> -(r->io_header.driver_status & SG_ERR_DRIVER_TIMEOUT)) {
>> -status = BUSY;
>> -BADF("Driver Timeout\n");
>> -} else if (r->io_header.host_status) {
>> -status = CHECK_CONDITION;
>> -scsi_req_build_sense(>req, SENSE_CODE(I_T_NEXUS_LOSS));
>> -} else if (r->io_header.status) {
>> -status = r->io_header.status;
>> -} else if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
>> -status = CHECK_CONDITION;
>> +status = sg_io_sense_from_errno(-ret, >io_header, );
>> +if (status == CHECK_CONDITION) {
>> +if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
>> +r->req.sense_len = r->io_header.sb_len_wr;
>>   } else {
>> -status = GOOD;
>> +scsi_req_build_sense(>req, sense);
>>   }
>>   }
>> +
>>   DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
>>   r, r->req.tag, status);
>>   diff --git a/include/scsi/utils.h b/include/scsi/utils.h
>> index 76c3db98c0..c12b34f2e5 100644
>> --- a/include/scsi/utils.h
>> +++ b/include/scsi/utils.h
>> @@ -113,6 +113,9 @@ int scsi_cdb_length(uint8_t *buf);
>>   #define SG_ERR_DID_TIME_OUT0x03
>> #define SG_ERR_DRIVER_SENSE0x08
>> +
>> +int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
>> +   SCSISense *sense);
>>   #endif
>> #endif
>> diff --git a/scsi/utils.c b/scsi/utils.c
>> index 54d6d4486a..ca5b058a73 100644
>> --- a/scsi/utils.c
>> +++ b/scsi/utils.c
>> @@ -412,3 +412,38 @@ const char *scsi_command_name(uint8_t cmd)
>>   }
>>   return names[cmd];
>>   }
>> +
>> +#ifdef CONFIG_LINUX
>> +int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
>> +   SCSISense *sense)
>> +{
>> +if (errno_value != 0) {
>> +switch (errno_value) {
>> +case EDOM:
>> +return TASK_SET_FULL;
>> +case ENOMEM:
>> +*sense = SENSE_CODE(TARGET_FAILURE);
>> +return CHECK_CONDITION;
>> +default:
>> +*sense = SENSE_CODE(IO_ERROR);
>> +return CHECK_CONDITION;
>> +}
>> +} else {
>> +if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
>> +io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
>> +io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
>> +(io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
>> +return BUSY;
>> +} else if (io_hdr->host_status) {
>> +*sense = SENSE_CODE(I_T_NEXUS_LOSS);
>> +return CHECK_CONDITION;
>> +} else if (io_hdr->status) {
>> +return io_hdr->status;
>> +} else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
>> +return CHECK_CONDITION;
>> +}
> 
>> +}
> I find it easier to read with the return GOOD out of the if/else:
> 
>return GOOD;

Yes, you're right.

Paolo

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent reservation managers

[no-reply]

Hi,

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

Type: series
Message-id: 20170822131832.20191-1-pbonz...@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent 
reservation managers

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20170822131832.20191-1-pbonz...@redhat.com 
-> patchew/20170822131832.20191-1-pbonz...@redhat.com
Switched to a new branch 'test'
e72ec38399 scsi: add persistent reservation manager using qemu-pr-helper
284895f2ab scsi: add multipath support to qemu-pr-helper
a946f484ca scsi: build qemu-pr-helper
05fe9d89ec io: add qio_channel_read/write_all
773bd4b8d7 scsi, file-posix: add support for persistent reservation management
2dd394b769 scsi: move block/scsi.h to include/scsi/constants.h
216ab8692e scsi: introduce sg_io_sense_from_errno
6a5a1d0624 scsi: introduce scsi_build_sense
7fa71258fa scsi: move non-emulation specific code to scsi/
4381f3f50d scsi: rename scsi_convert_sense

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-ilb8t7pz/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
  BUILD   centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-ilb8t7pz/src'
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPYRUNNER
RUN test-quick in qemu:centos6 
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
bison-2.4.1-5.el6.x86_64
bzip2-devel-1.0.5-7.el6_0.x86_64
ccache-3.1.6-2.el6.x86_64
csnappy-devel-0-6.20150729gitd7bc683.el6.x86_64
flex-2.5.35-9.el6.x86_64
gcc-4.4.7-18.el6.x86_64
git-1.7.1-8.el6.x86_64
glib2-devel-2.28.8-9.el6.x86_64
libepoxy-devel-1.2-3.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
librdmacm-devel-1.0.21-0.el6.x86_64
lzo-devel-2.03-3.1.el6_5.1.x86_64
make-3.81-23.el6.x86_64
mesa-libEGL-devel-11.0.7-4.el6.x86_64
mesa-libgbm-devel-11.0.7-4.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
spice-glib-devel-0.26-8.el6.x86_64
spice-server-devel-0.12.4-16.el6.x86_64
tar-1.23-15.el6_8.x86_64
vte-devel-0.25.1-9.el6.x86_64
xen-devel-4.6.3-15.el6.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=bison bzip2-devel ccache csnappy-devel flex g++
 gcc git glib2-devel libepoxy-devel libfdt-devel 
librdmacm-devel lzo-devel make mesa-libEGL-devel 
mesa-libgbm-devel pixman-devel SDL-devel spice-glib-devel 
spice-server-devel tar vte-devel xen-devel zlib-devel
HOSTNAME=e2df32bf6f5a
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=01;05;37;41:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arj=01;31:*.taz=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lz=01;31:*.xz=01;31:*.bz2=01;31:*.tbz=01;31:*.tbz2=01;31:*.bz=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.rar=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.jpg=01;35:*.jpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.axv=01;35:*.anx=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=01;36:*.au=01;36:*.flac=01;36:*.mid=01;36:*.midi=01;36:*.mka=01;36:*.mp3=01;36:*.mpc=01;36:*.ogg=01;36:*.ra=01;36:*.wav=01;36:*.axa=01;36:*.oga=01;36:*.spx=01;36:*.xspf=01;36:
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix

Re: [Qemu-block] [Qemu-devel] [PATCH] nbd-client: fix hang after server closes connection

[Philippe Mathieu-Daudé]

On 08/21/2017 01:15 PM, Stefan Hajnoczi wrote:

Commit 72b6ffc76653214b69a94a7b1643ff80df134486 ("nbd-client: Fix
regression when server sends garbage") improved NBD client behavior when
the connection enters a broken state.

The following still does not behave as expected:

   $ qemu-nbd -p 1234 -x drive0 -f qcow2 test.qcow2
   $ qemu-system-x86_64 -M accel=kvm -m 1G \
 -drive if=virtio,id=drive0,file=nbd://localhost:1234/drive0,format=raw
   $ pkill qemu-nbd
   (qemu) quit
   ...hang...

QEMU should be able to quit even when the connection was previously
closed by the NBD server.  Currently the nbd_read_reply_entry()
coroutine terminates without letting in-flight requests know that the
connection is dead.

This patch flags the connection as dead so in-flight requests can
complete.

Reported-by: Longxiang Lyu 
Cc: Eric Blake 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 


Acked-by: Philippe Mathieu-Daudé 


---
  block/nbd-client.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 422ecb4307..5a5fe02015 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -80,6 +80,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  error_report_err(local_err);
  }
  if (ret <= 0) {
+s->quit = true;
  break;
  }
  
@@ -107,9 +108,6 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)

  qemu_coroutine_yield();
  }
  
-if (ret < 0) {

-s->quit = true;
-}
  nbd_recv_coroutines_enter_all(s);
  s->read_reply_co = NULL;
  }

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent reservation managers

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170822131832.20191-1-pbonz...@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent 
reservation managers

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e72ec38399 scsi: add persistent reservation manager using qemu-pr-helper
284895f2ab scsi: add multipath support to qemu-pr-helper
a946f484ca scsi: build qemu-pr-helper
05fe9d89ec io: add qio_channel_read/write_all
773bd4b8d7 scsi, file-posix: add support for persistent reservation management
2dd394b769 scsi: move block/scsi.h to include/scsi/constants.h
216ab8692e scsi: introduce sg_io_sense_from_errno
6a5a1d0624 scsi: introduce scsi_build_sense
7fa71258fa scsi: move non-emulation specific code to scsi/
4381f3f50d scsi: rename scsi_convert_sense

=== OUTPUT BEGIN ===
Checking PATCH 1/10: scsi: rename scsi_convert_sense...
Checking PATCH 2/10: scsi: move non-emulation specific code to scsi/...
ERROR: space prohibited after that open square bracket '['
#1042: FILE: scsi/utils.c:282:
+[ TEST_UNIT_READY  ] = "TEST_UNIT_READY",

ERROR: space prohibited before that close square bracket ']'
#1042: FILE: scsi/utils.c:282:
+[ TEST_UNIT_READY  ] = "TEST_UNIT_READY",

ERROR: space prohibited after that open square bracket '['
#1043: FILE: scsi/utils.c:283:
+[ REWIND   ] = "REWIND",

ERROR: space prohibited before that close square bracket ']'
#1043: FILE: scsi/utils.c:283:
+[ REWIND   ] = "REWIND",

ERROR: space prohibited after that open square bracket '['
#1044: FILE: scsi/utils.c:284:
+[ REQUEST_SENSE] = "REQUEST_SENSE",

ERROR: space prohibited before that close square bracket ']'
#1044: FILE: scsi/utils.c:284:
+[ REQUEST_SENSE] = "REQUEST_SENSE",

ERROR: space prohibited after that open square bracket '['
#1045: FILE: scsi/utils.c:285:
+[ FORMAT_UNIT  ] = "FORMAT_UNIT",

ERROR: space prohibited before that close square bracket ']'
#1045: FILE: scsi/utils.c:285:
+[ FORMAT_UNIT  ] = "FORMAT_UNIT",

ERROR: space prohibited after that open square bracket '['
#1046: FILE: scsi/utils.c:286:
+[ READ_BLOCK_LIMITS] = "READ_BLOCK_LIMITS",

ERROR: space prohibited before that close square bracket ']'
#1046: FILE: scsi/utils.c:286:
+[ READ_BLOCK_LIMITS] = "READ_BLOCK_LIMITS",

WARNING: line over 80 characters
#1047: FILE: scsi/utils.c:287:
+[ REASSIGN_BLOCKS  ] = "REASSIGN_BLOCKS/INITIALIZE ELEMENT 
STATUS",

ERROR: space prohibited after that open square bracket '['
#1047: FILE: scsi/utils.c:287:
+[ REASSIGN_BLOCKS  ] = "REASSIGN_BLOCKS/INITIALIZE ELEMENT 
STATUS",

ERROR: space prohibited before that close square bracket ']'
#1047: FILE: scsi/utils.c:287:
+[ REASSIGN_BLOCKS  ] = "REASSIGN_BLOCKS/INITIALIZE ELEMENT 
STATUS",

WARNING: line over 80 characters
#1048: FILE: scsi/utils.c:288:
+/* LOAD_UNLOAD and INITIALIZE_ELEMENT_STATUS use the same operation 
code */

ERROR: space prohibited after that open square bracket '['
#1049: FILE: scsi/utils.c:289:
+[ READ_6   ] = "READ_6",

ERROR: space prohibited before that close square bracket ']'
#1049: FILE: scsi/utils.c:289:
+[ READ_6   ] = "READ_6",

ERROR: space prohibited after that open square bracket '['
#1050: FILE: scsi/utils.c:290:
+[ WRITE_6  ] = "WRITE_6",

ERROR: space prohibited before that close square bracket ']'
#1050: FILE: scsi/utils.c:290:
+[ WRITE_6  ] = "WRITE_6",

ERROR: space prohibited after that open square bracket '['
#1051: FILE: scsi/utils.c:291:
+[ SET_CAPACITY ] = "SET_CAPACITY",

ERROR: space prohibited before that close square bracket ']'
#1051: FILE: scsi/utils.c:291:
+[ SET_CAPACITY ] = "SET_CAPACITY",

ERROR: space prohibited after that open square bracket '['
#1052: FILE: scsi/utils.c:292:
+[ READ_REVERSE ] = "READ_REVERSE",

ERROR: space prohibited before that close square bracket ']'
#1052: FILE: scsi/utils.c:292:
+[ READ_REVERSE ] = "READ_REVERSE",

ERROR: space prohibited after that open square bracket '['
#1053: FILE: scsi/utils.c:293:
+[ WRITE_FILEMARKS  ] = 

Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] scsi: introduce sg_io_sense_from_errno

[Philippe Mathieu-Daudé]

Hi Paolo,

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:

Move more knowledge of SG_IO out of hw/scsi/scsi-generic.c, for
reusability.

Signed-off-by: Paolo Bonzini 
---
  hw/scsi/scsi-generic.c | 40 +++-
  include/scsi/utils.h   |  3 +++
  scsi/utils.c   | 35 +++
  3 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a8f500934..04c687ee76 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -81,6 +81,7 @@ static void scsi_free_request(SCSIRequest *req)
  static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
  {
  int status;
+SCSISense sense;
  
  assert(r->req.aiocb == NULL);
  
@@ -88,42 +89,15 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)

  scsi_req_cancel_complete(>req);
  goto done;
  }
-if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
-r->req.sense_len = r->io_header.sb_len_wr;
-}
-
-if (ret != 0) {
-switch (ret) {
-case -EDOM:
-status = TASK_SET_FULL;
-break;
-case -ENOMEM:
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(TARGET_FAILURE));
-break;
-default:
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(IO_ERROR));
-break;
-}
-} else {
-if (r->io_header.host_status == SG_ERR_DID_NO_CONNECT ||
-r->io_header.host_status == SG_ERR_DID_BUS_BUSY ||
-r->io_header.host_status == SG_ERR_DID_TIME_OUT ||
-(r->io_header.driver_status & SG_ERR_DRIVER_TIMEOUT)) {
-status = BUSY;
-BADF("Driver Timeout\n");
-} else if (r->io_header.host_status) {
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(I_T_NEXUS_LOSS));
-} else if (r->io_header.status) {
-status = r->io_header.status;
-} else if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
-status = CHECK_CONDITION;
+status = sg_io_sense_from_errno(-ret, >io_header, );
+if (status == CHECK_CONDITION) {
+if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
+r->req.sense_len = r->io_header.sb_len_wr;
  } else {
-status = GOOD;
+scsi_req_build_sense(>req, sense);
  }
  }
+
  DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
  r, r->req.tag, status);
  
diff --git a/include/scsi/utils.h b/include/scsi/utils.h

index 76c3db98c0..c12b34f2e5 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -113,6 +113,9 @@ int scsi_cdb_length(uint8_t *buf);
  #define SG_ERR_DID_TIME_OUT0x03
  
  #define SG_ERR_DRIVER_SENSE0x08

+
+int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
+   SCSISense *sense);
  #endif
  
  #endif

diff --git a/scsi/utils.c b/scsi/utils.c
index 54d6d4486a..ca5b058a73 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -412,3 +412,38 @@ const char *scsi_command_name(uint8_t cmd)
  }
  return names[cmd];
  }
+
+#ifdef CONFIG_LINUX
+int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
+   SCSISense *sense)
+{
+if (errno_value != 0) {
+switch (errno_value) {
+case EDOM:
+return TASK_SET_FULL;
+case ENOMEM:
+*sense = SENSE_CODE(TARGET_FAILURE);
+return CHECK_CONDITION;
+default:
+*sense = SENSE_CODE(IO_ERROR);
+return CHECK_CONDITION;
+}
+} else {
+if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
+io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
+io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
+(io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
+return BUSY;
+} else if (io_hdr->host_status) {
+*sense = SENSE_CODE(I_T_NEXUS_LOSS);
+return CHECK_CONDITION;
+} else if (io_hdr->status) {
+return io_hdr->status;
+} else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
+return CHECK_CONDITION;
+}



+}

I find it easier to read with the return GOOD out of the if/else:

   return GOOD;

Either ways:
Reviewed-by: Philippe Mathieu-Daudé 


+}
+#endif

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent reservation managers

[no-reply]

Hi,

This series failed build test on s390x host. Please find the details below.

Message-id: 20170822131832.20191-1-pbonz...@redhat.com
Type: series
Subject: [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent 
reservation managers

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20170822131832.20191-1-pbonz...@redhat.com -> 
patchew/20170822131832.20191-1-pbonz...@redhat.com
Switched to a new branch 'test'
e72ec38 scsi: add persistent reservation manager using qemu-pr-helper
284895f scsi: add multipath support to qemu-pr-helper
a946f48 scsi: build qemu-pr-helper
05fe9d8 io: add qio_channel_read/write_all
773bd4b scsi, file-posix: add support for persistent reservation management
2dd394b scsi: move block/scsi.h to include/scsi/constants.h
216ab86 scsi: introduce sg_io_sense_from_errno
6a5a1d0 scsi: introduce scsi_build_sense
7fa7125 scsi: move non-emulation specific code to scsi/
4381f3f scsi: rename scsi_convert_sense

=== OUTPUT BEGIN ===
=== ENV ===
XDG_SESSION_ID=38768
SHELL=/bin/sh
USER=fam
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
PATH=/usr/bin:/bin
PWD=/var/tmp/patchew-tester-tmp-wgqekxxc/src
LANG=en_US.UTF-8
HOME=/home/fam
SHLVL=2
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
xz-libs-5.2.2-2.fc24.s390x
libxshmfence-1.2-3.fc24.s390x
giflib-4.1.6-15.fc24.s390x
trousers-lib-0.3.13-6.fc24.s390x
ncurses-base-6.0-6.20160709.fc25.noarch
gmp-6.1.1-1.fc25.s390x
libidn-1.33-1.fc25.s390x
slang-2.3.0-7.fc25.s390x
pkgconfig-0.29.1-1.fc25.s390x
alsa-lib-1.1.1-2.fc25.s390x
yum-metadata-parser-1.1.4-17.fc25.s390x
python3-slip-dbus-0.6.4-4.fc25.noarch
python2-cssselect-0.9.2-1.fc25.noarch
createrepo_c-libs-0.10.0-6.fc25.s390x
initscripts-9.69-1.fc25.s390x
parted-3.2-21.fc25.s390x
flex-2.6.0-3.fc25.s390x
colord-libs-1.3.4-1.fc25.s390x
python-osbs-client-0.33-3.fc25.noarch
perl-Pod-Simple-3.35-1.fc25.noarch
python2-simplejson-3.10.0-1.fc25.s390x
brltty-5.4-2.fc25.s390x
librados2-10.2.4-2.fc25.s390x
tcp_wrappers-7.6-83.fc25.s390x
libcephfs_jni1-10.2.4-2.fc25.s390x
nettle-devel-3.3-1.fc25.s390x
bzip2-devel-1.0.6-21.fc25.s390x
libuuid-2.28.2-2.fc25.s390x
python3-dnf-1.1.10-6.fc25.noarch
texlive-kpathsea-doc-svn41139-33.fc25.1.noarch
openssh-7.4p1-4.fc25.s390x
texlive-kpathsea-bin-svn40473-33.20160520.fc25.1.s390x
texlive-graphics-svn41015-33.fc25.1.noarch
texlive-dvipdfmx-def-svn40328-33.fc25.1.noarch
texlive-mfware-svn40768-33.fc25.1.noarch
texlive-texlive-scripts-svn41433-33.fc25.1.noarch
texlive-euro-svn22191.1.1-33.fc25.1.noarch
texlive-etex-svn37057.0-33.fc25.1.noarch
texlive-iftex-svn29654.0.2-33.fc25.1.noarch
texlive-palatino-svn31835.0-33.fc25.1.noarch
texlive-texlive-docindex-svn41430-33.fc25.1.noarch
texlive-xunicode-svn30466.0.981-33.fc25.1.noarch
texlive-koma-script-svn41508-33.fc25.1.noarch
texlive-pst-grad-svn15878.1.06-33.fc25.1.noarch
texlive-pst-blur-svn15878.2.0-33.fc25.1.noarch
texlive-jknapltx-svn19440.0-33.fc25.1.noarch
texinfo-6.1-4.fc25.s390x
openssl-devel-1.0.2k-1.fc25.s390x
gdk-pixbuf2-2.36.6-1.fc25.s390x
nspr-4.14.0-2.fc25.s390x
nss-softokn-freebl-3.30.2-1.0.fc25.s390x
jansson-2.10-2.fc25.s390x
fedora-repos-25-4.noarch
python3-libs-3.5.3-6.fc25.s390x
perl-Errno-1.25-387.fc25.s390x
acl-2.2.52-13.fc25.s390x
pcre2-utf16-10.23-8.fc25.s390x
pango-1.40.5-1.fc25.s390x
systemd-pam-231-17.fc25.s390x
python2-gluster-3.10.4-1.fc25.s390x
NetworkManager-libnm-1.4.4-5.fc25.s390x
selinux-policy-3.13.1-225.18.fc25.noarch
poppler-0.45.0-5.fc25.s390x
ccache-3.3.4-1.fc25.s390x
valgrind-3.12.0-9.fc25.s390x
perl-open-1.10-387.fc25.noarch
libaio-0.3.110-6.fc24.s390x
libfontenc-1.1.3-3.fc24.s390x
lzo-2.08-8.fc24.s390x
isl-0.14-5.fc24.s390x
libXau-1.0.8-6.fc24.s390x
linux-atm-libs-2.5.1-14.fc24.s390x
libXext-1.3.3-4.fc24.s390x
libXxf86vm-1.1.4-3.fc24.s390x
bison-3.0.4-4.fc24.s390x
perl-srpm-macros-1-20.fc25.noarch
gawk-4.1.3-8.fc25.s390x
libwayland-client-1.12.0-1.fc25.s390x
perl-Exporter-5.72-366.fc25.noarch
perl-version-0.99.17-1.fc25.s390x
fftw-libs-double-3.3.5-3.fc25.s390x
libssh2-1.8.0-1.fc25.s390x
ModemManager-glib-1.6.4-1.fc25.s390x
newt-python3-0.52.19-2.fc25.s390x
python-munch-2.0.4-3.fc25.noarch
python-bugzilla-1.2.2-4.fc25.noarch
libedit-3.1-16.20160618cvs.fc25.s390x
createrepo_c-0.10.0-6.fc25.s390x

Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] scsi: rename scsi_convert_sense

[Philippe Mathieu-Daudé]

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:

After introducing the scsi/ subdirectory, there will be a scsi_build_sense
function that is the same as scsi_req_build_sense but without needing
a SCSIRequest.  The existing scsi_build_sense function gets in the way,
remove it.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Philippe Mathieu-Daudé 


---
  hw/scsi/scsi-bus.c | 10 +-
  hw/scsi/scsi-disk.c|  4 ++--
  include/hw/scsi/scsi.h |  4 ++--
  3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index e364410a23..890f8fcc83 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -769,7 +769,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int 
len)
  return 0;
  }
  
-ret = scsi_build_sense(req->sense, req->sense_len, buf, len, true);

+ret = scsi_convert_sense(req->sense, req->sense_len, buf, len, true);
  
  /*

   * FIXME: clearing unit attention conditions upon autosense should be done
@@ -790,7 +790,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int 
len)
  
  int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed)

  {
-return scsi_build_sense(dev->sense, dev->sense_len, buf, len, fixed);
+return scsi_convert_sense(dev->sense, dev->sense_len, buf, len, fixed);
  }
  
  void scsi_req_build_sense(SCSIRequest *req, SCSISense sense)

@@ -1510,12 +1510,12 @@ const struct SCSISense sense_code_SPACE_ALLOC_FAILED = {
  };
  
  /*

- * scsi_build_sense
+ * scsi_convert_sense
   *
   * Convert between fixed and descriptor sense buffers
   */
-int scsi_build_sense(uint8_t *in_buf, int in_len,
- uint8_t *buf, int len, bool fixed)
+int scsi_convert_sense(uint8_t *in_buf, int in_len,
+   uint8_t *buf, int len, bool fixed)
  {
  bool fixed_in;
  SCSISense sense;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5f1e5e8070..0a1f4ef0c7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1978,8 +1978,8 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
  break;
  case REQUEST_SENSE:
  /* Just return "NO SENSE".  */
-buflen = scsi_build_sense(NULL, 0, outbuf, r->buflen,
-  (req->cmd.buf[1] & 1) == 0);
+buflen = scsi_convert_sense(NULL, 0, outbuf, r->buflen,
+(req->cmd.buf[1] & 1) == 0);
  if (buflen < 0) {
  goto illegal_request;
  }
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 6b85786dbf..6ef67fb504 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -244,8 +244,8 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
  uint32_t scsi_data_cdb_xfer(uint8_t *buf);
  uint32_t scsi_cdb_xfer(uint8_t *buf);
  int scsi_cdb_length(uint8_t *buf);
-int scsi_build_sense(uint8_t *in_buf, int in_len,
- uint8_t *buf, int len, bool fixed);
+int scsi_convert_sense(uint8_t *in_buf, int in_len,
+   uint8_t *buf, int len, bool fixed);
  
  SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,

  uint32_t tag, uint32_t lun, void *hba_private);

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent reservation managers

[no-reply]

Hi,

This series failed build test on FreeBSD host. Please find the details below.

Message-id: 20170822131832.20191-1-pbonz...@redhat.com
Type: series
Subject: [Qemu-devel] [RFC PATCH 00/10] scsi, block: introduce persistent 
reservation managers

=== TEST SCRIPT BEGIN ===
#!/bin/sh
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
pkg info
echo "=== TEST BEGIN ==="
CC=/usr/local/libexec/ccache/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL --target-list=x86_64-softmmu
gmake -j4
# XXX: we need reliable clean up
# make check -j4 V=1
gmake install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]
patchew/1503387936-3483-1-git-send-email-douly.f...@cn.fujitsu.com -> 
patchew/1503387936-3483-1-git-send-email-douly.f...@cn.fujitsu.com
 * [new tag]   patchew/20170822131832.20191-1-pbonz...@redhat.com 
-> patchew/20170822131832.20191-1-pbonz...@redhat.com
Switched to a new branch 'test'
e72ec38399 scsi: add persistent reservation manager using qemu-pr-helper
284895f2ab scsi: add multipath support to qemu-pr-helper
a946f484ca scsi: build qemu-pr-helper
05fe9d89ec io: add qio_channel_read/write_all
773bd4b8d7 scsi, file-posix: add support for persistent reservation management
2dd394b769 scsi: move block/scsi.h to include/scsi/constants.h
216ab8692e scsi: introduce sg_io_sense_from_errno
6a5a1d0624 scsi: introduce scsi_build_sense
7fa71258fa scsi: move non-emulation specific code to scsi/
4381f3f50d scsi: rename scsi_convert_sense

=== OUTPUT BEGIN ===
=== ENV ===
LOGNAME=patchew-tester
PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin
PWD=/var/tmp/patchew-tester-tmp-tarewg4x/src
HOME=/home/patchew-tester
USER=patchew-tester
SHELL=/bin/sh
PATCHEW=/home/patchew-tester/patchew/patchew-cli -s http://patchew.org --nodebug
=== PACKAGES ===
bash-4.4.12_2  GNU Project's Bourne Again SHell
binutils-2.28,1GNU binary tools
bison-3.0.4,1  Parser generator from FSF, (mostly) compatible 
with Yacc
ca_root_nss-3.30.2 Root certificate bundle from the Mozilla Project
ccache-3.3.4_3 Tool to minimize the compile time of C/C++ 
programs
curl-7.54.0Non-interactive tool to get files from FTP, 
GOPHER, HTTP(S) servers
cvsps-2.1_2Create patchset information from CVS
dtc-1.4.2_1Device Tree Compiler
expat-2.2.0_1  XML 1.0 parser written in C
gcc-5.4.0  GNU Compiler Collection 5
gcc-ecj-4.5Eclipse Java Compiler used to build GCC Java
gettext-runtime-0.19.8.1_1 GNU gettext runtime libraries and programs
git-2.13.0 Distributed source code management tool
glib-2.46.2_5  Some useful routines of C programming (current 
stable version)
gmake-4.2.1_1  GNU version of 'make' utility
gmp-6.1.2  Free library for arbitrary precision arithmetic
indexinfo-0.2.6Utility to regenerate the GNU info page index
libffi-3.2.1   Foreign Function Interface
libiconv-1.14_10   Character set conversion library
libnghttp2-1.21.0  HTTP/2.0 C Library
m4-1.4.18,1GNU M4
mpc-1.0.3  Library of complex numbers with arbitrarily high 
precision
mpfr-3.1.5_1   Library for multiple-precision floating-point 
computations
p5-Authen-SASL-2.16_1  Perl5 module for SASL authentication
p5-Digest-HMAC-1.03_1  Perl5 interface to HMAC Message-Digest Algorithms
p5-Error-0.17024   Error/exception handling in object-oriented 
programming style
p5-GSSAPI-0.28_1   Perl extension providing access to the GSSAPIv2 
library
pcre-8.40  Perl Compatible Regular Expressions library
perl5-5.24.1   Practical Extraction and Report Language
pixman-0.34.0  Low-level pixel manipulation library
pkg-1.10.1 Package manager
pkgconf-1.3.0,1Utility to help to configure compiler and linker 
flags
python-2.7_3,2 "meta-port" for the default version of Python 
interpreter
python2-2_3The "meta-port" for version 2 of the Python 
interpreter
python27-2.7.13_3  Interpreted object-oriented programming language
python3-3_3The "meta-port" for version 3 of the Python 
interpreter
python35-3.5.3_1   Interpreted object-oriented programming language
readline-6.3.8 Library for editing command lines as they are 
typed
sudo-1.8.20p1  Allow 

Re: [Qemu-block] [PATCH v2 11/54] quorum: use qapi_enum_parse() in quorum_open

[Alberto Garcia]

On Tue 22 Aug 2017 03:22:12 PM CEST, Marc-André Lureau wrote:

> @@ -925,7 +908,13 @@ static int quorum_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  goto exit;
>  }
>  
> -ret = parse_read_pattern(qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN));
> +if (!qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN)) {
> +ret = QUORUM_READ_PATTERN_QUORUM;
> +} else {
> +ret = qapi_enum_parse(QuorumReadPattern_lookup,
> +  qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN),
> +  QUORUM_READ_PATTERN__MAX, -EINVAL, NULL);
> +}

I don't like so much that you call qemu_opt_get() twice with the same
parameters, but else the change makes sense.

Berto

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Markus Armbruster]

Igor Mammedov  writes:

> On Mon,  7 Aug 2017 16:45:16 +0200
> Markus Armbruster  wrote:
>
>> Sizes and addresses should use QAPI type 'size' (uint64_t).
>> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
>> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
>> 
>> Change these PCDIMMDeviceInfo members to 'size'.
>> 
>> query-memory-devices now reports sizes and addresses above 2^63-1
>> correctly instead of their (negative) two's complement.
>> 
>> HMP's "info memory-devices" already reported them correctly, because
>> it printed the signed integers with PRIx64 and PRIu32.
> s/signed/unsigned/

Before this patch: signed.  Afterwards: unsigned.  Would

   HMP's "info memory-devices" already reported them correctly, because
   it printed the signed (before the patch) integers with PRIx64 and
   PRIu32.

be clearer?

>> Signed-off-by: Markus Armbruster 
> Reviewed-by: Igor Mammedov 

Thanks!

Re: [Qemu-block] [Qemu-devel] [PATCH 05/10] scsi: move block/scsi.h to include/scsi/constants.h

[Philippe Mathieu-Daudé]

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:

Complete the transition by renaming this header, which was
shared by block/iscsi.c and the SCSI emulation code.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Philippe Mathieu-Daudé 


---
  block/iscsi.c  | 2 +-
  hw/block/virtio-blk.c  | 2 +-
  hw/scsi/megasas.c  | 2 +-
  hw/scsi/mptendian.c| 2 +-
  hw/scsi/mptsas.c   | 2 +-
  hw/scsi/scsi-bus.c | 2 +-
  hw/scsi/scsi-disk.c| 2 +-
  hw/scsi/scsi-generic.c | 2 +-
  hw/scsi/spapr_vscsi.c  | 2 +-
  hw/scsi/virtio-scsi-dataplane.c| 2 +-
  hw/scsi/virtio-scsi.c  | 2 +-
  hw/scsi/vmw_pvscsi.c   | 2 +-
  hw/usb/dev-uas.c   | 2 +-
  include/hw/ide/internal.h  | 2 +-
  include/{block/scsi.h => scsi/constants.h} | 0
  scsi/utils.c   | 2 +-
  tests/virtio-scsi-test.c   | 2 +-
  17 files changed, 16 insertions(+), 16 deletions(-)
  rename include/{block/scsi.h => scsi/constants.h} (100%)

diff --git a/block/iscsi.c b/block/iscsi.c
index d557c99668..c43c0953e1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -34,7 +34,7 @@
  #include "qemu/bitops.h"
  #include "qemu/bitmap.h"
  #include "block/block_int.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "qemu/iov.h"
  #include "qemu/uuid.h"
  #include "qmp-commands.h"
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a16ac75090..05d1440786 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -22,7 +22,7 @@
  #include "sysemu/blockdev.h"
  #include "hw/virtio/virtio-blk.h"
  #include "dataplane/virtio-blk.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #ifdef __linux__
  # include 
  #endif
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 734fdaef90..0db68aacee 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -27,7 +27,7 @@
  #include "hw/pci/msix.h"
  #include "qemu/iov.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "trace.h"
  #include "qapi/error.h"
  #include "mfi.h"
diff --git a/hw/scsi/mptendian.c b/hw/scsi/mptendian.c
index b7fe2a2a36..3415229b5e 100644
--- a/hw/scsi/mptendian.c
+++ b/hw/scsi/mptendian.c
@@ -28,7 +28,7 @@
  #include "hw/pci/msi.h"
  #include "qemu/iov.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "trace.h"
  
  #include "mptsas.h"

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 765ab53c34..8bae8f543e 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -30,7 +30,7 @@
  #include "hw/pci/msi.h"
  #include "qemu/iov.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "trace.h"
  #include "qapi/error.h"
  #include "mptsas.h"
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f71218f02a..b9244606f8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -3,7 +3,7 @@
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "hw/qdev.h"
  #include "sysemu/block-backend.h"
  #include "sysemu/blockdev.h"
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0a1f4ef0c7..5faf6682c5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -32,7 +32,7 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "sysemu/sysemu.h"
  #include "sysemu/block-backend.h"
  #include "sysemu/blockdev.h"
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 04c687ee76..bd0d9ff355 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -34,7 +34,7 @@ do { printf("scsi-generic: " fmt , ## __VA_ARGS__); } while 
(0)
  do { fprintf(stderr, "scsi-generic: " fmt , ## __VA_ARGS__); } while (0)
  
  #include 

-#include "block/scsi.h"
+#include "scsi/constants.h"
  
  #ifndef MAX_UINT

  #define MAX_UINT ((unsigned int)-1)
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 55ee48c4da..360db53ac8 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -36,7 +36,7 @@
  #include "cpu.h"
  #include "hw/hw.h"
  #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
  #include "srp.h"
  #include "hw/qdev.h"
  #include "hw/ppc/spapr.h"
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 944ea4eb53..add4b3f4a4 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -17,7 +17,7 @@
  #include "qemu/error-report.h"
  #include "sysemu/block-backend.h"
  #include "hw/scsi/scsi.h"
-#include 

Re: [Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

On Tue, Aug 22, 2017 at 03:25:41PM +0200, Alberto Garcia wrote:

+/* This function edits throttle_groups and must be called under the global
+ * mutex */
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
+{
+ThrottleGroup *tg = THROTTLE_GROUP(obj);
+ThrottleConfig cfg;
+
+/* set group name to object id if it exists */
+if (!tg->name && tg->parent_obj.parent) {
+tg->name = object_get_canonical_path_component(OBJECT(obj));
+}
+/* We must have a group name at this point */
+assert(tg->name);
+
+/* error if name is duplicate */
+if (throttle_group_exists(tg->name)) {
+error_setg(errp, "A group with this name already exists");
+return;
+}
+
+/* check validity */
+throttle_get_config(>ts, );
+if (!throttle_is_valid(, errp)) {
+return;
+}


My fault here because I suggested its removal, but I actually think we
still need to call throttle_config() here so bkt->max is initialized in
throttle_fix_bucket(). I'll take care of updating this call once my
patches to remove throttle_fix_bucket() are applied, but for now we
still need it.


Oh yes, that is the reason I originally had done it but forgot why.


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] scsi: move non-emulation specific code to scsi/

[Philippe Mathieu-Daudé]

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:

There is a bunch of SCSI code that is shared by block/iscsi.c and
hw/scsi, and the introduction of the persistent reservation helper
will add more instances of this.  There is also include/block/scsi.h,
which actually is not part of the core block layer.

Create a directory for this kind of shared code.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Philippe Mathieu-Daudé 


---
  MAINTAINERS|   7 +
  Makefile.objs  |   2 +-
  hw/scsi/scsi-bus.c | 397 
  hw/scsi/scsi-generic.c |   8 -
  include/block/scsi.h   |   2 -
  include/hw/scsi/scsi.h |  94 +---
  include/scsi/utils.h   | 116 ++
  scsi/Makefile.objs |   1 +
  scsi/utils.c   | 403 +
  9 files changed, 529 insertions(+), 501 deletions(-)
  create mode 100644 include/scsi/utils.h
  create mode 100644 scsi/Makefile.objs
  create mode 100644 scsi/utils.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ccee28b12d..fa6e21cd38 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1213,6 +1213,13 @@ F: migration/block*
  F: include/block/aio.h
  T: git git://github.com/stefanha/qemu.git block
  
+Block SCSI subsystem

+M: Paolo Bonzini 
+L: qemu-block@nongnu.org
+S: Supported
+F: include/scsi/*
+F: scsi/*
+
  Block Jobs
  M: Jeff Cody 
  L: qemu-block@nongnu.org
diff --git a/Makefile.objs b/Makefile.objs
index 24a4ea08b8..f68aa3b60d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -11,7 +11,7 @@ chardev-obj-y = chardev/
  
  block-obj-y += nbd/

  block-obj-y += block.o blockjob.o
-block-obj-y += block/
+block-obj-y += block/ scsi/
  block-obj-y += qemu-io-cmds.o
  block-obj-$(CONFIG_REPLICATION) += replication.o
  
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c

index 890f8fcc83..300912d213 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -935,36 +935,6 @@ static int ata_passthrough_16_xfer(SCSIDevice *dev, 
uint8_t *buf)
  return xfer * unit;
  }
  
-uint32_t scsi_data_cdb_xfer(uint8_t *buf)

-{
-if ((buf[0] >> 5) == 0 && buf[4] == 0) {
-return 256;
-} else {
-return scsi_cdb_xfer(buf);
-}
-}
-
-uint32_t scsi_cdb_xfer(uint8_t *buf)
-{
-switch (buf[0] >> 5) {
-case 0:
-return buf[4];
-break;
-case 1:
-case 2:
-return lduw_be_p([7]);
-break;
-case 4:
-return ldl_be_p([10]) & 0xULL;
-break;
-case 5:
-return ldl_be_p([6]) & 0xULL;
-break;
-default:
-return -1;
-}
-}
-
  static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
  {
  cmd->xfer = scsi_cdb_xfer(buf);
@@ -1277,53 +1247,6 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
  }
  }
  
-static uint64_t scsi_cmd_lba(SCSICommand *cmd)

-{
-uint8_t *buf = cmd->buf;
-uint64_t lba;
-
-switch (buf[0] >> 5) {
-case 0:
-lba = ldl_be_p([0]) & 0x1f;
-break;
-case 1:
-case 2:
-case 5:
-lba = ldl_be_p([2]) & 0xULL;
-break;
-case 4:
-lba = ldq_be_p([2]);
-break;
-default:
-lba = -1;
-
-}
-return lba;
-}
-
-int scsi_cdb_length(uint8_t *buf) {
-int cdb_len;
-
-switch (buf[0] >> 5) {
-case 0:
-cdb_len = 6;
-break;
-case 1:
-case 2:
-cdb_len = 10;
-break;
-case 4:
-cdb_len = 16;
-break;
-case 5:
-cdb_len = 12;
-break;
-default:
-cdb_len = -1;
-}
-return cdb_len;
-}
-
  int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
  {
  int rc;
@@ -1370,326 +1293,6 @@ void scsi_device_report_change(SCSIDevice *dev, 
SCSISense sense)
  }
  }
  
-/*

- * Predefined sense codes
- */
-
-/* No sense data available */
-const struct SCSISense sense_code_NO_SENSE = {
-.key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
-};
-
-/* LUN not ready, Manual intervention required */
-const struct SCSISense sense_code_LUN_NOT_READY = {
-.key = NOT_READY, .asc = 0x04, .ascq = 0x03
-};
-
-/* LUN not ready, Medium not present */
-const struct SCSISense sense_code_NO_MEDIUM = {
-.key = NOT_READY, .asc = 0x3a, .ascq = 0x00
-};
-
-/* LUN not ready, medium removal prevented */
-const struct SCSISense sense_code_NOT_READY_REMOVAL_PREVENTED = {
-.key = NOT_READY, .asc = 0x53, .ascq = 0x02
-};
-
-/* Hardware error, internal target failure */
-const struct SCSISense sense_code_TARGET_FAILURE = {
-.key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
-};
-
-/* Illegal request, invalid command operation code */
-const struct SCSISense sense_code_INVALID_OPCODE = {
-.key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
-};
-
-/* Illegal request, LBA out of range */
-const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
-.key = ILLEGAL_REQUEST, 

Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] scsi: introduce scsi_build_sense

[Philippe Mathieu-Daudé]

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:

Move more knowledge of sense data format out of hw/scsi/scsi-bus.c
for reusability.

Signed-off-by: Paolo Bonzini 


Reviewed-by: Philippe Mathieu-Daudé 


---
  hw/scsi/scsi-bus.c   |  8 +---
  include/scsi/utils.h |  2 ++
  scsi/utils.c | 11 +++
  3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 300912d213..f71218f02a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -797,13 +797,7 @@ void scsi_req_build_sense(SCSIRequest *req, SCSISense 
sense)
  {
  trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag,
 sense.key, sense.asc, sense.ascq);
-memset(req->sense, 0, 18);
-req->sense[0] = 0x70;
-req->sense[2] = sense.key;
-req->sense[7] = 10;
-req->sense[12] = sense.asc;
-req->sense[13] = sense.ascq;
-req->sense_len = 18;
+req->sense_len = scsi_build_sense(req->sense, sense);
  }
  
  static void scsi_req_enqueue_internal(SCSIRequest *req)

diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 35a74436bf..76c3db98c0 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -30,6 +30,8 @@ typedef struct SCSISense {
  uint8_t ascq;
  } SCSISense;
  
+int scsi_build_sense(uint8_t *buf, SCSISense sense);

+
  /*
   * Predefined sense codes
   */
diff --git a/scsi/utils.c b/scsi/utils.c
index 0db727591f..54d6d4486a 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -81,6 +81,17 @@ int scsi_cdb_length(uint8_t *buf)
  return cdb_len;
  }
  
+int scsi_build_sense(uint8_t *buf, SCSISense sense)

+{
+memset(buf, 0, 18);
+buf[0] = 0x70;
+buf[2] = sense.key;
+buf[7] = 10;
+buf[12] = sense.asc;
+buf[13] = sense.ascq;
+return 18;
+}
+
  /*
   * Predefined sense codes
   */

[Qemu-block] [PATCH v2 13/54] qapi: drop the sentinel in enum array

[Marc-André Lureau]

Now that all usages have been converted to user lookup helpers.

Signed-off-by: Marc-André Lureau 
---
 scripts/qapi.py   | 1 -
 block/parallels.c | 1 -
 ui/input-legacy.c | 2 +-
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 314d7e0365..73adb90379 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1863,7 +1863,6 @@ static const char *const %(c_name)s_array[] = {
 
 max_index = c_enum_const(name, '_MAX', prefix)
 ret += mcgen('''
-[%(max_index)s] = NULL,
 };
 
 const QEnumLookup %(c_name)s_lookup = {
diff --git a/block/parallels.c b/block/parallels.c
index f870bbac3e..d5de692c9c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -72,7 +72,6 @@ typedef enum ParallelsPreallocMode {
 static const char *prealloc_mode_array[] = {
 "falloc",
 "truncate",
-NULL,
 };
 
 static QEnumLookup prealloc_mode_lookup = {
diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index c597bdc711..d50a18a505 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -61,7 +61,7 @@ int index_from_key(const char *key, size_t key_length)
 {
 int i;
 
-for (i = 0; QKeyCode_lookup.array[i] != NULL; i++) {
+for (i = 0; i < QKeyCode_lookup.size; i++) {
 if (!strncmp(key, QKeyCode_lookup.array[i], key_length) &&
 !QKeyCode_lookup.array[i][key_length]) {
 break;
-- 
2.14.1.146.gd35faa819

[Qemu-block] [PATCH v2 12/54] qapi: change enum lookup structure

[Marc-André Lureau]

Store the length in the lookup table, i.e. change it from
const char *const[] to struct { int n, const char *const s[] }.

The following conditional enum entry change will create "hole"
elements in the generated lookup array, that should be skipped.

Signed-off-by: Marc-André Lureau 
---
 include/qapi/visitor.h  |  2 +-
 scripts/qapi.py | 11 +--
 scripts/qapi-types.py   |  6 
 scripts/qapi-visit.py   |  2 +-
 include/hw/qdev-core.h  |  2 +-
 include/qapi/util.h |  6 ++--
 include/qom/object.h|  4 +--
 qapi/qapi-visit-core.c  | 24 +++---
 backends/hostmem.c  |  4 +--
 block.c |  3 +-
 block/backup.c  |  2 +-
 block/blkdebug.c|  4 +--
 block/file-posix.c  | 17 +-
 block/file-win32.c  |  6 ++--
 block/gluster.c | 12 +++
 block/iscsi.c   |  2 +-
 block/nfs.c |  2 +-
 block/parallels.c   | 10 --
 block/qcow2.c   | 14 
 block/qed.c |  2 +-
 block/quorum.c  |  4 +--
 block/rbd.c |  2 +-
 block/sheepdog.c|  2 +-
 blockdev.c  |  7 ++--
 blockjob.c  |  6 ++--
 chardev/char.c  |  4 +--
 crypto/block-luks.c | 38 +-
 crypto/block.c  |  4 +--
 crypto/cipher-afalg.c   |  2 +-
 crypto/cipher-builtin.c |  8 ++---
 crypto/cipher-gcrypt.c  |  4 +--
 crypto/cipher-nettle.c  |  8 ++---
 crypto/hmac-gcrypt.c|  2 +-
 crypto/hmac-glib.c  |  2 +-
 crypto/hmac-nettle.c|  2 +-
 crypto/pbkdf-gcrypt.c   |  2 +-
 crypto/pbkdf-nettle.c   |  2 +-
 crypto/secret.c |  2 +-
 crypto/tlscreds.c   |  2 +-
 hmp.c   | 64 +
 hw/block/fdc.c  |  6 ++--
 hw/char/escc.c  |  2 +-
 hw/core/qdev-properties.c   | 10 +++---
 hw/input/virtio-input-hid.c |  4 +--
 migration/colo-failover.c   |  4 +--
 migration/colo.c| 14 
 migration/global_state.c|  5 ++-
 monitor.c   | 20 ++--
 net/filter.c|  2 +-
 net/net.c   |  4 +--
 qapi/qapi-util.c| 13 
 qapi/qmp-dispatch.c |  2 +-
 qemu-img.c  |  6 ++--
 qemu-nbd.c  |  3 +-
 qom/object.c| 16 +-
 tests/check-qom-proplist.c  |  7 +++-
 tests/test-qapi-util.c  | 17 --
 tests/test-qobject-input-visitor.c  |  8 ++---
 tests/test-qobject-output-visitor.c |  2 +-
 tests/test-string-input-visitor.c   |  4 +--
 tests/test-string-output-visitor.c  |  4 +--
 tpm.c   |  4 +--
 ui/input-legacy.c   |  6 ++--
 ui/input.c  | 12 +++
 ui/vnc.c|  6 ++--
 vl.c|  6 ++--
 66 files changed, 241 insertions(+), 248 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 0f3b8cb459..62a51a54cb 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -469,7 +469,7 @@ bool visit_optional(Visitor *v, const char *name, bool 
*present);
  * that visit_type_str() must have no unwelcome side effects.
  */
 void visit_type_enum(Visitor *v, const char *name, int *obj,
- const char *const strings[], Error **errp);
+ const QEnumLookup *lookup, Error **errp);
 
 /*
  * Check if visitor is an input visitor.
diff --git a/scripts/qapi.py b/scripts/qapi.py
index a3ac799535..314d7e0365 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1851,7 +1851,7 @@ def guardend(name):
 def gen_enum_lookup(name, values, prefix=None):
 ret = mcgen('''
 
-const char *const %(c_name)s_lookup[] = {
+static const char *const %(c_name)s_array[] = {
 ''',
 c_name=c_name(name))
 for value in values:
@@ -1865,8 +1865,13 @@ const char *const %(c_name)s_lookup[] = {
 ret += mcgen('''
 [%(max_index)s] = NULL,
 };
+
+const QEnumLookup %(c_name)s_lookup = {
+.array = %(c_name)s_array,
+.size = %(max_index)s
+};
 ''',
- max_index=max_index)
+ max_index=max_index, c_name=c_name(name))
 return ret
 
 
@@ -1896,7 +1901,7 @@ typedef enum %(c_name)s {
 
 ret += mcgen('''
 
-extern const char *const %(c_name)s_lookup[];
+extern const QEnumLookup 

[Qemu-block] [PATCH v2 06/54] qapi: introduce qapi_enum_lookup()

[Marc-André Lureau]

This will help with the introduction of a new structure to handle
enum lookup.

Signed-off-by: Marc-André Lureau 
---
 include/qapi/util.h |  2 +
 backends/hostmem.c  |  3 +-
 block/backup.c  |  3 +-
 block/file-posix.c  |  5 ++-
 block/file-win32.c  |  2 +-
 block/gluster.c |  4 +-
 block/iscsi.c   |  3 +-
 block/nfs.c |  3 +-
 block/qcow2.c   |  5 ++-
 block/qed.c |  3 +-
 block/rbd.c |  3 +-
 block/sheepdog.c|  3 +-
 blockdev.c  |  6 ++-
 blockjob.c  |  9 +++--
 chardev/char.c  |  7 +++-
 crypto/block-luks.c | 17 ++---
 crypto/block.c  |  5 ++-
 crypto/cipher-afalg.c   |  2 +-
 crypto/cipher-builtin.c |  8 ++--
 crypto/cipher-gcrypt.c  |  4 +-
 crypto/cipher-nettle.c  |  8 ++--
 crypto/cipher.c |  1 +
 crypto/hmac-gcrypt.c|  2 +-
 crypto/hmac-glib.c  |  3 +-
 crypto/hmac-nettle.c|  3 +-
 crypto/pbkdf-gcrypt.c   |  3 +-
 crypto/pbkdf-nettle.c   |  4 +-
 hmp.c   | 74 +++--
 hw/block/fdc.c  |  9 +++--
 hw/char/escc.c  |  3 +-
 hw/core/qdev-properties.c   |  9 +++--
 hw/input/virtio-input-hid.c |  5 ++-
 migration/colo-failover.c   |  6 ++-
 migration/colo.c| 17 +
 migration/global_state.c|  2 +-
 monitor.c   | 24 +++-
 net/net.c   |  5 ++-
 qapi/qapi-util.c|  7 
 qapi/qmp-dispatch.c |  4 +-
 tests/test-qobject-output-visitor.c |  4 +-
 tests/test-string-output-visitor.c  |  6 ++-
 tpm.c   |  4 +-
 ui/input.c  | 13 ---
 ui/vnc.c|  8 ++--
 vl.c|  7 ++--
 45 files changed, 206 insertions(+), 122 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7436ed815c..60733b6a80 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,6 +11,8 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
+const char *qapi_enum_lookup(const char * const lookup[], int val);
+
 int qapi_enum_parse(const char * const lookup[], const char *buf,
 int max, int def, Error **errp);
 
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4606b73849..c4f795475c 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -13,6 +13,7 @@
 #include "sysemu/hostmem.h"
 #include "hw/boards.h"
 #include "qapi/error.h"
+#include "qapi/util.h"
 #include "qapi/visitor.h"
 #include "qapi-types.h"
 #include "qapi-visit.h"
@@ -304,7 +305,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
Error **errp)
 return;
 } else if (maxnode == 0 && backend->policy != MPOL_DEFAULT) {
 error_setg(errp, "host-nodes must be set for policy %s",
-   HostMemPolicy_lookup[backend->policy]);
+qapi_enum_lookup(HostMemPolicy_lookup, backend->policy));
 return;
 }
 
diff --git a/block/backup.c b/block/backup.c
index 504a089150..a700cc0315 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -19,6 +19,7 @@
 #include "block/blockjob_int.h"
 #include "block/block_backup.h"
 #include "qapi/error.h"
+#include "qapi/util.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/cutils.h"
@@ -596,7 +597,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
-   MirrorSyncMode_lookup[sync_mode]);
+   qapi_enum_lookup(MirrorSyncMode_lookup, sync_mode));
 return NULL;
 }
 
diff --git a/block/file-posix.c b/block/file-posix.c
index cb3bfce147..48200aef0b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1725,7 +1725,7 @@ static int raw_regular_truncate(int fd, int64_t offset, 
PreallocMode prealloc,
 default:
 result = -ENOTSUP;
 error_setg(errp, "Unsupported preallocation mode: %s",
-   PreallocMode_lookup[prealloc]);
+   qapi_enum_lookup(PreallocMode_lookup, prealloc));
 return result;
 }
 
@@ -1760,7 +1760,8 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
offset,
 
 if (prealloc != PREALLOC_MODE_OFF) {
 error_setg(errp, "Preallocation mode '%s' unsupported for this "
-   "non-regular file", 

[Qemu-block] [PATCH v2 11/54] quorum: use qapi_enum_parse() in quorum_open

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 block/quorum.c | 27 ---
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index d04da4f430..e4271caa7a 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -22,6 +22,7 @@
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/util.h"
 #include "qapi-event.h"
 #include "crypto/hash.h"
 
@@ -867,24 +868,6 @@ static QemuOptsList quorum_runtime_opts = {
 },
 };
 
-static int parse_read_pattern(const char *opt)
-{
-int i;
-
-if (!opt) {
-/* Set quorum as default */
-return QUORUM_READ_PATTERN_QUORUM;
-}
-
-for (i = 0; i < QUORUM_READ_PATTERN__MAX; i++) {
-if (!strcmp(opt, QuorumReadPattern_lookup[i])) {
-return i;
-}
-}
-
-return -EINVAL;
-}
-
 static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
 {
@@ -925,7 +908,13 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto exit;
 }
 
-ret = parse_read_pattern(qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN));
+if (!qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN)) {
+ret = QUORUM_READ_PATTERN_QUORUM;
+} else {
+ret = qapi_enum_parse(QuorumReadPattern_lookup,
+  qemu_opt_get(opts, QUORUM_OPT_READ_PATTERN),
+  QUORUM_READ_PATTERN__MAX, -EINVAL, NULL);
+}
 if (ret < 0) {
 error_setg(_err, "Please set read-pattern as fifo or quorum");
 goto exit;
-- 
2.14.1.146.gd35faa819

[Qemu-block] [PATCH v2 10/54] block: use qemu_enum_parse() in blkdebug_debug_breakpoint

[Marc-André Lureau]

This slightly change the error report message from "Invalid event
name" to "Invalid parameter".

Signed-off-by: Marc-André Lureau 
---
 block/blkdebug.c | 28 
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index c19ab28f07..50edda2a31 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -32,6 +32,7 @@
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/util.h"
 #include "sysemu/qtest.h"
 
 typedef struct BDRVBlkdebugState {
@@ -149,20 +150,6 @@ static QemuOptsList *config_groups[] = {
 NULL
 };
 
-static int get_event_by_name(const char *name, BlkdebugEvent *event)
-{
-int i;
-
-for (i = 0; i < BLKDBG__MAX; i++) {
-if (!strcmp(BlkdebugEvent_lookup[i], name)) {
-*event = i;
-return 0;
-}
-}
-
-return -1;
-}
-
 struct add_rule_data {
 BDRVBlkdebugState *s;
 int action;
@@ -173,7 +160,7 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 struct add_rule_data *d = opaque;
 BDRVBlkdebugState *s = d->s;
 const char* event_name;
-BlkdebugEvent event;
+int event;
 struct BlkdebugRule *rule;
 int64_t sector;
 
@@ -182,8 +169,9 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 if (!event_name) {
 error_setg(errp, "Missing event name for rule");
 return -1;
-} else if (get_event_by_name(event_name, ) < 0) {
-error_setg(errp, "Invalid event name \"%s\"", event_name);
+}
+event = qapi_enum_parse(BlkdebugEvent_lookup, event_name, BLKDBG__MAX, -1, 
errp);
+if (event < 0) {
 return -1;
 }
 
@@ -743,13 +731,13 @@ static int blkdebug_debug_breakpoint(BlockDriverState 
*bs, const char *event,
 {
 BDRVBlkdebugState *s = bs->opaque;
 struct BlkdebugRule *rule;
-BlkdebugEvent blkdebug_event;
+int blkdebug_event;
 
-if (get_event_by_name(event, _event) < 0) {
+blkdebug_event = qapi_enum_parse(BlkdebugEvent_lookup, event, BLKDBG__MAX, 
-1, NULL);
+if (blkdebug_event < 0) {
 return -ENOENT;
 }
 
-
 rule = g_malloc(sizeof(*rule));
 *rule = (struct BlkdebugRule) {
 .event  = blkdebug_event,
-- 
2.14.1.146.gd35faa819

[Qemu-block] [PATCH 04/10] scsi: introduce sg_io_sense_from_errno

[Paolo Bonzini]

Move more knowledge of SG_IO out of hw/scsi/scsi-generic.c, for
reusability.

Signed-off-by: Paolo Bonzini 
---
 hw/scsi/scsi-generic.c | 40 +++-
 include/scsi/utils.h   |  3 +++
 scsi/utils.c   | 35 +++
 3 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a8f500934..04c687ee76 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -81,6 +81,7 @@ static void scsi_free_request(SCSIRequest *req)
 static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
 {
 int status;
+SCSISense sense;
 
 assert(r->req.aiocb == NULL);
 
@@ -88,42 +89,15 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, 
int ret)
 scsi_req_cancel_complete(>req);
 goto done;
 }
-if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
-r->req.sense_len = r->io_header.sb_len_wr;
-}
-
-if (ret != 0) {
-switch (ret) {
-case -EDOM:
-status = TASK_SET_FULL;
-break;
-case -ENOMEM:
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(TARGET_FAILURE));
-break;
-default:
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(IO_ERROR));
-break;
-}
-} else {
-if (r->io_header.host_status == SG_ERR_DID_NO_CONNECT ||
-r->io_header.host_status == SG_ERR_DID_BUS_BUSY ||
-r->io_header.host_status == SG_ERR_DID_TIME_OUT ||
-(r->io_header.driver_status & SG_ERR_DRIVER_TIMEOUT)) {
-status = BUSY;
-BADF("Driver Timeout\n");
-} else if (r->io_header.host_status) {
-status = CHECK_CONDITION;
-scsi_req_build_sense(>req, SENSE_CODE(I_T_NEXUS_LOSS));
-} else if (r->io_header.status) {
-status = r->io_header.status;
-} else if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
-status = CHECK_CONDITION;
+status = sg_io_sense_from_errno(-ret, >io_header, );
+if (status == CHECK_CONDITION) {
+if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
+r->req.sense_len = r->io_header.sb_len_wr;
 } else {
-status = GOOD;
+scsi_req_build_sense(>req, sense);
 }
 }
+
 DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
 r, r->req.tag, status);
 
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 76c3db98c0..c12b34f2e5 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -113,6 +113,9 @@ int scsi_cdb_length(uint8_t *buf);
 #define SG_ERR_DID_TIME_OUT0x03
 
 #define SG_ERR_DRIVER_SENSE0x08
+
+int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
+   SCSISense *sense);
 #endif
 
 #endif
diff --git a/scsi/utils.c b/scsi/utils.c
index 54d6d4486a..ca5b058a73 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -412,3 +412,38 @@ const char *scsi_command_name(uint8_t cmd)
 }
 return names[cmd];
 }
+
+#ifdef CONFIG_LINUX
+int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
+   SCSISense *sense)
+{
+if (errno_value != 0) {
+switch (errno_value) {
+case EDOM:
+return TASK_SET_FULL;
+case ENOMEM:
+*sense = SENSE_CODE(TARGET_FAILURE);
+return CHECK_CONDITION;
+default:
+*sense = SENSE_CODE(IO_ERROR);
+return CHECK_CONDITION;
+}
+} else {
+if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
+io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
+io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
+(io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
+return BUSY;
+} else if (io_hdr->host_status) {
+*sense = SENSE_CODE(I_T_NEXUS_LOSS);
+return CHECK_CONDITION;
+} else if (io_hdr->status) {
+return io_hdr->status;
+} else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
+return CHECK_CONDITION;
+} else {
+return GOOD;
+}
+}
+}
+#endif
-- 
2.13.5

[Qemu-block] [PATCH 01/10] scsi: rename scsi_convert_sense

[Paolo Bonzini]

After introducing the scsi/ subdirectory, there will be a scsi_build_sense
function that is the same as scsi_req_build_sense but without needing
a SCSIRequest.  The existing scsi_build_sense function gets in the way,
remove it.

Signed-off-by: Paolo Bonzini 
---
 hw/scsi/scsi-bus.c | 10 +-
 hw/scsi/scsi-disk.c|  4 ++--
 include/hw/scsi/scsi.h |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index e364410a23..890f8fcc83 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -769,7 +769,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int 
len)
 return 0;
 }
 
-ret = scsi_build_sense(req->sense, req->sense_len, buf, len, true);
+ret = scsi_convert_sense(req->sense, req->sense_len, buf, len, true);
 
 /*
  * FIXME: clearing unit attention conditions upon autosense should be done
@@ -790,7 +790,7 @@ int scsi_req_get_sense(SCSIRequest *req, uint8_t *buf, int 
len)
 
 int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed)
 {
-return scsi_build_sense(dev->sense, dev->sense_len, buf, len, fixed);
+return scsi_convert_sense(dev->sense, dev->sense_len, buf, len, fixed);
 }
 
 void scsi_req_build_sense(SCSIRequest *req, SCSISense sense)
@@ -1510,12 +1510,12 @@ const struct SCSISense sense_code_SPACE_ALLOC_FAILED = {
 };
 
 /*
- * scsi_build_sense
+ * scsi_convert_sense
  *
  * Convert between fixed and descriptor sense buffers
  */
-int scsi_build_sense(uint8_t *in_buf, int in_len,
- uint8_t *buf, int len, bool fixed)
+int scsi_convert_sense(uint8_t *in_buf, int in_len,
+   uint8_t *buf, int len, bool fixed)
 {
 bool fixed_in;
 SCSISense sense;
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5f1e5e8070..0a1f4ef0c7 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1978,8 +1978,8 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 break;
 case REQUEST_SENSE:
 /* Just return "NO SENSE".  */
-buflen = scsi_build_sense(NULL, 0, outbuf, r->buflen,
-  (req->cmd.buf[1] & 1) == 0);
+buflen = scsi_convert_sense(NULL, 0, outbuf, r->buflen,
+(req->cmd.buf[1] & 1) == 0);
 if (buflen < 0) {
 goto illegal_request;
 }
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 6b85786dbf..6ef67fb504 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -244,8 +244,8 @@ extern const struct SCSISense sense_code_SPACE_ALLOC_FAILED;
 uint32_t scsi_data_cdb_xfer(uint8_t *buf);
 uint32_t scsi_cdb_xfer(uint8_t *buf);
 int scsi_cdb_length(uint8_t *buf);
-int scsi_build_sense(uint8_t *in_buf, int in_len,
- uint8_t *buf, int len, bool fixed);
+int scsi_convert_sense(uint8_t *in_buf, int in_len,
+   uint8_t *buf, int len, bool fixed);
 
 SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
 uint32_t tag, uint32_t lun, void *hba_private);
-- 
2.13.5

[Qemu-block] [PATCH 03/10] scsi: introduce scsi_build_sense

[Paolo Bonzini]

Move more knowledge of sense data format out of hw/scsi/scsi-bus.c
for reusability.

Signed-off-by: Paolo Bonzini 
---
 hw/scsi/scsi-bus.c   |  8 +---
 include/scsi/utils.h |  2 ++
 scsi/utils.c | 11 +++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 300912d213..f71218f02a 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -797,13 +797,7 @@ void scsi_req_build_sense(SCSIRequest *req, SCSISense 
sense)
 {
 trace_scsi_req_build_sense(req->dev->id, req->lun, req->tag,
sense.key, sense.asc, sense.ascq);
-memset(req->sense, 0, 18);
-req->sense[0] = 0x70;
-req->sense[2] = sense.key;
-req->sense[7] = 10;
-req->sense[12] = sense.asc;
-req->sense[13] = sense.ascq;
-req->sense_len = 18;
+req->sense_len = scsi_build_sense(req->sense, sense);
 }
 
 static void scsi_req_enqueue_internal(SCSIRequest *req)
diff --git a/include/scsi/utils.h b/include/scsi/utils.h
index 35a74436bf..76c3db98c0 100644
--- a/include/scsi/utils.h
+++ b/include/scsi/utils.h
@@ -30,6 +30,8 @@ typedef struct SCSISense {
 uint8_t ascq;
 } SCSISense;
 
+int scsi_build_sense(uint8_t *buf, SCSISense sense);
+
 /*
  * Predefined sense codes
  */
diff --git a/scsi/utils.c b/scsi/utils.c
index 0db727591f..54d6d4486a 100644
--- a/scsi/utils.c
+++ b/scsi/utils.c
@@ -81,6 +81,17 @@ int scsi_cdb_length(uint8_t *buf)
 return cdb_len;
 }
 
+int scsi_build_sense(uint8_t *buf, SCSISense sense)
+{
+memset(buf, 0, 18);
+buf[0] = 0x70;
+buf[2] = sense.key;
+buf[7] = 10;
+buf[12] = sense.asc;
+buf[13] = sense.ascq;
+return 18;
+}
+
 /*
  * Predefined sense codes
  */
-- 
2.13.5

[Qemu-block] [RFC PATCH 00/10] scsi, block: introduce persistent reservation managers

[Paolo Bonzini]

SCSI persistent Reservations allow restricting access to block devices
to specific initiators in a shared storage setup.  When implementing
clustering of virtual machines, it is a common requirement for virtual
machines to send persistent reservation SCSI commands.  However,
the operating system restricts sending these commands to unprivileged
programs because incorrect usage can disrupt regular operation of the
storage fabric.

With these patches, the scsi-block and scsi-generic SCSI passthrough
devices learn to delegate the implementation of persistent reservations to
a separate object, the "persistent reservation manager".  The persistent
reservation manager talks to a separate privileged program, with a very
simple protocol based on SCM_RIGHTS.  In addition to invoking PERSISTENT
RESERVATION OUT and PERSISTENT RESERVATION IN commands, the privileged
component can also use libmpathpersist so that persistent reservations
are applied to all paths in a multipath setting.

Patches 1-5 introduce a common scsi/ directory used by block/iscsi.c,
hw/scsi/* and now by the persistent reservation helper.

Patch 6 defines the abstract QOM class and plugs it into block/file-posix.c.

Patch 7 to 9 introduce the privileged helper program, while patch 10
defines the concrete QOM class that talks to it.

Paolo

Paolo Bonzini (10):
  scsi: rename scsi_convert_sense
  scsi: move non-emulation specific code to scsi/
  scsi: introduce scsi_build_sense
  scsi: introduce sg_io_sense_from_errno
  scsi: move block/scsi.h to include/scsi/constants.h
  scsi, file-posix: add support for persistent reservation management
  io: add qio_channel_read/write_all
  scsi: build qemu-pr-helper
  scsi: add multipath support to qemu-pr-helper
  scsi: add persistent reservation manager using qemu-pr-helper

 MAINTAINERS|   7 +
 Makefile   |  12 +-
 Makefile.objs  |   3 +-
 block/file-posix.c |  30 +
 block/iscsi.c  |   2 +-
 configure  |  59 +-
 docs/interop/pr-helper.rst |  78 +++
 docs/pr-manager.rst| 111 
 hw/block/virtio-blk.c  |   2 +-
 hw/scsi/megasas.c  |   2 +-
 hw/scsi/mptendian.c|   2 +-
 hw/scsi/mptsas.c   |   2 +-
 hw/scsi/scsi-bus.c | 411 +---
 hw/scsi/scsi-disk.c|   6 +-
 hw/scsi/scsi-generic.c |  50 +-
 hw/scsi/spapr_vscsi.c  |   2 +-
 hw/scsi/virtio-scsi-dataplane.c|   2 +-
 hw/scsi/virtio-scsi.c  |   2 +-
 hw/scsi/vmw_pvscsi.c   |   2 +-
 hw/usb/dev-uas.c   |   2 +-
 include/hw/ide/internal.h  |   2 +-
 include/hw/scsi/scsi.h |  94 +--
 include/io/channel.h   |  36 +-
 include/{block/scsi.h => scsi/constants.h} |   2 -
 include/scsi/pr-manager.h  |  57 ++
 include/scsi/utils.h   | 127 
 io/channel.c   |  54 ++
 qapi/block-core.json   |   4 +
 scsi/Makefile.objs |   3 +
 scsi/pr-helper.h   |  13 +
 scsi/pr-manager-helper.c   | 288 +
 scsi/pr-manager.c  | 109 
 scsi/qemu-pr-helper.c  | 964 +
 scsi/utils.c   | 464 ++
 tests/virtio-scsi-test.c   |   2 +-
 vl.c   |   3 +-
 36 files changed, 2442 insertions(+), 567 deletions(-)
 create mode 100644 docs/interop/pr-helper.rst
 create mode 100644 docs/pr-manager.rst
 rename include/{block/scsi.h => scsi/constants.h} (99%)
 create mode 100644 include/scsi/pr-manager.h
 create mode 100644 include/scsi/utils.h
 create mode 100644 scsi/Makefile.objs
 create mode 100644 scsi/pr-helper.h
 create mode 100644 scsi/pr-manager-helper.c
 create mode 100644 scsi/pr-manager.c
 create mode 100644 scsi/qemu-pr-helper.c
 create mode 100644 scsi/utils.c

-- 
2.13.5

[Qemu-block] [PATCH 05/10] scsi: move block/scsi.h to include/scsi/constants.h

[Paolo Bonzini]

Complete the transition by renaming this header, which was
shared by block/iscsi.c and the SCSI emulation code.

Signed-off-by: Paolo Bonzini 
---
 block/iscsi.c  | 2 +-
 hw/block/virtio-blk.c  | 2 +-
 hw/scsi/megasas.c  | 2 +-
 hw/scsi/mptendian.c| 2 +-
 hw/scsi/mptsas.c   | 2 +-
 hw/scsi/scsi-bus.c | 2 +-
 hw/scsi/scsi-disk.c| 2 +-
 hw/scsi/scsi-generic.c | 2 +-
 hw/scsi/spapr_vscsi.c  | 2 +-
 hw/scsi/virtio-scsi-dataplane.c| 2 +-
 hw/scsi/virtio-scsi.c  | 2 +-
 hw/scsi/vmw_pvscsi.c   | 2 +-
 hw/usb/dev-uas.c   | 2 +-
 include/hw/ide/internal.h  | 2 +-
 include/{block/scsi.h => scsi/constants.h} | 0
 scsi/utils.c   | 2 +-
 tests/virtio-scsi-test.c   | 2 +-
 17 files changed, 16 insertions(+), 16 deletions(-)
 rename include/{block/scsi.h => scsi/constants.h} (100%)

diff --git a/block/iscsi.c b/block/iscsi.c
index d557c99668..c43c0953e1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -34,7 +34,7 @@
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "block/block_int.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "qemu/iov.h"
 #include "qemu/uuid.h"
 #include "qmp-commands.h"
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a16ac75090..05d1440786 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -22,7 +22,7 @@
 #include "sysemu/blockdev.h"
 #include "hw/virtio/virtio-blk.h"
 #include "dataplane/virtio-blk.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #ifdef __linux__
 # include 
 #endif
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 734fdaef90..0db68aacee 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -27,7 +27,7 @@
 #include "hw/pci/msix.h"
 #include "qemu/iov.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "trace.h"
 #include "qapi/error.h"
 #include "mfi.h"
diff --git a/hw/scsi/mptendian.c b/hw/scsi/mptendian.c
index b7fe2a2a36..3415229b5e 100644
--- a/hw/scsi/mptendian.c
+++ b/hw/scsi/mptendian.c
@@ -28,7 +28,7 @@
 #include "hw/pci/msi.h"
 #include "qemu/iov.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "trace.h"
 
 #include "mptsas.h"
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 765ab53c34..8bae8f543e 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -30,7 +30,7 @@
 #include "hw/pci/msi.h"
 #include "qemu/iov.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "trace.h"
 #include "qapi/error.h"
 #include "mptsas.h"
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f71218f02a..b9244606f8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -3,7 +3,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "hw/qdev.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0a1f4ef0c7..5faf6682c5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -32,7 +32,7 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 04c687ee76..bd0d9ff355 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -34,7 +34,7 @@ do { printf("scsi-generic: " fmt , ## __VA_ARGS__); } while 
(0)
 do { fprintf(stderr, "scsi-generic: " fmt , ## __VA_ARGS__); } while (0)
 
 #include 
-#include "block/scsi.h"
+#include "scsi/constants.h"
 
 #ifndef MAX_UINT
 #define MAX_UINT ((unsigned int)-1)
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 55ee48c4da..360db53ac8 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -36,7 +36,7 @@
 #include "cpu.h"
 #include "hw/hw.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "srp.h"
 #include "hw/qdev.h"
 #include "hw/ppc/spapr.h"
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 944ea4eb53..add4b3f4a4 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -17,7 +17,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/block-backend.h"
 #include "hw/scsi/scsi.h"
-#include "block/scsi.h"
+#include "scsi/constants.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 

[Qemu-block] [PATCH 02/10] scsi: move non-emulation specific code to scsi/

[Paolo Bonzini]

There is a bunch of SCSI code that is shared by block/iscsi.c and
hw/scsi, and the introduction of the persistent reservation helper
will add more instances of this.  There is also include/block/scsi.h,
which actually is not part of the core block layer.

Create a directory for this kind of shared code.

Signed-off-by: Paolo Bonzini 
---
 MAINTAINERS|   7 +
 Makefile.objs  |   2 +-
 hw/scsi/scsi-bus.c | 397 
 hw/scsi/scsi-generic.c |   8 -
 include/block/scsi.h   |   2 -
 include/hw/scsi/scsi.h |  94 +---
 include/scsi/utils.h   | 116 ++
 scsi/Makefile.objs |   1 +
 scsi/utils.c   | 403 +
 9 files changed, 529 insertions(+), 501 deletions(-)
 create mode 100644 include/scsi/utils.h
 create mode 100644 scsi/Makefile.objs
 create mode 100644 scsi/utils.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ccee28b12d..fa6e21cd38 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1213,6 +1213,13 @@ F: migration/block*
 F: include/block/aio.h
 T: git git://github.com/stefanha/qemu.git block
 
+Block SCSI subsystem
+M: Paolo Bonzini 
+L: qemu-block@nongnu.org
+S: Supported
+F: include/scsi/*
+F: scsi/*
+
 Block Jobs
 M: Jeff Cody 
 L: qemu-block@nongnu.org
diff --git a/Makefile.objs b/Makefile.objs
index 24a4ea08b8..f68aa3b60d 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -11,7 +11,7 @@ chardev-obj-y = chardev/
 
 block-obj-y += nbd/
 block-obj-y += block.o blockjob.o
-block-obj-y += block/
+block-obj-y += block/ scsi/
 block-obj-y += qemu-io-cmds.o
 block-obj-$(CONFIG_REPLICATION) += replication.o
 
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 890f8fcc83..300912d213 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -935,36 +935,6 @@ static int ata_passthrough_16_xfer(SCSIDevice *dev, 
uint8_t *buf)
 return xfer * unit;
 }
 
-uint32_t scsi_data_cdb_xfer(uint8_t *buf)
-{
-if ((buf[0] >> 5) == 0 && buf[4] == 0) {
-return 256;
-} else {
-return scsi_cdb_xfer(buf);
-}
-}
-
-uint32_t scsi_cdb_xfer(uint8_t *buf)
-{
-switch (buf[0] >> 5) {
-case 0:
-return buf[4];
-break;
-case 1:
-case 2:
-return lduw_be_p([7]);
-break;
-case 4:
-return ldl_be_p([10]) & 0xULL;
-break;
-case 5:
-return ldl_be_p([6]) & 0xULL;
-break;
-default:
-return -1;
-}
-}
-
 static int scsi_req_xfer(SCSICommand *cmd, SCSIDevice *dev, uint8_t *buf)
 {
 cmd->xfer = scsi_cdb_xfer(buf);
@@ -1277,53 +1247,6 @@ static void scsi_cmd_xfer_mode(SCSICommand *cmd)
 }
 }
 
-static uint64_t scsi_cmd_lba(SCSICommand *cmd)
-{
-uint8_t *buf = cmd->buf;
-uint64_t lba;
-
-switch (buf[0] >> 5) {
-case 0:
-lba = ldl_be_p([0]) & 0x1f;
-break;
-case 1:
-case 2:
-case 5:
-lba = ldl_be_p([2]) & 0xULL;
-break;
-case 4:
-lba = ldq_be_p([2]);
-break;
-default:
-lba = -1;
-
-}
-return lba;
-}
-
-int scsi_cdb_length(uint8_t *buf) {
-int cdb_len;
-
-switch (buf[0] >> 5) {
-case 0:
-cdb_len = 6;
-break;
-case 1:
-case 2:
-cdb_len = 10;
-break;
-case 4:
-cdb_len = 16;
-break;
-case 5:
-cdb_len = 12;
-break;
-default:
-cdb_len = -1;
-}
-return cdb_len;
-}
-
 int scsi_req_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf)
 {
 int rc;
@@ -1370,326 +1293,6 @@ void scsi_device_report_change(SCSIDevice *dev, 
SCSISense sense)
 }
 }
 
-/*
- * Predefined sense codes
- */
-
-/* No sense data available */
-const struct SCSISense sense_code_NO_SENSE = {
-.key = NO_SENSE , .asc = 0x00 , .ascq = 0x00
-};
-
-/* LUN not ready, Manual intervention required */
-const struct SCSISense sense_code_LUN_NOT_READY = {
-.key = NOT_READY, .asc = 0x04, .ascq = 0x03
-};
-
-/* LUN not ready, Medium not present */
-const struct SCSISense sense_code_NO_MEDIUM = {
-.key = NOT_READY, .asc = 0x3a, .ascq = 0x00
-};
-
-/* LUN not ready, medium removal prevented */
-const struct SCSISense sense_code_NOT_READY_REMOVAL_PREVENTED = {
-.key = NOT_READY, .asc = 0x53, .ascq = 0x02
-};
-
-/* Hardware error, internal target failure */
-const struct SCSISense sense_code_TARGET_FAILURE = {
-.key = HARDWARE_ERROR, .asc = 0x44, .ascq = 0x00
-};
-
-/* Illegal request, invalid command operation code */
-const struct SCSISense sense_code_INVALID_OPCODE = {
-.key = ILLEGAL_REQUEST, .asc = 0x20, .ascq = 0x00
-};
-
-/* Illegal request, LBA out of range */
-const struct SCSISense sense_code_LBA_OUT_OF_RANGE = {
-.key = ILLEGAL_REQUEST, .asc = 0x21, .ascq = 0x00
-};
-
-/* Illegal request, Invalid field in CDB */
-const struct SCSISense sense_code_INVALID_FIELD = {
-.key = 

[Qemu-block] [PATCH 09/10] scsi: add multipath support to qemu-pr-helper

[Paolo Bonzini]

Proper support of persistent reservation for multipath devices requires
communication with the multipath daemon, so that the reservation is
registered and applied when a path comes up.  The device mapper
utilities provide a library to do so; this patch makes qemu-pr-helper.c
detect multipath devices and, when one is found, delegate the operation
to libmpathpersist.

Signed-off-by: Paolo Bonzini 
---
 Makefile  |   3 +
 configure |  57 -
 docs/pr-manager.rst   |  27 +
 include/scsi/utils.h  |   6 +
 scsi/qemu-pr-helper.c | 311 +-
 scsi/utils.c  |  15 +++
 6 files changed, 414 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index bfd4f69ecd..f1acaad05b 100644
--- a/Makefile
+++ b/Makefile
@@ -388,6 +388,9 @@ fsdev/virtfs-proxy-helper$(EXESUF): 
fsdev/virtfs-proxy-helper.o fsdev/9p-marshal
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
 
 scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
$(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
+ifdef CONFIG_MPATH
+scsi/qemu-pr-helper$(EXESUF): LIBS += -ludev -lmultipath -lmpathpersist
+endif
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
$@,"GEN","$@")
diff --git a/configure b/configure
index 772aff18d6..d3c9371f7c 100755
--- a/configure
+++ b/configure
@@ -286,6 +286,7 @@ pixman=""
 sdl=""
 sdlabi=""
 virtfs=""
+mpath=""
 vnc="yes"
 sparse="no"
 vde=""
@@ -948,6 +949,10 @@ for opt do
   ;;
   --enable-virtfs) virtfs="yes"
   ;;
+  --disable-mpath) mpath="no"
+  ;;
+  --enable-mpath) mpath="yes"
+  ;;
   --disable-vnc) vnc="no"
   ;;
   --enable-vnc) vnc="yes"
@@ -1491,6 +1496,7 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   vnc-png PNG compression for VNC server
   cocoa   Cocoa UI (Mac OS X only)
   virtfs  VirtFS
+  mpath   Multipath persistent reservation passthrough
   xen xen backend driver support
   xen-pci-passthrough
   brlapi  BrlAPI (Braile)
@@ -3336,6 +3342,29 @@ else
 fi
 
 ##
+# libmpathpersist probe
+
+if test "$mpath" != "no" ; then
+  cat > $TMPC <
+#include 
+unsigned mpath_mx_alloc_len = 1024;
+int logsink;
+int main(void) {
+struct udev *udev = udev_new();
+mpath_lib_init(udev);
+}
+EOF
+  if compile_prog "" "-ludev -lmultipath -lmpathpersist" ; then
+mpathpersist=yes
+  else
+mpathpersist=no
+  fi
+else
+  mpathpersist=no
+fi
+
+##
 # libcap probe
 
 if test "$cap" != "no" ; then
@@ -5070,16 +5099,34 @@ if test "$want_tools" = "yes" ; then
   fi
 fi
 if test "$softmmu" = yes ; then
-  if test "$virtfs" != no ; then
-if test "$cap" = yes && test "$linux" = yes && test "$attr" = yes ; then
+  if test "$linux" = yes; then
+if test "$virtfs" != no && test "$cap" = yes && test "$attr" = yes ; then
   virtfs=yes
   tools="$tools fsdev/virtfs-proxy-helper\$(EXESUF)"
 else
   if test "$virtfs" = yes; then
-error_exit "VirtFS is supported only on Linux and requires libcap 
devel and libattr devel"
+error_exit "VirtFS requires libcap devel and libattr devel"
   fi
   virtfs=no
 fi
+if test "$mpath" != no && test "$mpathpersist" = yes ; then
+  mpath=yes
+  tools="$tools mpath/qemu-mpath-helper\$(EXESUF)"
+else
+  if test "$mpath" = yes; then
+error_exit "Multipath requires libmpathpersist devel"
+  fi
+  mpath=no
+fi
+  else
+if test "$virtfs" = yes; then
+  error_exit "VirtFS is supported only on Linux"
+fi
+virtfs=no
+if test "$mpath" = yes; then
+  error_exit "Multipath is supported only on Linux"
+fi
+mpath=no
   fi
 fi
 
@@ -5326,6 +5373,7 @@ echo "Audio drivers $audio_drv_list"
 echo "Block whitelist (rw) $block_drv_rw_whitelist"
 echo "Block whitelist (ro) $block_drv_ro_whitelist"
 echo "VirtFS support$virtfs"
+echo "Multipath support $mpath"
 echo "VNC support   $vnc"
 if test "$vnc" = "yes" ; then
 echo "VNC SASL support  $vnc_sasl"
@@ -5773,6 +5821,9 @@ fi
 if test "$virtfs" = "yes" ; then
   echo "CONFIG_VIRTFS=y" >> $config_host_mak
 fi
+if test "$mpath" = "yes" ; then
+  echo "CONFIG_MPATH=y" >> $config_host_mak
+fi
 if test "$vhost_scsi" = "yes" ; then
   echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
 fi
diff --git a/docs/pr-manager.rst b/docs/pr-manager.rst
index 7107e59fb8..9b1de198b1 100644
--- a/docs/pr-manager.rst
+++ b/docs/pr-manager.rst
@@ -60,6 +60,7 @@ system service and supports the following option:
 
 -d, --daemon  run in the background
 -q, --quiet   decrease verbosity
+-v, --verbose increase verbosity
 -f, --pidfile=pathPID file when running as a daemon
 -k, --socket=path path to the socket
 -T, --trace=trace-opts 

Re: [Qemu-block] [PATCH] nbd-client: avoid spurious qio_channel_yield() re-entry

[Paolo Bonzini]

On 22/08/2017 14:51, Stefan Hajnoczi wrote:
> This should fix the issue that Dave is seeing but I'm concerned that
> there are more problems in nbd-client.c.  We don't have good
> abstractions for writing coroutine socket I/O code.  Something like Go's
> channels would avoid manual low-level coroutine calls.  There is
> currently no way to cancel qio_channel_yield() so requests doing I/O may
> remain in-flight indefinitely and nbd-client.c doesn't join them...

The idea was that shutdown(2) would force them to reenter...

Paolo

[Qemu-block] [PATCH 10/10] scsi: add persistent reservation manager using qemu-pr-helper

[Paolo Bonzini]

This adds a concrete subclass of pr-manager that talks to qemu-pr-helper.

Signed-off-by: Paolo Bonzini 
---
 scsi/Makefile.objs   |   2 +-
 scsi/pr-manager-helper.c | 288 +++
 2 files changed, 289 insertions(+), 1 deletion(-)
 create mode 100644 scsi/pr-manager-helper.c

diff --git a/scsi/Makefile.objs b/scsi/Makefile.objs
index 5496d2ae6a..4d25e476cf 100644
--- a/scsi/Makefile.objs
+++ b/scsi/Makefile.objs
@@ -1,3 +1,3 @@
 block-obj-y += utils.o
 
-block-obj-$(CONFIG_LINUX) += pr-manager.o
+block-obj-$(CONFIG_LINUX) += pr-manager.o pr-manager-helper.o
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
new file mode 100644
index 00..c9d9606696
--- /dev/null
+++ b/scsi/pr-manager-helper.c
@@ -0,0 +1,288 @@
+/*
+ * Persistent reservation manager that talks to qemu-mpath-helper
+ *
+ * Copyright (c) 2017 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini 
+ *
+ * This code is licensed under the LGPL.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "scsi/constants.h"
+#include "scsi/pr-manager.h"
+#include "scsi/utils.h"
+#include "io/channel.h"
+#include "io/channel-socket.h"
+#include "pr-helper.h"
+
+#include 
+
+#define PR_MAX_RECONNECT_ATTEMPTS 5
+
+#define TYPE_PR_MANAGER_HELPER "pr-manager-helper"
+
+#define PR_MANAGER_HELPER(obj) \
+ INTERFACE_CHECK(PRManagerHelper, (obj), \
+ TYPE_PR_MANAGER_HELPER)
+
+typedef struct PRManagerHelper {
+/*  */
+PRManager parent;
+
+char *path;
+
+QemuMutex lock;
+QIOChannel *ioc;
+} PRManagerHelper;
+
+/* Called with lock held.  */
+static int pr_manager_helper_read(PRManagerHelper *pr_mgr,
+  void *buf, int sz, Error **errp)
+{
+ssize_t r = qio_channel_read_all(pr_mgr->ioc, buf, sz, errp);
+
+if (r < 0) {
+object_unref(OBJECT(pr_mgr->ioc));
+pr_mgr->ioc = NULL;
+return r;
+}
+
+return r < 0 ? r : 0;
+}
+
+/* Called with lock held.  */
+static int pr_manager_helper_write(PRManagerHelper *pr_mgr,
+   int fd,
+   const void *buf, int sz, Error **errp)
+{
+size_t nfds = (fd != -1);
+while (sz > 0) {
+struct iovec iov;
+ssize_t n_written;
+
+iov.iov_base = (void *)buf;
+iov.iov_len = sz;
+n_written = qio_channel_writev_full(QIO_CHANNEL(pr_mgr->ioc), , 1,
+nfds ?  : NULL, nfds, errp);
+
+if (n_written <= 0) {
+assert(n_written != QIO_CHANNEL_ERR_BLOCK);
+object_unref(OBJECT(pr_mgr->ioc));
+pr_mgr->ioc = NULL;
+return n_written;
+}
+
+nfds = 0;
+buf += n_written;
+sz -= n_written;
+}
+
+return 0;
+}
+
+/* Called with lock held.  */
+static int pr_manager_helper_initialize(PRManagerHelper *pr_mgr,
+Error **errp)
+{
+uint32_t flags;
+
+SocketAddress saddr = {
+.type = SOCKET_ADDRESS_TYPE_UNIX,
+.u.q_unix.path = g_strdup(pr_mgr->path)
+};
+QIOChannelSocket *sioc = qio_channel_socket_new();
+Error *local_err = NULL;
+
+qio_channel_set_name(QIO_CHANNEL(sioc), "pr-manager-helper");
+qio_channel_socket_connect_sync(sioc,
+,
+_err);
+if (local_err) {
+object_unref(OBJECT(sioc));
+error_propagate(errp, local_err);
+return -ENOTCONN;
+}
+
+qio_channel_set_delay(QIO_CHANNEL(sioc), false);
+pr_mgr->ioc = QIO_CHANNEL(sioc);
+
+/* A simple feature negotation protocol, even though there is
+ * no optional feature right now.
+ */
+if (pr_manager_helper_read(pr_mgr, , sizeof(flags), errp) < 0) {
+return -EINVAL;
+}
+
+flags = 0;
+if (pr_manager_helper_write(pr_mgr, -1, , sizeof(flags), errp) < 0) {
+return -EINVAL;
+}
+
+return 0;
+}
+
+static int pr_manager_helper_run(PRManager *p,
+ int fd, struct sg_io_hdr *io_hdr)
+{
+PRManagerHelper *pr_mgr = PR_MANAGER_HELPER(p);
+
+uint32_t len;
+PRHelperResponse resp;
+int ret;
+int expected_dir;
+int attempts;
+uint8_t cdb[PR_HELPER_CDB_SIZE] = { 0 };
+
+if (!io_hdr->cmd_len || io_hdr->cmd_len > PR_HELPER_CDB_SIZE) {
+return -EINVAL;
+}
+
+memcpy(cdb, io_hdr->cmdp, io_hdr->cmd_len);
+assert(cdb[0] == PERSISTENT_RESERVE_OUT || cdb[0] == 
PERSISTENT_RESERVE_IN);
+expected_dir =
+(cdb[0] == PERSISTENT_RESERVE_OUT ? SG_DXFER_TO_DEV : 
SG_DXFER_FROM_DEV);
+if (io_hdr->dxfer_direction != expected_dir) {
+return -EINVAL;
+}
+
+len = scsi_cdb_xfer(cdb);
+if (io_hdr->dxfer_len < len || len > PR_HELPER_DATA_SIZE) {
+return -EINVAL;
+}
+
+ret = 0;
+qemu_mutex_lock(_mgr->lock);
+
+/* Try 

[Qemu-block] [PATCH 06/10] scsi, file-posix: add support for persistent reservation management

[Paolo Bonzini]

It is a common requirement for virtual machine to send persistent
reservations, but this currently requires either running QEMU with
CAP_SYS_RAWIO, or using out-of-tree patches that let an unprivileged
QEMU bypass Linux's filter on SG_IO commands.

As an alternative mechanism, the next patches will introduce a
privileged helper to run persistent reservation commands without
expanding QEMU's attack surface unnecessarily.

The helper is invoked through a "pr-manager" QOM object, to which
file-posix.c passes SG_IO requests for PERSISTENT RESERVE OUT and
PERSISTENT RESERVE IN commands.  For example:

  $ qemu-system-x86_64
  -device virtio-scsi \
  -object pr-manager-helper,id=helper0,path=/var/run/qemu-pr-helper.sock
  -drive 
if=none,id=hd,driver=raw,file.filename=/dev/sdb,file.pr-manager=helper0
  -device scsi-block,drive=hd

or:

  $ qemu-system-x86_64
  -device virtio-scsi \
  -object pr-manager-helper,id=helper0,path=/var/run/qemu-pr-helper.sock
  -blockdev 
node-name=hd,driver=raw,file.driver=host_device,file.filename=/dev/sdb,file.pr-manager=helper0
  -device scsi-block,drive=hd

Multiple pr-manager implementations are conceivable and possible, though
only one is implemented right now.  For example, a pr-manager could:

- talk directly to the multipath daemon from a privileged QEMU
  (i.e. QEMU links to libmpathpersist); this makes reservation work
  properly with multipath, but still requires CAP_SYS_RAWIO

- use the Linux IOC_PR_* ioctls (they require CAP_SYS_ADMIN though)

- more interestingly, implement reservations directly in QEMU
  through file system locks or a shared database (e.g. sqlite)

Signed-off-by: Paolo Bonzini 
---
 Makefile.objs |   1 +
 block/file-posix.c|  30 +
 docs/pr-manager.rst   |  51 ++
 include/scsi/pr-manager.h |  57 
 qapi/block-core.json  |   4 ++
 scsi/Makefile.objs|   2 +
 scsi/pr-manager.c | 109 ++
 vl.c  |   3 +-
 8 files changed, 256 insertions(+), 1 deletion(-)
 create mode 100644 docs/pr-manager.rst
 create mode 100644 include/scsi/pr-manager.h
 create mode 100644 scsi/pr-manager.c

diff --git a/Makefile.objs b/Makefile.objs
index f68aa3b60d..64bebd05db 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -168,6 +168,7 @@ trace-events-subdirs += qapi
 trace-events-subdirs += accel/tcg
 trace-events-subdirs += accel/kvm
 trace-events-subdirs += nbd
+trace-events-subdirs += scsi
 
 trace-events-files = $(SRC_PATH)/trace-events 
$(trace-events-subdirs:%=$(SRC_PATH)/%/trace-events)
 
diff --git a/block/file-posix.c b/block/file-posix.c
index f4de022ae0..47aadbf45d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -34,6 +34,9 @@
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
 
+#include "scsi/pr-manager.h"
+#include "scsi/constants.h"
+
 #if defined(__APPLE__) && (__MACH__)
 #include 
 #include 
@@ -156,6 +159,8 @@ typedef struct BDRVRawState {
 bool page_cache_inconsistent:1;
 bool has_fallocate;
 bool needs_alignment;
+
+PRManager *pr_mgr;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -403,6 +408,11 @@ static QemuOptsList raw_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "file locking mode (on/off/auto, default: auto)",
 },
+{
+.name = "pr-manager",
+.type = QEMU_OPT_STRING,
+.help = "id of persistent reservation manager object (default: 
none)",
+},
 { /* end of list */ }
 },
 };
@@ -414,6 +424,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 QemuOpts *opts;
 Error *local_err = NULL;
 const char *filename = NULL;
+const char *str;
 BlockdevAioOptions aio, aio_default;
 int fd, ret;
 struct stat st;
@@ -478,6 +489,16 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
 abort();
 }
 
+str = qemu_opt_get(opts, "pr-manager");
+if (str) {
+s->pr_mgr = pr_manager_lookup(str, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+}
+
 s->open_flags = open_flags;
 raw_parse_flags(bdrv_flags, >open_flags);
 
@@ -2600,6 +2621,15 @@ static BlockAIOCB *hdev_aio_ioctl(BlockDriverState *bs,
 if (fd_open(bs) < 0)
 return NULL;
 
+if (req == SG_IO && s->pr_mgr) {
+struct sg_io_hdr *io_hdr = buf;
+if (io_hdr->cmdp[0] == PERSISTENT_RESERVE_OUT ||
+io_hdr->cmdp[0] == PERSISTENT_RESERVE_IN) {
+return pr_manager_execute(s->pr_mgr, bdrv_get_aio_context(bs),
+  s->fd, io_hdr, cb, opaque);
+}
+}
+
 acb = g_new(RawPosixAIOData, 1);
 acb->bs = bs;
 acb->aio_type = QEMU_AIO_IOCTL;
diff --git a/docs/pr-manager.rst 

[Qemu-block] [PATCH 08/10] scsi: build qemu-pr-helper

[Paolo Bonzini]

Introduce a privileged helper to run persistent reservation commands.
This lets virtual machines send persistent reservations without using
CAP_SYS_RAWIO or out-of-tree patches.  The helper uses Unix permissions
and SCM_RIGHTS to restrict access to processes that can access its socket
and prove that they have an open file descriptor for a raw SCSI device.

The next patch will also correct the usage of persistent reservations
with multipath devices.

It would also be possible to support for Linux's IOC_PR_* ioctls in
the future, to support NVMe devices.  For now, however, only SCSI is
supported.

Signed-off-by: Paolo Bonzini 
---
 Makefile   |   9 +-
 configure  |   2 +-
 docs/interop/pr-helper.rst |  78 ++
 docs/pr-manager.rst|  33 +++
 scsi/pr-helper.h   |  13 +
 scsi/qemu-pr-helper.c  | 657 +
 6 files changed, 790 insertions(+), 2 deletions(-)
 create mode 100644 docs/interop/pr-helper.rst
 create mode 100644 scsi/pr-helper.h
 create mode 100644 scsi/qemu-pr-helper.c

diff --git a/Makefile b/Makefile
index 97a58a0f4e..bfd4f69ecd 100644
--- a/Makefile
+++ b/Makefile
@@ -209,6 +209,9 @@ ifdef BUILD_DOCS
 DOCS=qemu-doc.html qemu-doc.txt qemu.1 qemu-img.1 qemu-nbd.8 qemu-ga.8
 DOCS+=docs/interop/qemu-qmp-ref.html docs/interop/qemu-qmp-ref.txt 
docs/interop/qemu-qmp-ref.7
 DOCS+=docs/interop/qemu-ga-ref.html docs/interop/qemu-ga-ref.txt 
docs/interop/qemu-ga-ref.7
+ifdef CONFIG_LINUX
+DOCS+=scsi/qemu-pr-helper.8
+endif
 ifdef CONFIG_VIRTFS
 DOCS+=fsdev/virtfs-proxy-helper.1
 endif
@@ -384,6 +387,8 @@ qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o 
$(COMMON_LDADDS)
 fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o 
fsdev/9p-marshal.o fsdev/9p-iov-marshal.o $(COMMON_LDADDS)
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
 
+scsi/qemu-pr-helper$(EXESUF): scsi/qemu-pr-helper.o scsi/utils.o 
$(crypto-obj-y) $(io-obj-y) $(qom-obj-y) $(COMMON_LDADDS)
+
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > 
$@,"GEN","$@")
 
@@ -491,7 +496,7 @@ clean:
rm -f *.msi
find . \( -name '*.so' -o -name '*.dll' -o -name '*.mo' -o -name 
'*.[oda]' \) -type f -exec rm {} +
rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* 
*.pod *~ */*~
-   rm -f fsdev/*.pod
+   rm -f fsdev/*.pod scsi/*.pod
rm -f qemu-img-cmds.h
rm -f ui/shader/*-vert.h ui/shader/*-frag.h
@# May not be present in GENERATED_FILES
@@ -590,6 +595,7 @@ endif
 ifdef CONFIG_VIRTFS
$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man1"
$(INSTALL_DATA) fsdev/virtfs-proxy-helper.1 "$(DESTDIR)$(mandir)/man1"
+   $(INSTALL_DATA) scsi/qemu-pr-helper.8 "$(DESTDIR)$(mandir)/man8"
 endif
 
 install-datadir:
@@ -718,6 +724,7 @@ qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi 
qemu-monitor-info.texi
 qemu.1: qemu-option-trace.texi
 qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi
 fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
+scsi/qemu-pr-helper.8: scsi/qemu-pr-helper.texi
 qemu-nbd.8: qemu-nbd.texi qemu-option-trace.texi
 qemu-ga.8: qemu-ga.texi
 
diff --git a/configure b/configure
index dd73cce62f..772aff18d6 100755
--- a/configure
+++ b/configure
@@ -6545,7 +6545,7 @@ fi
 
 # build tree in object directory in case the source is not in the current 
directory
 DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos 
tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests"
-DIRS="$DIRS docs docs/interop fsdev"
+DIRS="$DIRS docs docs/interop fsdev scsi"
 DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
 DIRS="$DIRS roms/seabios roms/vgabios"
 DIRS="$DIRS qapi-generated"
diff --git a/docs/interop/pr-helper.rst b/docs/interop/pr-helper.rst
new file mode 100644
index 00..765174c31f
--- /dev/null
+++ b/docs/interop/pr-helper.rst
@@ -0,0 +1,78 @@
+..
+
+==
+Persistent reservation helper protocol
+==
+
+QEMU's SCSI passthrough devices, ``scsi-block`` and ``scsi-generic``,
+can delegate implementation of persistent reservations to an external
+(and typically privilege) program.  Persistent Reservations allow
+restricting access to block devices to specific initiators in a shared
+storage setup.
+
+For a more detailed reference please refer the the SCSI Primary
+Commands standard, specifically the section on Reservations and the
+"PERSISTENT RESERVE IN" and "PERSISTENT RESERVE OUT" commands.
+
+This document describes the socket protocol used between QEMU's
+``pr-manager-helper`` object and the external program.
+
+.. contents::
+
+Connection and initialization
+-
+
+After connecting to the helper program's socket, the helper starts a simple
+feature negotiation process by writing four bytes corresponding 

[Qemu-block] [PATCH 07/10] io: add qio_channel_read/write_all

[Paolo Bonzini]

It is pretty common to read a fixed-size buffer from a socket.  Add a
function that does this, either with multiple reads (on blocking sockets)
or by yielding if called from a coroutine.

Cc: Daniel P. Berrange 
Signed-off-by: Paolo Bonzini 
---
 include/io/channel.h | 36 ++-
 io/channel.c | 54 
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index db9bb022a1..9cfb4d081f 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -299,7 +299,7 @@ ssize_t qio_channel_writev(QIOChannel *ioc,
Error **errp);
 
 /**
- * qio_channel_readv:
+ * qio_channel_read:
  * @ioc: the channel object
  * @buf: the memory region to read data into
  * @buflen: the length of @buf
@@ -315,6 +315,23 @@ ssize_t qio_channel_read(QIOChannel *ioc,
  Error **errp);
 
 /**
+ * qio_channel_read_all:
+ * @ioc: the channel object
+ * @buf: the memory region to read data into
+ * @buflen: the number of bytes to @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads @buflen bytes into @buf, possibly blocking or (if the
+ * channel is non-blocking) yielding from the current coroutine
+ * multiple times until the entire content is read.  Otherwise
+ * behaves as qio_channel_read().
+ */
+ssize_t coroutine_fn qio_channel_read_all(QIOChannel *ioc,
+  char *buf,
+  size_t buflen,
+  Error **errp);
+
+/**
  * qio_channel_write:
  * @ioc: the channel object
  * @buf: the memory regions to send data from
@@ -331,6 +348,23 @@ ssize_t qio_channel_write(QIOChannel *ioc,
   Error **errp);
 
 /**
+ * qio_channel_write_all:
+ * @ioc: the channel object
+ * @buf: the memory region to write data into
+ * @buflen: the number of bytes to @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Writes @buflen bytes from @buf, possibly blocking or (if the
+ * channel is non-blocking) yielding from the current coroutine
+ * multiple times until the entire content is written.  Otherwise
+ * behaves as qio_channel_write().
+ */
+ssize_t coroutine_fn qio_channel_write_all(QIOChannel *ioc,
+   const char *buf,
+   size_t buflen,
+   Error **errp);
+
+/**
  * qio_channel_set_blocking:
  * @ioc: the channel object
  * @enabled: the blocking flag state
diff --git a/io/channel.c b/io/channel.c
index 1cfb8b33a2..7ab3f4eede 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -113,6 +113,60 @@ ssize_t qio_channel_read(QIOChannel *ioc,
 }
 
 
+ssize_t qio_channel_read_all(QIOChannel *ioc,
+ char *buf,
+ size_t buflen,
+ Error **errp)
+{
+ssize_t total = 0;
+while (buflen > 0) {
+ssize_t n_read = qio_channel_read(ioc, buf, buflen, errp);
+
+if (n_read == QIO_CHANNEL_ERR_BLOCK) {
+assert(ioc->ctx);
+qio_channel_yield(ioc, G_IO_IN);
+continue;
+}
+if (n_read < 0) {
+return n_read;
+}
+
+buf += n_read;
+total += n_read;
+buflen -= n_read;
+}
+
+return total;
+}
+
+
+ssize_t qio_channel_write_all(QIOChannel *ioc,
+  const char *buf,
+  size_t buflen,
+  Error **errp)
+{
+ssize_t total = 0;
+while (buflen > 0) {
+ssize_t n_written = qio_channel_write(ioc, buf, buflen, errp);
+
+if (n_written == QIO_CHANNEL_ERR_BLOCK) {
+assert(ioc->ctx);
+qio_channel_yield(ioc, G_IO_OUT);
+continue;
+}
+if (n_written < 0) {
+return n_written;
+}
+
+buf += n_written;
+total += n_written;
+buflen -= n_written;
+}
+
+return total;
+}
+
+
 ssize_t qio_channel_write(QIOChannel *ioc,
   const char *buf,
   size_t buflen,
-- 
2.13.5

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 04/56] char: Make ringbuf-read size unsigned in QAPI/QMP

[Markus Armbruster]

Marc-André Lureau  writes:

> Hi
>
> On Mon, Aug 7, 2017 at 4:45 PM, Markus Armbruster  wrote:
>> Sizes should use QAPI type 'size' (uint64_t).  ringbuf-read parameter
>> @size is 'int' (int64_t).  qmp_ringbuf_read() rejects negative values,
>> then implicitly converts to size_t.
>>
>> Change the parameter to 'size' and drop the check for negative values.
>>
>> ringbuf-read now accepts size values between 2^63 and 2^64-1.  It
>> accepts negative values as before, because that's how the QObject
>> input visitor works for backward compatibility.
>>
>
> Negative values over json will be implicitly converted to positive
> values with this change, right? Or are they rejected earlier?

Yes.  For details, see my reply to Juan's review of PATCH 15.

> If so that is a change of behaviour that I am not sure is worth doing
> now (without explicit protocol break), but I don't mind.

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Igor Mammedov]

On Mon,  7 Aug 2017 16:45:16 +0200
Markus Armbruster  wrote:

> Sizes and addresses should use QAPI type 'size' (uint64_t).
> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
> 
> Change these PCDIMMDeviceInfo members to 'size'.
> 
> query-memory-devices now reports sizes and addresses above 2^63-1
> correctly instead of their (negative) two's complement.
> 
> HMP's "info memory-devices" already reported them correctly, because
> it printed the signed integers with PRIx64 and PRIu32.
s/signed/unsigned/

 
> Signed-off-by: Markus Armbruster 
Reviewed-by: Igor Mammedov 

> ---
>  qapi-schema.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 23eb60d..6aa6be9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -6057,8 +6057,8 @@
>  ##
>  { 'struct': 'PCDIMMDeviceInfo',
>'data': { '*id': 'str',
> -'addr': 'int',
> -'size': 'int',
> +'addr': 'size',
> +'size': 'size',
>  'slot': 'int',
>  'node': 'int',
>  'memdev': 'str',

[Qemu-block] [PATCH] nbd-client: avoid spurious qio_channel_yield() re-entry

[Stefan Hajnoczi]

The following scenario leads to an assertion failure in
qio_channel_yield():

1. Request coroutine calls qio_channel_yield() successfully when sending
   would block on the socket.  It is now yielded.
2. nbd_read_reply_entry() calls nbd_recv_coroutines_enter_all() because
   nbd_receive_reply() failed.
3. Request coroutine is entered and returns from qio_channel_yield().
   Note that the socket fd handler has not fired yet so
   ioc->write_coroutine is still set.
4. Request coroutine attempts to send the request body with nbd_rwv()
   but the socket would still block.  qio_channel_yield() is called
   again and assert(!ioc->write_coroutine) is hit.

The problem is that nbd_read_reply_entry() does not distinguish between
request coroutines that are waiting to receive a reply and those that
are not.

This patch adds a per-request bool receiving flag so
nbd_read_reply_entry() can avoid spurious aio_wake() calls.

Reported-by: Dr. David Alan Gilbert 
Signed-off-by: Stefan Hajnoczi 
---
This should fix the issue that Dave is seeing but I'm concerned that
there are more problems in nbd-client.c.  We don't have good
abstractions for writing coroutine socket I/O code.  Something like Go's
channels would avoid manual low-level coroutine calls.  There is
currently no way to cancel qio_channel_yield() so requests doing I/O may
remain in-flight indefinitely and nbd-client.c doesn't join them...

 block/nbd-client.h |  7 ++-
 block/nbd-client.c | 35 ++-
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index 1935ffbcaa..b435754b82 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -17,6 +17,11 @@
 
 #define MAX_NBD_REQUESTS16
 
+typedef struct {
+Coroutine *coroutine;
+bool receiving; /* waiting for read_reply_co? */
+} NBDClientRequest;
+
 typedef struct NBDClientSession {
 QIOChannelSocket *sioc; /* The master data channel */
 QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
@@ -27,7 +32,7 @@ typedef struct NBDClientSession {
 Coroutine *read_reply_co;
 int in_flight;
 
-Coroutine *recv_coroutine[MAX_NBD_REQUESTS];
+NBDClientRequest requests[MAX_NBD_REQUESTS];
 NBDReply reply;
 bool quit;
 } NBDClientSession;
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 422ecb4307..c2834f6b47 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -39,8 +39,10 @@ static void nbd_recv_coroutines_enter_all(NBDClientSession 
*s)
 int i;
 
 for (i = 0; i < MAX_NBD_REQUESTS; i++) {
-if (s->recv_coroutine[i]) {
-aio_co_wake(s->recv_coroutine[i]);
+NBDClientRequest *req = >requests[i];
+
+if (req->coroutine && req->receiving) {
+aio_co_wake(req->coroutine);
 }
 }
 }
@@ -88,28 +90,28 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque)
  * one coroutine is called until the reply finishes.
  */
 i = HANDLE_TO_INDEX(s, s->reply.handle);
-if (i >= MAX_NBD_REQUESTS || !s->recv_coroutine[i]) {
+if (i >= MAX_NBD_REQUESTS ||
+!s->requests[i].coroutine ||
+!s->requests[i].receiving) {
 break;
 }
 
-/* We're woken up by the recv_coroutine itself.  Note that there
+/* We're woken up again by the request itself.  Note that there
  * is no race between yielding and reentering read_reply_co.  This
  * is because:
  *
- * - if recv_coroutine[i] runs on the same AioContext, it is only
+ * - if the request runs on the same AioContext, it is only
  *   entered after we yield
  *
- * - if recv_coroutine[i] runs on a different AioContext, reentering
+ * - if the request runs on a different AioContext, reentering
  *   read_reply_co happens through a bottom half, which can only
  *   run after we yield.
  */
-aio_co_wake(s->recv_coroutine[i]);
+aio_co_wake(s->requests[i].coroutine);
 qemu_coroutine_yield();
 }
 
-if (ret < 0) {
-s->quit = true;
-}
+s->quit = true;
 nbd_recv_coroutines_enter_all(s);
 s->read_reply_co = NULL;
 }
@@ -128,14 +130,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
 s->in_flight++;
 
 for (i = 0; i < MAX_NBD_REQUESTS; i++) {
-if (s->recv_coroutine[i] == NULL) {
-s->recv_coroutine[i] = qemu_coroutine_self();
+if (s->requests[i].coroutine == NULL) {
 break;
 }
 }
 
 g_assert(qemu_in_coroutine());
 assert(i < MAX_NBD_REQUESTS);
+
+s->requests[i].coroutine = qemu_coroutine_self();
+s->requests[i].receiving = false;
+
 request->handle = INDEX_TO_HANDLE(s, i);
 
 if (s->quit) {
@@ -173,10 +178,13 @@ static void nbd_co_receive_reply(NBDClientSession *s,
  

Re: [Qemu-block] [PATCH v2 6/6] block: remove BlockBackendPublic

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:56 PM CEST, Manos Pitsidianakis wrote:
> All BlockBackend level throttling (via the implicit throttle filter node) is
> done in block/block-backend.c and block/throttle-groups.c doesn't know
> about BlockBackends anymore. Since BlockBackendPublic is not needed anymore, 
> remove it.
>
> Signed-off-by: Manos Pitsidianakis 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v2 4/6] block: remove legacy I/O throttling

[Alberto Garcia]

On Wed 09 Aug 2017 04:02:54 PM CEST, Manos Pitsidianakis wrote:
> +void blk_io_limits_update_group(BlockBackend *blk, const char *group, Error 
> **errp)
>  {
> +ThrottleGroupMember *tgm;
> +
>  /* this BB is not part of any group */
> -if (!blk->public.throttle_group_member.throttle_state) {
> +if (!blk->public.throttle_node) {
>  return;
>  }
>  
> +tgm = throttle_get_tgm(blk->public.throttle_node);
>  /* this BB is a part of the same group than the one we want */
> -if 
> (!g_strcmp0(throttle_group_get_name(>public.throttle_group_member),
> -group)) {
> +if (!g_strcmp0(throttle_group_get_name(tgm),
> +   group)) {

You can join these two lines, no need to have 'group' in its own line.

> @@ -2629,6 +2636,9 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>  BlockDriverState *bs;
>  BlockBackend *blk;
>  AioContext *aio_context;
> +BlockDriverState *throttle_node = NULL;
> +ThrottleGroupMember *tgm;
> +Error *local_err = NULL;
>  
>  blk = qmp_get_blk(arg->has_device ? arg->device : NULL,
>arg->has_id ? arg->id : NULL,
> @@ -2704,18 +2714,33 @@ void qmp_block_set_io_throttle(BlockIOThrottle *arg, 
> Error **errp)
>  if (throttle_enabled()) {
>  /* Enable I/O limits if they're not enabled yet, otherwise
>   * just update the throttling group. */
> -if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
> -blk_io_limits_enable(blk,
> - arg->has_group ? arg->group :
> - arg->has_device ? arg->device :
> - arg->id);
> -} else if (arg->has_group) {
> -blk_io_limits_update_group(blk, arg->group);
> +if (!blk_get_public(blk)->throttle_node) {
> +blk_io_limits_enable(blk, arg->has_group ? arg->group :
> + arg->has_device ? arg->device : arg->id,
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
>  }
> -/* Set the new throttling configuration */
> -blk_set_io_limits(blk, );
> -} else if (blk_get_public(blk)->throttle_group_member.throttle_state) {
> -/* If all throttling settings are set to 0, disable I/O limits */
> +
> +if (arg->has_group) {
> +/* move throttle node membership to arg->group */
> +blk_io_limits_update_group(blk, arg->group, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +goto out;
> +}
> +}

This used to be

   if (!blk_get_public(blk)->throttle_group_member.throttle_state) {
  /* Enable throttling */
   } else if (arg->has_group) {
  /* Update group */
   }

but now it is

   if (!blk_get_throttle_node(blk)) {
  /* Enable throttling */
   }
   if (arg->has_group) {
  /* Update group */
   }

Now if I/O limits are not set then the code sets the same group twice.

Everything else looks fine.

Berto

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 04/56] char: Make ringbuf-read size unsigned in QAPI/QMP

[Marc-André Lureau]

Hi

On Mon, Aug 7, 2017 at 4:45 PM, Markus Armbruster  wrote:
> Sizes should use QAPI type 'size' (uint64_t).  ringbuf-read parameter
> @size is 'int' (int64_t).  qmp_ringbuf_read() rejects negative values,
> then implicitly converts to size_t.
>
> Change the parameter to 'size' and drop the check for negative values.
>
> ringbuf-read now accepts size values between 2^63 and 2^64-1.  It
> accepts negative values as before, because that's how the QObject
> input visitor works for backward compatibility.
>

Negative values over json will be implicitly converted to positive
values with this change, right? Or are they rejected earlier?

If so that is a change of behaviour that I am not sure is worth doing
now (without explicit protocol break), but I don't mind.

> The HMP command's size parameter remains uint32_t, as HMP args_type
> strings can't do uint64_t byte counts: 'l' is signed, and 'o'
> multiplies by 2^20.
>
> Signed-off-by: Markus Armbruster 

> ---
>  chardev/char-ringbuf.c | 11 +++
>  qapi-schema.json   |  2 +-
>  2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/chardev/char-ringbuf.c b/chardev/char-ringbuf.c
> index df52b04..a9205ea 100644
> --- a/chardev/char-ringbuf.c
> +++ b/chardev/char-ringbuf.c
> @@ -65,10 +65,10 @@ static int ringbuf_chr_write(Chardev *chr, const uint8_t 
> *buf, int len)
>  return len;
>  }
>
> -static int ringbuf_chr_read(Chardev *chr, uint8_t *buf, int len)
> +static int ringbuf_chr_read(Chardev *chr, uint8_t *buf, size_t len)
>  {
>  RingBufChardev *d = RINGBUF_CHARDEV(chr);
> -int i;
> +size_t i;
>
>  qemu_mutex_lock(>chr_write_lock);
>  for (i = 0; i < len && d->cons != d->prod; i++) {
> @@ -151,7 +151,7 @@ void qmp_ringbuf_write(const char *device, const char 
> *data,
>  }
>  }
>
> -char *qmp_ringbuf_read(const char *device, int64_t size,
> +char *qmp_ringbuf_read(const char *device, uint64_t size,
> bool has_format, enum DataFormat format,
> Error **errp)
>  {
> @@ -171,11 +171,6 @@ char *qmp_ringbuf_read(const char *device, int64_t size,
>  return NULL;
>  }
>
> -if (size <= 0) {
> -error_setg(errp, "size must be greater than zero");
> -return NULL;
> -}
> -
>  count = ringbuf_count(chr);
>  size = size > count ? count : size;
>  read_data = g_malloc(size + 1);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index febe70e..18ec301 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -543,7 +543,7 @@
>  #
>  ##
>  { 'command': 'ringbuf-read',
> -  'data': {'device': 'str', 'size': 'int', '*format': 'DataFormat'},
> +  'data': {'device': 'str', 'size': 'size', '*format': 'DataFormat'},
>'returns': 'str' }
>
>  ##
> --
> 2.7.5
>
>



-- 
Marc-André Lureau

Re: [Qemu-block] [RFC PATCH 02/56] qdict: New helpers to put and get unsigned integers

[Marc-André Lureau]

Hi

Is this based on 
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg01894.html ?

If so, you should probably keep me signed-off.

My patch had also a test :)
 
- Original Message -
> Signed-off-by: Markus Armbruster 
> ---
>  include/qapi/qmp/qdict.h |  5 +
>  qobject/qdict.c  | 43 ---
>  2 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 363e431..3b52481 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -56,6 +56,8 @@ void qdict_destroy_obj(QObject *obj);
>  /* Helpers for int, bool, and string */
>  #define qdict_put_int(qdict, key, value) \
>  qdict_put(qdict, key, qnum_from_int(value))
> +#define qdict_put_uint(qdict, key, value) \
> +qdict_put(qdict, key, qnum_from_uint(value))
>  #define qdict_put_bool(qdict, key, value) \
>  qdict_put(qdict, key, qbool_from_bool(value))
>  #define qdict_put_str(qdict, key, value) \
> @@ -64,12 +66,15 @@ void qdict_destroy_obj(QObject *obj);
>  /* High level helpers */
>  double qdict_get_double(const QDict *qdict, const char *key);
>  int64_t qdict_get_int(const QDict *qdict, const char *key);
> +uint64_t qdict_get_uint(const QDict *qdict, const char *key);
>  bool qdict_get_bool(const QDict *qdict, const char *key);
>  QList *qdict_get_qlist(const QDict *qdict, const char *key);
>  QDict *qdict_get_qdict(const QDict *qdict, const char *key);
>  const char *qdict_get_str(const QDict *qdict, const char *key);
>  int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>int64_t def_value);
> +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> +uint64_t def_value);
>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool
>  def_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>  
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index d795079..be919b9 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -189,10 +189,9 @@ double qdict_get_double(const QDict *qdict, const char
> *key)
>  }
>  
>  /**
> - * qdict_get_int(): Get an integer mapped by @key
> + * qdict_get_int(): Get a signed integer mapped by @key
>   *
> - * This function assumes that @key exists and it stores a
> - * QNum representable as int.
> + * @qdict must map @key to an integer QNum that fits into int64_t.
>   *
>   * Return integer mapped by @key.
>   */
> @@ -202,6 +201,18 @@ int64_t qdict_get_int(const QDict *qdict, const char
> *key)
>  }
>  
>  /**
> + * qdict_get_uint(): Get an unsigned integer mapped by 'key'
> + *
> + * @qdict must map @key to an integer QNum that fits into uint64_t.
> + *
> + * Return integer mapped by 'key'.
> + */
> +uint64_t qdict_get_uint(const QDict *qdict, const char *key)
> +{
> +return qnum_get_uint(qobject_to_qnum(qdict_get(qdict, key)));
> +}
> +
> +/**
>   * qdict_get_bool(): Get a bool mapped by @key
>   *
>   * This function assumes that @key exists and it stores a
> @@ -245,11 +256,10 @@ const char *qdict_get_str(const QDict *qdict, const
> char *key)
>  }
>  
>  /**
> - * qdict_get_try_int(): Try to get integer mapped by @key
> + * qdict_get_try_int(): Try to get signed integer mapped by @key
>   *
> - * Return integer mapped by @key, if it is not present in the
> - * dictionary or if the stored object is not a QNum representing an
> - * integer, @def_value will be returned.
> + * If @qdict maps @key to an integer QNum that fits into int64_t,
> + * return it.  Else return @def_value.
>   */
>  int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>int64_t def_value)
> @@ -265,6 +275,25 @@ int64_t qdict_get_try_int(const QDict *qdict, const char
> *key,
>  }
>  
>  /**
> + * qdict_get_try_uint(): Try to get unsigned integer mapped by 'key'
> + *
> + * If @qdict maps @key to an integer QNum that fits into uint64_t,
> + * return it.  Else return @def_value.
> + */
> +uint64_t qdict_get_try_uint(const QDict *qdict, const char *key,
> +uint64_t def_value)
> +{
> +QNum *qnum = qobject_to_qnum(qdict_get(qdict, key));
> +uint64_t val;
> +
> +if (!qnum || !qnum_get_try_uint(qnum, )) {
> +return def_value;
> +}
> +
> +return val;
> +}
> +
> +/**
>   * qdict_get_try_bool(): Try to get a bool mapped by @key
>   *
>   * Return bool mapped by @key, if it is not present in the
> --
> 2.7.5
> 
> 

Re: [Qemu-block] [PATCH v2 3/6] block: require job-id when device is a node name

[Alberto Garcia]

On Tue 22 Aug 2017 12:31:26 PM CEST, Manos Pitsidianakis wrote:
> On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
>>On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
> -if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
> -job_id = bdrv_get_device_name(bs);
> -if (!*job_id) {
> -error_setg(errp, "An explicit job ID is required for this 
> node");
> -return NULL;
> -}
> -}
> -
> -if (job_id) {
> -if (flags & BLOCK_JOB_INTERNAL) {
> +if (flags & BLOCK_JOB_INTERNAL) {
> +if (job_id) {
>  error_setg(errp, "Cannot specify job ID for internal block 
> job");
>  return NULL;
>  }

When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...

> -
> +} else {
> +/* Require job-id. */
> +assert(job_id);
>  if (!id_wellformed(job_id)) {
>  error_setg(errp, "Invalid job ID '%s'", job_id);
>  return NULL;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index f13ad05c0d..ff906808a6 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>
>  /**
>   * block_job_create:
> - * @job_id: The id of the newly-created job, or %NULL to have one
> - * generated automatically.
> + * @job_id: The id of the newly-created job, must be non %NULL.

...but here you say that it must not be NULL.

And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().

> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>  blk[1] = create_blk("drive1");
>  blk[2] = create_blk("drive2");
>
> -/* No job ID provided and the block backend has no name */
> -job[0] = do_test_id(blk[0], NULL, false);
> -

If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.

Berto

>>>
>>> Thanks, I will change that. What cases are you thinking of besides
>>> internal jobs though?
>>
>>Both cases (external job with a NULL id and internal job with non-NULL
>>id), checking that block_job_create() does return an error.
>
> I thought we handled the external job case by requiring job_id is set 
> before calling block_job_create(). I will check my patch again.

Yes, that appears to be checked correctly, I don't think you can call
block_job_create() directly with a NULL id for external block job.

But I also don't see how you can create an internal job with a non-NULL
id, so you could assert() there as well.

Either assert on both places or use error_setg() in both places. I think
I prefer the latter.

Berto

Re: [Qemu-block] [PATCH v2 3/6] block: require job-id when device is a node name

[Manos Pitsidianakis]

On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:

On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:

-if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
-job_id = bdrv_get_device_name(bs);
-if (!*job_id) {
-error_setg(errp, "An explicit job ID is required for this node");
-return NULL;
-}
-}
-
-if (job_id) {
-if (flags & BLOCK_JOB_INTERNAL) {
+if (flags & BLOCK_JOB_INTERNAL) {
+if (job_id) {
 error_setg(errp, "Cannot specify job ID for internal block job");
 return NULL;
 }


When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...


-
+} else {
+/* Require job-id. */
+assert(job_id);
 if (!id_wellformed(job_id)) {
 error_setg(errp, "Invalid job ID '%s'", job_id);
 return NULL;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..ff906808a6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,7 @@ struct BlockJobDriver {

 /**
  * block_job_create:
- * @job_id: The id of the newly-created job, or %NULL to have one
- * generated automatically.
+ * @job_id: The id of the newly-created job, must be non %NULL.


...but here you say that it must not be NULL.

And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().


@@ -93,9 +93,6 @@ static void test_job_ids(void)
 blk[1] = create_blk("drive1");
 blk[2] = create_blk("drive2");

-/* No job ID provided and the block backend has no name */
-job[0] = do_test_id(blk[0], NULL, false);
-


If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.

Berto



Thanks, I will change that. What cases are you thinking of besides
internal jobs though?


Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.


I thought we handled the external job case by requiring job_id is set 
before calling block_job_create(). I will check my patch again.





And should documentation of block_job_create reflect that internal
jobs have job_id == NULL?


Yes, it should document the restrictions of the job_id parameter.

Berto



signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 3/6] block: require job-id when device is a node name

[Alberto Garcia]

On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
>>> -if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
>>> -job_id = bdrv_get_device_name(bs);
>>> -if (!*job_id) {
>>> -error_setg(errp, "An explicit job ID is required for this 
>>> node");
>>> -return NULL;
>>> -}
>>> -}
>>> -
>>> -if (job_id) {
>>> -if (flags & BLOCK_JOB_INTERNAL) {
>>> +if (flags & BLOCK_JOB_INTERNAL) {
>>> +if (job_id) {
>>>  error_setg(errp, "Cannot specify job ID for internal block 
>>> job");
>>>  return NULL;
>>>  }
>>
>>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>>
>>> -
>>> +} else {
>>> +/* Require job-id. */
>>> +assert(job_id);
>>>  if (!id_wellformed(job_id)) {
>>>  error_setg(errp, "Invalid job ID '%s'", job_id);
>>>  return NULL;
>>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>>> index f13ad05c0d..ff906808a6 100644
>>> --- a/include/block/blockjob_int.h
>>> +++ b/include/block/blockjob_int.h
>>> @@ -112,8 +112,7 @@ struct BlockJobDriver {
>>>
>>>  /**
>>>   * block_job_create:
>>> - * @job_id: The id of the newly-created job, or %NULL to have one
>>> - * generated automatically.
>>> + * @job_id: The id of the newly-created job, must be non %NULL.
>>
>>...but here you say that it must not be NULL.
>>
>>And since job_id can be NULL in some cases I think I'd replace the
>>assert(job_id) that you added to block_job_create() with a normal
>>pointer check + error_setg().
>>
>>> @@ -93,9 +93,6 @@ static void test_job_ids(void)
>>>  blk[1] = create_blk("drive1");
>>>  blk[2] = create_blk("drive2");
>>>
>>> -/* No job ID provided and the block backend has no name */
>>> -job[0] = do_test_id(blk[0], NULL, false);
>>> -
>>
>>If you decide to handle NULL ids by returning and error instead of
>>asserting, we should keep a couple of tests for that scenario.
>>
>>Berto
>>
>
> Thanks, I will change that. What cases are you thinking of besides
> internal jobs though?

Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.

> And should documentation of block_job_create reflect that internal
> jobs have job_id == NULL?

Yes, it should document the restrictions of the job_id parameter.

Berto

[Qemu-block] [PATCH v7 5/6] block: add throttle block filter driver

[Manos Pitsidianakis]

block/throttle.c uses existing I/O throttle infrastructure inside a
block filter driver. I/O operations are intercepted in the filter's
read/write coroutines, and referred to block/throttle-groups.c

The driver can be used with the syntax
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar

which registers the throttle filter node with the ThrottleGroup 'bar'. The
given group must be created beforehand with object-add or -object.

Signed-off-by: Manos Pitsidianakis 
---
 qapi/block-core.json|  18 ++-
 include/block/throttle-groups.h |   1 +
 include/qemu/throttle-options.h |   1 +
 block/throttle-groups.c |   7 +-
 block/throttle.c| 250 
 block/Makefile.objs |   1 +
 6 files changed, 276 insertions(+), 2 deletions(-)
 create mode 100644 block/throttle.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0bdc69aa5f..405c181954 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -,6 +,7 @@
 # Drivers that are supported in block device operations.
 #
 # @vxhs: Since 2.10
+# @throttle: Since 2.11
 #
 # Since: 2.9
 ##
@@ -2231,7 +2232,7 @@
 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
-'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
+'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -3095,6 +3096,20 @@
 '*tls-creds': 'str' } }
 
 ##
+# @BlockdevOptionsThrottle:
+#
+# Driver specific block device options for the throttle driver
+#
+# @throttle-group:   the name of the throttle-group object to use. It
+#must already exist.
+# @file: reference to or definition of the data source block device
+# Since: 2.11
+##
+{ 'struct': 'BlockdevOptionsThrottle',
+  'data': { '*throttle-group': 'str',
+'file' : 'BlockdevRef'
+ } }
+##
 # @BlockdevOptions:
 #
 # Options for creating a block device.  Many options are available for all
@@ -3155,6 +3170,7 @@
   'replication':'BlockdevOptionsReplication',
   'sheepdog':   'BlockdevOptionsSheepdog',
   'ssh':'BlockdevOptionsSsh',
+  'throttle':   'BlockdevOptionsThrottle',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
   'vmdk':   'BlockdevOptionsGenericCOWFormat',
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 82f030523f..ec969e84fe 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,5 +76,6 @@ void coroutine_fn 
throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
 void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
 void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
+bool throttle_group_exists(const char *name);
 
 #endif
diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h
index 182b7896e1..3528a8f4a2 100644
--- a/include/qemu/throttle-options.h
+++ b/include/qemu/throttle-options.h
@@ -29,6 +29,7 @@
 #define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
 #define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
 #define QEMU_OPT_IOPS_SIZE "iops-size"
+#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
 
 #define THROTTLE_OPT_PREFIX "throttling."
 #define THROTTLE_OPTS \
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index a4268a954e..4b483a16d4 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -101,7 +101,7 @@ static ThrottleGroup *throttle_group_by_name(const char 
*name)
 return NULL;
 }
 
-static bool throttle_group_exists(const char *name)
+bool throttle_group_exists(const char *name)
 {
 return throttle_group_by_name(name) ? true : false;
 }
@@ -548,6 +548,11 @@ void throttle_group_unregister_tgm(ThrottleGroupMember 
*tgm)
 ThrottleGroupMember *token;
 int i;
 
+if (!ts) {
+/* Discard uninitialized tgm */
+return;
+}
+
 assert(tgm->pending_reqs[0] == 0 && tgm->pending_reqs[1] == 0);
 assert(qemu_co_queue_empty(>throttled_reqs[0]));
 assert(qemu_co_queue_empty(>throttled_reqs[1]));
diff --git a/block/throttle.c b/block/throttle.c
new file mode 100644
index 00..910de27dcd
--- /dev/null
+++ b/block/throttle.c
@@ -0,0 +1,250 @@
+/*
+ * QEMU block throttling filter driver infrastructure
+ *
+ * Copyright (c) 2017 Manos Pitsidianakis
+ *
+ * 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 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that 

[Qemu-block] [PATCH v7 4/6] block: convert ThrottleGroup to object with QOM

[Manos Pitsidianakis]

ThrottleGroup is converted to an object. This will allow the future
throttle block filter drive easy creation and configuration of throttle
groups in QMP and cli.

A new QAPI struct, ThrottleLimits, is introduced to provide a shared
struct for all throttle configuration needs in QMP.

ThrottleGroups can be created via CLI as
-object throttle-group,id=foo,x-iops-total=100,x-..
where x-* are individual limit properties. Since we can't add non-scalar
properties in -object this interface must be used instead. However,
setting these properties must be disabled after initialization because
certain combinations of limits are forbidden and thus configuration
changes should be done in one transaction. The individual properties
will go away when support for non-scalar values in CLI is implemented
and thus are marked as experimental.

ThrottleGroup also has a `limits` property that uses the ThrottleLimits
struct.  It can be used to create ThrottleGroups or set the
configuration in existing groups as follows:

{ "execute": "object-add",
  "arguments": {
"qom-type": "throttle-group",
"id": "foo",
"props" : {
  "limits": {
  "iops-total": 100
  }
}
  }
}
{ "execute" : "qom-set",
"arguments" : {
"path" : "foo",
"property" : "limits",
"value" : {
"iops-total" : 99
}
}
}

This also means a group's configuration can be fetched with qom-get.

Signed-off-by: Manos Pitsidianakis 
---
 qapi/block-core.json|  48 +
 include/block/throttle-groups.h |   3 +
 include/qemu/throttle-options.h |  59 --
 include/qemu/throttle.h |   3 +
 block/throttle-groups.c | 419 
 tests/test-throttle.c   |   1 +
 util/throttle.c | 151 +++
 7 files changed, 624 insertions(+), 60 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602150..0bdc69aa5f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1905,6 +1905,54 @@
 '*iops_size': 'int', '*group': 'str' } }
 
 ##
+# @ThrottleLimits:
+#
+# Limit parameters for throttling.
+# Since some limit combinations are illegal, limits should always be set in one
+# transaction. All fields are optional. When setting limits, if a field is
+# missing the current value is not changed.
+#
+# @iops-total: limit total I/O operations per second
+# @iops-total-max: I/O operations burst
+# @iops-total-max-length:  length of the iops-total-max burst period, in 
seconds
+#  It must only be set if @iops-total-max is set as 
well.
+# @iops-read:  limit read operations per second
+# @iops-read-max:  I/O operations read burst
+# @iops-read-max-length:   length of the iops-read-max burst period, in seconds
+#  It must only be set if @iops-read-max is set as 
well.
+# @iops-write: limit write operations per second
+# @iops-write-max: I/O operations write burst
+# @iops-write-max-length:  length of the iops-write-max burst period, in 
seconds
+#  It must only be set if @iops-write-max is set as 
well.
+# @bps-total:  limit total bytes per second
+# @bps-total-max:  total bytes burst
+# @bps-total-max-length:   length of the bps-total-max burst period, in 
seconds.
+#  It must only be set if @bps-total-max is set as 
well.
+# @bps-read:   limit read bytes per second
+# @bps-read-max:   total bytes read burst
+# @bps-read-max-length:length of the bps-read-max burst period, in seconds
+#  It must only be set if @bps-read-max is set as well.
+# @bps-write:  limit write bytes per second
+# @bps-write-max:  total bytes write burst
+# @bps-write-max-length:   length of the bps-write-max burst period, in seconds
+#  It must only be set if @bps-write-max is set as 
well.
+# @iops-size:  when limiting by iops max size of an I/O in bytes
+#
+# Since: 2.11
+##
+{ 'struct': 'ThrottleLimits',
+  'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
+'*iops-total-max-length' : 'int', '*iops-read' : 'int',
+'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
+'*iops-write' : 'int', '*iops-write-max' : 'int',
+'*iops-write-max-length' : 'int', '*bps-total' : 'int',
+'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
+'*bps-read' : 'int', '*bps-read-max' : 'int',
+'*bps-read-max-length' : 'int', '*bps-write' : 'int',
+'*bps-write-max' : 'int', '*bps-write-max-length' : 'int',
+'*iops-size' : 'int' } }
+
+##
 # @block-stream:
 #
 # Copy data from a backing file into a block device.
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 

[Qemu-block] [PATCH v7 3/6] block: tidy ThrottleGroupMember initializations

[Manos Pitsidianakis]

Move the CoMutex and CoQueue inits inside throttle_group_register_tgm()
which is called whenever a ThrottleGroupMember is initialized. There's
no need for them to be separate.

Reviewed-by: Alberto Garcia 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 block/block-backend.c   | 3 ---
 block/throttle-groups.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9ba800e733..c51fb8c8aa 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -252,9 +252,6 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 blk->shared_perm = shared_perm;
 blk_set_enable_write_cache(blk, true);
 
-qemu_co_mutex_init(>public.throttle_group_member.throttled_reqs_lock);
-qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[0]);
-qemu_co_queue_init(>public.throttle_group_member.throttled_reqs[1]);
 block_acct_init(>stats);
 
 notifier_list_init(>remove_bs_notifiers);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 3b07b25f39..7749cf043f 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -508,6 +508,9 @@ void throttle_group_register_tgm(ThrottleGroupMember *tgm,
  read_timer_cb,
  write_timer_cb,
  tgm);
+qemu_co_mutex_init(>throttled_reqs_lock);
+qemu_co_queue_init(>throttled_reqs[0]);
+qemu_co_queue_init(>throttled_reqs[1]);
 
 qemu_mutex_unlock(>lock);
 }
-- 
2.11.0

[Qemu-block] [PATCH v7 1/6] block: move ThrottleGroup membership to ThrottleGroupMember

[Manos Pitsidianakis]

This commit eliminates the 1:1 relationship between BlockBackend and
throttle group state.  Users will be able to create multiple throttle
nodes, each with its own throttle group state, in the future.  The
throttle group state cannot be per-BlockBackend anymore, it must be
per-throttle node. This is done by gathering ThrottleGroup membership
details from BlockBackendPublic into ThrottleGroupMember and refactoring
existing code to use the structure.

Reviewed-by: Alberto Garcia 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 include/block/throttle-groups.h |  39 +-
 include/sysemu/block-backend.h  |  20 +--
 block/block-backend.c   |  66 +
 block/qapi.c|   8 +-
 block/throttle-groups.c | 288 
 blockdev.c  |   4 +-
 tests/test-throttle.c   |  53 
 7 files changed, 252 insertions(+), 226 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index d983d34074..1a6bcdae74 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -28,19 +28,44 @@
 #include "qemu/throttle.h"
 #include "block/block_int.h"
 
-const char *throttle_group_get_name(BlockBackend *blk);
+/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup
+ * and holds related data.
+ */
+
+typedef struct ThrottleGroupMember {
+/* throttled_reqs_lock protects the CoQueues for throttled requests.  */
+CoMutex  throttled_reqs_lock;
+CoQueue  throttled_reqs[2];
+
+/* Nonzero if the I/O limits are currently being ignored; generally
+ * it is zero.  Accessed with atomic operations.
+ */
+unsigned int io_limits_disabled;
+
+/* The following fields are protected by the ThrottleGroup lock.
+ * See the ThrottleGroup documentation for details.
+ * throttle_state tells us if I/O limits are configured. */
+ThrottleState *throttle_state;
+ThrottleTimers throttle_timers;
+unsigned   pending_reqs[2];
+QLIST_ENTRY(ThrottleGroupMember) round_robin;
+
+} ThrottleGroupMember;
+
+const char *throttle_group_get_name(ThrottleGroupMember *tgm);
 
 ThrottleState *throttle_group_incref(const char *name);
 void throttle_group_unref(ThrottleState *ts);
 
-void throttle_group_config(BlockBackend *blk, ThrottleConfig *cfg);
-void throttle_group_get_config(BlockBackend *blk, ThrottleConfig *cfg);
+void throttle_group_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
+void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
 
-void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
-void throttle_group_unregister_blk(BlockBackend *blk);
-void throttle_group_restart_blk(BlockBackend *blk);
+void throttle_group_register_tgm(ThrottleGroupMember *tgm,
+const char *groupname);
+void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
+void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
 
-void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk,
+void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember 
*tgm,
 unsigned int bytes,
 bool is_write);
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 4a3730596b..0e0cda7521 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -70,24 +70,10 @@ typedef struct BlockDevOps {
 
 /* This struct is embedded in (the private) BlockBackend struct and contains
  * fields that must be public. This is in particular for QLIST_ENTRY() and
- * friends so that BlockBackends can be kept in lists outside block-backend.c 
*/
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ * */
 typedef struct BlockBackendPublic {
-/* throttled_reqs_lock protects the CoQueues for throttled requests.  */
-CoMutex  throttled_reqs_lock;
-CoQueue  throttled_reqs[2];
-
-/* Nonzero if the I/O limits are currently being ignored; generally
- * it is zero.  Accessed with atomic operations.
- */
-unsigned int io_limits_disabled;
-
-/* The following fields are protected by the ThrottleGroup lock.
- * See the ThrottleGroup documentation for details.
- * throttle_state tells us if I/O limits are configured. */
-ThrottleState *throttle_state;
-ThrottleTimers throttle_timers;
-unsigned   pending_reqs[2];
-QLIST_ENTRY(BlockBackendPublic) round_robin;
+ThrottleGroupMember throttle_group_member;
 } BlockBackendPublic;
 
 BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm);
diff --git a/block/block-backend.c b/block/block-backend.c
index e9798e897d..7b981e00bb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ 

[Qemu-block] [PATCH v7 2/6] block: add aio_context field in ThrottleGroupMember

[Manos Pitsidianakis]

timer_cb() needs to know about the current Aio context of the throttle
request that is woken up. In order to make ThrottleGroupMember backend
agnostic, this information is stored in an aio_context field instead of
accessing it from BlockBackend.

Reviewed-by: Alberto Garcia 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Manos Pitsidianakis 
---
 include/block/throttle-groups.h |  7 -
 block/block-backend.c   | 15 --
 block/throttle-groups.c | 38 -
 tests/test-throttle.c   | 63 +
 4 files changed, 69 insertions(+), 54 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 1a6bcdae74..a0f27cac63 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -33,6 +33,7 @@
  */
 
 typedef struct ThrottleGroupMember {
+AioContext   *aio_context;
 /* throttled_reqs_lock protects the CoQueues for throttled requests.  */
 CoMutex  throttled_reqs_lock;
 CoQueue  throttled_reqs[2];
@@ -61,12 +62,16 @@ void throttle_group_config(ThrottleGroupMember *tgm, 
ThrottleConfig *cfg);
 void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg);
 
 void throttle_group_register_tgm(ThrottleGroupMember *tgm,
-const char *groupname);
+const char *groupname,
+AioContext *ctx);
 void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
 
 void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember 
*tgm,
 unsigned int bytes,
 bool is_write);
+void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
+   AioContext *new_context);
+void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
 
 #endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 7b981e00bb..9ba800e733 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1726,18 +1726,14 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB 
*acb)
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
 BlockDriverState *bs = blk_bs(blk);
-ThrottleTimers *tt;
+ThrottleGroupMember *tgm = >public.throttle_group_member;
 
 if (bs) {
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_detach_aio_context(tt);
+if (tgm->throttle_state) {
+throttle_group_detach_aio_context(tgm);
+throttle_group_attach_aio_context(tgm, new_context);
 }
 bdrv_set_aio_context(bs, new_context);
-if (blk->public.throttle_group_member.throttle_state) {
-tt = >public.throttle_group_member.throttle_timers;
-throttle_timers_attach_aio_context(tt, new_context);
-}
 }
 }
 
@@ -1970,7 +1966,8 @@ void blk_io_limits_disable(BlockBackend *blk)
 void blk_io_limits_enable(BlockBackend *blk, const char *group)
 {
 assert(!blk->public.throttle_group_member.throttle_state);
-throttle_group_register_tgm(>public.throttle_group_member, group);
+throttle_group_register_tgm(>public.throttle_group_member,
+group, blk_get_aio_context(blk));
 }
 
 void blk_io_limits_update_group(BlockBackend *blk, const char *group)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index c8ed16ddf8..3b07b25f39 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -391,9 +391,6 @@ static void coroutine_fn 
throttle_group_restart_queue_entry(void *opaque)
 
 static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool 
is_write)
 {
-BlockBackendPublic *blkp = container_of(tgm, BlockBackendPublic,
-throttle_group_member);
-BlockBackend *blk = blk_by_public(blkp);
 Coroutine *co;
 RestartData rd = {
 .tgm = tgm,
@@ -401,7 +398,7 @@ static void 
throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write
 };
 
 co = qemu_coroutine_create(throttle_group_restart_queue_entry, );
-aio_co_enter(blk_get_aio_context(blk), co);
+aio_co_enter(tgm->aio_context, co);
 }
 
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm)
@@ -449,13 +446,11 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, 
ThrottleConfig *cfg)
 /* ThrottleTimers callback. This wakes up a request that was waiting
  * because it had been throttled.
  *
- * @blk:   the BlockBackend whose request had been throttled
+ * @tgm:   the ThrottleGroupMember whose request had been throttled
  * @is_write:  the type of operation (read/write)
  */
-static void timer_cb(BlockBackend 

[Qemu-block] [PATCH v7 0/6] add throttle block driver filter

[Manos Pitsidianakis]

This series adds a throttle block driver filter. Currently throttling is done
at the BlockBackend level. Using block driver interfaces we can move the
throttling to any point in the BDS graph using a throttle node which uses the
existing throttling code. This allows for potentially more complex
configurations (throttling at any point in the graph, chained filters)

v7:
  remove anonymous groups
  error on missing throttle-group option on block/throttle.c
  change switch case format in block/throttle-groups.c (berto)

v6:
  don't allow named group limit configuration in block/throttle.c; allow either
-drive driver=throttle,file.filename=foo.qcow2, \
limits.iops-total=...
  or
-drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
  fixes suggested by berto

v5:
  fix crash in 'add aio_context field in ThrottleGroupMember'
  fix suggestions in block/throttle.c

v4:
  fix suggestions in block/throttle.c
  fix suggestions in block/throttle_groups.c
  add doc note in BlockDevOptionsThrottle

v3:
  fix style error in 'add aio_context field in ThrottleGroupMember'

v2:
  change QOM throttle group object name
  print valid ranges for uint on error
  move frees in throttle_group_obj_finalize()
  split throttle_group_{set,get}()
  add throttle_recurse_is_first_non_filter()

Manos Pitsidianakis (6):
  block: move ThrottleGroup membership to ThrottleGroupMember
  block: add aio_context field in ThrottleGroupMember
  block: tidy ThrottleGroupMember initializations
  block: convert ThrottleGroup to object with QOM
  block: add throttle block filter driver
  qemu-iotests: add 184 for throttle filter driver

 qapi/block-core.json|  66 +++-
 include/block/throttle-groups.h |  48 ++-
 include/qemu/throttle-options.h |  60 ++--
 include/qemu/throttle.h |   3 +
 include/sysemu/block-backend.h  |  20 +-
 block/block-backend.c   |  62 ++--
 block/qapi.c|   8 +-
 block/throttle-groups.c | 737 +---
 block/throttle.c| 250 ++
 blockdev.c  |   4 +-
 tests/test-throttle.c   | 111 +++---
 util/throttle.c | 151 
 block/Makefile.objs |   1 +
 tests/qemu-iotests/184  | 205 +++
 tests/qemu-iotests/184.out  | 300 
 tests/qemu-iotests/group|   1 +
 16 files changed, 1706 insertions(+), 321 deletions(-)
 create mode 100644 block/throttle.c
 create mode 100755 tests/qemu-iotests/184
 create mode 100644 tests/qemu-iotests/184.out

-- 
2.11.0