[Qemu-block] [PATCH 3/6] virtio-blk: convert to virtqueue_map

[Michael S. Tsirkin]

Drop deprecated use of virtqueue_map_sg.

Signed-off-by: Michael S. Tsirkin 
---
 hw/block/virtio-blk.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8beb26b..3e230de 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -839,10 +839,7 @@ static int virtio_blk_load_device(VirtIODevice *vdev, 
QEMUFile *f,
 req->next = s->rq;
 s->rq = req;
 
-virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-req->elem.in_num, 1);
-virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-req->elem.out_num, 0);
+virtqueue_map(>elem);
 }
 
 return 0;
-- 
MST

Re: [Qemu-block] [PATCH v8 01/15] block: Add blk_remove_bs()

[Alberto Garcia]

On Mon 26 Oct 2015 09:39:05 PM CET, Max Reitz  wrote:
> +void blk_remove_bs(BlockBackend *blk)
> +{
> +blk_update_root_state(blk);
> +
> +blk->bs->blk = NULL;
> +bdrv_unref(blk->bs);
> +blk->bs = NULL;
> +}

I cannot think of any example out of the blue but I wonder if removing
the reference to the BDS (and possibly destroying it) while blk->bs is
still pointing to it could be a source of problems.

Berto

Re: [Qemu-block] [PATCH 3/4] ide: add support for cancelable read requests

[Peter Lieven]

Am 26.10.2015 um 11:39 schrieb Stefan Hajnoczi:

On Mon, Oct 12, 2015 at 02:27:24PM +0200, Peter Lieven wrote:

this patch adds a new aio readv compatible function which copies
all data through a bounce buffer. The benefit is that these requests
can be flagged as canceled to avoid guest memory corruption when
a canceled request is completed by the backend at a later stage.

If an IDE protocol wants to use this function it has to pipe
all read requests through ide_readv_cancelable and it may then
enable requests_cancelable in the IDEState.

If this state is enable we can avoid the blocking blk_drain_all
in case of a BMDMA reset.

Currently only read operations are cancelable thus we can only
use this logic for read-only devices.

Naming is confusing here.  Requests are already "cancelable" using
bdv_aio_cancel().

Please use a different name, for example "orphan" requests.  These are
requests that QEMU still knows about but the guest believes are
complete.  Or maybe "IDEBufferedRequest" since data is transferred
through a bounce buffer.


Signed-off-by: Peter Lieven 
---
  hw/ide/core.c | 54 ++
  hw/ide/internal.h | 16 
  hw/ide/pci.c  | 42 --
  3 files changed, 98 insertions(+), 14 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 317406d..24547ce 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -561,6 +561,59 @@ static bool ide_sect_range_ok(IDEState *s,
  return true;
  }
  
+static void ide_readv_cancelable_cb(void *opaque, int ret)

+{
+IDECancelableRequest *req = opaque;
+if (!req->canceled) {
+if (!ret) {
+qemu_iovec_from_buf(req->org_qiov, 0, req->buf, 
req->org_qiov->size);
+}
+req->org_cb(req->org_opaque, ret);
+}
+QLIST_REMOVE(req, list);
+qemu_vfree(req->buf);
+qemu_iovec_destroy(>qiov);
+g_free(req);
+}
+
+#define MAX_CANCELABLE_REQS 16
+
+BlockAIOCB *ide_readv_cancelable(IDEState *s, int64_t sector_num,
+ QEMUIOVector *iov, int nb_sectors,
+ BlockCompletionFunc *cb, void *opaque)
+{
+BlockAIOCB *aioreq;
+IDECancelableRequest *req;
+int c = 0;
+
+QLIST_FOREACH(req, >cancelable_requests, list) {
+c++;
+}
+if (c > MAX_CANCELABLE_REQS) {
+return NULL;
+}

A BH is probably needed here to schedule an cb(-EIO) call since this
function isn't supposed to return NULL if it's a direct replacement for
blk_aio_readv().


You mean sth like:

acb = qemu_aio_get(_em_aiocb_info, bs, cb, opaque);
acb->bh = aio_bh_new(bdrv_get_aio_context(bs), bdrv_aio_bh_cb, acb);
acb->ret = -EIO;
qemu_bh_schedule(acb->bh);

return >common;




+
+req = g_new0(IDECancelableRequest, 1);
+qemu_iovec_init(>qiov, 1);

It saves a g_new() call if you add a struct iovec field to
IDECancelableRequest and use qemu_iovec_init_external() instead of
qemu_iovec_init().

The qemu_iovec_destroy() calls must be dropped when an external struct
iovec is used.

The qemu_iovec_init_external() call must be moved after the
qemu_blockalign() and struct iovec setup below.


okay




+req->buf = qemu_blockalign(blk_bs(s->blk), iov->size);
+qemu_iovec_add(>qiov, req->buf, iov->size);
+req->org_qiov = iov;
+req->org_cb = cb;
+req->org_opaque = opaque;
+
+aioreq = blk_aio_readv(s->blk, sector_num, >qiov, nb_sectors,
+   ide_readv_cancelable_cb, req);
+if (aioreq == NULL) {
+qemu_vfree(req->buf);
+qemu_iovec_destroy(>qiov);
+g_free(req);
+} else {
+QLIST_INSERT_HEAD(>cancelable_requests, req, list);
+}
+
+return aioreq;
+}
+
  static void ide_sector_read(IDEState *s);
  
  static void ide_sector_read_cb(void *opaque, int ret)

@@ -805,6 +858,7 @@ void ide_start_dma(IDEState *s, BlockCompletionFunc *cb)
  s->bus->retry_unit = s->unit;
  s->bus->retry_sector_num = ide_get_sector(s);
  s->bus->retry_nsector = s->nsector;
+s->bus->s = s;

How is 's' different from 'unit' and 'retry_unit'?

The logic for switching between units is already a little tricky since
the guest can write to the hardware registers while requests are
in-flight.

Please don't duplicate "active unit" state, that increases the risk of
inconsistencies.

Can you use idebus_active_if() to get an equivalent IDEState pointer
without storing s?


That should be possible.




  if (s->bus->dma->ops->start_dma) {
  s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
  }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 05e93ff..ad188c2 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -343,6 +343,16 @@ enum ide_dma_cmd {
  #define ide_cmd_is_read(s) \
((s)->dma_cmd == IDE_DMA_READ)
  
+typedef struct IDECancelableRequest {

+QLIST_ENTRY(IDECancelableRequest) list;
+QEMUIOVector qiov;
+uint8_t *buf;
+QEMUIOVector 

Re: [Qemu-block] [PATCH v2 1/3] qemu-io: fix cvtnum lval types

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> cvtnum() returns int64_t: we should not be storing this
> result inside of an int.
> 
> In a few cases, we need an extra sprinkling of error handling
> where we expect to pass this number on towards a function that
> expects something smaller than int64_t.
> 
> Reported-by: Max Reitz 
> Signed-off-by: John Snow 
> Reviewed-by: Eric Blake 
> ---
>  qemu-io-cmds.c | 31 +--
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 6e5d1e4..704db89 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -642,10 +642,11 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>  int c, cnt;
>  char *buf;
>  int64_t offset;
> -int count;
> +int64_t count;
>  /* Some compilers get confused and warn if this is not initialized.  */
>  int total = 0;
> -int pattern = 0, pattern_offset = 0, pattern_count = 0;
> +int pattern = 0;
> +int64_t pattern_offset = 0, pattern_count = 0;
>  
>  while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
>  switch (c) {
> @@ -734,7 +735,7 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>  return 0;
>  }
>  if (count & 0x1ff) {
> -printf("count %d is not sector aligned\n",
> +printf("count %"PRId64" is not sector aligned\n",
> count);
>  return 0;
>  }
> @@ -762,7 +763,7 @@ static int read_f(BlockBackend *blk, int argc, char 
> **argv)
>  memset(cmp_buf, pattern, pattern_count);
>  if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
>  printf("Pattern verification failed at offset %"
> -   PRId64 ", %d bytes\n",
> +   PRId64 ", %"PRId64" bytes\n",
> offset + pattern_offset, pattern_count);
>  }
>  g_free(cmp_buf);

read_f calls a few helper function which only take an int for count:

do_pread(), do_load_vmstate(), do_read() actually perform the request.
These should probably take int64_t as well (and if we want to be really
careful to avoid wraparounds, check limits individually).

qemu_io_alloc() takes size_t, so will wrap around on 32 bit hosts.
Should take int64_t and check against SIZE_MAX.

dump_buffer() also only takes an int, but I hope nobody tries to dump
more than 2 GB...

print_report() should probably be fixed to take int64_t.

And for total to make sense, it probably needs to be converted to
int64_t as well.

> @@ -957,7 +958,7 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>  int c, cnt;
>  char *buf = NULL;
>  int64_t offset;
> -int count;
> +int64_t count;
>  /* Some compilers get confused and warn if this is not initialized.  */
>  int total = 0;
>  int pattern = 0xcd;
> @@ -1029,7 +1030,7 @@ static int write_f(BlockBackend *blk, int argc, char 
> **argv)
>  }
>  
>  if (count & 0x1ff) {
> -printf("count %d is not sector aligned\n",
> +printf("count %"PRId64" is not sector aligned\n",
> count);
>  return 0;
>  }

For writes, the helper functions to perform the request are different,
but they also only take int: do_pwrite(), do_save_vmstate(),
do_co_write_zeroes(), do_write_compressed(), do_write().

The rest should be fixed when you fix the helpers for read.

> @@ -1777,8 +1778,7 @@ static int discard_f(BlockBackend *blk, int argc, char 
> **argv)
>  struct timeval t1, t2;
>  int Cflag = 0, qflag = 0;
>  int c, ret;
> -int64_t offset;
> -int count;
> +int64_t offset, count;
>  
>  while ((c = getopt(argc, argv, "Cq")) != -1) {
>  switch (c) {

Here, blk_discard() is called directly without a helper function. A
check that the number of sectors fits in an int is missing.

> @@ -1833,11 +1833,10 @@ out:
>  static int alloc_f(BlockBackend *blk, int argc, char **argv)
>  {
>  BlockDriverState *bs = blk_bs(blk);
> -int64_t offset, sector_num;
> -int nb_sectors, remaining;
> +int64_t offset, sector_num, nb_sectors, remaining;
>  char s1[64];
> -int num, sum_alloc;
> -int ret;
> +int num, ret;
> +int64_t sum_alloc;
>  
>  offset = cvtnum(argv[1]);
>  if (offset < 0) {
> @@ -1881,7 +1880,7 @@ static int alloc_f(BlockBackend *blk, int argc, char 
> **argv)
>  
>  cvtstr(offset, s1, sizeof(s1));
>  
> -printf("%d/%d sectors allocated at offset %s\n",
> +printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
> sum_alloc, nb_sectors, s1);
>  return 0;
>  }

remaining is passed to bdrv_is_allocated() without checking against
INT_MAX first.

Kevin

Re: [Qemu-block] [PATCH v5 5/6] block: Drop BlockDriverState.filename

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> That field is now only used during initialization of BlockDriverStates
> (opening images) and for error or warning messages. Performance is not
> that much of an issue here, so we can drop the field and replace its use
> by a call to bdrv_filename(). By doing so we can ensure the result
> always to be recent, whereas the contents of BlockDriverState.filename
> may have been obsoleted by manipulations of single BlockDriverStates or
> of the BDS graph.
> 
> The users of the BDS filename field were changed as follows:
> - copying the filename into another buffer is trivially replaced by
>   using bdrv_filename() instead of the copy function
> - strdup() on the filename is replaced by a call to
>   bdrv_filename(filename, NULL, 0)
> - bdrv_filename(bs, bs->filename, sizeof(bs->filename)) is replaced by
>   bdrv_refresh_filename(bs)
>   (these were introduced by the patch before the previous patch)
> - anywhere else bdrv_filename(..., NULL, 0) is used, any access to
>   BlockDriverState.filename is then replaced by the buffer returned, and
>   it is freed when it is no longer needed
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v8 02/15] block: Make bdrv_states public

[Alberto Garcia]

On Mon 26 Oct 2015 09:39:06 PM CET, Max Reitz wrote:
> When inserting a BDS tree into a BB, we will need to add the root BDS to
> this list. Since we will want to do that in the blockdev-insert-medium
> implementation in blockdev.c, we will need access to it there.
>
> This patch is not exactly elegant, but bdrv_states will be removed in
> the future anyway because we no longer need it since we have BBs.
>
> Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH v5 6/6] iotests: Test changed Quorum filename

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> After drive-mirror replacing a Quorum child, the filename of the Quorum
> BDS should reflect the change. This patch replaces the existing test for
> whether the operation did actually exchange the BDS (which simply tested
> whether the new BDS existed) by a test which examines the children list
> contained in the Quorum BDS's filename as returned by query-block.
> 
> As a nice side effect of confirming that the new BDS actually belongs to
> the Quorum BDS, this checks whether the filename was properly updated.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 2/3] qemu-io: Check for trailing chars

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> Make sure there's not trailing garbage, e.g.
> "64k-whatever-i-want-here"
> 
> Reported-by: Max Reitz 
> Signed-off-by: John Snow 
> Reviewed-by: Eric Blake 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

Am 27.10.2015 um 00:45 hat John Snow geschrieben:
> Reported-by: Max Reitz 
> Signed-off-by: John Snow 
> Reviewed-by: Eric Blake 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

Am 27.10.2015 um 03:26 hat Eric Blake geschrieben:
> On 10/26/2015 05:45 PM, John Snow wrote:
> > Reported-by: Max Reitz 
> > Signed-off-by: John Snow 
> > Reviewed-by: Eric Blake 
> > ---
> >  qemu-io-cmds.c | 53 ++---
> >  1 file changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > index 44d24e8..92c6b87 100644
> > --- a/qemu-io-cmds.c
> > +++ b/qemu-io-cmds.c
> > @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
> >  return ret;
> >  }
> >  
> > +static void print_cvtnum_err(int64_t rc, const char *arg)
> > +{
> > +switch (rc) {
> > +case -EINVAL:
> > +printf("Parsing error: non-numeric argument,"
> > +   " or extraneous/unrecognized suffix -- %s\n", arg);
> > +break;
> > +case -ERANGE:
> > +printf("Parsing error: argument too large -- %s\n", arg);
> > +break;
> > +default:
> > +printf("Parsing error -- %s\n", arg);
> 
> I still think ':' is better than ' --' in error messages, but I'll leave
> it up to the maintainer.

This isn't important enough for a maintainer decision - if this isn't
something that the patch submitter can decide by himself, what else
would be left? In particular because the patch only retains the existing
format. I'm happy to merge a patch that uses colons instead, but I won't
reject anything just because it doesn't do the conversion.

Kevin


pgp5NcMWTkGXm.pgp
Description: PGP signature

[Qemu-block] [PATCH 1/1] qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

[Alberto Garcia]

The 'block-commit' command needs the overlay image of 'top' to
be opened in read-write mode in order to update the backing file
string. If 'top' is not the active layer or its backing file then its
overlay needs to be reopened during the block job.

This is a test case for that scenario.

Signed-off-by: Alberto Garcia 
---
 tests/qemu-iotests/040 | 30 ++
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index ea2f98e..5bdaf3d 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -41,6 +41,7 @@ class ImageCommitTestCase(iotests.QMPTestCase):
 while not completed:
 for event in self.vm.get_qmp_events(wait=True):
 if event['event'] == 'BLOCK_JOB_COMPLETED':
+self.assert_qmp_absent(event, 'data/error')
 self.assert_qmp(event, 'data/type', 'commit')
 self.assert_qmp(event, 'data/device', 'drive0')
 self.assert_qmp(event, 'data/offset', event['data']['len'])
@@ -251,5 +252,34 @@ class TestSetSpeed(ImageCommitTestCase):
 class TestActiveZeroLengthImage(TestSingleDrive):
 image_len = 0
 
+class TestReopenOverlay(ImageCommitTestCase):
+image_len = 1024 * 1024
+img0 = os.path.join(iotests.test_dir, '0.img')
+img1 = os.path.join(iotests.test_dir, '1.img')
+img2 = os.path.join(iotests.test_dir, '2.img')
+img3 = os.path.join(iotests.test_dir, '3.img')
+
+def setUp(self):
+iotests.create_image(self.img0, self.image_len)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
self.img0, self.img1)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
self.img1, self.img2)
+qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
self.img2, self.img3)
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xab 0 128K', self.img1)
+self.vm = iotests.VM().add_drive(self.img3)
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+os.remove(self.img0)
+os.remove(self.img1)
+os.remove(self.img2)
+os.remove(self.img3)
+
+# This tests what happens when the overlay image of the 'top' node
+# needs to be reopened in read-write mode in order to update the
+# backing image string.
+def test_reopen_overlay(self):
+self.run_commit_test(self.img1, self.img0)
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 42314e9..4fd1c2d 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-
+.
 --
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
2.6.1

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[John Snow]

On 10/26/2015 10:26 PM, Eric Blake wrote:
> On 10/26/2015 05:45 PM, John Snow wrote:
>> Reported-by: Max Reitz 
>> Signed-off-by: John Snow 
>> Reviewed-by: Eric Blake 
>> ---
>>  qemu-io-cmds.c | 53 ++---
>>  1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index 44d24e8..92c6b87 100644
>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
>>  return ret;
>>  }
>>  
>> +static void print_cvtnum_err(int64_t rc, const char *arg)
>> +{
>> +switch (rc) {
>> +case -EINVAL:
>> +printf("Parsing error: non-numeric argument,"
>> +   " or extraneous/unrecognized suffix -- %s\n", arg);
>> +break;
>> +case -ERANGE:
>> +printf("Parsing error: argument too large -- %s\n", arg);
>> +break;
>> +default:
>> +printf("Parsing error -- %s\n", arg);
> 
> I still think ':' is better than ' --' in error messages, but I'll leave
> it up to the maintainer.
> 

Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
just trying to match the existing format. I can change it and send again
if you want. Whatever is easiest for people.

--js

Re: [Qemu-block] [PATCH v8 0/5] Add 'blockdev-snapshot' command

[Kevin Wolf]

Am 26.10.2015 um 13:27 hat Alberto Garcia geschrieben:
> This series adds a new 'blockdev-snapshot' command, that is similar to
> 'blockdev-snapshot-sync' but takes references to two existing block
> devices.
> 
> This finally applies (and works) cleanly on master. Max's patch to
> allow creating BlockDriverState trees without a BlockBackend is now
> available as be4b67bc7d99da26b7878f7f45370f50a3bd4af5.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Kevin Wolf]

Am 27.10.2015 um 16:50 hat John Snow geschrieben:
> 
> 
> On 10/26/2015 10:26 PM, Eric Blake wrote:
> > On 10/26/2015 05:45 PM, John Snow wrote:
> >> Reported-by: Max Reitz 
> >> Signed-off-by: John Snow 
> >> Reviewed-by: Eric Blake 
> >> ---
> >>  qemu-io-cmds.c | 53 ++---
> >>  1 file changed, 34 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> >> index 44d24e8..92c6b87 100644
> >> --- a/qemu-io-cmds.c
> >> +++ b/qemu-io-cmds.c
> >> @@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
> >>  return ret;
> >>  }
> >>  
> >> +static void print_cvtnum_err(int64_t rc, const char *arg)
> >> +{
> >> +switch (rc) {
> >> +case -EINVAL:
> >> +printf("Parsing error: non-numeric argument,"
> >> +   " or extraneous/unrecognized suffix -- %s\n", arg);
> >> +break;
> >> +case -ERANGE:
> >> +printf("Parsing error: argument too large -- %s\n", arg);
> >> +break;
> >> +default:
> >> +printf("Parsing error -- %s\n", arg);
> > 
> > I still think ':' is better than ' --' in error messages, but I'll leave
> > it up to the maintainer.
> 
> Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
> just trying to match the existing format. I can change it and send again
> if you want. Whatever is easiest for people.

I think you need to respin for patch 1 anyway, so changing it in the
next version sounds good. You can keep my R-b when doing this.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/3] qemu-io: Correct error messages

[Eric Blake]

On 10/27/2015 09:50 AM, John Snow wrote:

>>> +default:
>>> +printf("Parsing error -- %s\n", arg);
>>
>> I still think ':' is better than ' --' in error messages, but I'll leave
>> it up to the maintainer.
>>
> 
> Crud, sorry Eric -- I didn't do this on purpose. As Kevin notes, I was
> just trying to match the existing format. I can change it and send again
> if you want. Whatever is easiest for people.

And Kevin has a valid point that you just did code motion, so keeping --
is no worse than before.  At this point, I'll leave it up to you; my R-b
stands either way.

-- 
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] ide: remove hardcoded 2GiB transactional limit

[Stefan Hajnoczi]

On Mon, Oct 26, 2015 at 07:38:02PM -0400, John Snow wrote:
> Not that you can request a >2GiB transaction, but that's why checking
> for it makes no sense anymore.
> 
> With the newer 'limit' parameter to prepare_buf, we no longer need a
> static limit. The maximum limit is still 2GiB, but the limit parameter
> is set to the current transaction size, which cannot surpass 32MiB
> (512 * 65536). If the PRDT surpasses the transactional size, then,
> we'll just carry out the normative underflow handling pathways instead
> of needing an extra, strange pathway that worries about hitting some
> logistical cap for the largest sglist we can support -- we'll never
> even attempt to build one that big anymore.
> 
> Reported-by: Kevin Wolf 
> Signed-off-by: John Snow 
> ---
>  hw/ide/ahci.c | 30 ++
>  hw/ide/internal.h |  2 +-
>  hw/ide/pci.c  |  7 ---
>  3 files changed, 15 insertions(+), 24 deletions(-)

Acked-by: Stefan Hajnoczi 

[Qemu-block] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Alberto Garcia]

Hi, looks like we have a bug in the bdrv_reopen() code.

It turns out that 'block-commit' fails if the 'top' node is not the
active layer or its immediate backing file, and none of our test cases
has detected that. I'm attaching one that reproduces the problem.

What happens is that 'block-commit' reopens the overlay of the top
node in read-write mode in order to update the backing file string. In
addition to that, the 'base' image also needs to be reopened in r/w.

Here's the relevant code from commit_start():

if (!(orig_base_flags & BDRV_O_RDWR)) {
reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
 orig_base_flags | BDRV_O_RDWR);
}
if (!(orig_overlay_flags & BDRV_O_RDWR)) {
reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
 orig_overlay_flags | BDRV_O_RDWR);
}
if (reopen_queue) {
bdrv_reopen_multiple(reopen_queue, _err);
/*...*/
}

'base' is reopened first in r/w mode, then 'overlay_bs'. However it
seems that the latter has the side effect or reopening 'base' again in
read-only mode, therefore the job ends up failing with -EPERM.

Just swapping the order of the bdrv_reopen_queue() calls is enough to
fix the problem, but I'm sure this needs deeper changes in the
bdrv_reopen() code instead.

Berto

Alberto Garcia (1):
  qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

 tests/qemu-iotests/040 | 30 ++
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.6.1

Re: [Qemu-block] [PATCH v8 00/15] blockdev: BlockBackend and media

[Kevin Wolf]

Am 26.10.2015 um 21:39 hat Max Reitz geschrieben:
> Now that the main rework part of this series is merged, these remaining
> patches here implement atomic tray/medium operations and add the
> read-only-mode parameter to change and blockdev-change-medium (which was
> the original purpose of this series!).
> 
> Once again, I'd like to thank all the reviewers for sticking to this
> long series over many iterations. Thanks a lot! :-)

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v8 06/15] blockdev: Add blockdev-remove-medium

[Kevin Wolf]

Am 26.10.2015 um 21:39 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 

> +if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
> +goto out;
> +}
> +
> +/* This follows the convention established by bdrv_make_anon() */
> +if (bs->device_list.tqe_prev) {
> +QTAILQ_REMOVE(_states, bs, device_list);
> +bs->device_list.tqe_prev = NULL;
> +}
> +
> +blk_remove_bs(blk);

Wouldn't it be nicer to move the bdrv_states update into
blk_remove_bs() and blk_insert_bs()? Can be done on top of this series,
though, if you don't need to respin for another reason.

Kevin