Re: [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

2016-06-10 Thread Eduardo Habkost 
On Fri, Jun 10, 2016 at 02:59:55PM -0600, Eric Blake wrote:
> On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> > This patch simplifies code that uses a local_err variable just to
> > immediately use it for an error_propagate() call.
> > 
> > Coccinelle patch used to perform the changes added to
> > scripts/coccinelle/remove_local_err.cocci.
> > 
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  block.c   |  8 ++--
> >  block/raw-posix.c |  8 ++--
> >  block/raw_bsd.c   |  4 +---
> >  blockdev.c| 16 +---
> >  hw/s390x/s390-virtio-ccw.c|  5 +
> >  hw/s390x/virtio-ccw.c | 28 +++-
> >  scripts/coccinelle/remove_local_err.cocci | 27 +++
> >  target-i386/cpu.c |  4 +---
> >  8 files changed, 46 insertions(+), 54 deletions(-)
> >  create mode 100644 scripts/coccinelle/remove_local_err.cocci
> > 
> 
> > +++ b/block.c
> > @@ -294,14 +294,12 @@ typedef struct CreateCo {
> >  
> >  static void coroutine_fn bdrv_create_co_entry(void *opaque)
> >  {
> > -Error *local_err = NULL;
> >  int ret;
> >  
> >  CreateCo *cco = opaque;
> >  assert(cco->drv);
> >  
> > -ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
> > -error_propagate(>err, local_err);
> > +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);
> >  cco->ret = ret;
> 
> This hunk doesn't get simplified by 3/3; you may want to consider a
> manual followup to drop 'int ret' and just assign
> cco->drv->bdrv_create() directly to cco->ret.  But doesn't change this
> patch.

This could become yet another Coccinelle script, but we need to
be careful about type conversions, and tell it to do it only if
the types of 'ret', 'cc->drv->bdrv_create()' and 'cco->ret' are
the same.

-- 
Eduardo

Re: [Qemu-block] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

2016-06-10 Thread Eric Blake 
On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> We don't really want to go through the block layer in order to read from
> or write to the vmstate in a qcow2 image. Doing so required a few ugly
> hacks like saving and restoring the old image size (because writing to
> vmstate offsets would increase the image size) or disabling the "reads
> after EOF = zeroes" logic. When calling the right functions directly,
> these hacks aren't necessary any more.
> 
> Note that .bdrv_vmstate_load/save() return 0 instead of the number of
> bytes in case of success now.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/qcow2.c | 24 
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 

> +++ b/block/qcow2.c
> @@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, 
> QEMUIOVector *qiov,
>int64_t pos)
>  {
>  BDRVQcow2State *s = bs->opaque;
> -int64_t total_sectors = bs->total_sectors;
> -bool zero_beyond_eof = bs->zero_beyond_eof;
> -int ret;
>  
>  BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
> -bs->zero_beyond_eof = false;
> -ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
> -bs->zero_beyond_eof = zero_beyond_eof;
> -
> -/* bdrv_co_do_writev will have increased the total_sectors value to 
> include
> - * the VM state - the VM state is however not an actual part of the block
> - * device, therefore, we need to restore the old value. */
> -bs->total_sectors = total_sectors;
> -
> -return ret;
> +return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
> +qiov->size, qiov, 0);
>  }

bs->drv->bdrv_co_pwritev() is an optional interface; not all the drivers
have it yet.  Should you be asserting that it exists, and/or returning
an error if it does not?

>  
>  static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>int64_t pos)
>  {
>  BDRVQcow2State *s = bs->opaque;
> -bool zero_beyond_eof = bs->zero_beyond_eof;
> -int ret;
>  
>  BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
> -bs->zero_beyond_eof = false;
> -ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
> -bs->zero_beyond_eof = zero_beyond_eof;
> -
> -return ret;
> +return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos,
> +   qiov->size, qiov, 0);

Ditto.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 4/6] block: Make bdrv_load/save_vmstate coroutine_fns

2016-06-10 Thread Eric Blake 
On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> This allows drivers to share code between normal I/O and vmstate
> accesses.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 80 
> ++-
>  include/block/block_int.h | 10 +++---
>  2 files changed, 64 insertions(+), 26 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] block-backend: allow flush on devices with open tray

2016-06-10 Thread John Snow 
If a device still has an attached BDS because the medium has not yet
been removed, we will be unable to migrate to a new host because
blk_flush will return an error for that backend.

Replace the call to blk_is_available to blk_is_inserted to weaken
the check and allow flushes from the backend to work, while still
disallowing flushes from the frontend/device model to work.

This fixes a regression present in 2.6.0 caused by the following commit:
fe1a9cbc339bb54d20f1ca4c1e8788d16944d5cf
block: Move some bdrv_*_all() functions to BB

Signed-off-by: John Snow 
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..d1e875e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1122,7 +1122,7 @@ int blk_co_flush(BlockBackend *blk)
 
 int blk_flush(BlockBackend *blk)
 {
-if (!blk_is_available(blk)) {
+if (!blk_is_inserted(blk)) {
 return -ENOMEDIUM;
 }
 
-- 
2.4.11

Re: [Qemu-block] [PATCH 3/6] block: Allow .bdrv_load/save_vmstate() to return 0/-errno

2016-06-10 Thread Eric Blake 
On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> The return value of .bdrv_load/save_vmstate() can be any non-negative
> number in case of success now. It used to be bytes/-errno.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 602c7d3..bca244c 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1839,9 +1839,16 @@ int bdrv_save_vmstate(BlockDriverState *bs, const 
> uint8_t *buf,
>  .iov_base   = (void *) buf,
>  .iov_len= size,
>  };
> +int ret;
>  
>  qemu_iovec_init_external(, , 1);
> -return bdrv_writev_vmstate(bs, , pos);
> +
> +ret = bdrv_writev_vmstate(bs, , pos);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +return size;
>  }
>  
>  int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t 
> pos)
> @@ -1870,7 +1877,12 @@ int bdrv_load_vmstate(BlockDriverState *bs, uint8_t 
> *buf,
>  int ret;

Aha, my complaint in v2 about it being dead means you need to reinstate
it here.

>  
>  qemu_iovec_init_external(, , 1);
> -return bdrv_readv_vmstate(bs, , pos);
> +ret = bdrv_readv_vmstate(bs, , pos);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +return size;
>  }
>  
>  int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> 

Matches the semantics we have elsewhere (I'm not sure if 'size' is the
best choice if we ever need to support short read/write, but doesn't
seem to hurt).

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/6] block: Make .bdrv_load_vmstate() vectored

2016-06-10 Thread Eric Blake 
On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> This brings it in line with .bdrv_save_vmstate().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 26 +-
>  block/qcow2.c |  6 +++---
>  block/sheepdog.c  | 13 ++---
>  include/block/block.h |  1 +
>  include/block/block_int.h |  4 ++--
>  5 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 11510cf..602c7d3 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1862,13 +1862,29 @@ int bdrv_writev_vmstate(BlockDriverState *bs, 
> QEMUIOVector *qiov, int64_t pos)
>  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>int64_t pos, int size)
>  {
> +QEMUIOVector qiov;
> +struct iovec iov = {
> +.iov_base   = buf,
> +.iov_len= size,
> +};
> +int ret;

Dead variable.

> +
> +qemu_iovec_init_external(, , 1);
> +return bdrv_readv_vmstate(bs, , pos);
> +}
> +
> +int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
> +{
>  BlockDriver *drv = bs->drv;
> -if (!drv)
> +
> +if (!drv) {
>  return -ENOMEDIUM;
> -if (drv->bdrv_load_vmstate)
> -return drv->bdrv_load_vmstate(bs, buf, pos, size);
> -if (bs->file)
> -return bdrv_load_vmstate(bs->file->bs, buf, pos, size);
> +} else if (drv->bdrv_load_vmstate) {
> +return drv->bdrv_load_vmstate(bs, qiov, pos);
> +} else if (bs->file) {
> +return bdrv_readv_vmstate(bs->file->bs, qiov, pos);
> +}

Don't know that I would have used 'else if' after a return, but it's not
wrong, so no need to change.

With the dead 'ret' gone,
Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC v2 3/3] Remove unnecessary variables for function return value

2016-06-10 Thread Eric Blake 
On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> Use Coccinelle script to replace 'ret = E; return ret' with
> 'return E'. The script will do the substitution only when the
> function return type and variable type are the same.
> 
> Sending as RFC because the patch looks more intrusive than the
> others. Probably better to split it per subsystem and let each
> maintainer review and apply it?

Borderline on size, so yeah, splitting it across several subsystems may
ease review (although then the patch will be committed in piecemeal
fashion, and you'd have to ensure the script/coccinelle/ patch goes in
first...)

At any rate, it's fairly mechanical, so I'll review it as is:

> 
> Signed-off-by: Eduardo Habkost 
> ---

>  47 files changed, 90 insertions(+), 254 deletions(-)
>  create mode 100644 scripts/coccinelle/return_directly.cocci

Nice diffstat.

> +++ b/block/qcow2-cluster.c
> @@ -154,11 +154,8 @@ static int l2_load(BlockDriverState *bs, uint64_t 
> l2_offset,
>  uint64_t **l2_table)
>  {
>  BDRVQcow2State *s = bs->opaque;
> -int ret;
> -
> -ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) 
> l2_table);
> -
> -return ret;
> +return qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> +   (void **)l2_table);

Coccinelle changed spacing of the cast. I don't care strongly enough to
require a touchup if this is the only thing, but may be worth fixing if
you have to respin (for example to split up by submaintainers).

> +++ b/block/raw_bsd.c
> @@ -190,10 +190,7 @@ static int raw_has_zero_init(BlockDriverState *bs)
>  
>  static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>  {
> -int ret;
> -
> -ret = bdrv_create_file(filename, opts, errp);
> -return ret;
> +return bdrv_create_file(filename, opts, errp);
>  }

Potential followup patch: delete raw_create(), and:
- .bdrv_create = _create,
+ .bdrv_create = bdrv_create_file,

but doesn't affect this patch.

> +++ b/block/rbd.c
> @@ -875,10 +875,7 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs,
>const char *snapshot_name)
>  {
>  BDRVRBDState *s = bs->opaque;
> -int r;
> -
> -r = rbd_snap_rollback(s->image, snapshot_name);
> -return r;
> +return rbd_snap_rollback(s->image, snapshot_name);

Coccinelle lost the blank line between declarations and statements;
might be nice to manually touch that up and add it back in.

> +++ b/hw/ppc/spapr_vio.c
> @@ -57,12 +57,7 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev)
>  {
>  VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
>  VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> -char *name;
> -
> -/* Device tree style name device@reg */
> -name = g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
> -
> -return name;
> +return g_strdup_printf("%s@%x", pc->dt_name, dev->reg);

Coccinelle lost the comment; might be worth keeping it.

> +++ b/hw/scsi/megasas.c
> @@ -410,17 +410,9 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t 
> lba,
>  static uint64_t megasas_fw_time(void)
>  {
>  struct tm curtime;
> -uint64_t bcd_time;
>  
>  qemu_get_timedate(, 0);
> -bcd_time = ((uint64_t)curtime.tm_sec & 0xff) << 48 |
> -((uint64_t)curtime.tm_min & 0xff)  << 40 |
> -((uint64_t)curtime.tm_hour & 0xff) << 32 |
> -((uint64_t)curtime.tm_mday & 0xff) << 24 |
> -((uint64_t)curtime.tm_mon & 0xff)  << 16 |
> -((uint64_t)(curtime.tm_year + 1900) & 0x);
> -
> -return bcd_time;
> +return ((uint64_t)curtime.tm_sec & 0xff) << 48 | 
> ((uint64_t)curtime.tm_min & 0xff) << 40 | ((uint64_t)curtime.tm_hour & 0xff) 
> << 32 | ((uint64_t)curtime.tm_mday & 0xff) << 24 | ((uint64_t)curtime.tm_mon 
> & 0xff) << 16 | ((uint64_t)(curtime.tm_year + 1900) & 0x);

Eww. Coccinelle botched that formatting.  You'll need to manually fix
this one.

> +++ b/hw/timer/mc146818rtc.c
> @@ -105,12 +105,9 @@ static inline bool rtc_running(RTCState *s)
>  
>  static uint64_t get_guest_rtc_ns(RTCState *s)
>  {
> -uint64_t guest_rtc;
>  uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
>  
> -guest_rtc = s->base_rtc * NANOSECONDS_PER_SECOND +
> -guest_clock - s->last_update + s->offset;
> -return guest_rtc;
> +return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - 
> s->last_update + s->offset;
>  }

Worth wrapping that line again (not as bad as the megasas one, though).

> +++ b/qga/commands-win32.c
> @@ -1150,7 +1150,6 @@ out:
>  int64_t qmp_guest_get_time(Error **errp)
>  {
>  SYSTEMTIME ts = {0};
> -int64_t time_ns;
>  FILETIME tf;
>  
>  GetSystemTime();
> @@ -1164,10 +1163,7 @@ int64_t qmp_guest_get_time(Error **errp)
>  return -1;
>  }
>  
> -time_ns = int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> -- W32_FT_OFFSET) * 100;
> -
> -return time_ns;
> +return

Re: [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

2016-06-10 Thread Eric Blake 
On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> This patch simplifies code that uses a local_err variable just to
> immediately use it for an error_propagate() call.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/remove_local_err.cocci.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  block.c   |  8 ++--
>  block/raw-posix.c |  8 ++--
>  block/raw_bsd.c   |  4 +---
>  blockdev.c| 16 +---
>  hw/s390x/s390-virtio-ccw.c|  5 +
>  hw/s390x/virtio-ccw.c | 28 +++-
>  scripts/coccinelle/remove_local_err.cocci | 27 +++
>  target-i386/cpu.c |  4 +---
>  8 files changed, 46 insertions(+), 54 deletions(-)
>  create mode 100644 scripts/coccinelle/remove_local_err.cocci
> 

> +++ b/block.c
> @@ -294,14 +294,12 @@ typedef struct CreateCo {
>  
>  static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  {
> -Error *local_err = NULL;
>  int ret;
>  
>  CreateCo *cco = opaque;
>  assert(cco->drv);
>  
> -ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
> -error_propagate(>err, local_err);
> +ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);
>  cco->ret = ret;

This hunk doesn't get simplified by 3/3; you may want to consider a
manual followup to drop 'int ret' and just assign
cco->drv->bdrv_create() directly to cco->ret.  But doesn't change this
patch.


> +++ b/blockdev.c
> @@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char 
> *target,
>  BlockBackend *blk;
>  BlockDriverState *target_bs;
>  AioContext *aio_context;
> -Error *local_err = NULL;
>  
>  blk = blk_by_name(device);
>  if (!blk) {
> @@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const 
> char *target,
>  
>  bdrv_set_aio_context(target_bs, aio_context);
>  
> -blockdev_mirror_common(bs, target_bs,
> -   has_replaces, replaces, sync,
> -   has_speed, speed,
> -   has_granularity, granularity,
> -   has_buf_size, buf_size,
> -   has_on_source_error, on_source_error,
> -   has_on_target_error, on_target_error,
> -   true, true,
> -   _err);
> -error_propagate(errp, local_err);
> +blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
> +   has_speed, speed, has_granularity, granularity,
> +   has_buf_size, buf_size, has_on_source_error,
> +   on_source_error, has_on_target_error,
> +   on_target_error, true, true, errp);

Coccinelle messes a bit with the formatting (the old way explicitly
tried to pair related has_foo with foo). But I'm going to mess with it
again with my qapi patches for passing a boxed parameter rather than
lots of arguments, so don't worry about it.

> +++ b/scripts/coccinelle/remove_local_err.cocci
> @@ -0,0 +1,27 @@
> +// Replace unnecessary usage of local_err variable with
> +// direct usage of errp argument
> +
> +@@
> +expression list ARGS;
> +expression F2;
> +identifier LOCAL_ERR;
> +expression ERRP;
> +idexpression V;
> +typedef Error;
> +expression I;
> +@@
> + {
> + ...
> +-Error *LOCAL_ERR;
> + ... when != LOCAL_ERR
> +(
> +-F2(ARGS, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++F2(ARGS, ERRP);
> +|
> +-V = F2(ARGS, _ERR);
> +-error_propagate(ERRP, LOCAL_ERR);
> ++V = F2(ARGS, ERRP);
> +)
> + ... when != LOCAL_ERR
> + }

Looks good.
Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 1/3] error: Remove NULL checks on error_propagate() calls

2016-06-10 Thread Eric Blake 
On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> error_propagate() already ignores local_err==NULL, so there's no
> need to check it before calling.
> 
> Coccinelle patch used to perform the changes added to
> scripts/coccinelle/error_propagate_null.cocci.
> 
> Signed-off-by: Eduardo Habkost 
> ---
>  block.c   | 20 +--
>  block/qcow2.c |  4 +---
>  block/quorum.c|  4 +---
>  block/raw-posix.c | 16 ---
>  block/raw_bsd.c   |  4 +---
>  block/snapshot.c  |  4 +---
>  blockdev.c| 12 +++-
>  bootdevice.c  |  4 +---
>  dump.c|  4 +---
>  hw/ide/qdev.c |  4 +---
>  hw/net/ne2000-isa.c   |  4 +---
>  hw/s390x/virtio-ccw.c | 28 
> +++
>  hw/usb/dev-storage.c  |  4 +---
>  qga/commands-win32.c  |  8 ++--
>  qom/object.c  |  4 +---
>  scripts/coccinelle/error_propagate_null.cocci | 10 ++
>  16 files changed, 41 insertions(+), 93 deletions(-)
>  create mode 100644 scripts/coccinelle/error_propagate_null.cocci

You can do:
git config diff.orderFile /path/to/file

and then set up a list of globs in /path/to/file in order to influence
your diffs; in my case, I stuck 'scripts/coccinelle/*' near the top of
my order file, as I find that to be a more useful part of the patch than
the churn from running it.  But it doesn't affect patch correctness,
just ease of review.

Reviewed-by: Eric Blake 

> +++ b/scripts/coccinelle/error_propagate_null.cocci
> @@ -0,0 +1,10 @@
> +// error_propagate() already ignores local_err==NULL, so there's
> +// no need to check it before calling.
> +
> +@@
> +identifier L;
> +expression E;
> +@@
> +-if (L) {
> + error_propagate(E, L);
> +-}
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/6] block: Introduce bdrv_preadv()

2016-06-10 Thread Eric Blake 
On 06/10/2016 10:05 AM, Kevin Wolf wrote:
> We already have a byte-based bdrv_pwritev(), but the read counterpart
> was still missing. This commit adds it.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 20 +---
>  include/block/block.h |  1 +
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 

Worth adding a flags argument while at it? But bdrv_pwritev() lacks one,
so for symmetry reasons, I'm okay if you don't bother.

> +int bdrv_preadv(BlockDriverState *bs, int64_t offset, QEMUIOVector *qiov)
> +{
> +int ret;
> +
> +ret = bdrv_prwv_co(bs, offset, qiov, false, 0);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +return qiov->size;

This implies we never have a short read, it's an all-or-none error or
success.  Matches what we've done elsewhere, so I guess it's right.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

2016-06-10 Thread Eduardo Habkost 
This patch simplifies code that uses a local_err variable just to
immediately use it for an error_propagate() call.

Coccinelle patch used to perform the changes added to
scripts/coccinelle/remove_local_err.cocci.

Signed-off-by: Eduardo Habkost 
---
 block.c   |  8 ++--
 block/raw-posix.c |  8 ++--
 block/raw_bsd.c   |  4 +---
 blockdev.c| 16 +---
 hw/s390x/s390-virtio-ccw.c|  5 +
 hw/s390x/virtio-ccw.c | 28 +++-
 scripts/coccinelle/remove_local_err.cocci | 27 +++
 target-i386/cpu.c |  4 +---
 8 files changed, 46 insertions(+), 54 deletions(-)
 create mode 100644 scripts/coccinelle/remove_local_err.cocci

diff --git a/block.c b/block.c
index ecca55a..d516ab6 100644
--- a/block.c
+++ b/block.c
@@ -294,14 +294,12 @@ typedef struct CreateCo {
 
 static void coroutine_fn bdrv_create_co_entry(void *opaque)
 {
-Error *local_err = NULL;
 int ret;
 
 CreateCo *cco = opaque;
 assert(cco->drv);
 
-ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-error_propagate(>err, local_err);
+ret = cco->drv->bdrv_create(cco->filename, cco->opts, >err);
 cco->ret = ret;
 }
 
@@ -353,7 +351,6 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-Error *local_err = NULL;
 int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
@@ -361,8 +358,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create(drv, filename, opts, errp);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index cb663d8..d7397bf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -582,12 +582,10 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_FILE;
-ret = raw_open_common(bs, options, flags, 0, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, 0, errp);
 return ret;
 }
 
@@ -2442,14 +2440,12 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
   Error **errp)
 {
 BDRVRawState *s = bs->opaque;
-Error *local_err = NULL;
 int ret;
 
 s->type = FTYPE_CD;
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-error_propagate(errp, local_err);
+ret = raw_open_common(bs, options, flags, O_NONBLOCK, errp);
 return ret;
 }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5af11b6..b51ac98 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -190,11 +190,9 @@ static int raw_has_zero_init(BlockDriverState *bs)
 
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
-Error *local_err = NULL;
 int ret;
 
-ret = bdrv_create_file(filename, opts, _err);
-error_propagate(errp, local_err);
+ret = bdrv_create_file(filename, opts, errp);
 return ret;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 028dba3..3b6d242 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3654,7 +3654,6 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 BlockBackend *blk;
 BlockDriverState *target_bs;
 AioContext *aio_context;
-Error *local_err = NULL;
 
 blk = blk_by_name(device);
 if (!blk) {
@@ -3678,16 +3677,11 @@ void qmp_blockdev_mirror(const char *device, const char 
*target,
 
 bdrv_set_aio_context(target_bs, aio_context);
 
-blockdev_mirror_common(bs, target_bs,
-   has_replaces, replaces, sync,
-   has_speed, speed,
-   has_granularity, granularity,
-   has_buf_size, buf_size,
-   has_on_source_error, on_source_error,
-   has_on_target_error, on_target_error,
-   true, true,
-   _err);
-error_propagate(errp, local_err);
+blockdev_mirror_common(bs, target_bs, has_replaces, replaces, sync,
+   has_speed, speed, has_granularity, granularity,
+   has_buf_size, buf_size, has_on_source_error,
+   on_source_error, has_on_target_error,
+   on_target_error, true, true, errp);
 
 aio_context_release(aio_context);
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 95ff5e3..b7112d0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c

[Qemu-block] [PATCH v2 1/3] error: Remove NULL checks on error_propagate() calls

2016-06-10 Thread Eduardo Habkost 
error_propagate() already ignores local_err==NULL, so there's no
need to check it before calling.

Coccinelle patch used to perform the changes added to
scripts/coccinelle/error_propagate_null.cocci.

Signed-off-by: Eduardo Habkost 
---
 block.c   | 20 +--
 block/qcow2.c |  4 +---
 block/quorum.c|  4 +---
 block/raw-posix.c | 16 ---
 block/raw_bsd.c   |  4 +---
 block/snapshot.c  |  4 +---
 blockdev.c| 12 +++-
 bootdevice.c  |  4 +---
 dump.c|  4 +---
 hw/ide/qdev.c |  4 +---
 hw/net/ne2000-isa.c   |  4 +---
 hw/s390x/virtio-ccw.c | 28 +++
 hw/usb/dev-storage.c  |  4 +---
 qga/commands-win32.c  |  8 ++--
 qom/object.c  |  4 +---
 scripts/coccinelle/error_propagate_null.cocci | 10 ++
 16 files changed, 41 insertions(+), 93 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci

diff --git a/block.c b/block.c
index f54bc25..ecca55a 100644
--- a/block.c
+++ b/block.c
@@ -301,9 +301,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
 assert(cco->drv);
 
 ret = cco->drv->bdrv_create(cco->filename, cco->opts, _err);
-if (local_err) {
-error_propagate(>err, local_err);
-}
+error_propagate(>err, local_err);
 cco->ret = ret;
 }
 
@@ -364,9 +362,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, 
Error **errp)
 }
 
 ret = bdrv_create(drv, filename, opts, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -1760,18 +1756,14 @@ fail:
 QDECREF(options);
 bs->options = NULL;
 bdrv_unref(bs);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 
 close_and_fail:
 bdrv_unref(bs);
 QDECREF(snapshot_options);
 QDECREF(options);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return NULL;
 }
 
@@ -3591,9 +3583,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 out:
 qemu_opts_del(opts);
 qemu_opts_free(create_opts);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6f5fb81..4504846 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2394,9 +2394,7 @@ static int qcow2_create(const char *filename, QemuOpts 
*opts, Error **errp)
 ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
 cluster_size, prealloc, opts, version, refcount_order,
 _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 
 finish:
 g_free(backing_file);
diff --git a/block/quorum.c b/block/quorum.c
index ec6f3b9..331b726 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -971,9 +971,7 @@ close_exit:
 exit:
 qemu_opts_del(opts);
 /* propagate error */
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ce2e20f..cb663d8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -587,9 +587,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 s->type = FTYPE_FILE;
 ret = raw_open_common(bs, options, flags, 0, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2239,9 +2237,7 @@ hdev_open_Mac_error:
 
 ret = raw_open_common(bs, options, flags, 0, _err);
 if (ret < 0) {
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
 if (*bsd_path) {
 filename = bsd_path;
@@ -2453,9 +2449,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
 ret = raw_open_common(bs, options, flags, O_NONBLOCK, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-}
+error_propagate(errp, local_err);
 return ret;
 }
 
@@ -2573,9 +2567,7 @@ static int cdrom_open(BlockDriverState *bs, QDict 
*options, int flags,
 
 ret = raw_open_common(bs,

[Qemu-block] [RFC v2 3/3] Remove unnecessary variables for function return value

2016-06-10 Thread Eduardo Habkost 
Use Coccinelle script to replace 'ret = E; return ret' with
'return E'. The script will do the substitution only when the
function return type and variable type are the same.

Sending as RFC because the patch looks more intrusive than the
others. Probably better to split it per subsystem and let each
maintainer review and apply it?

Signed-off-by: Eduardo Habkost 
---
 audio/audio.c| 10 ++
 block.c  |  4 +---
 block/archipelago.c  |  4 +---
 block/qcow2-cluster.c|  7 ++-
 block/qcow2-refcount.c   |  7 ++-
 block/raw-posix.c|  8 ++--
 block/raw_bsd.c  |  5 +
 block/rbd.c  |  5 +
 block/vmdk.c |  6 ++
 block/vvfat.c|  5 +
 hw/acpi/aml-build.c  | 13 +++--
 hw/audio/intel-hda.c |  5 +
 hw/display/vga.c |  4 +---
 hw/intc/s390_flic_kvm.c  |  5 ++---
 hw/pci-host/uninorth.c   |  5 +
 hw/ppc/spapr_vio.c   |  7 +--
 hw/scsi/megasas.c| 10 +-
 hw/scsi/scsi-generic.c   |  5 +
 hw/timer/mc146818rtc.c   |  5 +
 hw/virtio/virtio-pci.c   |  4 +---
 linux-user/signal.c  | 15 ---
 page_cache.c |  5 +
 qga/commands-posix.c |  4 +---
 qga/commands-win32.c |  6 +-
 qobject/qlist.c  |  5 +
 scripts/coccinelle/return_directly.cocci | 19 +++
 target-i386/fpu_helper.c | 10 ++
 target-i386/kvm.c|  5 ++---
 target-mips/dsp_helper.c | 15 +++
 target-mips/op_helper.c  |  4 +---
 target-s390x/helper.c|  6 +-
 target-sparc/cc_helper.c | 25 +
 target-tricore/op_helper.c   | 13 -
 tests/display-vga-test.c |  6 +-
 tests/endianness-test.c  |  5 +
 tests/i440fx-test.c  |  4 +---
 tests/intel-hda-test.c   |  6 +-
 tests/test-filter-redirector.c   |  6 +-
 tests/virtio-blk-test.c  |  5 +
 tests/virtio-console-test.c  |  6 +-
 tests/virtio-net-test.c  |  6 +-
 tests/virtio-scsi-test.c |  6 +-
 tests/wdt_ib700-test.c   |  6 +-
 ui/cursor.c  | 10 ++
 ui/qemu-pixman.c | 11 +++
 util/module.c|  6 +-
 vl.c |  5 +
 47 files changed, 90 insertions(+), 254 deletions(-)
 create mode 100644 scripts/coccinelle/return_directly.cocci

diff --git a/audio/audio.c b/audio/audio.c
index e60c124..9d4dcc7 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1131,8 +1131,6 @@ static void audio_timer (void *opaque)
  */
 int AUD_write (SWVoiceOut *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return size;
@@ -1143,14 +1141,11 @@ int AUD_write (SWVoiceOut *sw, void *buf, int size)
 return 0;
 }
 
-bytes = sw->hw->pcm_ops->write (sw, buf, size);
-return bytes;
+return sw->hw->pcm_ops->write(sw, buf, size);
 }
 
 int AUD_read (SWVoiceIn *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return size;
@@ -1161,8 +1156,7 @@ int AUD_read (SWVoiceIn *sw, void *buf, int size)
 return 0;
 }
 
-bytes = sw->hw->pcm_ops->read (sw, buf, size);
-return bytes;
+return sw->hw->pcm_ops->read(sw, buf, size);
 }
 
 int AUD_get_buffer_size_out (SWVoiceOut *sw)
diff --git a/block.c b/block.c
index d516ab6..c537307 100644
--- a/block.c
+++ b/block.c
@@ -351,15 +351,13 @@ out:
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
 {
 BlockDriver *drv;
-int ret;
 
 drv = bdrv_find_protocol(filename, true, errp);
 if (drv == NULL) {
 return -ENOENT;
 }
 
-ret = bdrv_create(drv, filename, opts, errp);
-return ret;
+return bdrv_create(drv, filename, opts, errp);
 }
 
 /**
diff --git a/block/archipelago.c b/block/archipelago.c
index b9f5e69..37b8aca 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -974,11 +974,9 @@ err_exit2:
 
 static int64_t qemu_archipelago_getlength(BlockDriverState *bs)
 {
-int64_t ret;
 BDRVArchipelagoState *s = bs->opaque;
 
-ret = archipelago_volume_info(s);
-return ret;
+return archipelago_volume_info(s);
 }
 
 static int

[Qemu-block] [PATCH v2 0/3] coccinelle: Clean up error checks and return value variables

2016-06-10 Thread Eduardo Habkost 
v2 of the previous "error: Remove NULL checks on
error_propagate() calls" patch, now it became a series.

Changes v1 -> v2:
* The Coccinelle scripts were simplified by using "when"
  constraints to detect when a variable is not used elsewhere
  inside the function.
* Added script to remove unnecessary variables for function
  return value.
* Coccinelle scripts added to scripts/coccinelle.

Eduardo Habkost (3):
  error: Remove NULL checks on error_propagate() calls
  error: Remove unnecessary local_err variables
  [RFC] Remove unnecessary variables for function return value

 audio/audio.c | 10 ++-
 block.c   | 26 -
 block/archipelago.c   |  4 +--
 block/qcow2-cluster.c |  7 ++---
 block/qcow2-refcount.c|  7 ++---
 block/qcow2.c |  4 +--
 block/quorum.c|  4 +--
 block/raw-posix.c | 24 +++
 block/raw_bsd.c   |  9 +-
 block/rbd.c   |  5 +---
 block/snapshot.c  |  4 +--
 block/vmdk.c  |  6 ++--
 block/vvfat.c |  5 +---
 blockdev.c| 26 +
 bootdevice.c  |  4 +--
 dump.c|  4 +--
 hw/acpi/aml-build.c   | 13 ++---
 hw/audio/intel-hda.c  |  5 +---
 hw/display/vga.c  |  4 +--
 hw/ide/qdev.c |  4 +--
 hw/intc/s390_flic_kvm.c   |  5 ++--
 hw/net/ne2000-isa.c   |  4 +--
 hw/pci-host/uninorth.c|  5 +---
 hw/ppc/spapr_vio.c|  7 +
 hw/s390x/s390-virtio-ccw.c|  5 +---
 hw/s390x/virtio-ccw.c | 42 +--
 hw/scsi/megasas.c | 10 +--
 hw/scsi/scsi-generic.c|  5 +---
 hw/timer/mc146818rtc.c|  5 +---
 hw/usb/dev-storage.c  |  4 +--
 hw/virtio/virtio-pci.c|  4 +--
 linux-user/signal.c   | 15 +++---
 page_cache.c  |  5 +---
 qga/commands-posix.c  |  4 +--
 qga/commands-win32.c  | 14 ++---
 qobject/qlist.c   |  5 +---
 qom/object.c  |  4 +--
 scripts/coccinelle/error_propagate_null.cocci | 10 +++
 scripts/coccinelle/remove_local_err.cocci | 27 +
 scripts/coccinelle/return_directly.cocci  | 19 
 target-i386/cpu.c |  4 +--
 target-i386/fpu_helper.c  | 10 ++-
 target-i386/kvm.c |  5 ++--
 target-mips/dsp_helper.c  | 15 ++
 target-mips/op_helper.c   |  4 +--
 target-s390x/helper.c |  6 +---
 target-sparc/cc_helper.c  | 25 
 target-tricore/op_helper.c| 13 +++--
 tests/display-vga-test.c  |  6 +---
 tests/endianness-test.c   |  5 +---
 tests/i440fx-test.c   |  4 +--
 tests/intel-hda-test.c|  6 +---
 tests/test-filter-redirector.c|  6 +---
 tests/virtio-blk-test.c   |  5 +---
 tests/virtio-console-test.c   |  6 +---
 tests/virtio-net-test.c   |  6 +---
 tests/virtio-scsi-test.c  |  6 +---
 tests/wdt_ib700-test.c|  6 +---
 ui/cursor.c   | 10 ++-
 ui/qemu-pixman.c  | 11 ++-
 util/module.c |  6 +---
 vl.c  |  5 +---
 62 files changed, 160 insertions(+), 384 deletions(-)
 create mode 100644 scripts/coccinelle/error_propagate_null.cocci
 create mode 100644 scripts/coccinelle/remove_local_err.cocci
 create mode 100644 scripts/coccinelle/return_directly.cocci

-- 
2.5.5

[Qemu-block] [PATCH v3 2/5] block/mirror: Fix target backing BDS

2016-06-10 Thread Max Reitz 
Currently, we are trying to move the backing BDS from the source to the
target in bdrv_replace_in_backing_chain() which is called from
mirror_exit(). However, mirror_complete() already tries to open the
target's backing chain with a call to bdrv_open_backing_file().

First, we should only set the target's backing BDS once. Second, the
mirroring block job has a better idea of what to set it to than the
generic code in bdrv_replace_in_backing_chain() (in fact, the latter's
conditions on when to move the backing BDS from source to target are not
really correct).

Therefore, remove that code from bdrv_replace_in_backing_chain() and
leave it to mirror_complete().

Depending on what kind of mirroring is performed, we furthermore want to
use different strategies to open the target's backing chain:

- If blockdev-mirror is used, we can assume the user made sure that the
  target already has the correct backing chain. In particular, we should
  not try to open a backing file if the target does not have any yet.

- If drive-mirror with mode=absolute-paths is used, we can and should
  reuse the already existing chain of nodes that the source BDS is in.
  In case of sync=full, no backing BDS is required; with sync=top, we
  just link the source's backing BDS to the target, and with sync=none,
  we use the source BDS as the target's backing BDS.
  We should not try to open these backing files anew because this would
  lead to two BDSs existing per physical file in the backing chain, and
  we would like to avoid such concurrent access.

- If drive-mirror with mode=existing is used, we have to use the
  information provided in the physical image file which means opening
  the target's backing chain completely anew, just as it has been done
  already.
  If the target's backing chain shares images with the source, this may
  lead to multiple BDSs per physical image file. But since we cannot
  reliably ascertain this case, there is nothing we can do about it.

Signed-off-by: Max Reitz 
---
 block.c   |  8 
 block/mirror.c| 39 ---
 blockdev.c| 15 ---
 include/block/block_int.h | 18 +-
 4 files changed, 57 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index dc76c159..2691c2f 100644
--- a/block.c
+++ b/block.c
@@ -2289,14 +2289,6 @@ void bdrv_replace_in_backing_chain(BlockDriverState 
*old, BlockDriverState *new)
 
 change_parent_backing_link(old, new);
 
-/* Change backing files if a previously independent node is added to the
- * chain. For active commit, we replace top by its own (indirect) backing
- * file and don't do anything here so we don't build a loop. */
-if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) {
-bdrv_set_backing_hd(new, backing_bs(old));
-bdrv_set_backing_hd(old, NULL);
-}
-
 bdrv_unref(old);
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..13abe8c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -44,6 +44,7 @@ typedef struct MirrorBlockJob {
 /* Used to block operations on the drive-mirror-replace target */
 Error *replace_blocker;
 bool is_none_mode;
+BlockMirrorBackingMode backing_mode;
 BlockdevOnError on_source_error, on_target_error;
 bool synced;
 bool should_complete;
@@ -742,20 +743,26 @@ static void mirror_set_speed(BlockJob *job, int64_t 
speed, Error **errp)
 static void mirror_complete(BlockJob *job, Error **errp)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-Error *local_err = NULL;
-int ret;
+BlockDriverState *src, *target;
+
+src = blk_bs(job->blk);
+target = blk_bs(s->target);
 
-ret = bdrv_open_backing_file(blk_bs(s->target), NULL, "backing",
- _err);
-if (ret < 0) {
-error_propagate(errp, local_err);
-return;
-}
 if (!s->synced) {
 error_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id);
 return;
 }
 
+if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
+int ret;
+
+assert(!target->backing);
+ret = bdrv_open_backing_file(target, NULL, "backing", errp);
+if (ret < 0) {
+return;
+}
+}
+
 /* check the target bs is not blocked and block all operations on it */
 if (s->replaces) {
 AioContext *replace_aio_context;
@@ -777,6 +784,13 @@ static void mirror_complete(BlockJob *job, Error **errp)
 aio_context_release(replace_aio_context);
 }
 
+if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+BlockDriverState *backing = s->is_none_mode ? src : s->base;
+if (backing_bs(target) != backing) {
+bdrv_set_backing_hd(target, backing);
+}
+}
+
 s->should_complete = true;
 block_job_enter(>common);
 }
@@ -799,6 +813,7 @@ static void mirror_start_job(BlockDriverState *bs,

[Qemu-block] [PATCH v3 5/5] iotests: Add test for oVirt-like storage migration

2016-06-10 Thread Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/156 | 174 +
 tests/qemu-iotests/156.out |  48 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 223 insertions(+)
 create mode 100755 tests/qemu-iotests/156
 create mode 100644 tests/qemu-iotests/156.out

diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
new file mode 100755
index 000..cc95ff1
--- /dev/null
+++ b/tests/qemu-iotests/156
@@ -0,0 +1,174 @@
+#!/bin/bash
+#
+# Tests oVirt-like storage migration:
+#  - Create snapshot
+#  - Create target image with (not yet existing) target backing chain
+#(i.e. just write the name of a soon-to-be-copied-over backing file into 
it)
+#  - drive-mirror the snapshot to the target with mode=existing and sync=top
+#  - In the meantime, copy the original source files to the destination via
+#conventional means (i.e. outside of qemu)
+#  - Complete the drive-mirror job
+#  - Delete all source images
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1   # failure is the default!
+
+_cleanup()
+{
+rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2 qed
+_supported_proto generic
+_supported_os Linux
+
+# Create source disk
+TEST_IMG="$TEST_IMG.backing" _make_test_img 1M
+_make_test_img -b "$TEST_IMG.backing" 1M
+
+$QEMU_IO -c 'write -P 1 0 256k' "$TEST_IMG.backing" | _filter_qemu_io
+$QEMU_IO -c 'write -P 2 64k 192k' "$TEST_IMG" | _filter_qemu_io
+
+_launch_qemu -drive if=none,id=source,file="$TEST_IMG"
+
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'qmp_capabilities' }" \
+'return'
+
+# Create snapshot
+TEST_IMG="$TEST_IMG.overlay" _make_test_img -b "$TEST_IMG" 1M
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'blockdev-snapshot-sync',
+   'arguments': { 'device': 'source',
+  'snapshot-file': '$TEST_IMG.overlay',
+  'format': '$IMGFMT',
+  'mode': 'existing' } }" \
+'return'
+
+# Write something to the snapshot
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io source \"write -P 3 128k 128k\"' } }" \
+'return'
+
+# Create target image
+TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
+
+# Mirror snapshot
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'drive-mirror',
+   'arguments': { 'device': 'source',
+  'target': '$TEST_IMG.target.overlay',
+  'mode': 'existing',
+  'sync': 'top' } }" \
+'return'
+
+# Wait for convergence
+_send_qemu_cmd $QEMU_HANDLE \
+'' \
+'BLOCK_JOB_READY'
+
+# Write some more
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io source \"write -P 4 192k 64k\"' } }" \
+'return'
+
+# Copy source backing chain to the target before completing the job
+cp "$TEST_IMG.backing" "$TEST_IMG.target.backing"
+cp "$TEST_IMG" "$TEST_IMG.target"
+$QEMU_IMG rebase -u -b "$TEST_IMG.target.backing" "$TEST_IMG.target"
+
+# Complete block job
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'block-job-complete',
+   'arguments': { 'device': 'source' } }" \
+''
+
+_send_qemu_cmd $QEMU_HANDLE \
+'' \
+'BLOCK_JOB_COMPLETED'
+
+# Remove the source images
+rm -f "$TEST_IMG{,.backing,.overlay}"
+
+echo
+
+# Check online disk contents
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io source \"read -P 1 0k 64k\"' } }" \
+'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io source \"read -P 2 64k 64k\"' } }" \
+'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+"{ 'execute': 'human-monitor-command',
+   'arguments': { 'command-line':
+  'qemu-io source \"read -P 3 128k 64k\"' } }" \
+

[Qemu-block] [PATCH v3 3/5] block/null: Implement bdrv_refresh_filename()

2016-06-10 Thread Max Reitz 
Signed-off-by: Max Reitz 
---
 block/null.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/block/null.c b/block/null.c
index 396500b..b511010 100644
--- a/block/null.c
+++ b/block/null.c
@@ -12,6 +12,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
 #include "block/block_int.h"
 
 #define NULL_OPT_LATENCY "latency-ns"
@@ -223,6 +225,20 @@ static int64_t coroutine_fn 
null_co_get_block_status(BlockDriverState *bs,
 }
 }
 
+static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+QINCREF(opts);
+qdict_del(opts, "filename");
+
+if (!qdict_size(opts)) {
+snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+ bs->drv->format_name);
+}
+
+qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
+bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_null_co = {
 .format_name= "null-co",
 .protocol_name  = "null-co",
@@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
 .bdrv_reopen_prepare= null_reopen_prepare,
 
 .bdrv_co_get_block_status   = null_co_get_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static BlockDriver bdrv_null_aio = {
@@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
 .bdrv_reopen_prepare= null_reopen_prepare,
 
 .bdrv_co_get_block_status   = null_co_get_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static void bdrv_null_init(void)
-- 
2.8.3

[Qemu-block] [PATCH v3 1/5] block: Allow replacement of a BDS by its overlay

2016-06-10 Thread Max Reitz 
change_parent_backing_link() asserts that the BDS to be replaced is not
used as a backing file. However, we may want to replace a BDS by its
overlay in which case that very link should not be redirected.

For instance, when doing a sync=none drive-mirror operation, we may have
the following BDS/BB forest before block job completion:

  target

  base <- source <- BlockBackend

During job completion, we want to establish the source BDS as the
target's backing node:

  target
|
v
  base <- source <- BlockBackend

This makes the target a valid replacement for the source:

  target <- BlockBackend
|
v
  base <- source

Without this modification to change_parent_backing_link() we have to
inject the target into the graph before the source is its backing node,
thus temporarily creating a wrong graph:

  target <- BlockBackend

  base <- source

Signed-off-by: Max Reitz 
---
 block.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index f54bc25..dc76c159 100644
--- a/block.c
+++ b/block.c
@@ -2224,9 +2224,23 @@ void bdrv_close_all(void)
 static void change_parent_backing_link(BlockDriverState *from,
BlockDriverState *to)
 {
-BdrvChild *c, *next;
+BdrvChild *c, *next, *to_c;
 
 QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
+if (c->role == _backing) {
+/* @from is generally not allowed to be a backing file, except for
+ * when @to is the overlay. In that case, @from may not be replaced
+ * by @to as @to's backing node. */
+QLIST_FOREACH(to_c, >children, next) {
+if (to_c == c) {
+break;
+}
+}
+if (to_c) {
+continue;
+}
+}
+
 assert(c->role != _backing);
 bdrv_ref(to);
 bdrv_replace_child(c, to);
-- 
2.8.3

[Qemu-block] [PATCH v3 4/5] iotests: Add test for post-mirror backing chains

2016-06-10 Thread Max Reitz 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/155 | 263 +
 tests/qemu-iotests/155.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 269 insertions(+)
 create mode 100755 tests/qemu-iotests/155
 create mode 100644 tests/qemu-iotests/155.out

diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
new file mode 100755
index 000..06ddc5f
--- /dev/null
+++ b/tests/qemu-iotests/155
@@ -0,0 +1,263 @@
+#!/usr/bin/env python
+#
+# Test whether the backing BDSs are correct after completion of a
+# mirror block job; in "existing" modes (drive-mirror with
+# mode=existing and blockdev-mirror) the backing chain should not be
+# overridden.
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import stat
+import time
+import iotests
+from iotests import qemu_img
+
+back0_img = os.path.join(iotests.test_dir, 'back0.' + iotests.imgfmt)
+back1_img = os.path.join(iotests.test_dir, 'back1.' + iotests.imgfmt)
+back2_img = os.path.join(iotests.test_dir, 'back2.' + iotests.imgfmt)
+source_img = os.path.join(iotests.test_dir, 'source.' + iotests.imgfmt)
+target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)
+
+
+# Class variables for controlling its behavior:
+#
+# existing: If True, explicitly create the target image and blockdev-add it
+# target_backing: If existing is True: Use this filename as the backing file
+# of the target image
+# (None: no backing file)
+# target_blockdev_backing: If existing is True: Pass this dict as "backing"
+#  for the blockdev-add command
+#  (None: do not pass "backing")
+# target_real_backing: If existing is True: The real filename of the backing
+#  image during runtime, only makes sense if
+#  target_blockdev_backing is not None
+#  (None: same as target_backing)
+
+class BaseClass(iotests.QMPTestCase):
+target_blockdev_backing = None
+target_real_backing = None
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, back0_img, '1M')
+qemu_img('create', '-f', iotests.imgfmt, '-b', back0_img, back1_img)
+qemu_img('create', '-f', iotests.imgfmt, '-b', back1_img, back2_img)
+qemu_img('create', '-f', iotests.imgfmt, '-b', back2_img, source_img)
+
+self.vm = iotests.VM()
+self.vm.add_drive(None, '', 'none')
+self.vm.launch()
+
+# Add the BDS via blockdev-add so it stays around after the mirror 
block
+# job has been completed
+result = self.vm.qmp('blockdev-add',
+ options={'node-name': 'source',
+  'driver': iotests.imgfmt,
+  'file': {'driver': 'file',
+   'filename': source_img}})
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('x-blockdev-insert-medium',
+ device='drive0', node_name='source')
+self.assert_qmp(result, 'return', {})
+
+self.assertIntactSourceBackingChain()
+
+if self.existing:
+if self.target_backing:
+qemu_img('create', '-f', iotests.imgfmt,
+ '-b', self.target_backing, target_img, '1M')
+else:
+qemu_img('create', '-f', iotests.imgfmt, target_img, '1M')
+
+if self.cmd == 'blockdev-mirror':
+options = { 'node-name': 'target',
+'driver': iotests.imgfmt,
+'file': { 'driver': 'file',
+  'filename': target_img } }
+if self.target_blockdev_backing:
+options['backing'] = self.target_blockdev_backing
+
+result = self.vm.qmp('blockdev-add', options=options)
+self.assert_qmp(result, 'return', {})
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(source_img)
+os.remove(back2_img)
+os.remove(back1_img)
+os.remove(back0_img)
+try:
+os.remove(target_img)
+except OSError:
+pass
+
+def findBlockNode(self, node_name, id=None):
+

[Qemu-block] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

2016-06-10 Thread Max Reitz 
Issue #1: If the target image does not have a backing BDS before mirror
completion, qemu tries really hard to give it a backing BDS. If the
source has a backing BDS, it will actually always "succeed".
In some cases, the target is not supposed to have a backing BDS, though
(absolute-paths: because of sync=full; existing: because the target
image does not have a backing file; blockdev-mirror: because of an
explicit "backing": ""). Then, this is pretty bad behavior.

This should generally not change the target's visible data, but it still
is ugly.

Issue #2: Currently the backing chain of the target is basically opened
using bdrv_open_backing_file() (except for sometimes™). This results in
multiple BDSs for a single physical file, which is bad. In most use
cases, this is only temporary, but it still is bad.

If we can reuse the existing backing chain of the source (which is with
drive-mirror in "absolute-paths" mode), we should just do so.


v3:
- Patch 1:
  - More verbose commit message [Kevin]
  - Changed comment to match code [Kevin]
- Patch 2:
  - Do not force use of the source backing chain for the target in
"existing" mode or with blockdev-mirror [Kevin]
- Instead keep doing what we've been doing for
  drive-mirror/existing, only that we should still drop the
  bdrv_set_backing_hd() in bdrv_replace_in_backing_chain()
- And for blockdev-mirror, just do not change the current backing
  chain at all; this is what we've been doing until now, unless the
  target BDS did not have a backing BDS yet
- Patch 3: Added, because it makes the next test a bit nicer
- Patch 4: Adjusted to v3 behavior, and added a new test for
  blockdev-mirror with a target whose backing file has been overridden
  using the "backing" option
- Patch 5: Added [Kevin]


git-backport-diff against v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0005] [FC] 'block: Allow replacement of a BDS by its overlay'
002/5:[0057] [FC] 'block/mirror: Fix target backing BDS'
003/5:[down] 'block/null: Implement bdrv_refresh_filename()'
004/5:[0073] [FC] 'iotests: Add test for post-mirror backing chains'
005/5:[down] 'iotests: Add test for oVirt-like storage migration'


Max Reitz (5):
  block: Allow replacement of a BDS by its overlay
  block/mirror: Fix target backing BDS
  block/null: Implement bdrv_refresh_filename()
  iotests: Add test for post-mirror backing chains
  iotests: Add test for oVirt-like storage migration

 block.c|  24 +++--
 block/mirror.c |  39 +--
 block/null.c   |  20 
 blockdev.c |  15 ++-
 include/block/block_int.h  |  18 +++-
 tests/qemu-iotests/155 | 263 +
 tests/qemu-iotests/155.out |   5 +
 tests/qemu-iotests/156 | 174 ++
 tests/qemu-iotests/156.out |  48 +
 tests/qemu-iotests/group   |   2 +
 10 files changed, 584 insertions(+), 24 deletions(-)
 create mode 100755 tests/qemu-iotests/155
 create mode 100644 tests/qemu-iotests/155.out
 create mode 100755 tests/qemu-iotests/156
 create mode 100644 tests/qemu-iotests/156.out

-- 
2.8.3

[Qemu-block] [PATCH 5/6] qcow2: Let vmstate call qcow2_co_preadv/pwrite directly

2016-06-10 Thread Kevin Wolf 
We don't really want to go through the block layer in order to read from
or write to the vmstate in a qcow2 image. Doing so required a few ugly
hacks like saving and restoring the old image size (because writing to
vmstate offsets would increase the image size) or disabling the "reads
after EOF = zeroes" logic. When calling the right functions directly,
these hacks aren't necessary any more.

Note that .bdrv_vmstate_load/save() return 0 instead of the number of
bytes in case of success now.

Signed-off-by: Kevin Wolf 
---
 block/qcow2.c | 24 
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 72ae2bf..c40baca 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2903,36 +2903,20 @@ static int qcow2_save_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov,
   int64_t pos)
 {
 BDRVQcow2State *s = bs->opaque;
-int64_t total_sectors = bs->total_sectors;
-bool zero_beyond_eof = bs->zero_beyond_eof;
-int ret;
 
 BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_SAVE);
-bs->zero_beyond_eof = false;
-ret = bdrv_pwritev(bs, qcow2_vm_state_offset(s) + pos, qiov);
-bs->zero_beyond_eof = zero_beyond_eof;
-
-/* bdrv_co_do_writev will have increased the total_sectors value to include
- * the VM state - the VM state is however not an actual part of the block
- * device, therefore, we need to restore the old value. */
-bs->total_sectors = total_sectors;
-
-return ret;
+return bs->drv->bdrv_co_pwritev(bs, qcow2_vm_state_offset(s) + pos,
+qiov->size, qiov, 0);
 }
 
 static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
   int64_t pos)
 {
 BDRVQcow2State *s = bs->opaque;
-bool zero_beyond_eof = bs->zero_beyond_eof;
-int ret;
 
 BLKDBG_EVENT(bs->file, BLKDBG_VMSTATE_LOAD);
-bs->zero_beyond_eof = false;
-ret = bdrv_preadv(bs, qcow2_vm_state_offset(s) + pos, qiov);
-bs->zero_beyond_eof = zero_beyond_eof;
-
-return ret;
+return bs->drv->bdrv_co_preadv(bs, qcow2_vm_state_offset(s) + pos,
+   qiov->size, qiov, 0);
 }
 
 /*
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH V3] block/iscsi: allow caching of the allocation map

2016-06-10 Thread Peter Lieven 
Am 30.05.2016 um 08:33 schrieb Peter Lieven:
> Am 25.05.2016 um 01:10 schrieb Eric Blake:
>> On 05/24/2016 02:40 AM, Peter Lieven wrote:
>>> until now the allocation map was used only as a hint if a cluster
>>> is allocated or not. If a block was not allocated (or Qemu had
>>> no info about the allocation status) a get_block_status call was
>>> issued to check the allocation status and possibly avoid
>>> a subsequent read of unallocated sectors. If a block known to be
>>> allocated the get_block_status call was omitted. In the other case
>>> a get_block_status call was issued before every read to avoid
>>> the necessity for a consistent allocation map. To avoid the
>>> potential overhead of calling get_block_status for each and
>>> every read request this took only place for the bigger requests.
>>>
>>> This patch enhances this mechanism to cache the allocation
>>> status and avoid calling get_block_status for blocks where
>>> the allocation status has been queried before. This allows
>>> for bypassing the read request even for smaller requests and
>>> additionally omits calling get_block_status for known to be
>>> unallocated blocks.
>>>
>>> Signed-off-by: Peter Lieven 
>>> ---
>>> +static int iscsi_allocmap_init(IscsiLun *iscsilun, int open_flags)
>>>   {
>>> -if (iscsilun->allocationmap == NULL) {
>>> -return;
>>> +iscsi_allocmap_free(iscsilun);
>>> +
>>> +iscsilun->allocmap_size =
>>> +DIV_ROUND_UP(sector_lun2qemu(iscsilun->num_blocks, iscsilun),
>>> + iscsilun->cluster_sectors);
>>> +
>> Computes: ceiling( (num_blocks * block_size / 512) / (cluster_size /
>> 512) ); aka number of clusters.  But we don't independently track the
>> cluster size, so I don't see any simpler way of writing it, even if we
>> could be more efficient by not having to first scale through qemu's
>> sector size.
>>
>>> +iscsilun->allocmap = bitmap_try_new(iscsilun->allocmap_size);
>>> +if (!iscsilun->allocmap) {
>>> +return -ENOMEM;
>>> +}
>>> +
>>> +if (open_flags & BDRV_O_NOCACHE) {
>>> +/* in case that cache.direct = on all allocmap entries are
>>> + * treated as invalid to force a relookup of the block
>>> + * status on every read request */
>>> +return 0;
>> Can we cache that we are opened BDRV_O_NOCACHE, so that we don't even
>> have to bother allocating allocmap when we know we are never changing
>> its bits?  In other words, can you swap this to be before the
>> bitmap_try_new()?
>
> The idea of the allocmap in cache.direct = on mode is that we can
> still speed up block jobs by skipping large unallocated areas. In this case
> the allocmap has only a hint character. If we don't know the status
> we issue a get_block_status request and verify the status. If its unallocated
> we return zeroes. If we new through an earlier get block status request
> that the area is allocated we can skip the useless get_block_status request.
> This is the old behaviour without this patch.
>
>>
>>> +}
>>> +
>>> +iscsilun->allocmap_valid = bitmap_try_new(iscsilun->allocmap_size);
>>> +if (!iscsilun->allocmap_valid) {
>>> +/* if we are under memory pressure free the allocmap as well */
>>> +iscsi_allocmap_free(iscsilun);
>>> +return -ENOMEM;
>>>   }
>>> -bitmap_set(iscsilun->allocationmap,
>>> -   sector_num / iscsilun->cluster_sectors,
>>> -   DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
>>> +
>>> +return 0;
>>>   }
>>>   -static void iscsi_allocationmap_clear(IscsiLun *iscsilun, int64_t 
>>> sector_num,
>>> -  int nb_sectors)
>>> +static void
>>> +iscsi_allocmap_update(IscsiLun *iscsilun, int64_t sector_num,
>>> +  int nb_sectors, bool allocated, bool valid)
>>>   {
>>>   int64_t cluster_num, nb_clusters;
>>> -if (iscsilun->allocationmap == NULL) {
>>> +
>>> +if (iscsilun->allocmap == NULL) {
>>>   return;
>>>   }
>> Here, you are short-circuiting when there is no allocmap, but shouldn't
>> you also short-circuit if you are BDRV_O_NOCACHE?
>>
>> Should you assert(!(allocated && !valid)) [or by deMorgan's,
>> assert(!allocated || valid)], to make sure we are only tracking 3 states
>> rather than 4?
>
> sure, we thats a good enhancement altough allocated and invalid doesn't
> hurt.
>
>>
>>>   cluster_num = DIV_ROUND_UP(sector_num, iscsilun->cluster_sectors);
>>>   nb_clusters = (sector_num + nb_sectors) / iscsilun->cluster_sectors
>>> - cluster_num;
>>> -if (nb_clusters > 0) {
>>> -bitmap_clear(iscsilun->allocationmap, cluster_num, nb_clusters);
>>> +if (allocated) {
>>> +bitmap_set(iscsilun->allocmap,
>>> +   sector_num / iscsilun->cluster_sectors,
>>> +   DIV_ROUND_UP(nb_sectors, iscsilun->cluster_sectors));
>> This says that if we have a sub-cluster request, we can round out to
>>