Re: [Qemu-block] [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

2018-02-12 Thread Paolo Bonzini 
On 12/02/2018 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 13:09, Paolo Bonzini wrote:>> We have three cases:
>>
>> 1) monitor creates and destroy bitmaps.
>>
>> 2) monitor also has to read the list.  We know it operates with BQL.
>>
>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>> (under BQL), but they can operate without BQL in a separate iothread so
>> we create a separate lock (bitmap->mutex).
>>
>> While in the second and third case, bitmaps cannot disappear.  So in the
>> first case you operate with BQL+dirty bitmap mutex.  The result is that
>> you lock out both the second and the third case while creating and
>> destroying bitmaps.
>>
>>> Why do we do not need them
>>> on read from the bitmap, only on write?
>>
>> Indeed, reading the bitmap also requires taking the lock.  So
>> s/Modifying/Accessing/ in that comment.
> 
> So, finally, the whole thing is:
> 
> 1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
> 2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex

3. any access to a dirty bitmap needs dirty_bitmap_mutex

Paolo

> yes?
> 
> and one more question:
> Do we really have users, which accesses dirty bitmaps with only BQL?
> query-block uses dirty_bitmap_mutex..
> 
>

[Qemu-block] [PATCH v2] block/nvme: fix Coverity reports

2018-02-12 Thread Fam Zheng 
From: Paolo Bonzini 

1) string not null terminated in sysfs_find_group_file

2) NULL pointer dereference and dead local variable in nvme_init.

Signed-off-by: Paolo Bonzini 
Signed-off-by: Fam Zheng 

---

v2: Fix error path.
---
 block/nvme.c| 10 +++---
 util/vfio-helpers.c |  2 +-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index e9d0e218fc..a62c92a190 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -644,7 +644,7 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
false, nvme_handle_event, nvme_poll_cb);
 
-nvme_identify(bs, namespace, errp);
+nvme_identify(bs, namespace, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 ret = -EIO;
@@ -665,8 +665,12 @@ fail_queue:
 nvme_free_queue_pair(bs, s->queues[0]);
 fail:
 g_free(s->queues);
-qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
-qemu_vfio_close(s->vfio);
+if (s->regs) {
+qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
+}
+if (s->vfio) {
+qemu_vfio_close(s->vfio);
+}
 event_notifier_cleanup(>irq_notifier);
 return ret;
 }
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index f478b68400..006674c916 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -104,7 +104,7 @@ static char *sysfs_find_group_file(const char *device, 
Error **errp)
 char *path = NULL;
 
 sysfs_link = g_strdup_printf("/sys/bus/pci/devices/%s/iommu_group", 
device);
-sysfs_group = g_malloc(PATH_MAX);
+sysfs_group = g_malloc0(PATH_MAX);
 if (readlink(sysfs_link, sysfs_group, PATH_MAX - 1) == -1) {
 error_setg_errno(errp, errno, "Failed to find iommu group sysfs path");
 goto out;
-- 
2.14.3

Re: [Qemu-block] [Qemu-devel] [PATCH v3] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

2018-02-12 Thread Eric Blake 

On 02/12/2018 12:48 PM, Daniel P. Berrangé wrote:

From: "Daniel P. Berrange" 

qemu-io puts the TTY into non-canonical mode, which means no EOF processing is
done and thus getchar() will never return the EOF constant. Instead we have to
query the TTY attributes to determine the configured EOF character (usually
Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
regression that prevented Ctrl-D from triggering an exit of qemu-io that has
existed since readline was first added in

   commit 0cf17e181798063c3824c8200ba46f25f54faa1a
   Author: Stefan Hajnoczi 
   Date:   Thu Nov 14 11:54:17 2013 +0100

 qemu-io: use readline.c

It also ensures that a newline is printed when exiting, to complete the
line output by the "qemu-io> " prompt.

Signed-off-by: Daniel P. Berrange 
---


Reviewed-by: Eric Blake 

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

Re: [Qemu-block] [PATCH v3] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

2018-02-12 Thread Kevin Wolf 
Am 12.02.2018 um 19:48 hat Daniel P. Berrangé geschrieben:
> From: "Daniel P. Berrange" 
> 
> qemu-io puts the TTY into non-canonical mode, which means no EOF processing is
> done and thus getchar() will never return the EOF constant. Instead we have to
> query the TTY attributes to determine the configured EOF character (usually
> Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
> regression that prevented Ctrl-D from triggering an exit of qemu-io that has
> existed since readline was first added in
> 
>   commit 0cf17e181798063c3824c8200ba46f25f54faa1a
>   Author: Stefan Hajnoczi 
>   Date:   Thu Nov 14 11:54:17 2013 +0100
> 
> qemu-io: use readline.c
> 
> It also ensures that a newline is printed when exiting, to complete the
> line output by the "qemu-io> " prompt.
> 
> Signed-off-by: Daniel P. Berrange 

Thanks, applied to the block branch.

Kevin

[Qemu-block] [PATCH v3] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

2018-02-12 Thread Daniel P . Berrangé 
From: "Daniel P. Berrange" 

qemu-io puts the TTY into non-canonical mode, which means no EOF processing is
done and thus getchar() will never return the EOF constant. Instead we have to
query the TTY attributes to determine the configured EOF character (usually
Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
regression that prevented Ctrl-D from triggering an exit of qemu-io that has
existed since readline was first added in

  commit 0cf17e181798063c3824c8200ba46f25f54faa1a
  Author: Stefan Hajnoczi 
  Date:   Thu Nov 14 11:54:17 2013 +0100

qemu-io: use readline.c

It also ensures that a newline is printed when exiting, to complete the
line output by the "qemu-io> " prompt.

Signed-off-by: Daniel P. Berrange 
---

Changed in v3:

 - print newline when exiting

 qemu-io.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index f554ab614b..2c00ea068e 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -11,6 +11,9 @@
 #include "qemu/osdep.h"
 #include 
 #include 
+#ifndef _WIN32
+#include 
+#endif
 
 #include "qapi/error.h"
 #include "qemu-io.h"
@@ -42,6 +45,26 @@ static bool imageOpts;
 
 static ReadLineState *readline_state;
 
+static int ttyEOF;
+
+static int get_eof_char(void)
+{
+#ifdef _WIN32
+return 0x4; /* Ctrl-D */
+#else
+struct termios tty;
+if (tcgetattr(STDIN_FILENO, ) != 0) {
+if (errno == ENOTTY) {
+return 0x0; /* just expect read() == 0 */
+} else {
+return 0x4; /* Ctrl-D */
+}
+}
+
+return tty.c_cc[VEOF];
+#endif
+}
+
 static int close_f(BlockBackend *blk, int argc, char **argv)
 {
 blk_unref(qemuio_blk);
@@ -323,7 +346,8 @@ static char *fetchline_readline(void)
 readline_start(readline_state, get_prompt(), 0, readline_func, );
 while (!line) {
 int ch = getchar();
-if (ch == EOF) {
+if (ttyEOF != 0x0 && ch == ttyEOF) {
+printf("\n");
 break;
 }
 readline_handle_byte(readline_state, ch);
@@ -593,6 +617,7 @@ int main(int argc, char **argv)
 qemuio_add_command(_cmd);
 
 if (isatty(STDIN_FILENO)) {
+ttyEOF = get_eof_char();
 readline_state = readline_init(readline_printf_func,
readline_flush_func,
NULL,
-- 
2.14.3

Re: [Qemu-block] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

2018-02-12 Thread Kevin Wolf 
Am 08.12.2017 um 14:34 hat Daniel P. Berrange geschrieben:
> qemu-io puts the TTY into non-canonical mode, which means no EOF processing is
> done and thus getchar() will never return the EOF constant. Instead we have to
> query the TTY attributes to determine the configured EOF character (usually
> Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
> regression that prevented Ctrl-D from triggering an exit of qemu-io that has
> existed since readline was first added in
> 
>   commit 0cf17e181798063c3824c8200ba46f25f54faa1a
>   Author: Stefan Hajnoczi 
>   Date:   Thu Nov 14 11:54:17 2013 +0100
> 
> qemu-io: use readline.c
> 
> Signed-off-by: Daniel P. Berrange 

Thanks, applied to the block branch.

It would be nice if we also printed a newline when Ctrl-D is pressed. If
you want to send a v3 for this, I'll just update the patch in my queue.

Kevin

Re: [Qemu-block] [PATCH 27/27] qemu-iotests: Test ssh image creation over QMP

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/207 | 261 
> +
>  tests/qemu-iotests/207.out |  75 +
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 337 insertions(+)
>  create mode 100755 tests/qemu-iotests/207
>  create mode 100644 tests/qemu-iotests/207.out

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 26/27] qemu-iotests: Test qcow2 over file image creation with QMP

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/206 | 436 
> +
>  tests/qemu-iotests/206.out | 209 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 646 insertions(+)
>  create mode 100755 tests/qemu-iotests/206
>  create mode 100644 tests/qemu-iotests/206.out

Reviewed-by: Max Reitz 

> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> new file mode 100755
> index 00..0a18b2b19a
> --- /dev/null
> +++ b/tests/qemu-iotests/206
> @@ -0,0 +1,436 @@

[...]

> +# creator
> +owner=kw...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1 # failure is the default!

Hmmm...  Didn't we want to remove this boilerplate at some point?

> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +

[...]

> +echo
> +echo "=== Invalid sizes ==="
> +echo
> +
> +# TODO Negative image sizes aren't handled correctly, but this is a problem
> +# with QAPI's implementation of the 'size' type and affects other commands as
> +# well. Once this is fixed, we may want to add a test case here.
> +
> +# 1. Misaligned image size
> +# 2. 2^64 - 512
> +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
> +# 4. 2^63 - 512 (generally valid, but qcow2 can't handle images this size)
> +
> +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 < +{ "execute": "qmp_capabilities" }
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +  "driver": "$IMGFMT",
> +  "file": "node0",
> +  "size": 1234
> +  }
> +}
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +  "driver": "$IMGFMT",
> +  "file": "node0",
> +  "size": 18446744073709551104

I was about to propose $((2**64 - 512)), but then I noticed that yields
-512.  Nice.

> +  }
> +}
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +  "driver": "$IMGFMT",
> +  "file": "node0",
> +  "size": 9223372036854775808
> +  }
> +}
> +{ "execute": "x-blockdev-create",
> +  "arguments": {
> +  "driver": "$IMGFMT",
> +  "file": "node0",
> +  "size": 9223372036854775296
> +  }
> +}
> +{ "execute": "quit" }
> +EOF



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2] qemu-io: fix EOF Ctrl-D handling in qemu-io readline code

2018-02-12 Thread Daniel P . Berrangé 
Re-ping.

On Thu, Jan 25, 2018 at 05:05:01PM +, Daniel P. Berrangé wrote:
> ping, does any block maintainer want to queue this one ?
> 
> On Fri, Dec 08, 2017 at 01:34:16PM +, Daniel P. Berrange wrote:
> > qemu-io puts the TTY into non-canonical mode, which means no EOF processing 
> > is
> > done and thus getchar() will never return the EOF constant. Instead we have 
> > to
> > query the TTY attributes to determine the configured EOF character (usually
> > Ctrl-D / 0x4), and then explicitly check for that value. This fixes the
> > regression that prevented Ctrl-D from triggering an exit of qemu-io that has
> > existed since readline was first added in
> > 
> >   commit 0cf17e181798063c3824c8200ba46f25f54faa1a
> >   Author: Stefan Hajnoczi 
> >   Date:   Thu Nov 14 11:54:17 2013 +0100
> > 
> > qemu-io: use readline.c
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> > Changed in v2:
> > 
> >   - Query termios settings for EOF character
> > 
> >  qemu-io.c | 26 +-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qemu-io.c b/qemu-io.c
> > index c70bde3eb1..fa4972d734 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -10,6 +10,9 @@
> >  #include "qemu/osdep.h"
> >  #include 
> >  #include 
> > +#ifndef _WIN32
> > +#include 
> > +#endif
> >  
> >  #include "qapi/error.h"
> >  #include "qemu-io.h"
> > @@ -41,6 +44,26 @@ static bool imageOpts;
> >  
> >  static ReadLineState *readline_state;
> >  
> > +static int ttyEOF;
> > +
> > +static int get_eof_char(void)
> > +{
> > +#ifdef _WIN32
> > +return 0x4; /* Ctrl-D */
> > +#else
> > +struct termios tty;
> > +if (tcgetattr(STDIN_FILENO, ) != 0) {
> > +if (errno == ENOTTY) {
> > +return 0x0; /* just expect read() == 0 */
> > +} else {
> > +return 0x4; /* Ctrl-D */
> > +}
> > +}
> > +
> > +return tty.c_cc[VEOF];
> > +#endif
> > +}
> > +
> >  static int close_f(BlockBackend *blk, int argc, char **argv)
> >  {
> >  blk_unref(qemuio_blk);
> > @@ -322,7 +345,7 @@ static char *fetchline_readline(void)
> >  readline_start(readline_state, get_prompt(), 0, readline_func, );
> >  while (!line) {
> >  int ch = getchar();
> > -if (ch == EOF) {
> > +if (ttyEOF != 0x0 && ch == ttyEOF) {
> >  break;
> >  }
> >  readline_handle_byte(readline_state, ch);
> > @@ -592,6 +615,7 @@ int main(int argc, char **argv)
> >  qemuio_add_command(_cmd);
> >  
> >  if (isatty(STDIN_FILENO)) {
> > +ttyEOF = get_eof_char();
> >  readline_state = readline_init(readline_printf_func,
> > readline_flush_func,
> > NULL,
> > -- 
> > 2.14.3
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] Qemu aborted in ide_restart_bh after migration

2018-02-12 Thread John Snow 


On 02/10/2018 03:29 AM, Wang King wrote:
> Empty IDE CD-ROM configured on the VM:
> 
>   
>   
>   
>   
> 
> Make migration for this VM, then qemu aborted in ide_restart_bh. IDEState 
> expect
> end_transfer_func equal to ide_atapi_cmd, but it refer to 
> ide_dummy_transfer_stop.
> I have no idea about this, can anyone help me?
> 

Do you have an easy way to reproduce this? 2.8.1 is a bit old at this
point, but I don't think we've changed anything in the IDE emulator
substantively since then, so I'm curious to see if I can get this to
reproduce.

I'm surprised an empty CD-ROM would cause this, though, since it
shouldn't really have any commands in-flight that might get us to a
weird edge case, so I want to take a close look at this.

Denis Lunev noted some issues with migration that I couldn't solve at
the time either. A reproducer would be fantastic.

> qemu version is 2.8.1
> (gdb) bt
> #0  0x7fcff7c4b157 in raise () from /usr/lib64/libc.so.6
> #1  0x7fcff7c4c848 in abort () from /usr/lib64/libc.so.6
> #2  0x7fcff7c441c6 in __assert_fail_base () from /usr/lib64/libc.so.6
> #3  0x7fcff7c44272 in __assert_fail () from /usr/lib64/libc.so.6
> #4  0x006207ab in ide_restart_bh (opaque=0x38b3430) at 
> hw/ide/core.c:2570
> #5  0x00763a6f in aio_bh_poll (ctx=ctx@entry=0x234f940) at async.c:115
> #6  0x00770948 in aio_dispatch (ctx=0x234f940) at aio_posix.c:303
> #7  0x007638e1 in aio_ctx_dispatch (source=, 
> callback=, user_data=) at async.c:254
> #8  0x7fcff8e6799a in g_main_context_dispatch () from 
> /usr/lib64/libglib-2.0.so.0
> #9  0x0076e606 in glib_pollfds_poll () at main_loop.c:228
> #10 0x0076e6ab in os_host_main_loop_wait (timeout=0) at 
> main_loop.c:273
> #11 0x0076e7d5 in main_loop_wait (nonblocking=nonblocking@entry=0) at 
> main_loop.c:521
> #12 0x0056b911 in main_loop () at vl.c:2089
> #13 0x00420805 in main (argc=, argv=, 
> envp=) at vl.c:4964
> (gdb) f 4
> #4  0x006207ab in ide_restart_bh (opaque=0x38b3430)
> 2570assert(s->end_transfer_func == ide_atapi_cmd);
> (gdb) p *bus
> $7 = {qbus = {obj = {class = 0x2313a30, free = 0x0, properties = 0x3871520, 
> ref = 2, parent = 0x38b2b00}, parent = 0x38b2b00, name = 0x3980af0 "ide.0", 
> hotplug_handler = 0x0, max_index = 1, realized = true,
> children = {tqh_first = 0x349e050, tqh_last = 0x349e060}, sibling = 
> {le_next = 0x0, le_prev = 0x38b3d68}}, master = 0x0, slave = 0x349e3c0, ifs = 
> {{bus = 0x38b3430, unit = 0 '\000', drive_kind = IDE_HD,
>   cylinders = 0, heads = 0, sectors = 0, chs_trans = 0, nb_sectors = 0, 
> mult_sectors = 16, identify_set = 0, identify_data = '\000'  times>, drive_serial = 1,
>   drive_serial_str = '\000' , drive_model_str = '\000' 
> , wwn = 0, feature = 0 '\000', error = 1 '\001', nsector = 
> 0, sector = 0 '\000', lcyl = 96 '`',
>   hcyl = 0 '\000', hob_feature = 0 '\000', hob_nsector = 0 '\000', 
> hob_sector = 0 '\000', hob_lcyl = 0 '\000', hob_hcyl = 0 '\000', select = 160 
> '\240', status = 80 'P', lba48 = 0 '\000', blk = 0x0,
>   version = "\000\000\000\000\000\000\000\000", events = {eject_request = 
> false, new_media = false}, sense_key = 0 '\000', asc = 0 '\000', tray_open = 
> false, tray_locked = false,
>   cdrom_changed = 0 '\000', packet_transfer_size = 0, 
> elementary_transfer_size = 0, io_buffer_index = 0, lba = 0, cd_sector_size = 
> 0, atapi_dma = 0, acct = {bytes = 0, start_time_ns = 0,
> type = BLOCK_ACCT_READ}, pio_aiocb = 0x0, iov = {iov_base = 0x0, 
> iov_len = 0}, qiov = {iov = 0x0, niov = 0, nalloc = 0, size = 0}, 
> buffered_requests = {lh_first = 0x0}, io_buffer_offset = 0,
>   io_buffer_size = 0, sg = {sg = 0x0, nsg = 0, nalloc = 0, size = 0, dev 
> = 0x0, as = 0x0}, req_nb_sectors = 0, end_transfer_func = 0x61b780 
> ,
>   data_ptr = 0x7fcffd126800 "\377\377\377\377", data_end = 0x7fcffd126800 
> "\377\377\377\377", io_buffer = 0x7fcffd126800 "\377\377\377\377", 
> io_buffer_total_len = 131076, cur_io_buffer_offset = 0,
>   cur_io_buffer_len = 0, end_transfer_fn_idx = 0 '\000', 
> sector_write_timer = 0x39e5c60, irq_count = 0, ext_error = 0 '\000', 
> mdata_size = 0, mdata_storage = 0x0, media_changed = 0,
>   dma_cmd = IDE_DMA_READ, smart_enabled = 0 '\000', smart_autosave = 0 
> '\000', smart_errors = 0, smart_selftest_count = 0 '\000', 
> smart_selftest_data = 0x39e6000 "", ncq_queues = 0}, {bus = 0x38b3430,
>   unit = 1 '\001', drive_kind = IDE_CD, cylinders = 0, heads = 0, sectors 
> = 0, chs_trans = 0, nb_sectors = 0, mult_sectors = 16, identify_set = 1,
>   identify_data = "\300\205", '\000' , "MQ 2", ' ' 
> , "\003\000\000\002\004\000.2+5EQUMD DVR-MO", ' ' 
> , "\000\000\001\000\000\003\000\000\000\000\000\000\a", 
> '\000' , 
> "\a\000\a\000\003\000\264\000\264\000,\001\264\000\000\000\000\000\036\000\036",
>  '\000' , "\036", '\000' , "?", '\000' 
> , drive_serial = 2,

Re: [Qemu-block] [PATCH 24/27] file-posix: Fix no-op bdrv_truncate() with falloc preallocation

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> If bdrv_truncate() is called, but the requested size is the same as
> before, don't call posix_fallocate(), which returns -EINVAL for length
> zero and would therefore make bdrv_truncate() fail.
> 
> The problem can be triggered by creating a zero-sized raw image with
> 'falloc' preallocation mode.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/file-posix.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 23/27] ssh: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to ssh, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 16 -
>  block/ssh.c  | 92 
> +---
>  2 files changed, 67 insertions(+), 41 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 22/27] ssh: Pass BlockdevOptionsSsh to connect_to_ssh()

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Move the parsing of the QDict options up to the callers, in preparation
> for the .bdrv_co_create implementation that directly gets a QAPI type.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/ssh.c | 34 +-
>  1 file changed, 21 insertions(+), 13 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

2018-02-12 Thread Vladimir Sementsov-Ogievskiy 

18.01.2018 13:09, Paolo Bonzini wrote:

On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo, could you please explain about bitmap locking in more details?
Why do we need mutexes?

We have three cases:

1) monitor creates and destroy bitmaps.

2) monitor also has to read the list.  We know it operates with BQL.

3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).

While in the second and third case, bitmaps cannot disappear.  So in the
first case you operate with BQL+dirty bitmap mutex.  The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.


Why do we do not need them
on read from the bitmap, only on write?

Indeed, reading the bitmap also requires taking the lock.  So
s/Modifying/Accessing/ in that comment.

Paolo


So, finally, the whole thing is:

1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex

yes?

and one more question:
Do we really have users, which accesses dirty bitmaps with only BQL? 
query-block uses dirty_bitmap_mutex..



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 21/27] ssh: QAPIfy host-key-check option

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This makes the host-key-check option available in blockdev-add.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 63 +++--
>  block/ssh.c  | 88 
> +---
>  2 files changed, 117 insertions(+), 34 deletions(-)

Reviewed-by: Max Reitz 

(And it looks like even with the libssh patch only md5 and sha1 are
available.)



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 20/27] ssh: Use QAPI BlockdevOptionsSsh object

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Create a BlockdevOptionsSsh object in connect_to_ssh() and take the
> options from there. 'host_key_check' is still processed separately
> because it's not in the schema yet.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/ssh.c | 136 
> +++-
>  1 file changed, 61 insertions(+), 75 deletions(-)

You might want to base this patch on Pino's patch to use libssh instead
of libssh2.

Anyway:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

2018-02-12 Thread Anton Nefedov 



On 3/2/2018 6:59 PM, Markus Armbruster wrote:

Eric Blake  writes:


On 01/19/2018 06:50 AM, Anton Nefedov wrote:

+
+##
+# @BlockDriverStats:
+#
+# Statistics of a block driver (driver-specific)
+#
+# Since: 2.12
+##
+{ 'union': 'BlockDriverStats',
+  'data': {
+  'file': 'BlockDriverStatsFile'
+  } }


Markus has been adamant that we add no new "simple unions" (unions with
a 'discriminator' field) - because they are anything but simple in the
long run.


Indeed.  You could make this a flat union, similar to BlockdevOptions:

{ 'union': 'BlockDriverStats':
   'base': { 'driver': 'BlockdevDriver' },
   'discriminator': 'driver',
   'data': {
   'file': 'BlockDriverStatsFile',
   ... } }

However:


+
+##
  # @BlockStats:
  #
  # Statistics of a virtual block device or a block backing device.
@@ -785,6 +819,8 @@
  #
  # @stats:  A @BlockDeviceStats for the device.
  #
+# @driver-stats: Optional driver-specific statistics. (Since 2.12)
+#
  # @parent: This describes the file block device if it has one.
  #  Contains recursively the statistics of the underlying
  #  protocol (e.g. the host file for a qcow2 image). If there is
@@ -798,6 +834,7 @@
  { 'struct': 'BlockStats',
'data': {'*device': 'str', '*node-name': 'str',
 'stats': 'BlockDeviceStats',
+   '*driver-stats': 'BlockDriverStats',


You're adding a union of driver-specific stats to a struct of generic
stats.  That's unnecessarily complicated.  Instead, turn the struct of
generic stats into a flat union, like this:

{ 'union': 'BlockStats',
   'base': { ... the generic stats, i.e. the members of BlockStats
 before this patch ...
 'driver': 'BlockdevDriver' }
   'discriminator': 'driver',
   'data': {
   'file': 'BlockDriverStatsFile',
   ... } }


...[1] You are using it alongside a struct that already uses '-'
(node-name), so you should use dashes.

So, the difference between your proposal (a simple union) and using a
"flat union", on the wire, is yours:

"return": { ..., "driver-stats": { "type": "file", "data": {
"discard_nb_ok: ... } } }

vs. a flat union:

"return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok":
... } }

where you can benefit from less nesting and a saner discriminator name.


My proposal peels off yet another level of nesting.



The output is better indeed, thanks; a little drawback is now we need to
pass the whole BlockStats to the driver so it fills its stats.

e.g. the interface:

void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);

And that BlockdevDriver subset type (or a generator patch) still seems
to be needed

  { 'enum' : 'BlockdevDriverWithStats',
'data' : [ 'file' ] }

Re: [Qemu-block] [PATCH 19/27] sheepdog: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to sheepdog, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  24 +-
>  block/sheepdog.c | 209 
> ---
>  2 files changed, 170 insertions(+), 63 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7d004dddf9..08217e3e38 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3492,6 +3492,28 @@
>  'erasure-coded': 'SheepdogRedundancyErasureCoded' } }
>  
>  ##
> +# @BlockdevCreateOptionsSheepdog:
> +#
> +# Driver specific image creation options for Sheepdog.
> +#
> +# @location Where to store the new image file
> +# @size Size of the virtual disk in bytes
> +# @backing_file File name of a base image

s/_/-/

With that fixed:

Reviewed-by: Max Reitz 

> +# @preallocationPreallocation mode (allowed values: off, full)
> +# @redundancy   Redundancy of the image
> +# @object-size  Object size of the image
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsSheepdog',
> +  'data': { 'location': 'BlockdevOptionsSheepdog',
> +'size': 'size',
> +'*backing-file':'str',
> +'*preallocation':   'PreallocMode',
> +'*redundancy':  'SheepdogRedundancy',
> +'*object-size': 'size' } }
> +
> +##
>  # @BlockdevCreateNotSupported:
>  #
>  # This is used for all drivers that don't support creating images.

[...]

> index dc0348f120..05129dc809 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c

[...]

> @@ -2142,15 +2144,95 @@ static int sd_create(const char *filename, QemuOpts 
> *opts,

[...]

> +static int sd_create(const char *filename, QemuOpts *opts,
> + Error **errp)
> +{

[...]

> +v = qobject_input_visitor_new_keyval(crumpled);
> +visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
> +visit_free(v);
> +qobject_decref(crumpled);
> +
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +create_options->u.sheepdog.size =
> +ROUND_UP(create_options->u.sheepdog.size, BDRV_SECTOR_SIZE);

I think I'd prefer an assertion that the type is indeed sheepdog before
this.

> +
> +if (redundancy) {
> +create_options->u.sheepdog.has_redundancy = true;
> +create_options->u.sheepdog.redundancy =
> +parse_redundancy_str(redundancy);
> +if (create_options->u.sheepdog.redundancy == NULL) {
> +error_setg(errp, "Invalid redundancy mode");
> +ret = -EINVAL;
> +goto fail;
> +}
> +}



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 1/2] iotest 033: add misaligned write-zeroes test via truncate

2018-02-12 Thread Anton Nefedov 



On 12/2/2018 6:54 PM, Alberto Garcia wrote:

On Mon 12 Feb 2018 02:14:00 PM CET, Anton Nefedov wrote:

+# Trigger truncate that would shrink qcow2 L1 table, which is done by
+#   clearing one entry (8 bytes) with bdrv_co_pwrite_zeroes()
+
+echo
+echo "=== Test misaligned write zeroes via truncate ==="
+echo
+
+CLUSTER_SIZE=$((64 * 1024))
+L2_COVERAGE=$(($CLUSTER_SIZE * $CLUSTER_SIZE / 8))
+_make_test_img $(($L2_COVERAGE * 2))


If my numbers are correct, that's a 1GB image. For qcow2 this is not a
problem but I wonder if it's ok to create such large images for other
formats (for raw they should be sparse by default, but still).

Berto



Good point. Actually even 512 byte clusters will work here, resulting
in a just 64k image.

Re: [Qemu-block] [PATCH 18/27] sheepdog: QAPIfy "redundacy" create option

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> The "redundacy" option for Sheepdog image creation is currently a string
> that can encode one or two integers depending on its format, which at
> the same time implicitly selects a mode.
> 
> This patch turns it into a QAPI union and converts the string into such
> a QAPI object before interpreting the values.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 45 ++
>  block/sheepdog.c | 89 
> 
>  2 files changed, 107 insertions(+), 27 deletions(-)

Reviewed-by: Max Reitz 

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index f684477328..dc0348f120 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c

[...]

> @@ -1912,35 +1954,28 @@ static int parse_redundancy(BDRVSheepdogState *s, 
> const char *opt)
>  return -EINVAL;
>  }
>  
> -copy = strtol(n1, NULL, 10);
>  /* FIXME fix error checking by switching to qemu_strtol() */

But this is not the time? ;-)

> -if (copy > SD_MAX_COPIES || copy < 1) {
> -return -EINVAL;
> -}
> -if (!n2) {
> -inode->copy_policy = 0;
> -inode->nr_copies = copy;
> -return 0;
> -}
> +copy = strtol(n1, NULL, 10);
>  



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 7/8] file-posix: account discard operations

2018-02-12 Thread Anton Nefedov 



On 7/2/2018 6:10 PM, Alberto Garcia wrote:

On Fri 19 Jan 2018 01:50:06 PM CET, Anton Nefedov wrote:

This will help to identify how many of the user-issued discard operations
(accounted on a device level) have actually suceeded down on the host file
(even though the numbers will not be exactly the same if non-raw format
driver is used (e.g. qcow2 sending metadata discards)).

Signed-off-by: Anton Nefedov 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
  block/file-posix.c | 21 +++--
  1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e..544ae58 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -158,6 +158,11 @@ typedef struct BDRVRawState {
  bool page_cache_inconsistent:1;
  bool has_fallocate;
  bool needs_alignment;
+struct {
+int64_t discard_nb_ok;
+int64_t discard_nb_failed;
+int64_t discard_bytes_ok;
+} stats;


Shouldn't this new structure be defined in a header file so other
drivers can use it? Or did you define it here because you don't see that
happening soon?



I guess there's no reason to burden the common header files as long as
it's not really used anywhere else.

Re: [Qemu-block] [PATCH v2 1/8] qapi: group BlockDeviceStats fields

2018-02-12 Thread Anton Nefedov 



On 6/2/2018 4:12 PM, Alberto Garcia wrote:

On Fri 19 Jan 2018 01:50:00 PM CET, Anton Nefedov wrote:

Make the stat fields definition slightly more readable.
Cosmetic change only.

Signed-off-by: Anton Nefedov 
---
  qapi/block-core.json | 24 ++--
  1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e94a688..2e0665e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -733,14 +733,26 @@
  # Since: 0.14.0
  ##
  { 'struct': 'BlockDeviceStats',
-  'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
-   'wr_operations': 'int', 'flush_operations': 'int',
+  'data': {'rd_bytes': 'int', 'wr_bytes': 'int',
+
+   'rd_operations': 'int', 'wr_operations': 'int',
+   'flush_operations': 'int',
+
 'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-   'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
-   'rd_merged': 'int', 'wr_merged': 'int', '*idle_time_ns': 'int',
+   'rd_total_time_ns': 'int',


It would be nice to reorder these following the read-write-flush order.
This way these would be rd_total_time_ns, wr_total_time_ns and
flush_total_time_ns.

The order of the fields in the documentation would also need to be
changed.

Berto



Agree, done.
I'm also moving here the spelling fixes proposed by Erik.

Re: [Qemu-block] [PATCH 17/27] nfs: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to nfs, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 16 +++-
>  block/nfs.c  | 74 
> +---
>  2 files changed, 74 insertions(+), 16 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 2/2] block: fix write with zero flag set and iovector provided

2018-02-12 Thread Anton Nefedov 



On 12/2/2018 6:03 PM, Alberto Garcia wrote:

On Mon 12 Feb 2018 02:14:01 PM CET, Anton Nefedov wrote:

The normal bdrv_co_pwritev() use is either
   - BDRV_REQ_ZERO_WRITE clear and iovector provided
   - BDRV_REQ_ZERO_WRITE set and iovector == NULL

while
   - the flag clear and iovector == NULL is an assertion failure
 in bdrv_co_do_zero_pwritev()
   - the flag set and iovector provided is in fact allowed
 (the flag prevails and zeroes are written)

However the alignment logic does not support the latter case so the
padding areas get overwritten with zeroes.


Oh, so this doesn't simply write zeroes in [offset, offset+bytes), but
also in the head and tail areas, instead of keeping the previous
contents.

This is a pretty serious bug, but I assume it can't be triggered
(bdrv_pwrite_zeroes() is used in complete clusters). Did you check if
there was any other scenario where this could happen?



I was a bit lazy to look deep but as far as I can say currently it's
only bdrv_pwrite_zeroes(). It's mostly called for large extents like
clusters, but not everywhere, another case is I guess
qcow2_crypto_hdr_init_func(); also it's probably not guaranteed (though
being quite exotic) that the cluster size is not smaller than the
protocol driver alignment requirements.

At least external (block.h) write interfaces don't accept or set any
flags

Re: [Qemu-block] [PATCH v2 1/2] iotest 033: add misaligned write-zeroes test via truncate

2018-02-12 Thread Alberto Garcia 
On Mon 12 Feb 2018 02:14:00 PM CET, Anton Nefedov wrote:
> +# Trigger truncate that would shrink qcow2 L1 table, which is done by
> +#   clearing one entry (8 bytes) with bdrv_co_pwrite_zeroes()
> +
> +echo
> +echo "=== Test misaligned write zeroes via truncate ==="
> +echo
> +
> +CLUSTER_SIZE=$((64 * 1024))
> +L2_COVERAGE=$(($CLUSTER_SIZE * $CLUSTER_SIZE / 8))
> +_make_test_img $(($L2_COVERAGE * 2))

If my numbers are correct, that's a 1GB image. For qcow2 this is not a
problem but I wonder if it's ok to create such large images for other
formats (for raw they should be sparse by default, but still).

Berto

Re: [Qemu-block] [PATCH 16/27] nfs: Use QAPI options in nfs_client_open()

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Using the QAPI visitor to turn all options into QAPI BlockdevOptionsNfs
> simplifies the code a lot. It will also be useful for implementing the
> QAPI based .bdrv_co_create callback.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/nfs.c | 176 
> ++--
>  1 file changed, 53 insertions(+), 123 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 15/27] rbd: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to rbd, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  20 +++-
>  block/rbd.c  | 137 
> +--
>  2 files changed, 108 insertions(+), 49 deletions(-)

Reviewed-by: Max Reitz 

Some comments below.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5b4cd6bd12..370fcd9584 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3415,6 +3415,24 @@
>  '*refcount-bits':   'int' } }
>  
>  ##
> +# @BlockdevCreateOptionsRbd:
> +#
> +# Driver specific image creation options for rbd/Ceph.
> +#
> +# @location Where to store the new image file

Maybe this should mention that location.snapshot is not allowed?

(And that location.server is ignored.  But is that even intended?)

> +# @size Size of the virtual disk in bytes
> +# @password-secret  ID of secret providing the password
> +# @cluster_size RBD object size

s/_/-/

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsRbd',
> +  'data': { 'location': 'BlockdevOptionsRbd',
> +'size': 'size',
> +'*password-secret': 'str',
> +'*cluster-size' :   'size' } }
> +
> +##
>  # @BlockdevCreateNotSupported:
>  #
>  # This is used for all drivers that don't support creating images.

[...]

> diff --git a/block/rbd.c b/block/rbd.c
> index a76a5e8755..c164f70167 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c

[...]

> @@ -432,24 +409,87 @@ static int qemu_rbd_create(const char *filename, 
> QemuOpts *opts, Error **errp)

[...]

> +static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error 
> **errp)
> +{
> +BlockdevCreateOptions *create_options;
> +BlockdevCreateOptionsRbd *rbd_opts;
> +BlockdevOptionsRbd *loc;
> +Error *local_err = NULL;
> +const char *keypairs;
> +QDict *options = NULL;
> +int ret = 0;
> +
> +create_options = g_new0(BlockdevCreateOptions, 1);
> +create_options->driver = BLOCKDEV_DRIVER_RBD;
> +rbd_opts = _options->u.rbd;
> +
> +rbd_opts->location = g_new0(BlockdevOptionsRbd, 1);
> +
> +rbd_opts->password_secret = (char *) qemu_opt_get(opts, 
> "password-secret");
> +
> +/* Read out options */
> +rbd_opts->size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> +  BDRV_SECTOR_SIZE);
> +rbd_opts->cluster_size = qemu_opt_get_size_del(opts,
> +   BLOCK_OPT_CLUSTER_SIZE, 
> 0);
> +rbd_opts->has_cluster_size = (rbd_opts->cluster_size != 0);
> +
> +options = qdict_new();
> +qemu_rbd_parse_filename(filename, options, _err);
> +if (local_err) {
> +ret = -EINVAL;
> +error_propagate(errp, local_err);
> +goto exit;
> +}
> +
> +/*
> + * Caution: while qdict_get_try_str() is fine, getting non-string
> + * types would require more care.  When @options come from -blockdev
> + * or blockdev_add, its members are typed according to the QAPI
> + * schema, but when they come from -drive, they're all QString.
> + */
> +loc = rbd_opts->location;
> +loc->pool = g_strdup(qdict_get_try_str(options, "pool"));
> +loc->conf = g_strdup(qdict_get_try_str(options, "conf"));
> +loc->has_conf = !!rbd_opts->location->conf;
> +loc->user = g_strdup(qdict_get_try_str(options, "user"));
> +loc->has_user = !!rbd_opts->location->user;

"!!loc->conf" and "!!loc->user" would be shorter and maybe a bit easier
to get.

Max

> +loc->image= g_strdup(qdict_get_try_str(options, "image"));
> +keypairs  = qdict_get_try_str(options, "=keyvalue-pairs");
> +
> +ret = qemu_rbd_do_create(create_options, keypairs, errp);
> +if (ret < 0) {
> +goto exit;
> +}
>  
>  exit:
>  QDECREF(options);
> +qapi_free_BlockdevCreateOptions(create_options);
>  return ret;
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 2/2] block: fix write with zero flag set and iovector provided

2018-02-12 Thread Alberto Garcia 
On Mon 12 Feb 2018 02:14:01 PM CET, Anton Nefedov wrote:
> The normal bdrv_co_pwritev() use is either
>   - BDRV_REQ_ZERO_WRITE clear and iovector provided
>   - BDRV_REQ_ZERO_WRITE set and iovector == NULL
>
> while
>   - the flag clear and iovector == NULL is an assertion failure
> in bdrv_co_do_zero_pwritev()
>   - the flag set and iovector provided is in fact allowed
> (the flag prevails and zeroes are written)
>
> However the alignment logic does not support the latter case so the
> padding areas get overwritten with zeroes.

Oh, so this doesn't simply write zeroes in [offset, offset+bytes), but
also in the head and tail areas, instead of keeping the previous
contents.

This is a pretty serious bug, but I assume it can't be triggered
(bdrv_pwrite_zeroes() is used in complete clusters). Did you check if
there was any other scenario where this could happen?

> Signed-off-by: Anton Nefedov 
> Reviewed-by: Eric Blake 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Paolo Bonzini 
On 12/02/2018 15:48, Kevin Wolf wrote:
> Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
>> Okay, we are in agreement about this and you expressed very well why I
>> (at the gut feeling level) didn't like the old op blockers.  But you
>> bypassed the real question, which is: should I send a pull request for
>> these two patches or not? :)
> 
> I didn't spell it out that explicitly, but this is essentially a NACK.
> I'd very much prefer if you could replace it with the proper solution.
> Of course, we can always make exceptions when there is a good reason,
> but with 2.12 still two months away, I doubt we have one.

Ok, I don't mind explicitness.  I'll keep these two patches in the queue
for now.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Kevin Wolf 
Am 12.02.2018 um 15:32 hat Paolo Bonzini geschrieben:
> On 12/02/2018 15:30, Kevin Wolf wrote:
> >>> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> >>> find the time yet to remove the existing ones, but any new protections
> >>> should be using the permission system.
> >> I agree.  But does this include not fixing bugs wherever clients are
> >> using the old op blockers?
> > I'm not saying that we shouldn't fix the bug, just that we should fix it
> > properly with the best infrastructure we have.
> > 
> > The old op blockers are "fixing" the problem at the symptom level, and
> > you have to check for each high-level operation if it does something
> > problematic internally. You have to repeat this analysis every time you
> > add a new operation or modifiy an existing one (which noone ever does).
> > The risk that this breaks sooner or later is pretty high.
> > 
> > The new permission system, on the other hand, directly addresses the
> > root cause, and any new feature that uses dirty bitmaps will then
> > automatically get the protection, too.
> > 
> > So in fact, I would say that the bug isn't really fixed (but at best
> > papered over) until we add a proper fix on the permission level.
> 
> Okay, we are in agreement about this and you expressed very well why I
> (at the gut feeling level) didn't like the old op blockers.  But you
> bypassed the real question, which is: should I send a pull request for
> these two patches or not? :)

I didn't spell it out that explicitly, but this is essentially a NACK.
I'd very much prefer if you could replace it with the proper solution.
Of course, we can always make exceptions when there is a good reason,
but with 2.12 still two months away, I doubt we have one.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Paolo Bonzini 
On 12/02/2018 15:30, Kevin Wolf wrote:
>>> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
>>> find the time yet to remove the existing ones, but any new protections
>>> should be using the permission system.
>> I agree.  But does this include not fixing bugs wherever clients are
>> using the old op blockers?
> I'm not saying that we shouldn't fix the bug, just that we should fix it
> properly with the best infrastructure we have.
> 
> The old op blockers are "fixing" the problem at the symptom level, and
> you have to check for each high-level operation if it does something
> problematic internally. You have to repeat this analysis every time you
> add a new operation or modifiy an existing one (which noone ever does).
> The risk that this breaks sooner or later is pretty high.
> 
> The new permission system, on the other hand, directly addresses the
> root cause, and any new feature that uses dirty bitmaps will then
> automatically get the protection, too.
> 
> So in fact, I would say that the bug isn't really fixed (but at best
> papered over) until we add a proper fix on the permission level.

Okay, we are in agreement about this and you expressed very well why I
(at the gut feeling level) didn't like the old op blockers.  But you
bypassed the real question, which is: should I send a pull request for
these two patches or not? :)

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Kevin Wolf 
Am 12.02.2018 um 15:00 hat Paolo Bonzini geschrieben:
> On 12/02/2018 14:52, Kevin Wolf wrote:
> > Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
> >> On 08/02/2018 02:35, Fam Zheng wrote:
> >>> On Wed, 02/07 17:36, Paolo Bonzini wrote:
>  @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, 
>  Error **errp)
>   
>   scsi_realize(>qdev, errp);
>   scsi_generic_read_device_identification(>qdev);
>  +
>  +/* For op blockers, due to lack of support for dirty bitmaps.  */
>  +error_setg(>mirror_source,
>  +   "scsi-block does not support acting as a mirroring 
>  source");
>  +error_setg(>commit_source,
>  +   "scsi-block does not support acting as an active commit 
>  source");
> >>>
> >>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error 
> >>> message
> >>> will not be as nice but it can be useful for another (blockjob) operation 
> >>> that
> >>> requires dirty bitmap support, or another device that doesn't support 
> >>> dirty
> >>> bitmaps. Though there isn't one for now.
> >>
> >> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
> >> bitmask.  Then you can easily add a single Error * for multiple
> >> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
> >> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
> >> notifiers below.
> > 
> > We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> > find the time yet to remove the existing ones, but any new protections
> > should be using the permission system.
> 
> I agree.  But does this include not fixing bugs wherever clients are
> using the old op blockers?

I'm not saying that we shouldn't fix the bug, just that we should fix it
properly with the best infrastructure we have.

The old op blockers are "fixing" the problem at the symptom level, and
you have to check for each high-level operation if it does something
problematic internally. You have to repeat this analysis every time you
add a new operation or modifiy an existing one (which noone ever does).
The risk that this breaks sooner or later is pretty high.

The new permission system, on the other hand, directly addresses the
root cause, and any new feature that uses dirty bitmaps will then
automatically get the protection, too.

So in fact, I would say that the bug isn't really fixed (but at best
papered over) until we add a proper fix on the permission level.

Kevin

Re: [Qemu-block] [PATCH 14/27] gluster: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to gluster, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  18 ++-
>  block/gluster.c  | 149 
> +--
>  2 files changed, 115 insertions(+), 52 deletions(-)

[...]

> diff --git a/block/gluster.c b/block/gluster.c
> index 0f4265a3a4..b7e2b7fa2b 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c

[...]

> @@ -962,64 +976,33 @@ static coroutine_fn int 
> qemu_gluster_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  #endif
>  
> -static int qemu_gluster_create(const char *filename,
> -   QemuOpts *opts, Error **errp)
> +static int qemu_gluster_co_create(BlockdevCreateOptions *options,
> +  Error **errp)
>  {
> -BlockdevOptionsGluster *gconf;
> +BlockdevCreateOptionsGluster *opts = >u.gluster;

In the other drivers so far you have asserted first that options->driver
is as expected and then retrieved the appropriate part of the union.  I
liked that a bit better, although of course it doesn't matter functionally.

Anyway:

Reviewed-by: Max Reitz 

>  struct glfs *glfs;
>  struct glfs_fd *fd;
>  int ret = 0;
> -PreallocMode prealloc;
> -int64_t total_size = 0;
> -char *tmp = NULL;
> -Error *local_err = NULL;
> -
> -gconf = g_new0(BlockdevOptionsGluster, 1);
> -gconf->debug = qemu_opt_get_number_del(opts, GLUSTER_OPT_DEBUG,
> -   GLUSTER_DEBUG_DEFAULT);
> -if (gconf->debug < 0) {
> -gconf->debug = 0;
> -} else if (gconf->debug > GLUSTER_DEBUG_MAX) {
> -gconf->debug = GLUSTER_DEBUG_MAX;
> -}
> -gconf->has_debug = true;
>  
> -gconf->logfile = qemu_opt_get_del(opts, GLUSTER_OPT_LOGFILE);
> -if (!gconf->logfile) {
> -gconf->logfile = g_strdup(GLUSTER_LOGFILE_DEFAULT);
> -}
> -gconf->has_logfile = true;
> +assert(options->driver == BLOCKDEV_DRIVER_GLUSTER);
>  



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/27] qcow2: Handle full/falloc preallocation in qcow2_create2()

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> Once qcow2_create2() can be called directly on an already existing node,
> we must provide the 'full' and 'falloc' preallocation modes outside of
> creating the image on the protocol layer. Fortunately, we have
> preallocated truncate now which can provide this functionality.

When reviewing the gluster patch, I noticed that this will break
full/falloc preallocation on anything but the file protocol because
nothing else yet supports preallocated truncate...

Max

> Signed-off-by: Kevin Wolf 
> Reviewed-by: Eric Blake 
> ---
>  block/qcow2.c | 28 +++-
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3f08cff1fa..0316335614 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2845,6 +2845,25 @@ static int qcow2_create2(BlockdevCreateOptions 
> *create_options, Error **errp)
>  }
>  blk_set_allow_write_beyond_eof(blk, true);
>  
> +/* Clear the protocol layer and preallocate it if necessary */
> +ret = blk_truncate(blk, 0, PREALLOC_MODE_OFF, errp);
> +if (ret < 0) {
> +goto out;
> +}
> +
> +if (qcow2_opts->preallocation == PREALLOC_MODE_FULL ||
> +qcow2_opts->preallocation == PREALLOC_MODE_FALLOC)
> +{
> +int64_t prealloc_size =
> +qcow2_calc_prealloc_size(qcow2_opts->size, cluster_size,
> + refcount_order);
> +
> +ret = blk_truncate(blk, prealloc_size, qcow2_opts->preallocation, 
> errp);
> +if (ret < 0) {
> +goto out;
> +}
> +}
> +
>  /* Write the header */
>  QEMU_BUILD_BUG_ON((1 << MIN_CLUSTER_BITS) < sizeof(*header));
>  header = g_malloc0(cluster_size);
> @@ -3081,15 +3100,6 @@ static int qcow2_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>  
>  
>  /* Create and open the file (protocol layer) */
> -if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) {
> -int refcount_order = ctz32(refcount_bits);
> -int64_t prealloc_size =
> -qcow2_calc_prealloc_size(size, cluster_size, refcount_order);
> -qemu_opt_set_number(opts, BLOCK_OPT_SIZE, prealloc_size, 
> _abort);
> -qemu_opt_set(opts, BLOCK_OPT_PREALLOC, PreallocMode_str(prealloc),
> - _abort);
> -}
> -
>  ret = bdrv_create_file(filename, opts, errp);
>  if (ret < 0) {
>  goto finish;
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Paolo Bonzini 
On 12/02/2018 14:52, Kevin Wolf wrote:
> Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
>> On 08/02/2018 02:35, Fam Zheng wrote:
>>> On Wed, 02/07 17:36, Paolo Bonzini wrote:
 @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, 
 Error **errp)
  
  scsi_realize(>qdev, errp);
  scsi_generic_read_device_identification(>qdev);
 +
 +/* For op blockers, due to lack of support for dirty bitmaps.  */
 +error_setg(>mirror_source,
 +   "scsi-block does not support acting as a mirroring 
 source");
 +error_setg(>commit_source,
 +   "scsi-block does not support acting as an active commit 
 source");
>>>
>>> An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error 
>>> message
>>> will not be as nice but it can be useful for another (blockjob) operation 
>>> that
>>> requires dirty bitmap support, or another device that doesn't support dirty
>>> bitmaps. Though there isn't one for now.
>>
>> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
>> bitmask.  Then you can easily add a single Error * for multiple
>> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
>> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
>> notifiers below.
> 
> We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
> find the time yet to remove the existing ones, but any new protections
> should be using the permission system.

I agree.  But does this include not fixing bugs wherever clients are
using the old op blockers?

Paolo

> I propose a new BLK_PERM_BYPASS that allows its users to bypass the
> block layer I/O functions. In other words, bdrv_aio_ioctl() would
> require that you got this permission. A dirty bitmap would keep a
> BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
> can never have a dirty bitmap and a device using ioctls attached to the
> BDS at the same time.

Re: [Qemu-block] [PATCH 13/27] file-win32: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to file-win32, which
> enables image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/file-win32.c | 45 +
>  1 file changed, 37 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 12/27] file-posix: Support .bdrv_co_create

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to file, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 20 +-
>  block/file-posix.c   | 77 
> +---
>  2 files changed, 74 insertions(+), 23 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] Add save-snapshot, load-snapshot and delete-snapshot to QAPI

2018-02-12 Thread Richard Palethorpe 
Hello,

Juan Quintela writes:

> "Daniel P. Berrange"  wrote:
>> On Thu, Jan 11, 2018 at 01:23:05PM +, Dr. David Alan Gilbert wrote:
>>> * Daniel P. Berrange (berra...@redhat.com) wrote:
>>> > On Thu, Jan 11, 2018 at 01:46:38PM +0100, Max Reitz wrote:
>>> > > On 2018-01-08 14:52, Eric Blake wrote:
>>> > > > On 01/07/2018 06:23 AM, Richard Palethorpe wrote:
>>> > > >> Add QAPI wrapper functions for the existing snapshot functionality. 
>>> > > >> These
>>> > > >> functions behave the same way as the HMP savevm, loadvm and delvm
>>> > > >> commands. This will allow applications, such as OpenQA, to
>>> > > >> programmatically
>>> > > >> revert the VM to a previous state with no dependence on HMP or 
>>> > > >> qemu-img.
>>> > > > 
>>> > > > That's already possible; libvirt uses QMP's human-monitor-command to
>>> > > > access these HMP commands programmatically.
>>> > > > 
>>> > > > We've had discussions in the past about what it would take to have
>>> > > > specific QMP commands for these operations; the biggest problem is 
>>> > > > that
>>> > > > these commands promote the use of internal snapshots, and there are
>>> > > > enough performance and other issues with internal snapshots that we 
>>> > > > are
>>> > > > not yet ready to commit to a long-term interface for making their use
>>> > > > easier.  At this point, our recommendation is to prefer external 
>>> > > > snapshots.
>>> > > 
>>> > > We already have QMP commands for internal snapshots, though.  Isn't the
>>> > > biggest issue that savevm takes too much time to be a synchronous QMP
>>> > > command?
>>> > 
>>> > Ultimately savevm/loadvm are using much of the migration code internally,
>>> > but are not exposed as URI schemes. Could we perhaps take advantage of
>>> > the internal common layer and define a migration URI scheme
>>> > 
>>> >snapshot:
>>> > 
>>> > where '' is the name of the internal snapshot in the qcow2 file.
>>> 
>>> I had wondered about that; I'd just thought of doing the migration
>>> saving to a block device rather than the rest of the snapshot
>>> activity around it;
>>> but I guess that's possible.
>>
>> One possible gotcha is whether the current savevm/loadvm QEMUFile impl
>> actually does non-blocking I/O properly. eg same reason why we don't
>> support a plain  file: protocol - POSIX I/O on plain files doesn't
>> honour O_NONBLOCK.  The block layer does AIO though, so we might be OK,
>> depending on which block layer APIs the QEMUFile impl uses. I've not
>> looked at the code recently though.
>
> The blocking part is less important (for the write side), because we
> have a thread there.  For loading  it would be great to get one
> migration thread also.
>
>>> > Then you could just use the regular migrate QMP commands for loading
>>> > and saving snapshots.  Might need a little extra work on the incoming
>>> > side, since we need to be able to load snapshots, despite QEMU not
>>> > being started with '-incoming defer', but might still be doable ?
>>> > This would theoretically give us progress monitoring, cancellation,
>>> > etc for free.
>>> 
>>> What actually stops this working other than the sanity check in
>>> migrate_incoming ?
>>
>> No idea really - not looked closely at the code implications.
>
> It would be a plus for migration code, right now there are _two_
> implementations, and savevm/loadvm one gets less love.
>
> And we will check "much more" the way to load migration in a
> non-pristine qemu, so 
>
> Later, Juan.

This looks like the best option so far for my use case.

-- 
Thank you,
Richard.

Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] scsi: add block job opblockers for scsi-block

2018-02-12 Thread Kevin Wolf 
Am 08.02.2018 um 11:42 hat Paolo Bonzini geschrieben:
> On 08/02/2018 02:35, Fam Zheng wrote:
> > On Wed, 02/07 17:36, Paolo Bonzini wrote:
> >> @@ -2626,6 +2656,36 @@ static void scsi_block_realize(SCSIDevice *dev, 
> >> Error **errp)
> >>  
> >>  scsi_realize(>qdev, errp);
> >>  scsi_generic_read_device_identification(>qdev);
> >> +
> >> +/* For op blockers, due to lack of support for dirty bitmaps.  */
> >> +error_setg(>mirror_source,
> >> +   "scsi-block does not support acting as a mirroring 
> >> source");
> >> +error_setg(>commit_source,
> >> +   "scsi-block does not support acting as an active commit 
> >> source");
> > 
> > An alternative way would be adding BLOCK_OP_TYPE_DIRTY_BITMAP. The error 
> > message
> > will not be as nice but it can be useful for another (blockjob) operation 
> > that
> > requires dirty bitmap support, or another device that doesn't support dirty
> > bitmaps. Though there isn't one for now.
> 
> Yeah, I thought about it.  Another possibility is make BLOCK_OP_TYPE_* a
> bitmask.  Then you can easily add a single Error * for multiple
> blockers, and BLOCK_OP_TYPE_DIRTY_BITMAP can be defined as
> BLOCK_OP_TYPE_MIRROR_SOURCE|BLOCK_OP_TYPE_COMMIT_SOURCE; likewise for
> notifiers below.

We shouldn't be adding new instances of BLOCK_OP_* at all. I couldn't
find the time yet to remove the existing ones, but any new protections
should be using the permission system.

I propose a new BLK_PERM_BYPASS that allows its users to bypass the
block layer I/O functions. In other words, bdrv_aio_ioctl() would
require that you got this permission. A dirty bitmap would keep a
BdrvChild with perm=0, shared=BLK_PERM_ALL & ~BLK_PERM_BYPASS, so you
can never have a dirty bitmap and a device using ioctls attached to the
BDS at the same time.

Kevin

Re: [Qemu-block] [PATCH 11/27] block: x-blockdev-create QMP command

2018-02-12 Thread Max Reitz 
On 2018-02-08 20:23, Kevin Wolf wrote:
> This adds a synchronous x-blockdev-create QMP command that can create
> qcow2 images on a given node name.
> 
> We don't want to block while creating an image, so this is not the final
> interface in all aspects, but BlockdevCreateOptionsQcow2 and
> .bdrv_co_create() are what they actually might look like in the end. In
> any case, this should be good enough to test whether we interpret
> BlockdevCreateOptions as we should.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json  | 12 
>  include/block/block.h |  1 +
>  include/block/block_int.h |  2 ++
>  block.c   |  2 +-
>  block/create.c| 75 
> +++
>  block/qcow2.c |  3 +-
>  block/Makefile.objs   |  2 +-
>  7 files changed, 94 insertions(+), 3 deletions(-)
>  create mode 100644 block/create.c

[...]

> diff --git a/block/create.c b/block/create.c
> new file mode 100644
> index 00..e95446a0f3
> --- /dev/null
> +++ b/block/create.c
> @@ -0,0 +1,75 @@

[...]

> +void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
> +{
> +const char *fmt = BlockdevDriver_str(options->driver);
> +BlockDriver *drv = bdrv_find_format(fmt);
> +Coroutine *co;
> +BlockdevCreateCo cco;
> +
> +/* If the driver is in the schema, we know that it exists. But it may not
> + * be whitelisted. */
> +assert(drv);
> +if (bdrv_uses_whitelist() && !bdrv_is_whitelisted(drv, true)) {

Isn't this more of an R/W case than RO?

Max

> +error_setg(errp, "Driver is not whitelisted");
> +return;
> +}



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [ovirt-users] qcow2 images corruption

2018-02-12 Thread Nicolas Ecarnot 

Hello,

As I got no answer here, maybe someone could advise me a better place to 
find some hints about this qemu qcow2 corruption issue?


Thank you

--
Nicolas Ecarnot

Le 07/02/2018 à 18:06, Nicolas Ecarnot a écrit :

Hello,

TL; DR : qcow2 images keep getting corrupted. Any workaround?

Long version:
This discussion has already been launched by me on the oVirt and on 
qemu-block mailing list, under similar circumstances but I learned 
further things since months and here are some informations :


- We are using 2 oVirt 3.6.7.5-1.el7.centos datacenters, using CentOS 
7.{2,3} hosts

- Hosts :
   - CentOS 7.2 1511 :
     - Kernel = 3.10.0 327
     - KVM : 2.3.0-31
     - libvirt : 1.2.17
     - vdsm : 4.17.32-1
   - CentOS 7.3 1611 :
     - Kernel 3.10.0 514
     - KVM : 2.3.0-31
     - libvirt 2.0.0-10
     - vdsm : 4.17.32-1
- Our storage is 2 Equallogic SANs connected via iSCSI on a dedicated 
network
- Depends on weeks, but all in all, there are around 32 hosts, 8 storage 
domains and for various reasons, very few VMs (less than 200).
- One peculiar point is that most of our VMs are provided an additional 
dedicated network interface that is iSCSI-connected to some volumes of 
our SAN - these volumes not being part of the oVirt setup. That could 
lead to a lot of additional iSCSI traffic.


 From times to times, a random VM appears paused by oVirt.
Digging into the oVirt engine logs, then into the host vdsm logs, it 
appears that the host considers the qcow2 image as corrupted.
Along what I consider as a conservative behavior, vdsm stops any 
interaction with this image and marks it as paused.

Any try to unpause it leads to the same conservative pause.

After having found (https://access.redhat.com/solutions/1173623) the 
right logical volume hosting the qcow2 image, I can run qemu-img check 
on it.

- On 80% of my VMs, I find no errors.
- On 15% of them, I find Leaked cluster errors that I can correct using 
"qemu-img check -r all"
- On 5% of them, I find Leaked clusters errors and further fatal errors, 
which can not be corrected with qemu-img.
In rare cases, qemu-img can correct them, but destroys large parts of 
the image (becomes unusable), and on other cases it can not correct them 
at all.


Months ago, I already sent a similar message but the error message was 
about No space left on device 
(https://www.mail-archive.com/qemu-block@gnu.org/msg00110.html).


This time, I don't have this message about space, but only corruption.

I kept reading and found a similar discussion in the Proxmox group :
https://lists.ovirt.org/pipermail/users/2018-February/086750.html

https://forum.proxmox.com/threads/qcow2-corruption-after-snapshot-or-heavy-disk-i-o.32865/page-2 



What I read similar to my case is :
- usage of qcow2
- heavy disk I/O
- using the virtio-blk driver

In the proxmox thread, they tend to say that using virtio-scsi is the 
solution. Having asked this question to oVirt experts 
(https://lists.ovirt.org/pipermail/users/2018-February/086753.html) but 
it's not clear the driver is to blame.


I agree with the answer Yaniv Kaul gave to me, saying I have to properly 
report the issue, so I'm longing to know which peculiar information I 
can give you now.


As you can imagine, all this setup is in production, and for most of the 
VMs, I can not "play" with them. Moreover, we launched a campaign of 
nightly stopping every VM, qemu-img check them one by one, then boot.

So it might take some time before I find another corrupted image.
(which I'll preciously store for debug)

Other informations : We very rarely do snapshots, but I'm close to 
imagine that automated migrations of VMs could trigger similar behaviors 
on qcow2 images.


Last point about the versions we use : yes that's old, yes we're 
planning to upgrade, but we don't know when.


Regards,




--
Nicolas ECARNOT

Re: [Qemu-block] [PATCH] iotests: Use virtio-blk in 155

2018-02-12 Thread Kevin Wolf 
Am 12.02.2018 um 13:47 hat Max Reitz geschrieben:
> Only a few select machine types support floppy drives and there is
> actually nothing preventing us from using virtio here, so let's do it.
> 
> Reported-by: Christian Borntraeger 
> Signed-off-by: Max Reitz 

Thanks, applied to the block branch.

Kevin

[Qemu-block] [PATCH v2 2/2] block: fix write with zero flag set and iovector provided

2018-02-12 Thread Anton Nefedov 
The normal bdrv_co_pwritev() use is either
  - BDRV_REQ_ZERO_WRITE clear and iovector provided
  - BDRV_REQ_ZERO_WRITE set and iovector == NULL

while
  - the flag clear and iovector == NULL is an assertion failure
in bdrv_co_do_zero_pwritev()
  - the flag set and iovector provided is in fact allowed
(the flag prevails and zeroes are written)

However the alignment logic does not support the latter case so the padding
areas get overwritten with zeroes.

Currently, general functions like bdrv_rw_co() do provide iovector
regardless of flags. So, keep it supported and use bdrv_co_do_zero_pwritev()
alignment for it which also makes the code a bit more obvious anyway.

Signed-off-by: Anton Nefedov 
Reviewed-by: Eric Blake 
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 89d0745..40df3be 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1701,7 +1701,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
  */
 tracked_request_begin(, bs, offset, bytes, BDRV_TRACKED_WRITE);
 
-if (!qiov) {
+if (flags & BDRV_REQ_ZERO_WRITE) {
 ret = bdrv_co_do_zero_pwritev(child, offset, bytes, flags, );
 goto out;
 }
-- 
2.7.4

[Qemu-block] [PATCH v2 1/2] iotest 033: add misaligned write-zeroes test via truncate

2018-02-12 Thread Anton Nefedov 
This new test case only makes sense for qcow2 while iotest 033 is generic;
however it matches the test purpose perfectly and also 033 contains those
do_test() tricks to pass the alignment, which won't look nice being
duplicated in other tests or moved to the common code.

Signed-off-by: Anton Nefedov 
---
 tests/qemu-iotests/033 | 28 
 tests/qemu-iotests/033.out | 13 +
 2 files changed, 41 insertions(+)

diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index 2cdfd13..5fa3983 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -64,6 +64,9 @@ do_test()
} | $QEMU_IO $IO_EXTRA_ARGS
 }
 
+echo
+echo "=== Test aligned and misaligned write zeroes operations ==="
+
 for write_zero_cmd in "write -z" "aio_write -z"; do
 for align in 512 4k; do
echo
@@ -102,7 +105,32 @@ for align in 512 4k; do
 done
 done
 
+
+# Trigger truncate that would shrink qcow2 L1 table, which is done by
+#   clearing one entry (8 bytes) with bdrv_co_pwrite_zeroes()
+
+echo
+echo "=== Test misaligned write zeroes via truncate ==="
+echo
+
+CLUSTER_SIZE=$((64 * 1024))
+L2_COVERAGE=$(($CLUSTER_SIZE * $CLUSTER_SIZE / 8))
+_make_test_img $(($L2_COVERAGE * 2))
+
+do_test 512 "write -P 1 0 0x200" "$TEST_IMG" | _filter_qemu_io
+# next L2 table
+do_test 512 "write -P 1 $L2_COVERAGE 0x200" "$TEST_IMG" | _filter_qemu_io
+
+# only interested in qcow2 here; also other formats might respond with
+#  "not supported" error message
+if [ $IMGFMT = "qcow2" ]; then
+do_test 512 "truncate $L2_COVERAGE" "$TEST_IMG" | _filter_qemu_io
+fi
+
+do_test 512 "read -P 1 0 0x200" "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
+echo
 echo "*** done"
 rm -f $seq.full
 status=0
diff --git a/tests/qemu-iotests/033.out b/tests/qemu-iotests/033.out
index 95929ef..57799cb 100644
--- a/tests/qemu-iotests/033.out
+++ b/tests/qemu-iotests/033.out
@@ -1,6 +1,8 @@
 QA output created by 033
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 
+=== Test aligned and misaligned write zeroes operations ===
+
 == preparing image ==
 wrote 1024/1024 bytes at offset 512
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -164,4 +166,15 @@ read 512/512 bytes at offset 512
 read 3072/3072 bytes at offset 1024
 3 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+
+=== Test misaligned write zeroes via truncate ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 536870912
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 *** done
-- 
2.7.4

[Qemu-block] [PATCH v2 0/2] block: fix write with zero flag set and iovector provided

2018-02-12 Thread Anton Nefedov 
v2:
  patch 1: commit message fixed
  test added (patch 2)

v1: http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg00131.html

Anton Nefedov (2):
  iotest 033: add misaligned write-zeroes test via truncate
  block: fix write with zero flag set and iovector provided

 block/io.c |  2 +-
 tests/qemu-iotests/033 | 28 
 tests/qemu-iotests/033.out | 13 +
 3 files changed, 42 insertions(+), 1 deletion(-)

-- 
2.7.4

Re: [Qemu-block] [PATCH] iotests: Use virtio-blk in 155

2018-02-12 Thread Christian Borntraeger 

On 02/12/2018 01:47 PM, Max Reitz wrote:
> Only a few select machine types support floppy drives and there is
> actually nothing preventing us from using virtio here, so let's do it.
> 
> Reported-by: Christian Borntraeger 
> Signed-off-by: Max Reitz 
Tested-by: Christian Borntraeger 



> ---
>  tests/qemu-iotests/155 | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
> index fc9fa975be..42dae04c83 100755
> --- a/tests/qemu-iotests/155
> +++ b/tests/qemu-iotests/155
> @@ -64,7 +64,7 @@ class BaseClass(iotests.QMPTestCase):
>  'file': {'driver': 'file',
>   'filename': source_img}}
>  self.vm.add_blockdev(self.qmp_to_opts(blockdev))
> -self.vm.add_device('floppy,id=qdev0,drive=source')
> +self.vm.add_device('virtio-blk,id=qdev0,drive=source')
>  self.vm.launch()
> 
>  self.assertIntactSourceBackingChain()
> @@ -173,21 +173,24 @@ class MirrorBaseClass(BaseClass):
>  def testFull(self):
>  self.runMirror('full')
> 
> -node = self.findBlockNode('target', 'qdev0')
> +node = self.findBlockNode('target',
> +  '/machine/peripheral/qdev0/virtio-backend')
>  self.assertCorrectBackingImage(node, None)
>  self.assertIntactSourceBackingChain()
> 
>  def testTop(self):
>  self.runMirror('top')
> 
> -node = self.findBlockNode('target', 'qdev0')
> +node = self.findBlockNode('target',
> +  '/machine/peripheral/qdev0/virtio-backend')
>  self.assertCorrectBackingImage(node, back2_img)
>  self.assertIntactSourceBackingChain()
> 
>  def testNone(self):
>  self.runMirror('none')
> 
> -node = self.findBlockNode('target', 'qdev0')
> +node = self.findBlockNode('target',
> +  '/machine/peripheral/qdev0/virtio-backend')
>  self.assertCorrectBackingImage(node, source_img)
>  self.assertIntactSourceBackingChain()
> 
> @@ -239,7 +242,8 @@ class TestCommit(BaseClass):
> 
>  self.vm.event_wait('BLOCK_JOB_COMPLETED')
> 
> -node = self.findBlockNode(None, 'qdev0')
> +node = self.findBlockNode(None,
> +  '/machine/peripheral/qdev0/virtio-backend')
>  self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename',
>  back1_img)
>  self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename',
>

Re: [Qemu-block] [Qemu-devel] [PATCH] block: early check for blockers on drive-mirror

2018-02-12 Thread Daniel P . Berrangé 
On Mon, Feb 12, 2018 at 01:42:11PM +0100, Kevin Wolf wrote:
> Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben:
> > On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> > > On 10/02/2018 00:07, John Snow wrote:
> > > >> +/* Early check to avoid creating target */
> > > >> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> > > >> +return;
> > > >> +}
> > > >> +
> > > >>  aio_context = bdrv_get_aio_context(bs);
> > > >>  aio_context_acquire(aio_context);
> > > >>  
> > > >>
> > > > What's the implication of the temporarily-extant target node that it
> > > > needs to be avoided so strictly?
> > > > 
> > > 
> > > Creating a file on disk, that no one will ever remvoe. :)
> > 
> > Fortunately libvirt's SELinux policy will probably prevent QEMU creating
> > it in the first place :-)
> 
> Well, calling drive-mirror without allowing QEMU to create the target
> image would be a bit pointless, so I think we can assume that libvirt
> did set up the file permission so that QEMU can create it. (Unless
> mode=existing is used, but I understand that libvirt doesn't want to
> create images with qemu-img, so that doesn't seem to be the case...)

We use either mode=existing or mode=absolute-paths depending on what
the mgmt app asked for in the API call to libvirt. I'm still kind of
suprised if mode=absolute-paths will work because we ought to be
blocking the creation of the file AFAIK and we can't pre-label a file
that doesn't exist yet.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

[Qemu-block] [PATCH] iotests: Use virtio-blk in 155

2018-02-12 Thread Max Reitz 
Only a few select machine types support floppy drives and there is
actually nothing preventing us from using virtio here, so let's do it.

Reported-by: Christian Borntraeger 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/155 | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index fc9fa975be..42dae04c83 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -64,7 +64,7 @@ class BaseClass(iotests.QMPTestCase):
 'file': {'driver': 'file',
  'filename': source_img}}
 self.vm.add_blockdev(self.qmp_to_opts(blockdev))
-self.vm.add_device('floppy,id=qdev0,drive=source')
+self.vm.add_device('virtio-blk,id=qdev0,drive=source')
 self.vm.launch()
 
 self.assertIntactSourceBackingChain()
@@ -173,21 +173,24 @@ class MirrorBaseClass(BaseClass):
 def testFull(self):
 self.runMirror('full')
 
-node = self.findBlockNode('target', 'qdev0')
+node = self.findBlockNode('target',
+  '/machine/peripheral/qdev0/virtio-backend')
 self.assertCorrectBackingImage(node, None)
 self.assertIntactSourceBackingChain()
 
 def testTop(self):
 self.runMirror('top')
 
-node = self.findBlockNode('target', 'qdev0')
+node = self.findBlockNode('target',
+  '/machine/peripheral/qdev0/virtio-backend')
 self.assertCorrectBackingImage(node, back2_img)
 self.assertIntactSourceBackingChain()
 
 def testNone(self):
 self.runMirror('none')
 
-node = self.findBlockNode('target', 'qdev0')
+node = self.findBlockNode('target',
+  '/machine/peripheral/qdev0/virtio-backend')
 self.assertCorrectBackingImage(node, source_img)
 self.assertIntactSourceBackingChain()
 
@@ -239,7 +242,8 @@ class TestCommit(BaseClass):
 
 self.vm.event_wait('BLOCK_JOB_COMPLETED')
 
-node = self.findBlockNode(None, 'qdev0')
+node = self.findBlockNode(None,
+  '/machine/peripheral/qdev0/virtio-backend')
 self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename',
 back1_img)
 self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename',
-- 
2.14.3

Re: [Qemu-block] [Qemu-devel] [PATCH] block: early check for blockers on drive-mirror

2018-02-12 Thread Kevin Wolf 
Am 12.02.2018 um 11:02 hat Daniel P. Berrangé geschrieben:
> On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> > On 10/02/2018 00:07, John Snow wrote:
> > >> +/* Early check to avoid creating target */
> > >> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> > >> +return;
> > >> +}
> > >> +
> > >>  aio_context = bdrv_get_aio_context(bs);
> > >>  aio_context_acquire(aio_context);
> > >>  
> > >>
> > > What's the implication of the temporarily-extant target node that it
> > > needs to be avoided so strictly?
> > > 
> > 
> > Creating a file on disk, that no one will ever remvoe. :)
> 
> Fortunately libvirt's SELinux policy will probably prevent QEMU creating
> it in the first place :-)

Well, calling drive-mirror without allowing QEMU to create the target
image would be a bit pointless, so I think we can assume that libvirt
did set up the file permission so that QEMU can create it. (Unless
mode=existing is used, but I understand that libvirt doesn't want to
create images with qemu-img, so that doesn't seem to be the case...)

I don't know if libvirt takes care to remove a potentially already
created file if the command then fails, but hopefully it does and the
patch is not actually needed with libvirt.

Kevin

Re: [Qemu-block] [Qemu-devel] [PULL 06/29] iotests: Make BD-{remove, insert}-medium use @id

2018-02-12 Thread Max Reitz 
On 2018-02-12 12:33, Christian Borntraeger wrote:
> Adding Max and Alberto,
> 
> I think the issue is that on s390 you cannot add a floppy

Thanks for letting me know.  I don't know why I resorted to floppy when
virtio would have worked (in this test) just as well...

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PULL 06/29] iotests: Make BD-{remove, insert}-medium use @id

2018-02-12 Thread Christian Borntraeger 
Adding Max and Alberto,

I think the issue is that on s390 you cannot add a floppy


On 02/12/2018 12:16 PM, Christian Borntraeger wrote:
> 
> On 01/23/2018 03:01 PM, Kevin Wolf wrote:
>> From: Max Reitz 
>>
>> In some cases, these commands still use the deprecated @device
>> parameter.  Fix that so we can later drop that parameter from their
>> interface.
>>
>> Signed-off-by: Max Reitz 
>> Message-id: 20171110224302.14424-2-mre...@redhat.com
>> Reviewed-by: Alberto Garcia 
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/118 | 184 
>> +++--
>>  tests/qemu-iotests/155 |  60 
> 
> This broke 155 on s390:
> 
> -...
> +EEE
> +==
> +ERROR: testFull (__main__.TestBlockdevMirrorBacking)
> +--
> +Traceback (most recent call last):
> +  File "155", line 68, in setUp
> +self.vm.launch()
> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", 
> line 203, in launch
> +self._post_launch()
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qtest.py", line 
> 100, in _post_launch
> +super(QEMUQtestMachine, self)._post_launch()
> +  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", 
> line 181, in _post_launch
> +self._qmp.accept()
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
> 157, in accept
> +return self.__negotiate_capabilities()
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
> 75, in __negotiate_capabilities
> +resp = self.cmd('qmp_capabilities')
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
> 191, in cmd
> +return self.cmd_obj(qmp_cmd)
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
> 174, in cmd_obj
> +resp = self.__json_read()
> +  File 
> "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
> 82, in __json_read
> +data = self.__sockfile.readline()
> +  File "/usr/lib64/python2.7/socket.py", line 451, in readline
> +data = self._sock.recv(self._rbufsize)
> +error: [Errno 104] Connection reset by peer
> [...]
> 
> 
> 
>>  2 files changed, 113 insertions(+), 131 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
>> index 8a9e838c90..ca6965d23c 100755
>> --- a/tests/qemu-iotests/118
>> +++ b/tests/qemu-iotests/118
>> @@ -28,6 +28,14 @@ from iotests import qemu_img
>>  old_img = os.path.join(iotests.test_dir, 'test0.img')
>>  new_img = os.path.join(iotests.test_dir, 'test1.img')
>>
>> +def interface_to_device_name(interface):
>> +if interface == 'ide':
>> +return 'ide-cd'
>> +elif interface == 'floppy':
>> +return 'floppy'
>> +else:
>> +return None
>> +
>>  class ChangeBaseClass(iotests.QMPTestCase):
>>  has_opened = False
>>  has_closed = False
>> @@ -63,7 +71,7 @@ class ChangeBaseClass(iotests.QMPTestCase):
>>
>>  class GeneralChangeTestsBaseClass(ChangeBaseClass):
>>
>> -device_name = None
>> +device_name = 'qdev0'
>>
>>  def test_change(self):
>>  result = self.vm.qmp('change', device='drive0', target=new_img,
>> @@ -79,14 +87,9 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>>  self.assert_qmp(result, 'return[0]/inserted/image/filename', 
>> new_img)
>>
>>  def test_blockdev_change_medium(self):
>> -if self.device_name is not None:
>> -result = self.vm.qmp('blockdev-change-medium',
>> - id=self.device_name, filename=new_img,
>> - format=iotests.imgfmt)
>> -else:
>> -result = self.vm.qmp('blockdev-change-medium',
>> - device='drive0', filename=new_img,
>> - format=iotests.imgfmt)
>> +result = self.vm.qmp('blockdev-change-medium',
>> + id=self.device_name, filename=new_img,
>> + format=iotests.imgfmt)
>>
>>  self.assert_qmp(result, 'return', {})
>>
>> @@ -99,10 +102,7 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>>  self.assert_qmp(result, 'return[0]/inserted/image/filename', 
>> new_img)
>>
>>  def test_eject(self):
>> -if self.device_name is not None:
>> -result = self.vm.qmp('eject', id=self.device_name, force=True)
>> -else:
>> -result = self.vm.qmp('eject', device='drive0', force=True)
>> +result = self.vm.qmp('eject', id=self.device_name, force=True)
>>  self.assert_qmp(result, 'return', {})
>>
>>  self.wait_for_open()
>> @@ -113,10 +113,7 @@ class

Re: [Qemu-block] [Qemu-devel] [PULL 06/29] iotests: Make BD-{remove, insert}-medium use @id

2018-02-12 Thread Christian Borntraeger 

On 01/23/2018 03:01 PM, Kevin Wolf wrote:
> From: Max Reitz 
> 
> In some cases, these commands still use the deprecated @device
> parameter.  Fix that so we can later drop that parameter from their
> interface.
> 
> Signed-off-by: Max Reitz 
> Message-id: 20171110224302.14424-2-mre...@redhat.com
> Reviewed-by: Alberto Garcia 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/118 | 184 
> +++--
>  tests/qemu-iotests/155 |  60 

This broke 155 on s390:

-...
+EEE
+==
+ERROR: testFull (__main__.TestBlockdevMirrorBacking)
+--
+Traceback (most recent call last):
+  File "155", line 68, in setUp
+self.vm.launch()
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", 
line 203, in launch
+self._post_launch()
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qtest.py", 
line 100, in _post_launch
+super(QEMUQtestMachine, self)._post_launch()
+  File "/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qemu.py", 
line 181, in _post_launch
+self._qmp.accept()
+  File 
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
157, in accept
+return self.__negotiate_capabilities()
+  File 
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
75, in __negotiate_capabilities
+resp = self.cmd('qmp_capabilities')
+  File 
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
191, in cmd
+return self.cmd_obj(qmp_cmd)
+  File 
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
174, in cmd_obj
+resp = self.__json_read()
+  File 
"/home/cborntra/REPOS/qemu/tests/qemu-iotests/../../scripts/qmp/qmp.py", line 
82, in __json_read
+data = self.__sockfile.readline()
+  File "/usr/lib64/python2.7/socket.py", line 451, in readline
+data = self._sock.recv(self._rbufsize)
+error: [Errno 104] Connection reset by peer
[...]



>  2 files changed, 113 insertions(+), 131 deletions(-)
> 
> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
> index 8a9e838c90..ca6965d23c 100755
> --- a/tests/qemu-iotests/118
> +++ b/tests/qemu-iotests/118
> @@ -28,6 +28,14 @@ from iotests import qemu_img
>  old_img = os.path.join(iotests.test_dir, 'test0.img')
>  new_img = os.path.join(iotests.test_dir, 'test1.img')
> 
> +def interface_to_device_name(interface):
> +if interface == 'ide':
> +return 'ide-cd'
> +elif interface == 'floppy':
> +return 'floppy'
> +else:
> +return None
> +
>  class ChangeBaseClass(iotests.QMPTestCase):
>  has_opened = False
>  has_closed = False
> @@ -63,7 +71,7 @@ class ChangeBaseClass(iotests.QMPTestCase):
> 
>  class GeneralChangeTestsBaseClass(ChangeBaseClass):
> 
> -device_name = None
> +device_name = 'qdev0'
> 
>  def test_change(self):
>  result = self.vm.qmp('change', device='drive0', target=new_img,
> @@ -79,14 +87,9 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>  self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
> 
>  def test_blockdev_change_medium(self):
> -if self.device_name is not None:
> -result = self.vm.qmp('blockdev-change-medium',
> - id=self.device_name, filename=new_img,
> - format=iotests.imgfmt)
> -else:
> -result = self.vm.qmp('blockdev-change-medium',
> - device='drive0', filename=new_img,
> - format=iotests.imgfmt)
> +result = self.vm.qmp('blockdev-change-medium',
> + id=self.device_name, filename=new_img,
> + format=iotests.imgfmt)
> 
>  self.assert_qmp(result, 'return', {})
> 
> @@ -99,10 +102,7 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>  self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
> 
>  def test_eject(self):
> -if self.device_name is not None:
> -result = self.vm.qmp('eject', id=self.device_name, force=True)
> -else:
> -result = self.vm.qmp('eject', device='drive0', force=True)
> +result = self.vm.qmp('eject', id=self.device_name, force=True)
>  self.assert_qmp(result, 'return', {})
> 
>  self.wait_for_open()
> @@ -113,10 +113,7 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
>  self.assert_qmp_absent(result, 'return[0]/inserted')
> 
>  def test_tray_eject_change(self):
> -if self.device_name is not None:
> -result = self.vm.qmp('eject', id=self.device_name, force=True)
> -else:
> -result =

Re: [Qemu-block] [Qemu-devel] [PATCH] block: early check for blockers on drive-mirror

2018-02-12 Thread Daniel P . Berrangé 
On Mon, Feb 12, 2018 at 10:58:31AM +0100, Paolo Bonzini wrote:
> On 10/02/2018 00:07, John Snow wrote:
> >> +/* Early check to avoid creating target */
> >> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
> >> +return;
> >> +}
> >> +
> >>  aio_context = bdrv_get_aio_context(bs);
> >>  aio_context_acquire(aio_context);
> >>  
> >>
> > What's the implication of the temporarily-extant target node that it
> > needs to be avoided so strictly?
> > 
> 
> Creating a file on disk, that no one will ever remvoe. :)

Fortunately libvirt's SELinux policy will probably prevent QEMU creating
it in the first place :-)

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [Qemu-devel] [PATCH] block: early check for blockers on drive-mirror

2018-02-12 Thread Paolo Bonzini 
On 10/02/2018 00:07, John Snow wrote:
>> +/* Early check to avoid creating target */
>> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR_SOURCE, errp)) {
>> +return;
>> +}
>> +
>>  aio_context = bdrv_get_aio_context(bs);
>>  aio_context_acquire(aio_context);
>>  
>>
> What's the implication of the temporarily-extant target node that it
> needs to be avoided so strictly?
> 

Creating a file on disk, that no one will ever remvoe. :)

Paolo