Re: [Qemu-block] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/7 14:37, Fam Zheng 写道:

On Tue, 03/07 03:53, QingFeng Hao wrote:

Hi All,
I am not sure if the fix is correct because I am not very clear about the
logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
due to the assert. So just send this draft patch for your comments!
The patch is based on commit 17783ac828a "Merge remote-tracking branch
'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".

I cannot reproduce the failure on either 17783ac828a or current head
56b51708e9e. Both passes for me. I wonder where do you get the size=0.
The error happens when running "savevm 0" in case 068. It can be 
manually reproduced

by "./check -qcow2 68" or "./s390x-softmmu/qemu-system-s390x -nodefaults \
-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak", then 
type

"savevm 0".  t.img.bak is the backup image for t.img generated by 068.

I added the print in vmstate_save_state:
   QJSON *vmdesc_loop = vmdesc;
+ error_report("haoqf:%s:opaque:%p, offset:%lx, size:%d, field name:%s, 
vname:%s\n", __FUNCTION__, opaque, field->offset, size, field->name, 
vmsd->name);


And here is the test log:
haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122e1, size:1, 
field name:env.sigp_order, vname:cpu


haoqf:vmstate_size: field size:4, offset:0

haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:12300, size:4, 
field name:irqstate_saved_size, vname:cpu


haoqf:vmstate_size: field size:0, offset:74496

haoqf:vmstate_size: calculated size:0

haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122f8, size:0, 
field name:irqstate, vname:cpu


haoqf:vmstate_save_state:firstelem:(nil), elements: 1

qemu-system-s390x: ../migration/vmstate.c:336: vmstate_save_state: 
Assertion `first_elem || !n_elems' failed.

Aborted (core dumped)

I also did the test for x86 with: "./x86_64-softmmu/qemu-system-x86_64 
-nodefaults \

-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak",
and then ran "savevm 0", but it didn't core and the size are all non-zero:
haoqf:vmstate_save calling vmstate_save_state:

haoqf:vmstate_size: field size:4, offset:0

haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:4, size:4, field 
name:size, vname:globalstate


haoqf:vmstate_size: field size:100, offset:0

haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:8, size:100, field 
name:runstate, vname:globalstate


haoqf:vmstate_save:called vmstate_save_state

So probably x86 doesn't have this problem.

Fam



--
Regards
QingFeng Hao

Re: [Qemu-block] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[Fam Zheng]

On Tue, 03/07 03:53, QingFeng Hao wrote:
> Hi All,
> I am not sure if the fix is correct because I am not very clear about the
> logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
> due to the assert. So just send this draft patch for your comments!
> The patch is based on commit 17783ac828a "Merge remote-tracking branch
> 'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".

I cannot reproduce the failure on either 17783ac828a or current head
56b51708e9e. Both passes for me. I wonder where do you get the size=0.

Fam

[Qemu-block] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[QingFeng Hao]

Hi All,
I am not sure if the fix is correct because I am not very clear about the
logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
due to the assert. So just send this draft patch for your comments!
The patch is based on commit 17783ac828a "Merge remote-tracking branch
'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".
Thanks!

QingFeng Hao (1):
  vmstate: fix the failure of iotests cases 68 and 91

 migration/vmstate.c | 8 
 1 file changed, 8 insertions(+)

-- 
2.8.4

[Qemu-block] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

I am not very clear about the logic in vmstate.c, but from its context in
vmstate_save_state, it seems size should not be 0, otherwise the followed
for loop will keep working on the same element. So I just add a simple
check to pass that case, not sure if it's right but it can pass iotest
case 68 and 91 now.

The iotest's failed output is:
068 1s ... - output mismatch (see 068.out.bad)
--- 
/home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out
   2017-03-06 05:52:24.817328899 +0100
+++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
@@ -3,9 +3,13 @@
 === Saving and reloading a VM state to/from a qcow2 image ===

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion 
`first_elem || !n_elems' failed.
+./common.config: line 109: 52497 Aborted ( if [ -n 
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
-(qemu) quit
+qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) quit
 *** done

091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
--- tests/qemu-iotests/091.out  2016-08-30 12:35:04.207683276 +0200
+++ 091.out.bad 2017-03-06 13:08:03.717135426 +0100
@@ -11,18 +11,23 @@
 
 vm1: qemu-io disk write complete
 vm1: live migration started
-vm1: live migration completed
-
-=== VM 2: Post-migration, write to disk, verify running ===
-
-vm2: qemu-io disk write complete
-vm2: qemu process running successfully
-vm2: flush io, and quit
-Check image pattern
-read 4194304/4194304 bytes at offset 0
-4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Running 'qemu-img check -r all $TEST_IMG'
-No errors were found on the image.
-80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
-Image end offset: 5570560
-*** done
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+Timeout waiting for completed on handle 0

Signed-off-by: QingFeng Hao 
---
 migration/vmstate.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..ff28dde 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 int i, n_elems = vmstate_n_elems(opaque, field);
 int size = vmstate_size(opaque, field);
 
+if (size == 0) {
+field++;
+continue;
+}
 vmstate_handle_alloc(first_elem, field, opaque);
 if (field->flags & VMS_POINTER) {
 first_elem = *(void **)first_elem;
@@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 int64_t old_offset, written_bytes;
 QJSON *vmdesc_loop = vmdesc;
 
+if (size == 0) {
+field++;
+continue;
+}
 trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
 if (field->flags & VMS_POINTER) {
 first_elem = *(void **)first_elem;
-- 
2.8.4

[Qemu-block] [PATCH] file-posix: Incoporate max_segments in block limit

[Fam Zheng]

Linux exposes a separate limit, /sys/block/.../queue/max_segments, which
in the worst case can be more restrictive than BLKSECTGET (as they are
two different things). Similar to the BLKSECTGET story, guests don't see
this limit and send big requests will get -EINVAL error on SG_IO.

Lean on the safer side to clamp max_transfer according to max_segments
and page size, because in the end what host HBA gets is the mapped host
pages rather than a guest buffer.

Signed-off-by: Fam Zheng 
---
 block/file-posix.c | 58 ++
 1 file changed, 58 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 4de1abd..b615262 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -668,6 +668,59 @@ static int hdev_get_max_transfer_length(BlockDriverState 
*bs, int fd)
 #endif
 }
 
+static int hdev_get_max_segments(BlockDriverState *bs)
+{
+#ifdef CONFIG_LINUX
+char buf[32];
+const char *end;
+char *sysfspath, *fullpath;
+int ret;
+int fd = -1;
+long max_segments;
+
+fullpath = realpath(bs->exact_filename, NULL);
+if (!fullpath) {
+return -errno;
+}
+if (strncmp(fullpath, "/dev/", 5) || !fullpath[5]) {
+ret = -ENOTSUP;
+goto out;
+}
+sysfspath = g_strdup_printf("/sys/block/%s/queue/max_segments",
+[5]);
+fd = open(sysfspath, O_RDONLY);
+if (fd == -1) {
+ret = -errno;
+goto out;
+}
+do {
+ret = read(fd, buf, sizeof(buf));
+} while (ret == -1 && errno == EINTR);
+if (ret < 0) {
+ret = -errno;
+goto out;
+} else if (ret == 0) {
+ret = -EIO;
+goto out;
+}
+buf[ret] = 0;
+/* The file is ended with '\n', pass 'end' to accept that. */
+ret = qemu_strtol(buf, , 10, _segments);
+if (ret == 0 && end && *end == '\n') {
+ret = max_segments;
+}
+
+out:
+if (fd >= 0) {
+close(fd);
+}
+free(fullpath);
+return ret;
+#else
+return -ENOTSUP;
+#endif
+}
+
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
@@ -679,6 +732,11 @@ static void raw_refresh_limits(BlockDriverState *bs, Error 
**errp)
 if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
 bs->bl.max_transfer = pow2floor(ret);
 }
+ret = hdev_get_max_segments(bs);
+if (ret > 0) {
+bs->bl.max_transfer = MIN(bs->bl.max_transfer,
+  ret * getpagesize());
+}
 }
 }
 
-- 
2.9.3

Re: [Qemu-block] [PATCH v5 1/7] qcow2: Assert that cluster operations are aligned

[Eric Blake]

On 02/15/2017 06:25 AM, Kevin Wolf wrote:
> Am 14.02.2017 um 20:25 hat Eric Blake geschrieben:
>> qcow2_discard_clusters() is set up to silently ignore sub-cluster
>> head or tail on unaligned requests.  However, it is easy to audit
>> the various callers: qcow2_snapshot_create() has always passed
>> aligned data since the call was introduced in 1ebf561;
>> qcow2_co_pdiscard() has passed aligned clusters since commit
>> ecdbead taught the block layer the preferred discard alignment (the
>> block layer can still pass sub-cluster values, but those are
>> handled directly in qcow2_co_pdiscard()); and qcow2_make_empty()
>> was fixed to pass aligned clusters in commit a3e1505.
> 
> I don't think this is true for the very part in the image if the image
> size isn't cluster aligned:
> 
> ret = qcow2_discard_clusters(bs, start_sector * BDRV_SECTOR_SIZE,
>  MIN(sector_step,
>  bs->total_sectors - start_sector),
>  QCOW2_DISCARD_SNAPSHOT, true);
> 
> sector_step is alright after commit a3e1505, but bs->total_sectors can
> be unaligned.

Indeed; a slight tweak to qemu-iotests/097 exposes the problem.  I'll
include that in my next spin (although I note that Dan has patches on
the list to split 097 into two tests, in order to give qcow coverage
rather than just qcow2).

-- 
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 0/4] some migration bugs

[Denis V. Lunev]

On 02/25/2017 10:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here are some migration related bugs, two about INACTIVE bdses and one
> use-after-free.
>
> I'm absolutely not sure, that these bugs should be fixed like I'm fixing,
> but problem definitely exists.
>
> Reset in stopped state is strange case, may be such usage should be
> restricted.
> About INACTIVE - looks like it should be a separate run-state, not only
> bdrv-flag.
> Situation with migration state, which is global, but is set/reset/changed
> in not controlled manner is not very good too..
>
> Vladimir Sementsov-Ogievskiy (4):
>   iotests: add migration corner cases test
>   qmp-cont: invalidate on RUN_STATE_PRELAUNCH
>   savevm: fix savevm after migration
>   migration: fix use-after-free of to_dst_file
>
>  block/snapshot.c   |  3 +-
>  migration/savevm.c | 16 +++
>  qmp.c  |  3 +-
>  tests/qemu-iotests/175 | 71 
> ++
>  tests/qemu-iotests/175.out |  5 
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 97 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemu-iotests/175
>  create mode 100644 tests/qemu-iotests/175.out
>
guys, what about patches 1-3?

Den

Re: [Qemu-block] [Qemu-devel] [PATCH 10/10] block: Fix error handling in bdrv_replace_in_backing_chain()

[Eric Blake]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> When adding an Error parameter, bdrv_replace_in_backing_chain() would
> become nothing more than a wrapper around change_parent_backing_link().
> So make the latter public, renamed as bdrv_replace_node(), and remove
> bdrv_replace_in_backing_chain().
> 
> Most of the callers just remove a node from the graph that they just
> inserted, so they can use _abort, but completion of a mirror job
> with 'replaces' set can actually fail.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 25 ++---
>  block/mirror.c| 15 +--
>  blockdev.c|  2 +-
>  include/block/block.h |  4 ++--
>  include/block/block_int.h |  4 ++--
>  5 files changed, 20 insertions(+), 30 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 09/10] block: Handle permission errors in change_parent_backing_link()

[Eric Blake]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> Instead of just trying to change parents by parent over to reference @to
> instead of @from, and abort()ing whenever the permissions don't allow
> this, do proper permission checking beforehand and pass any error to the
> callers.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 50 --
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 

> +/* Now actually perform the change. We performed the permission check for
> + * all elements of @list at once, so set the permissions all at once at 
> the
> + * very end. */
> +for (p = list; p != NULL; p = p->next) {
> +c = p->data;
>  
>  bdrv_ref(to);
> -/* FIXME Are we sure that bdrv_replace_child() can't run into
> - * _abort because of permissions? */
> -bdrv_replace_child(c, to, true);
> +bdrv_replace_child_noperm(c, to);
>  bdrv_unref(from);

Looks awesome to get rid of a FIXME...

> @@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState 
> *old, BlockDriverState *new)
>  
>  bdrv_ref(old);
>  
> -change_parent_backing_link(old, new);
> +/* FIXME Proper error handling */
> +change_parent_backing_link(old, new, _abort);

...until this shows we are merely trimming it down to one less instance,
but are still fighting the overall battle.  But we'll get there in the
end :)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[Max Reitz]

On 06.03.2017 20:58, no-re...@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20170306195255.15806-1-mre...@redhat.com
> Subject: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, 
> drv}_turncate()
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]  
> patchew/1488826849-32384-1-git-send-email-arm...@redhat.com -> 
> patchew/1488826849-32384-1-git-send-email-arm...@redhat.com
>  * [new tag] patchew/20170306195255.15806-1-mre...@redhat.com -> 
> patchew/20170306195255.15806-1-mre...@redhat.com
> Auto packing the repository for optimum performance. You may also
> run "git gc" manually. See "git help gc" for more information.
> Switched to a new branch 'test'
> 4ef1757 block: Add some bdrv_truncate() error messages
> 3e348c6 block: Add errp to BD.bdrv_truncate()
> 1ba2c64 block: Add errp to b{lk, drv}_truncate()
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: block: Add errp to b{lk, drv}_truncate()...
> Checking PATCH 2/3: block: Add errp to BD.bdrv_truncate()...
> Checking PATCH 3/3: block: Add some bdrv_truncate() error messages...
> ERROR: suspect code indent for conditional statements (7, 11)
> #35: FILE: block/file-posix.c:1358:
> if (offset > raw_getlength(bs)) {
> +   error_setg(errp, "Cannot grow device files");
> 
> total: 1 errors, 0 warnings, 73 lines checked

Surrounding conditional block is not correctly aligned; pre-existing,
didn't notice. Will fix in v2.

Max

> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-de...@freelists.org
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 08/10] block: Ignore multiple children in bdrv_check_update_perm()

[Eric Blake]

On 03/06/2017 10:22 AM, Kevin Wolf wrote:
> change_parent_backing_link() will need to update multiple BdrvChild
> objects at once. Checking permissions reference by reference doesn't
> work because permissions need to be consistent only with all parents
> moved to the new child.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 35 ++-
>  include/block/block_int.h |  2 +-
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 


>  static int bdrv_check_update_perm(BlockDriverState *bs, uint64_t 
> new_used_perm,
>uint64_t new_shared_perm,
> -  BdrvChild *ignore_child, Error **errp)
> +  GSList *ignore_children, Error **errp)
>  {
>  BdrvChild *c;
>  uint64_t cumulative_perms = new_used_perm;
> @@ -1572,7 +1574,7 @@ static int bdrv_check_update_perm(BlockDriverState *bs, 
> uint64_t new_used_perm,
>  assert(new_shared_perm & BLK_PERM_WRITE_UNCHANGED);
>  
>  QLIST_FOREACH(c, >parents, next_parent) {
> -if (c == ignore_child) {
> +if (g_slist_find(ignore_children, c)) {

Quadratic complexity (searching one list for each element of another
list). Hopefully the combination of lists isn't too long in practice (it
might be a potential problem if someone creates 200 snapshots then tries
to commit them down to 1 image in a single operation, where
ignore_children would then be a long list of intermediate children being
iterated on multiple times, but I don't know if bs->parents can easily
be made as long).  We could always create some sort of hash table to
reduce complexity, as a followup patch, if it turns out to be a problem
in practice, but for now I'm okay with it.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

On 03/06/2017 02:48 PM, Max Reitz wrote:
> On 06.03.2017 21:21, Eric Blake wrote:
>> On 03/06/2017 02:14 PM, Max Reitz wrote:
>>
>  if (S_ISREG(st.st_mode)) {
>  if (ftruncate(s->fd, offset) < 0) {
> +/* The generic error message will be fine */
>  return -errno;

 Relying on a generic error message in the caller is awkward. I see it as
 evidence of a partial conversion if we have an interface that requires a
 return of a negative errno value to make up for when errp is not set.
>>>
>>> I'm not sure what you mean by this exactly.
>>
>> The ideal conversion would be having .bdrv_truncate switch to a void
>> return; then no caller is messing with negative errno values (especially
>> since some of them are just synthesizing errno values, rather than
>> actually encountering one, and since some of them are probably using -1
>> when they should be using errno). But such a conversion requires that
>> all drivers be updated to properly set errp.
> 
> Not sure if that is an ideal conversion. I very much prefer negative
> return value + error object because that allows constructs like
> 
> if (foo(..., errp) < 0) {
> return;
> }

Fair point - Markus has argued that we should convert functions from
void to easy-to-spot return sentinels for easier logic (less boilerplate
in creating a local error, checking it, and propagating it).

But the point remains that returning -1 is simpler to code than
returning negative errno, when some of the existing drivers are already
synthesizing errno.  And (temporarily) forcing a void return would make
it easy to spot who still needs conversion, even if we then go back to
returning int (but this time, with the simpler -1 for error, rather than
negative errno).

> For me, the most important thing is that errp will always be set if the
> function fails.

Yes, that's what you are guaranteed with a void return, and what we
would also have with a (sane) integer return.

> 
> (Of course, I'd be just fine with a boolean return value, too.)

boolean works too, except it's a little harder to remember when 'true'
means success, compared to other code where '0' is success and negative
is failure.

>> In other words, I view the generic bdrv_truncate() supplying an error
>> based on negative errno is a crutch, and not something that we should
>> rely on, at least not for the drivers that we fix to set errp directly.
> 
> Well, the more detailed the error messages are the better, but I don't
> see any real improvement over a generic message supplied by
> bdrv_truncate() if that is good enough.

Maybe Markus has some opinions, since error reporting is his area.

> 
> Of course, I can easily be convinced that the generic message is in fact
> not good enough.

Maybe it's hard to argue that from the quality-of-message standpoint,
but that's exactly what I'm arguing from the ease-of-maintenance
standpoint.  Relying on the generic message is harder to maintain.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

On 06.03.2017 21:21, Eric Blake wrote:
> On 03/06/2017 02:14 PM, Max Reitz wrote:
> 
  if (S_ISREG(st.st_mode)) {
  if (ftruncate(s->fd, offset) < 0) {
 +/* The generic error message will be fine */
  return -errno;
>>>
>>> Relying on a generic error message in the caller is awkward. I see it as
>>> evidence of a partial conversion if we have an interface that requires a
>>> return of a negative errno value to make up for when errp is not set.
>>
>> I'm not sure what you mean by this exactly.
> 
> The ideal conversion would be having .bdrv_truncate switch to a void
> return; then no caller is messing with negative errno values (especially
> since some of them are just synthesizing errno values, rather than
> actually encountering one, and since some of them are probably using -1
> when they should be using errno). But such a conversion requires that
> all drivers be updated to properly set errp.

Not sure if that is an ideal conversion. I very much prefer negative
return value + error object because that allows constructs like

if (foo(..., errp) < 0) {
return;
}

Or:

ret = foo(..., errp);
if (ret < 0) {
return ret;
}

Instead of:

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


For me, the most important thing is that errp will always be set if the
function fails.

(Of course, I'd be just fine with a boolean return value, too.)

>> I can agree that it's not
>> nice within a single driver. But I think it's fine to have some drivers
>> that generate an Error object and others that do not, as long as the
>> generic interface (bdrv_truncate()) masks this behavior.
> 
> I agree that as an interim thing, having some drivers that aren't
> converted yet is the best way to make progress. But it's still best if
> every driver is switched on an all-or-none basis, so that we don't
> forget to go back and re-fix drivers that half-heartedly set errp if we
> eventually reach the point where all other drivers have been fixed.
> 
> In other words, I view the generic bdrv_truncate() supplying an error
> based on negative errno is a crutch, and not something that we should
> rely on, at least not for the drivers that we fix to set errp directly.

Well, the more detailed the error messages are the better, but I don't
see any real improvement over a generic message supplied by
bdrv_truncate() if that is good enough.

Of course, I can easily be convinced that the generic message is in fact
not good enough.

>>> know you aren't comfortable converting all drivers, but for the drivers
>>> you do convert, I'd rather guarantee that ALL errors set errp.
>>
>> Can do, I just felt bad that it be would exactly the same as the message
>> that would be generated in bdrv_truncate() anyway.
> 
> There may be some duplication of error messages, but it still seems
> cleaner to consistently set the error within the driver than to rely on
> the generic crutch.

As I said, I don't necessarily think it's a crutch. :-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 07/10] block: Factor out bdrv_replace_child_noperm()

[Eric Blake]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 38 +-
>  1 file changed, 25 insertions(+), 13 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 05/10] block: Fix blockdev-snapshot error handling

[Eric Blake]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
> node reference at first and only checks later whether it already has a
> backing file. Between those places, other errors can occur.
> 
> Therefore checking in external_snapshot_abort() whether state->new_bs
> has a backing file is not sufficient to tell whether bdrv_append() was
> already completed or not. Trying to undo the bdrv_append() when it
> wasn't even executed is wrong.
> 
> Introduce a new boolean flag in the state to fix this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

By the way, how are you finding all these spots? Is it existing qemu-io
tests that are failing? And if so, would mentioning which test exposed
the problem being fixed be worth adding in the commit messages?  If not,
are there some qemu-io tests to be added?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

On 03/06/2017 02:14 PM, Max Reitz wrote:

>>>  if (S_ISREG(st.st_mode)) {
>>>  if (ftruncate(s->fd, offset) < 0) {
>>> +/* The generic error message will be fine */
>>>  return -errno;
>>
>> Relying on a generic error message in the caller is awkward. I see it as
>> evidence of a partial conversion if we have an interface that requires a
>> return of a negative errno value to make up for when errp is not set.
> 
> I'm not sure what you mean by this exactly.

The ideal conversion would be having .bdrv_truncate switch to a void
return; then no caller is messing with negative errno values (especially
since some of them are just synthesizing errno values, rather than
actually encountering one, and since some of them are probably using -1
when they should be using errno). But such a conversion requires that
all drivers be updated to properly set errp.


> I can agree that it's not
> nice within a single driver. But I think it's fine to have some drivers
> that generate an Error object and others that do not, as long as the
> generic interface (bdrv_truncate()) masks this behavior.

I agree that as an interim thing, having some drivers that aren't
converted yet is the best way to make progress. But it's still best if
every driver is switched on an all-or-none basis, so that we don't
forget to go back and re-fix drivers that half-heartedly set errp if we
eventually reach the point where all other drivers have been fixed.

In other words, I view the generic bdrv_truncate() supplying an error
based on negative errno is a crutch, and not something that we should
rely on, at least not for the drivers that we fix to set errp directly.

> 
>>I
>> know you aren't comfortable converting all drivers, but for the drivers
>> you do convert, I'd rather guarantee that ALL errors set errp.
> 
> Can do, I just felt bad that it be would exactly the same as the message
> that would be generated in bdrv_truncate() anyway.

There may be some duplication of error messages, but it still seems
cleaner to consistently set the error within the driver than to rely on
the generic crutch.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 04/10] mirror: Fix error path for dirty bitmap creation

[Eric Blake]

On 03/06/2017 10:21 AM, Kevin Wolf wrote:
> mirror_top_bs must be removed from the graph again when creating the
> dirty bitmap fails.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/mirror.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 001b5f0..f24dc51 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1197,10 +1197,7 @@ static void mirror_start_job(const char *job_id, 
> BlockDriverState *bs,
>  
>  s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>  if (!s->dirty_bitmap) {
> -g_free(s->replaces);
> -blk_unref(s->target);
> -block_job_unref(>common);
> -return;
> +goto fail;
>  }
>  
>  /* Required permissions are already taken with blk_new() */
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

On 06.03.2017 21:09, Eric Blake wrote:
> On 03/06/2017 01:54 PM, Max Reitz wrote:
>> Add missing error messages for the drivers I am comfortable to do this
>> in.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  block/file-posix.c | 8 +++-
>>  block/qcow2.c  | 2 ++
>>  block/qed.c| 4 +++-
>>  block/raw-format.c | 2 ++
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 9d2bea730d..553213221c 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, 
>> int64_t offset, Error **errp)
>>  {
>>  BDRVRawState *s = bs->opaque;
>>  struct stat st;
>> +int ret;
>>  
>>  if (fstat(s->fd, )) {
>> -return -errno;
>> +ret = -errno;
>> +error_setg_errno(errp, -ret, "Failed to fstat() the file");
>> +return ret;
>>  }
>>  
>>  if (S_ISREG(st.st_mode)) {
>>  if (ftruncate(s->fd, offset) < 0) {
>> +/* The generic error message will be fine */
>>  return -errno;
> 
> Relying on a generic error message in the caller is awkward. I see it as
> evidence of a partial conversion if we have an interface that requires a
> return of a negative errno value to make up for when errp is not set.

I'm not sure what you mean by this exactly. I can agree that it's not
nice within a single driver. But I think it's fine to have some drivers
that generate an Error object and others that do not, as long as the
generic interface (bdrv_truncate()) masks this behavior.

>I
> know you aren't comfortable converting all drivers, but for the drivers
> you do convert, I'd rather guarantee that ALL errors set errp.

Can do, I just felt bad that it be would exactly the same as the message
that would be generated in bdrv_truncate() anyway.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

On 03/06/2017 01:54 PM, Max Reitz wrote:
> Add missing error messages for the drivers I am comfortable to do this
> in.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/file-posix.c | 8 +++-
>  block/qcow2.c  | 2 ++
>  block/qed.c| 4 +++-
>  block/raw-format.c | 2 ++
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 9d2bea730d..553213221c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
> offset, Error **errp)
>  {
>  BDRVRawState *s = bs->opaque;
>  struct stat st;
> +int ret;
>  
>  if (fstat(s->fd, )) {
> -return -errno;
> +ret = -errno;
> +error_setg_errno(errp, -ret, "Failed to fstat() the file");
> +return ret;
>  }
>  
>  if (S_ISREG(st.st_mode)) {
>  if (ftruncate(s->fd, offset) < 0) {
> +/* The generic error message will be fine */
>  return -errno;

Relying on a generic error message in the caller is awkward. I see it as
evidence of a partial conversion if we have an interface that requires a
return of a negative errno value to make up for when errp is not set.  I
know you aren't comfortable converting all drivers, but for the drivers
you do convert, I'd rather guarantee that ALL errors set errp.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[Max Reitz]

Having an Error parameter for these functions makes sense because we
sometimes want a bit more information than just "Something failed". Some
drivers already use error_report() and the like to emit this additional
information, so it's rather obvious that we do want a real error object
here.

Max Reitz (3):
  block: Add errp to b{lk,drv}_truncate()
  block: Add errp to BD.bdrv_truncate()
  block: Add some bdrv_truncate() error messages

 include/block/block.h  |  2 +-
 include/block/block_int.h  |  2 +-
 include/sysemu/block-backend.h |  2 +-
 block.c| 18 +-
 block/archipelago.c|  3 ++-
 block/blkdebug.c   |  4 ++--
 block/block-backend.c  |  5 +++--
 block/commit.c |  5 +++--
 block/crypto.c |  5 +++--
 block/file-posix.c | 10 --
 block/file-win32.c |  6 +++---
 block/gluster.c|  3 ++-
 block/iscsi.c  |  4 ++--
 block/mirror.c |  2 +-
 block/nfs.c|  2 +-
 block/parallels.c  | 13 -
 block/qcow.c   |  6 +++---
 block/qcow2-refcount.c |  5 -
 block/qcow2.c  | 23 ++-
 block/qed.c|  8 +---
 block/raw-format.c |  6 --
 block/rbd.c|  2 +-
 block/sheepdog.c   | 14 ++
 block/vdi.c|  4 ++--
 block/vhdx-log.c   |  2 +-
 block/vhdx.c   |  6 +++---
 block/vmdk.c   | 13 +++--
 block/vpc.c|  2 +-
 blockdev.c | 21 +
 qemu-img.c | 17 -
 qemu-io-cmds.c |  2 +-
 31 files changed, 107 insertions(+), 110 deletions(-)

-- 
2.12.0

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[no-reply]

Hi,

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

Message-id: 20170306195255.15806-1-mre...@redhat.com
Subject: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, 
drv}_turncate()
Type: series

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

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

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/1488826849-32384-1-git-send-email-arm...@redhat.com -> 
patchew/1488826849-32384-1-git-send-email-arm...@redhat.com
 * [new tag] patchew/20170306195255.15806-1-mre...@redhat.com -> 
patchew/20170306195255.15806-1-mre...@redhat.com
Auto packing the repository for optimum performance. You may also
run "git gc" manually. See "git help gc" for more information.
Switched to a new branch 'test'
4ef1757 block: Add some bdrv_truncate() error messages
3e348c6 block: Add errp to BD.bdrv_truncate()
1ba2c64 block: Add errp to b{lk, drv}_truncate()

=== OUTPUT BEGIN ===
Checking PATCH 1/3: block: Add errp to b{lk, drv}_truncate()...
Checking PATCH 2/3: block: Add errp to BD.bdrv_truncate()...
Checking PATCH 3/3: block: Add some bdrv_truncate() error messages...
ERROR: suspect code indent for conditional statements (7, 11)
#35: FILE: block/file-posix.c:1358:
if (offset > raw_getlength(bs)) {
+   error_setg(errp, "Cannot grow device files");

total: 1 errors, 0 warnings, 73 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

[Qemu-block] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

Add missing error messages for the drivers I am comfortable to do this
in.

Signed-off-by: Max Reitz 
---
 block/file-posix.c | 8 +++-
 block/qcow2.c  | 2 ++
 block/qed.c| 4 +++-
 block/raw-format.c | 2 ++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9d2bea730d..553213221c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
offset, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 struct stat st;
+int ret;
 
 if (fstat(s->fd, )) {
-return -errno;
+ret = -errno;
+error_setg_errno(errp, -ret, "Failed to fstat() the file");
+return ret;
 }
 
 if (S_ISREG(st.st_mode)) {
 if (ftruncate(s->fd, offset) < 0) {
+/* The generic error message will be fine */
 return -errno;
 }
 } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
if (offset > raw_getlength(bs)) {
+   error_setg(errp, "Cannot grow device files");
return -EINVAL;
}
 } else {
+error_setg(errp, "Resizing this file is not supported");
 return -ENOTSUP;
 }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 17585fbb89..53b0bd61a7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2550,6 +2550,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset, Error **errp)
 new_l1_size = size_to_l1(s, offset);
 ret = qcow2_grow_l1_table(bs, new_l1_size, true);
 if (ret < 0) {
+error_setg(errp, "Failed to grow the L1 table");
 return ret;
 }
 
@@ -2558,6 +2559,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t 
offset, Error **errp)
 ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
, sizeof(uint64_t));
 if (ret < 0) {
+error_setg(errp, "Failed to update the image size");
 return ret;
 }
 
diff --git a/block/qed.c b/block/qed.c
index fa2aeee471..eb346d645b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1526,11 +1526,12 @@ static int bdrv_qed_truncate(BlockDriverState *bs, 
int64_t offset, Error **errp)
 
 if (!qed_is_image_size_valid(offset, s->header.cluster_size,
  s->header.table_size)) {
+error_setg(errp, "Invalid image size specified");
 return -EINVAL;
 }
 
-/* Shrinking is currently not supported */
 if ((uint64_t)offset < s->header.image_size) {
+error_setg(errp, "Shrinking images is currently not supported");
 return -ENOTSUP;
 }
 
@@ -1539,6 +1540,7 @@ static int bdrv_qed_truncate(BlockDriverState *bs, 
int64_t offset, Error **errp)
 ret = qed_write_header_sync(s);
 if (ret < 0) {
 s->header.image_size = old_image_size;
+error_setg(errp, "Failed to update the image size");
 }
 return ret;
 }
diff --git a/block/raw-format.c b/block/raw-format.c
index 9761bdaff8..36e65036f0 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -332,10 +332,12 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
offset, Error **errp)
 BDRVRawState *s = bs->opaque;
 
 if (s->has_size) {
+error_setg(errp, "Cannot resize fixed-size raw disks");
 return -ENOTSUP;
 }
 
 if (INT64_MAX - offset < s->offset) {
+error_setg(errp, "Disk size too large for the chosen offset");
 return -EINVAL;
 }
 
-- 
2.12.0

[Qemu-block] [PATCH for-2.10 2/3] block: Add errp to BD.bdrv_truncate()

[Max Reitz]

Add an Error parameter to the block drivers' bdrv_truncate() interface.
If a block driver does not set this in case of an error, the generic
bdrv_truncate() implementation will do so.

Where it is obvious, this patch also makes some block drivers set this
value.

Signed-off-by: Max Reitz 
---
 include/block/block_int.h |  2 +-
 block.c   |  4 ++--
 block/archipelago.c   |  3 ++-
 block/blkdebug.c  |  4 ++--
 block/crypto.c|  5 +++--
 block/file-posix.c|  2 +-
 block/file-win32.c|  6 +++---
 block/gluster.c   |  3 ++-
 block/iscsi.c |  4 ++--
 block/nfs.c   |  2 +-
 block/qcow2.c |  8 
 block/qed.c   |  2 +-
 block/raw-format.c|  4 ++--
 block/rbd.c   |  2 +-
 block/sheepdog.c  | 14 ++
 15 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index a57c0bfb55..9374e5db74 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -196,7 +196,7 @@ struct BlockDriver {
 int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
 
 const char *protocol_name;
-int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
+int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset, Error **errp);
 
 int64_t (*bdrv_getlength)(BlockDriverState *bs);
 bool has_variable_length;
diff --git a/block.c b/block.c
index dd24161260..46ce64fc46 100644
--- a/block.c
+++ b/block.c
@@ -3165,13 +3165,13 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, 
Error **errp)
 return -EACCES;
 }
 
-ret = drv->bdrv_truncate(bs, offset);
+ret = drv->bdrv_truncate(bs, offset, errp);
 if (ret == 0) {
 ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
 bdrv_dirty_bitmap_truncate(bs);
 bdrv_parent_cb_resize(bs);
 ++bs->write_gen;
-} else {
+} else if (errp && !*errp) {
 error_setg_errno(errp, -ret, "Failed to resize image");
 }
 return ret;
diff --git a/block/archipelago.c b/block/archipelago.c
index 2449cfc702..0015178381 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -976,7 +976,8 @@ static int64_t qemu_archipelago_getlength(BlockDriverState 
*bs)
 return archipelago_volume_info(s);
 }
 
-static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset)
+static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset,
+ Error *errp)
 {
 int ret, targetlen;
 struct xseg_request *req;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 15a9966096..c795ae9e72 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -661,9 +661,9 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
 return bdrv_getlength(bs->file->bs);
 }
 
-static int blkdebug_truncate(BlockDriverState *bs, int64_t offset)
+static int blkdebug_truncate(BlockDriverState *bs, int64_t offset, Error 
**errp)
 {
-return bdrv_truncate(bs->file, offset, NULL);
+return bdrv_truncate(bs->file, offset, errp);
 }
 
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
diff --git a/block/crypto.c b/block/crypto.c
index 52e4f2b20f..17b3140998 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -381,7 +381,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat 
format,
 return ret;
 }
 
-static int block_crypto_truncate(BlockDriverState *bs, int64_t offset)
+static int block_crypto_truncate(BlockDriverState *bs, int64_t offset,
+ Error **errp)
 {
 BlockCrypto *crypto = bs->opaque;
 size_t payload_offset =
@@ -389,7 +390,7 @@ static int block_crypto_truncate(BlockDriverState *bs, 
int64_t offset)
 
 offset += payload_offset;
 
-return bdrv_truncate(bs->file, offset, NULL);
+return bdrv_truncate(bs->file, offset, errp);
 }
 
 static void block_crypto_close(BlockDriverState *bs)
diff --git a/block/file-posix.c b/block/file-posix.c
index 4de1abd023..9d2bea730d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1337,7 +1337,7 @@ static void raw_close(BlockDriverState *bs)
 }
 }
 
-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 struct stat st;
diff --git a/block/file-win32.c b/block/file-win32.c
index 800fabdd72..3f3925623f 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -461,7 +461,7 @@ static void raw_close(BlockDriverState *bs)
 }
 }
 
-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 LONG low, high;
@@ -476,11 +476,11 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
offset)
  */
 dwPtrLow = SetFilePointer(s->hfile, low, , 

[Qemu-block] [PATCH for-2.10 1/3] block: Add errp to b{lk, drv}_truncate()

[Max Reitz]

For one thing, this allows us to drop the error message generation from
qemu-img.c and blockdev.c and instead have it unified in
bdrv_truncate().

Signed-off-by: Max Reitz 
---
 include/block/block.h  |  2 +-
 include/sysemu/block-backend.h |  2 +-
 block.c| 16 
 block/blkdebug.c   |  2 +-
 block/block-backend.c  |  5 +++--
 block/commit.c |  5 +++--
 block/crypto.c |  2 +-
 block/mirror.c |  2 +-
 block/parallels.c  | 13 -
 block/qcow.c   |  6 +++---
 block/qcow2-refcount.c |  5 -
 block/qcow2.c  | 13 -
 block/qed.c|  2 +-
 block/raw-format.c |  2 +-
 block/vdi.c|  4 ++--
 block/vhdx-log.c   |  2 +-
 block/vhdx.c   |  6 +++---
 block/vmdk.c   | 13 +++--
 block/vpc.c|  2 +-
 blockdev.c | 21 +
 qemu-img.c | 17 -
 qemu-io-cmds.c |  2 +-
 22 files changed, 64 insertions(+), 80 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index c7c4a3ac3a..c0d1e52ea6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -294,7 +294,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState 
*bs,
 const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
-int bdrv_truncate(BdrvChild *child, int64_t offset);
+int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 096c17fce0..c142106006 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -217,7 +217,7 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, 
int64_t offset,
   int count, BdrvRequestFlags flags);
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
   int count);
-int blk_truncate(BlockBackend *blk, int64_t offset);
+int blk_truncate(BlockBackend *blk, int64_t offset, Error **errp);
 int blk_pdiscard(BlockBackend *blk, int64_t offset, int count);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
  int64_t pos, int size);
diff --git a/block.c b/block.c
index f293ccb5af..dd24161260 100644
--- a/block.c
+++ b/block.c
@@ -3144,7 +3144,7 @@ exit:
 /**
  * Truncate file to 'offset' bytes (needed only for file protocols)
  */
-int bdrv_truncate(BdrvChild *child, int64_t offset)
+int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
 {
 BlockDriverState *bs = child->bs;
 BlockDriver *drv = bs->drv;
@@ -3152,12 +3152,18 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
 
 assert(child->perm & BLK_PERM_RESIZE);
 
-if (!drv)
+if (!drv) {
+error_setg(errp, "No medium inserted");
 return -ENOMEDIUM;
-if (!drv->bdrv_truncate)
+}
+if (!drv->bdrv_truncate) {
+error_setg(errp, "Image format driver does not support resize");
 return -ENOTSUP;
-if (bs->read_only)
+}
+if (bs->read_only) {
+error_setg(errp, "Image is read-only");
 return -EACCES;
+}
 
 ret = drv->bdrv_truncate(bs, offset);
 if (ret == 0) {
@@ -3165,6 +3171,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
 bdrv_dirty_bitmap_truncate(bs);
 bdrv_parent_cb_resize(bs);
 ++bs->write_gen;
+} else {
+error_setg_errno(errp, -ret, "Failed to resize image");
 }
 return ret;
 }
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 67e8024e36..15a9966096 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -663,7 +663,7 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
 
 static int blkdebug_truncate(BlockDriverState *bs, int64_t offset)
 {
-return bdrv_truncate(bs->file, offset);
+return bdrv_truncate(bs->file, offset, NULL);
 }
 
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
diff --git a/block/block-backend.c b/block/block-backend.c
index daa7908d01..97c9b0d315 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1693,13 +1693,14 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t 
offset, const void *buf,
BDRV_REQ_WRITE_COMPRESSED);
 }
 
-int blk_truncate(BlockBackend *blk, int64_t offset)
+int blk_truncate(BlockBackend *blk, int64_t offset, Error **errp)
 {
 if (!blk_is_available(blk)) {
+error_setg(errp, "No medium inserted");
 return -ENOMEDIUM;
 }
 
-return bdrv_truncate(blk->root, offset);
+

Re: [Qemu-block] [PATCH v2 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Eric Blake]

On 03/06/2017 01:00 PM, Markus Armbruster wrote:
> As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
> error and return negative errno on failure.  It sometimes returns -1,
> and sometimes neglects to set an error.  It also prints error messages
> with error_report().  Fix all that.
> 
> Moreover, its handling of an attempt to delete an nonexistent snapshot

s/an nonexistent/a nonexistent/

> is wrong: it error_report()s and succeeds.  Fix it to set an error and
> return -ENOENT instead.
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Eric Blake 
> ---
>  block/sheepdog.c | 41 +++--
>  1 file changed, 19 insertions(+), 22 deletions(-)

R-by stands.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Eric Blake]

On 03/06/2017 01:00 PM, Markus Armbruster wrote:
> When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
> sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
> fail, because:
> 
> 1. it only fails when qemu_opt_parse() fails, and
> 2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
> 3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.
> 
> Defuse this ticking time bomb by jumping behind the file descriptor
> cleanup on error.
> 
> Also do that for the error paths where sd->fd is still -1.  The file
> descriptor cleanup happens to do nothing then, but let's not rely on
> that here.
> 
> While there, rename label out to err, because it's on the error path,
> not the normal path out of the function.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  block/sheepdog.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
the discriminator value for variant InetSocketAddress is 'tcp' instead
of 'inet'.  Rename.

The type is so far only used by the Gluster block drivers.  Take care
to keep 'tcp' working in things like -drive's file.server.0.type=tcp.
The "gluster+tcp" URI scheme in pseudo-filenames stays the same.
blockdev-add changes, but it has changed incompatibly since 2.8
already.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/gluster.c  | 59 +---
 qapi-schema.json |  8 
 2 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 64b0217..a577dae 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -152,7 +152,7 @@ static QemuOptsList runtime_type_opts = {
 {
 .name = GLUSTER_OPT_TYPE,
 .type = QEMU_OPT_STRING,
-.help = "tcp|unix",
+.help = "inet|unix",
 },
 { /* end of list */ }
 },
@@ -171,14 +171,14 @@ static QemuOptsList runtime_unix_opts = {
 },
 };
 
-static QemuOptsList runtime_tcp_opts = {
-.name = "gluster_tcp",
-.head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
+static QemuOptsList runtime_inet_opts = {
+.name = "gluster_inet",
+.head = QTAILQ_HEAD_INITIALIZER(runtime_inet_opts.head),
 .desc = {
 {
 .name = GLUSTER_OPT_TYPE,
 .type = QEMU_OPT_STRING,
-.help = "tcp|unix",
+.help = "inet|unix",
 },
 {
 .name = GLUSTER_OPT_HOST,
@@ -337,14 +337,14 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
 
 /* transport */
 if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
 } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
 } else if (!strcmp(uri->scheme, "gluster+unix")) {
 gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
 is_unix = true;
 } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_INET;
 error_report("Warning: rdma feature is not supported, falling "
  "back to tcp");
 } else {
@@ -374,11 +374,11 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
 }
 gsconf->u.q_unix.path = g_strdup(qp->p[0].value);
 } else {
-gsconf->u.tcp.host = g_strdup(uri->server ? uri->server : "localhost");
+gsconf->u.inet.host = g_strdup(uri->server ? uri->server : 
"localhost");
 if (uri->port) {
-gsconf->u.tcp.port = g_strdup_printf("%d", uri->port);
+gsconf->u.inet.port = g_strdup_printf("%d", uri->port);
 } else {
-gsconf->u.tcp.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
+gsconf->u.inet.port = g_strdup_printf("%d", GLUSTER_DEFAULT_PORT);
 }
 }
 
@@ -416,15 +416,15 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 ret = glfs_set_volfile_server(glfs, "unix",
server->value->u.q_unix.path, 0);
 } else {
-if (parse_uint_full(server->value->u.tcp.port, , 10) < 0 ||
+if (parse_uint_full(server->value->u.inet.port, , 10) < 0 ||
 port > 65535) {
 error_setg(errp, "'%s' is not a valid port number",
-   server->value->u.tcp.port);
+   server->value->u.inet.port);
 errno = EINVAL;
 goto out;
 }
 ret = glfs_set_volfile_server(glfs, "tcp",
-   server->value->u.tcp.host,
+   server->value->u.inet.host,
(int)port);
 }
 
@@ -448,8 +448,8 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
   server->value->u.q_unix.path);
 } else {
 error_append_hint(errp, "hint: failed on host %s and port %s ",
-  server->value->u.tcp.host,
-  server->value->u.tcp.port);
+  server->value->u.inet.host,
+  server->value->u.inet.port);
 }
 }
 
@@ -536,21 +536,24 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 
 }
 gsconf = g_new0(SocketAddressFlat, 1);
+if (!strcmp(ptr, "tcp")) {
+ptr = "inet";   /* accept legacy "tcp" */
+}
 gsconf->type = 

[Qemu-block] [PATCH v2 09/15] sheepdog: Implement bdrv_parse_filename()

[Markus Armbruster]

This permits configuration with driver-specific options in addition to
pseudo-filename parsed as URI.  For instance,

--drive driver=sheepdog,host=fido,vdi=dolly

instead of

--drive driver=sheepdog,file=sheepdog://fido/dolly

It's also a first step towards supporting blockdev-add.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/sheepdog.c | 234 +--
 1 file changed, 176 insertions(+), 58 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 9b1e121..89e98ed 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -14,6 +14,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qint.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
@@ -526,6 +528,25 @@ static void sd_aio_setup(SheepdogAIOCB *acb, 
BDRVSheepdogState *s,
 QLIST_INSERT_HEAD(>inflight_aiocb_head, acb, aiocb_siblings);
 }
 
+static SocketAddress *sd_socket_address(const char *path,
+const char *host, const char *port)
+{
+SocketAddress *addr = g_new0(SocketAddress, 1);
+
+if (path) {
+addr->type = SOCKET_ADDRESS_KIND_UNIX;
+addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+addr->u.q_unix.data->path = g_strdup(path);
+} else {
+addr->type = SOCKET_ADDRESS_KIND_INET;
+addr->u.inet.data = g_new0(InetSocketAddress, 1);
+addr->u.inet.data->host = g_strdup(host ?: SD_DEFAULT_ADDR);
+addr->u.inet.data->port = g_strdup(port ?: stringify(SD_DEFAULT_PORT));
+}
+
+return addr;
+}
+
 /* Return -EIO in case of error, file descriptor on success */
 static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
@@ -951,17 +972,37 @@ static bool sd_parse_snapid_or_tag(const char *str,
 return true;
 }
 
-static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
- char *vdi, uint32_t *snapid, char *tag,
+typedef struct {
+const char *path;   /* non-null iff transport is tcp */
+const char *host;   /* valid when transport is tcp */
+int port;   /* valid when transport is tcp */
+char vdi[SD_MAX_VDI_LEN];
+char tag[SD_MAX_VDI_TAG_LEN];
+uint32_t snap_id;
+/* Remainder is only for sd_config_done() */
+URI *uri;
+QueryParams *qp;
+} SheepdogConfig;
+
+static void sd_config_done(SheepdogConfig *cfg)
+{
+if (cfg->qp) {
+query_params_free(cfg->qp);
+}
+uri_free(cfg->uri);
+}
+
+static void sd_parse_uri(SheepdogConfig *cfg, const char *filename,
  Error **errp)
 {
 Error *err = NULL;
 QueryParams *qp = NULL;
-SocketAddress *addr = NULL;
 bool is_unix;
 URI *uri;
 
-uri = uri_parse(filename);
+memset(cfg, 0, sizeof(*cfg));
+
+cfg->uri = uri = uri_parse(filename);
 if (!uri) {
 error_setg(, "invalid URI");
 goto out;
@@ -984,13 +1025,13 @@ static void sd_parse_uri(BDRVSheepdogState *s, const 
char *filename,
 error_setg(, "missing file path in URI");
 goto out;
 }
-if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+if (g_strlcpy(cfg->vdi, uri->path + 1, SD_MAX_VDI_LEN)
+>= SD_MAX_VDI_LEN) {
 error_setg(, "VDI name is too long");
 goto out;
 }
 
-qp = query_params_parse(uri->query);
-addr = g_new0(SocketAddress, 1);
+cfg->qp = qp = query_params_parse(uri->query);
 
 if (is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
@@ -1009,44 +1050,34 @@ static void sd_parse_uri(BDRVSheepdogState *s, const 
char *filename,
 error_setg(, "unexpected query parameters");
 goto out;
 }
-addr->type = SOCKET_ADDRESS_KIND_UNIX;
-addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
+cfg->path = qp->p[0].value;
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
 if (qp->n) {
 error_setg(, "unexpected query parameters");
 goto out;
 }
-addr->type = SOCKET_ADDRESS_KIND_INET;
-addr->u.inet.data = g_new0(InetSocketAddress, 1);
-addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
-addr->u.inet.data->port = g_strdup_printf("%d",
-uri->port ?: SD_DEFAULT_PORT);
+cfg->host = uri->server;
+cfg->port = uri->port;
 }
 
 /* snapshot tag */
 if (uri->fragment) {
-if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
+if (!sd_parse_snapid_or_tag(uri->fragment,
+>snap_id, cfg->tag)) {
 error_setg(, "'%s' is not a valid snapshot ID",
uri->fragment);
 goto out;
 }
 } 

[Qemu-block] [PATCH v2 10/15] gluster: Drop assumptions on SocketTransport names

[Markus Armbruster]

qemu_gluster_glfs_init() passes the names of QAPI enumeration type
SocketTransport to glfs_set_volfile_server().  Works, because they
were chosen to match.  But the coupling is artificial.  Use the
appropriate literal strings instead.

Signed-off-by: Markus Armbruster 
Reviewed-by: Niels de Vos 
---
 block/gluster.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 56b4abe..7236d59 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -412,8 +412,7 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 
 for (server = gconf->server; server; server = server->next) {
 if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
-ret = glfs_set_volfile_server(glfs,
-   
GlusterTransport_lookup[server->value->type],
+ret = glfs_set_volfile_server(glfs, "unix",
server->value->u.q_unix.path, 0);
 } else {
 if (parse_uint_full(server->value->u.tcp.port, , 10) < 0 ||
@@ -423,8 +422,7 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 errno = EINVAL;
 goto out;
 }
-ret = glfs_set_volfile_server(glfs,
-   
GlusterTransport_lookup[server->value->type],
+ret = glfs_set_volfile_server(glfs, "tcp",
server->value->u.tcp.host,
(int)port);
 }
-- 
2.7.4

[Qemu-block] [PATCH v2 08/15] sheepdog: Use SocketAddress and socket_connect()

[Markus Armbruster]

sd_parse_uri() builds a string from host and port parts for
inet_connect().  inet_connect() parses it into host, port and options.
Whether this gets exactly the same host, port and no options for all
inputs is not obvious.

Cut out the string middleman and build a SocketAddress for
socket_connect() instead.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/sheepdog.c | 53 ++---
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 161932d..9b1e121 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -374,8 +374,7 @@ struct BDRVSheepdogState {
 uint32_t cache_flags;
 bool discard_supported;
 
-char *host_spec;
-bool is_unix;
+SocketAddress *addr;
 int fd;
 
 CoMutex lock;
@@ -532,16 +531,12 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error 
**errp)
 {
 int fd;
 
-if (s->is_unix) {
-fd = unix_connect(s->host_spec, errp);
-} else {
-fd = inet_connect(s->host_spec, errp);
+fd = socket_connect(s->addr, errp, NULL, NULL);
 
-if (fd >= 0) {
-int ret = socket_set_nodelay(fd);
-if (ret < 0) {
-error_report("%s", strerror(errno));
-}
+if (s->addr->type == SOCKET_ADDRESS_KIND_INET && fd >= 0) {
+int ret = socket_set_nodelay(fd);
+if (ret < 0) {
+error_report("%s", strerror(errno));
 }
 }
 
@@ -820,8 +815,7 @@ static void coroutine_fn aio_read_response(void *opaque)
 case AIOCB_DISCARD_OBJ:
 switch (rsp.result) {
 case SD_RES_INVALID_PARMS:
-error_report("sheep(%s) doesn't support discard command",
- s->host_spec);
+error_report("server doesn't support discard command");
 rsp.result = SD_RES_SUCCESS;
 s->discard_supported = false;
 break;
@@ -962,8 +956,10 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
  Error **errp)
 {
 Error *err = NULL;
-URI *uri;
 QueryParams *qp = NULL;
+SocketAddress *addr = NULL;
+bool is_unix;
+URI *uri;
 
 uri = uri_parse(filename);
 if (!uri) {
@@ -973,11 +969,11 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
 
 /* transport */
 if (!strcmp(uri->scheme, "sheepdog")) {
-s->is_unix = false;
+is_unix = false;
 } else if (!strcmp(uri->scheme, "sheepdog+tcp")) {
-s->is_unix = false;
+is_unix = false;
 } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
-s->is_unix = true;
+is_unix = true;
 } else {
 error_setg(, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
" or 'sheepdog+unix'");
@@ -994,8 +990,9 @@ static void sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
 }
 
 qp = query_params_parse(uri->query);
+addr = g_new0(SocketAddress, 1);
 
-if (s->is_unix) {
+if (is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
 if (uri->server || uri->port) {
 error_setg(, "URI scheme %s doesn't accept a server address",
@@ -1012,15 +1009,20 @@ static void sd_parse_uri(BDRVSheepdogState *s, const 
char *filename,
 error_setg(, "unexpected query parameters");
 goto out;
 }
-s->host_spec = g_strdup(qp->p[0].value);
+addr->type = SOCKET_ADDRESS_KIND_UNIX;
+addr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
+addr->u.q_unix.data->path = g_strdup(qp->p[0].value);
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
 if (qp->n) {
 error_setg(, "unexpected query parameters");
 goto out;
 }
-s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
-   uri->port ?: SD_DEFAULT_PORT);
+addr->type = SOCKET_ADDRESS_KIND_INET;
+addr->u.inet.data = g_new0(InetSocketAddress, 1);
+addr->u.inet.data->host = g_strdup(uri->server ?: SD_DEFAULT_ADDR);
+addr->u.inet.data->port = g_strdup_printf("%d",
+uri->port ?: SD_DEFAULT_PORT);
 }
 
 /* snapshot tag */
@@ -1035,7 +1037,12 @@ static void sd_parse_uri(BDRVSheepdogState *s, const 
char *filename,
 }
 
 out:
-error_propagate(errp, err);
+if (err) {
+error_propagate(errp, err);
+qapi_free_SocketAddress(addr);
+} else {
+s->addr = addr;
+}
 if (qp) {
 query_params_free(qp);
 }
@@ -1998,7 +2005,7 @@ static void sd_close(BlockDriverState *bs)
 aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
false, NULL, NULL, NULL, NULL);
 closesocket(s->fd);
-g_free(s->host_spec);
+qapi_free_SocketAddress(s->addr);
 }
 
 static int64_t 

[Qemu-block] [PATCH v2 13/15] qapi-schema: Rename GlusterServer to SocketAddressFlat

[Markus Armbruster]

As its documentation says, it's not specific to Gluster.  Rename it,
as I'm going to use it for something else.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/gluster.c  | 38 +++---
 qapi-schema.json | 38 ++
 qapi/block-core.json | 46 +-
 3 files changed, 58 insertions(+), 64 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 991f18f..64b0217 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -321,7 +321,7 @@ static int parse_volume_options(BlockdevOptionsGluster 
*gconf, char *path)
 static int qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
   const char *filename)
 {
-GlusterServer *gsconf;
+SocketAddressFlat *gsconf;
 URI *uri;
 QueryParams *qp = NULL;
 bool is_unix = false;
@@ -332,19 +332,19 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster 
*gconf,
 return -EINVAL;
 }
 
-gconf->server = g_new0(GlusterServerList, 1);
-gconf->server->value = gsconf = g_new0(GlusterServer, 1);
+gconf->server = g_new0(SocketAddressFlatList, 1);
+gconf->server->value = gsconf = g_new0(SocketAddressFlat, 1);
 
 /* transport */
 if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
-gsconf->type = GLUSTER_TRANSPORT_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
 } else if (!strcmp(uri->scheme, "gluster+tcp")) {
-gsconf->type = GLUSTER_TRANSPORT_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
 } else if (!strcmp(uri->scheme, "gluster+unix")) {
-gsconf->type = GLUSTER_TRANSPORT_UNIX;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_UNIX;
 is_unix = true;
 } else if (!strcmp(uri->scheme, "gluster+rdma")) {
-gsconf->type = GLUSTER_TRANSPORT_TCP;
+gsconf->type = SOCKET_ADDRESS_FLAT_TYPE_TCP;
 error_report("Warning: rdma feature is not supported, falling "
  "back to tcp");
 } else {
@@ -396,7 +396,7 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 struct glfs *glfs;
 int ret;
 int old_errno;
-GlusterServerList *server;
+SocketAddressFlatList *server;
 unsigned long long port;
 
 glfs = glfs_find_preopened(gconf->volume);
@@ -412,7 +412,7 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 glfs_set_preopened(gconf->volume, glfs);
 
 for (server = gconf->server; server; server = server->next) {
-if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
 ret = glfs_set_volfile_server(glfs, "unix",
server->value->u.q_unix.path, 0);
 } else {
@@ -443,7 +443,7 @@ static struct glfs 
*qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 error_setg(errp, "Gluster connection for volume %s, path %s failed"
  " to connect", gconf->volume, gconf->path);
 for (server = gconf->server; server; server = server->next) {
-if (server->value->type  == GLUSTER_TRANSPORT_UNIX) {
+if (server->value->type  == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
 error_append_hint(errp, "hint: failed on socket %s ",
   server->value->u.q_unix.path);
 } else {
@@ -480,8 +480,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
   QDict *options, Error **errp)
 {
 QemuOpts *opts;
-GlusterServer *gsconf = NULL;
-GlusterServerList *curr = NULL;
+SocketAddressFlat *gsconf = NULL;
+SocketAddressFlatList *curr = NULL;
 QDict *backing_options = NULL;
 Error *local_err = NULL;
 char *str = NULL;
@@ -535,9 +535,9 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 goto out;
 
 }
-gsconf = g_new0(GlusterServer, 1);
-gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-   GLUSTER_TRANSPORT__MAX, -1,
+gsconf = g_new0(SocketAddressFlat, 1);
+gsconf->type = qapi_enum_parse(SocketAddressFlatType_lookup, ptr,
+   SOCKET_ADDRESS_FLAT_TYPE__MAX, -1,
_err);
 if (local_err) {
 error_append_hint(_err,
@@ -548,7 +548,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 }
 qemu_opts_del(opts);
 
-if (gsconf->type == GLUSTER_TRANSPORT_TCP) {
+if (gsconf->type == SOCKET_ADDRESS_FLAT_TYPE_TCP) {
 /* create opts info from runtime_tcp_opts list */
 opts = qemu_opts_create(_tcp_opts, NULL, 0, _abort);
 qemu_opts_absorb_qdict(opts, backing_options, _err);
@@ -617,11 

[Qemu-block] [PATCH v2 02/15] sheepdog: Fix error handling in sd_snapshot_delete()

[Markus Armbruster]

As a bdrv_snapshot_delete() method, sd_snapshot_delete() must set an
error and return negative errno on failure.  It sometimes returns -1,
and sometimes neglects to set an error.  It also prints error messages
with error_report().  Fix all that.

Moreover, its handling of an attempt to delete an nonexistent snapshot
is wrong: it error_report()s and succeeds.  Fix it to set an error and
return -ENOENT instead.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/sheepdog.c | 41 +++--
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index c3ee4ce..0a0803e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2405,18 +2405,15 @@ out:
 
 #define NR_BATCHED_DISCARD 128
 
-static bool remove_objects(BDRVSheepdogState *s)
+static int remove_objects(BDRVSheepdogState *s, Error **errp)
 {
 int fd, i = 0, nr_objs = 0;
-Error *local_err = NULL;
-int ret = 0;
-bool result = true;
+int ret;
 SheepdogInode *inode = >inode;
 
-fd = connect_to_sdog(s, _err);
+fd = connect_to_sdog(s, errp);
 if (fd < 0) {
-error_report_err(local_err);
-return false;
+return fd;
 }
 
 nr_objs = count_data_objs(inode);
@@ -2446,15 +2443,15 @@ static bool remove_objects(BDRVSheepdogState *s)
 data_vdi_id[start_idx]),
false, s->cache_flags);
 if (ret < 0) {
-error_report("failed to discard snapshot inode.");
-result = false;
+error_setg(errp, "Failed to discard snapshot inode");
 goto out;
 }
 }
 
+ret = 0;
 out:
 closesocket(fd);
-return result;
+return ret;
 }
 
 static int sd_snapshot_delete(BlockDriverState *bs,
@@ -2464,7 +2461,6 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 {
 unsigned long snap_id = 0;
 char snap_tag[SD_MAX_VDI_TAG_LEN];
-Error *local_err = NULL;
 int fd, ret;
 char buf[SD_MAX_VDI_LEN + SD_MAX_VDI_TAG_LEN];
 BDRVSheepdogState *s = bs->opaque;
@@ -2477,8 +2473,9 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 };
 SheepdogVdiRsp *rsp = (SheepdogVdiRsp *)
 
-if (!remove_objects(s)) {
-return -1;
+ret = remove_objects(s, errp);
+if (ret) {
+return ret;
 }
 
 memset(buf, 0, sizeof(buf));
@@ -2498,36 +2495,36 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
 }
 
-ret = find_vdi_name(s, s->name, snap_id, snap_tag, , true,
-_err);
+ret = find_vdi_name(s, s->name, snap_id, snap_tag, , true, errp);
 if (ret) {
 return ret;
 }
 
-fd = connect_to_sdog(s, _err);
+fd = connect_to_sdog(s, errp);
 if (fd < 0) {
-error_report_err(local_err);
-return -1;
+return fd;
 }
 
 ret = do_req(fd, s->bs, (SheepdogReq *),
  buf, , );
 closesocket(fd);
 if (ret) {
+error_setg_errno(errp, -ret, "Couldn't send request to server");
 return ret;
 }
 
 switch (rsp->result) {
 case SD_RES_NO_VDI:
-error_report("%s was already deleted", s->name);
+error_setg(errp, "Can't find the snapshot");
+return -ENOENT;
 case SD_RES_SUCCESS:
 break;
 default:
-error_report("%s, %s", sd_strerror(rsp->result), s->name);
-return -1;
+error_setg(errp, "%s", sd_strerror(rsp->result));
+return -EIO;
 }
 
-return ret;
+return 0;
 }
 
 static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
-- 
2.7.4

[Qemu-block] [PATCH v2 06/15] sheepdog: Don't truncate long VDI name in _open(), _create()

[Markus Armbruster]

sd_parse_uri() truncates long VDI names silently.  Reject them
instead.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 block/sheepdog.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index d3d3731..fed7264 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -985,7 +985,10 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
 ret = -EINVAL;
 goto out;
 }
-pstrcpy(vdi, SD_MAX_VDI_LEN, uri->path + 1);
+if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
+ret = -EINVAL;
+goto out;
+}
 
 qp = query_params_parse(uri->query);
 if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-- 
2.7.4

[Qemu-block] [PATCH v2 11/15] gluster: Don't duplicate qapi-util.c's qapi_enum_parse()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Niels de Vos 
---
 block/gluster.c | 30 +-
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 7236d59..6fbcf9e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -12,6 +12,7 @@
 #include "block/block_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/util.h"
 #include "qemu/uri.h"
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
@@ -472,23 +473,6 @@ out:
 return NULL;
 }
 
-static int qapi_enum_parse(const char *opt)
-{
-int i;
-
-if (!opt) {
-return GLUSTER_TRANSPORT__MAX;
-}
-
-for (i = 0; i < GLUSTER_TRANSPORT__MAX; i++) {
-if (!strcmp(opt, GlusterTransport_lookup[i])) {
-return i;
-}
-}
-
-return i;
-}
-
 /*
  * Convert the json formatted command line into qapi.
 */
@@ -546,16 +530,20 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 
 ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
 gsconf = g_new0(GlusterServer, 1);
-gsconf->type = qapi_enum_parse(ptr);
+gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
+   GLUSTER_TRANSPORT__MAX,
+   GLUSTER_TRANSPORT__MAX,
+   _err);
 if (!ptr) {
 error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
 error_append_hint(_err, GERR_INDEX_HINT, i);
 goto out;
 
 }
-if (gsconf->type == GLUSTER_TRANSPORT__MAX) {
-error_setg(_err, QERR_INVALID_PARAMETER_VALUE,
-   GLUSTER_OPT_TYPE, "tcp or unix");
+if (local_err) {
+error_append_hint(_err,
+  "Parameter '%s' may be 'tcp' or 'unix'\n",
+  GLUSTER_OPT_TYPE);
 error_append_hint(_err, GERR_INDEX_HINT, i);
 goto out;
 }
-- 
2.7.4

[Qemu-block] [PATCH v2 12/15] gluster: Plug memory leaks in qemu_gluster_parse_json()

[Markus Armbruster]

To reproduce, run

$ valgrind qemu-system-x86_64 --nodefaults -S --drive 
driver=gluster,volume=testvol,path=/a/b/c,server.0.type=xxx

Signed-off-by: Markus Armbruster 
Reviewed-by: Niels de Vos 
---
 block/gluster.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 6fbcf9e..991f18f 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -480,7 +480,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
   QDict *options, Error **errp)
 {
 QemuOpts *opts;
-GlusterServer *gsconf;
+GlusterServer *gsconf = NULL;
 GlusterServerList *curr = NULL;
 QDict *backing_options = NULL;
 Error *local_err = NULL;
@@ -529,17 +529,16 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 }
 
 ptr = qemu_opt_get(opts, GLUSTER_OPT_TYPE);
+if (!ptr) {
+error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
+error_append_hint(_err, GERR_INDEX_HINT, i);
+goto out;
+
+}
 gsconf = g_new0(GlusterServer, 1);
 gsconf->type = qapi_enum_parse(GlusterTransport_lookup, ptr,
-   GLUSTER_TRANSPORT__MAX,
-   GLUSTER_TRANSPORT__MAX,
+   GLUSTER_TRANSPORT__MAX, -1,
_err);
-if (!ptr) {
-error_setg(_err, QERR_MISSING_PARAMETER, GLUSTER_OPT_TYPE);
-error_append_hint(_err, GERR_INDEX_HINT, i);
-goto out;
-
-}
 if (local_err) {
 error_append_hint(_err,
   "Parameter '%s' may be 'tcp' or 'unix'\n",
@@ -626,8 +625,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 curr->next->value = gsconf;
 curr = curr->next;
 }
+gsconf = NULL;
 
-qdict_del(backing_options, str);
+QDECREF(backing_options);
+backing_options = NULL;
 g_free(str);
 str = NULL;
 }
@@ -636,11 +637,10 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster 
*gconf,
 
 out:
 error_propagate(errp, local_err);
+qapi_free_GlusterServer(gsconf);
 qemu_opts_del(opts);
-if (str) {
-qdict_del(backing_options, str);
-g_free(str);
-}
+g_free(str);
+QDECREF(backing_options);
 errno = EINVAL;
 return -errno;
 }
-- 
2.7.4

[Qemu-block] [PATCH v2 15/15] sheepdog: Support blockdev-add

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 qapi/block-core.json | 27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index d63be0a..9bb7f4a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2124,6 +2124,7 @@
 # @ssh: Since 2.8
 # @iscsi: Since 2.9
 # @rbd: Since 2.9
+# @sheepdog: Since 2.9
 #
 # Since: 2.0
 ##
@@ -2132,8 +2133,8 @@
 'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs',
 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed',
-'quorum', 'raw', 'rbd', 'replication', 'ssh', 'vdi', 'vhdx', 
'vmdk',
-'vpc', 'vvfat' ] }
+'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
+'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile:
@@ -2692,6 +2693,26 @@
 '*password-secret': 'str' } }
 
 ##
+# @BlockdevOptionsSheepdog:
+#
+# Driver specific block device options for sheepdog
+#
+# @vdi: Virtual disk image name
+# @addr:The Sheepdog server to connect to
+# @snap-id: Snapshot ID
+# @tag: Snapshot tag name
+#
+# Only one of @snap-id and @tag may be present.
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsSheepdog',
+  'data': { 'addr': 'SocketAddressFlat',
+'vdi': 'str',
+'*snap-id': 'uint32',
+'*tag': 'str' } }
+
+##
 # @ReplicationMode:
 #
 # An enumeration of replication modes.
@@ -2891,7 +2912,7 @@
   'raw':'BlockdevOptionsRaw',
   'rbd':'BlockdevOptionsRbd',
   'replication':'BlockdevOptionsReplication',
-# TODO sheepdog: Wait for structured options
+  'sheepdog':   'BlockdevOptionsSheepdog',
   'ssh':'BlockdevOptionsSsh',
   'vdi':'BlockdevOptionsGenericFormat',
   'vhdx':   'BlockdevOptionsGenericFormat',
-- 
2.7.4

[Qemu-block] [PATCH v2 00/15] block: A bunch of fixes for Sheepdog and Gluster

[Markus Armbruster]

Bad error handling, memory leaks, and lack of blockdev-add support.

v2:
* Straightforward rebase
* PATCH 01: Superfluous conditional dropped, labels renamed, commit
  message updated accordingly [Eric]
* PATCH 02: More pleasant use of @ret, error message capitalized,
  line break tidied up [Kevin, Eric]
* PATCH 05: Whitespace tidied up [Kevin]
* PATCH 06: Missing assignment to ret fixed up [Eric]
* PATCH 07: Missing assignment to ret fixed up [Kevin]
* PATCH 12: Use -1 instead of 0 for unused argument [Niels]
* PATCH 14: Better explain compatibility impact in commit message,
  whitespace tidied up [Eric]


Markus Armbruster (15):
  sheepdog: Defuse time bomb in sd_open() error handling
  sheepdog: Fix error handling in sd_snapshot_delete()
  sheepdog: Fix error handling sd_create()
  sheepdog: Mark sd_snapshot_delete() lossage FIXME
  sheepdog: Fix snapshot ID parsing in _open(), _create, _goto()
  sheepdog: Don't truncate long VDI name in _open(), _create()
  sheepdog: Report errors in pseudo-filename more usefully
  sheepdog: Use SocketAddress and socket_connect()
  sheepdog: Implement bdrv_parse_filename()
  gluster: Drop assumptions on SocketTransport names
  gluster: Don't duplicate qapi-util.c's qapi_enum_parse()
  gluster: Plug memory leaks in qemu_gluster_parse_json()
  qapi-schema: Rename GlusterServer to SocketAddressFlat
  qapi-schema: Rename SocketAddressFlat's variant tcp to inet
  sheepdog: Support blockdev-add

 block/gluster.c  | 127 +++
 block/sheepdog.c | 453 +--
 qapi-schema.json |  38 +
 qapi/block-core.json |  73 +++--
 4 files changed, 451 insertions(+), 240 deletions(-)

-- 
2.7.4

[Qemu-block] [PATCH v2 04/15] sheepdog: Mark sd_snapshot_delete() lossage FIXME

[Markus Armbruster]

sd_snapshot_delete() should delete the snapshot whose ID matches
@snapshot_id and whose name matches @name.  But that's not what it
does.  If @snapshot_id is a valid ID, it deletes the snapshot with
that ID, else it deletes the snapshot with that name.  It doesn't use
@name at all.  Add suitable FIXME comments, so someone who actually
knows Sheepdog can fix it.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 block/sheepdog.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index be3db1f..187bcd8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2457,6 +2457,10 @@ static int sd_snapshot_delete(BlockDriverState *bs,
   const char *name,
   Error **errp)
 {
+/*
+ * FIXME should delete the snapshot matching both @snapshot_id and
+ * @name, but @name not used here
+ */
 unsigned long snap_id = 0;
 char snap_tag[SD_MAX_VDI_TAG_LEN];
 int fd, ret;
@@ -2481,6 +2485,11 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 pstrcpy(buf, SD_MAX_VDI_LEN, s->name);
 ret = qemu_strtoul(snapshot_id, NULL, 10, _id);
 if (ret || snap_id > UINT32_MAX) {
+/*
+ * FIXME Since qemu_strtoul() returns -EINVAL when
+ * @snapshot_id is null, @snapshot_id is mandatory.  Correct
+ * would be to require at least one of @snapshot_id and @name.
+ */
 error_setg(errp, "Invalid snapshot ID: %s",
  snapshot_id ? snapshot_id : "");
 return -EINVAL;
@@ -2489,6 +2498,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 if (snap_id) {
 hdr.snapid = (uint32_t) snap_id;
 } else {
+/* FIXME I suspect we should use @name here */
 pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id);
 pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
 }
-- 
2.7.4

[Qemu-block] [PATCH v2 01/15] sheepdog: Defuse time bomb in sd_open() error handling

[Markus Armbruster]

When qemu_opts_absorb_qdict() fails, sd_open() closes stdin, because
sd->fd is still zero.  Fortunately, qemu_opts_absorb_qdict() can't
fail, because:

1. it only fails when qemu_opt_parse() fails, and
2. the only member of runtime_opts.desc[] is a QEMU_OPT_STRING, and
3. qemu_opt_parse() can't fail for QEMU_OPT_STRING.

Defuse this ticking time bomb by jumping behind the file descriptor
cleanup on error.

Also do that for the error paths where sd->fd is still -1.  The file
descriptor cleanup happens to do nothing then, but let's not rely on
that here.

While there, rename label out to err, because it's on the error path,
not the normal path out of the function.

Signed-off-by: Markus Armbruster 
---
 block/sheepdog.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 7434710..c3ee4ce 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1392,7 +1392,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, 
int flags,
 if (local_err) {
 error_propagate(errp, local_err);
 ret = -EINVAL;
-goto out;
+goto err_no_fd;
 }
 
 filename = qemu_opt_get(opts, "filename");
@@ -1412,17 +1412,17 @@ static int sd_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 if (ret < 0) {
 error_setg(errp, "Can't parse filename");
-goto out;
+goto err_no_fd;
 }
 s->fd = get_sheep_fd(s, errp);
 if (s->fd < 0) {
 ret = s->fd;
-goto out;
+goto err_no_fd;
 }
 
 ret = find_vdi_name(s, vdi, snapid, tag, , true, errp);
 if (ret) {
-goto out;
+goto err;
 }
 
 /*
@@ -1443,7 +1443,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, 
int flags,
 fd = connect_to_sdog(s, errp);
 if (fd < 0) {
 ret = fd;
-goto out;
+goto err;
 }
 
 buf = g_malloc(SD_INODE_SIZE);
@@ -1454,7 +1454,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, 
int flags,
 
 if (ret) {
 error_setg(errp, "Can't read snapshot inode");
-goto out;
+goto err;
 }
 
 memcpy(>inode, buf, sizeof(s->inode));
@@ -1466,12 +1466,12 @@ static int sd_open(BlockDriverState *bs, QDict 
*options, int flags,
 qemu_opts_del(opts);
 g_free(buf);
 return 0;
-out:
+
+err:
 aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
false, NULL, NULL, NULL, NULL);
-if (s->fd >= 0) {
-closesocket(s->fd);
-}
+closesocket(s->fd);
+err_no_fd:
 qemu_opts_del(opts);
 g_free(buf);
 return ret;
-- 
2.7.4

[Qemu-block] [PATCH v2 07/15] sheepdog: Report errors in pseudo-filename more usefully

[Markus Armbruster]

Errors in the pseudo-filename are all reported with the same laconic
"Can't parse filename" message.

Add real error reporting, such as:

$ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepdog:///
qemu-system-x86_64: --drive driver=sheepdog,filename=sheepdog:///: missing 
file path in URI
$ qemu-system-x86_64 --drive driver=sheepdog,filename=sheepgod:///vdi
qemu-system-x86_64: --drive driver=sheepdog,filename=sheepgod:///vdi: URI 
scheme must be 'sheepdog', 'sheepdog+tcp', or 'sheepdog+unix'
$ qemu-system-x86_64 --drive 
driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock
qemu-system-x86_64: --drive 
driver=sheepdog,filename=sheepdog+unix:///vdi?socke=sheepdog.sock: unexpected 
query parameters

The code to translate legacy syntax to URI fails to escape URI
meta-characters.  The new error messages are misleading then.  Replace
them by the old "Can't parse filename" message.  "Internal error"
would be more honest.  Anyway, no worse than before.  Also add a FIXME
comment.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Kevin Wolf 
---
 block/sheepdog.c | 88 +---
 1 file changed, 59 insertions(+), 29 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index fed7264..161932d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -957,16 +957,18 @@ static bool sd_parse_snapid_or_tag(const char *str,
 return true;
 }
 
-static int sd_parse_uri(BDRVSheepdogState *s, const char *filename,
-char *vdi, uint32_t *snapid, char *tag)
+static void sd_parse_uri(BDRVSheepdogState *s, const char *filename,
+ char *vdi, uint32_t *snapid, char *tag,
+ Error **errp)
 {
+Error *err = NULL;
 URI *uri;
 QueryParams *qp = NULL;
-int ret = 0;
 
 uri = uri_parse(filename);
 if (!uri) {
-return -EINVAL;
+error_setg(, "invalid URI");
+goto out;
 }
 
 /* transport */
@@ -977,34 +979,46 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
 } else if (!strcmp(uri->scheme, "sheepdog+unix")) {
 s->is_unix = true;
 } else {
-ret = -EINVAL;
+error_setg(, "URI scheme must be 'sheepdog', 'sheepdog+tcp',"
+   " or 'sheepdog+unix'");
 goto out;
 }
 
 if (uri->path == NULL || !strcmp(uri->path, "/")) {
-ret = -EINVAL;
+error_setg(, "missing file path in URI");
 goto out;
 }
 if (g_strlcpy(vdi, uri->path + 1, SD_MAX_VDI_LEN) >= SD_MAX_VDI_LEN) {
-ret = -EINVAL;
+error_setg(, "VDI name is too long");
 goto out;
 }
 
 qp = query_params_parse(uri->query);
-if (qp->n > 1 || (s->is_unix && !qp->n) || (!s->is_unix && qp->n)) {
-ret = -EINVAL;
-goto out;
-}
 
 if (s->is_unix) {
 /* sheepdog+unix:///vdiname?socket=path */
-if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
-ret = -EINVAL;
+if (uri->server || uri->port) {
+error_setg(, "URI scheme %s doesn't accept a server address",
+   uri->scheme);
+goto out;
+}
+if (!qp->n) {
+error_setg(,
+   "URI scheme %s requires query parameter 'socket'",
+   uri->scheme);
+goto out;
+}
+if (qp->n != 1 || strcmp(qp->p[0].name, "socket")) {
+error_setg(, "unexpected query parameters");
 goto out;
 }
 s->host_spec = g_strdup(qp->p[0].value);
 } else {
 /* sheepdog[+tcp]://[host:port]/vdiname */
+if (qp->n) {
+error_setg(, "unexpected query parameters");
+goto out;
+}
 s->host_spec = g_strdup_printf("%s:%d", uri->server ?: SD_DEFAULT_ADDR,
uri->port ?: SD_DEFAULT_PORT);
 }
@@ -1012,7 +1026,8 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char 
*filename,
 /* snapshot tag */
 if (uri->fragment) {
 if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) {
-ret = -EINVAL;
+error_setg(, "'%s' is not a valid snapshot ID",
+   uri->fragment);
 goto out;
 }
 } else {
@@ -1020,11 +1035,11 @@ static int sd_parse_uri(BDRVSheepdogState *s, const 
char *filename,
 }
 
 out:
+error_propagate(errp, err);
 if (qp) {
 query_params_free(qp);
 }
 uri_free(uri);
-return ret;
 }
 
 /*
@@ -1044,12 +1059,14 @@ out:
  * You can run VMs outside the Sheepdog cluster by specifying
  * `hostname' and `port' (experimental).
  */
-static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
- char *vdi, uint32_t *snapid, char *tag)
+static void 

[Qemu-block] [PATCH v2 03/15] sheepdog: Fix error handling sd_create()

[Markus Armbruster]

As a bdrv_create() method, sd_create() must set an error and return
negative errno on failure.  It prints the error instead of setting it
when connect_to_sdog() fails.  Fix that.

While there, return the value of connect_to_sdog() like we do
elsewhere, instead of -EIO.  No functional change, as
connect_to_sdog() returns no other error code.

Many more suspicious uses of error_report() and error_report_err()
remain in other functions.  Left for another day.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Kevin Wolf 
---
 block/sheepdog.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0a0803e..be3db1f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1829,14 +1829,12 @@ static int sd_create(const char *filename, QemuOpts 
*opts,
 if (s->inode.block_size_shift == 0) {
 SheepdogVdiReq hdr;
 SheepdogClusterRsp *rsp = (SheepdogClusterRsp *)
-Error *local_err = NULL;
 int fd;
 unsigned int wlen = 0, rlen = 0;
 
-fd = connect_to_sdog(s, _err);
+fd = connect_to_sdog(s, errp);
 if (fd < 0) {
-error_report_err(local_err);
-ret = -EIO;
+ret = fd;
 goto out;
 }
 
-- 
2.7.4

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Peter Lieven]

Am 06.03.2017 um 17:46 schrieb Eric Blake:

On 02/28/2017 07:35 AM, Peter Lieven wrote:

img_convert has been around before there was an ImgConvertState or
a block backend, but it has never been modified to directly use
these structs. Change this by parsing parameters directly into
the ImgConvertState and directly use BlockBackend where possible.
Futhermore variable initalization has been reworked and sorted.

s/Futhermore/Furthermore/
s/initalization/initialization/

I almost wonder if this should be split into multiple patches for ease
of review.  But since you've already submitted it, I'll review it as-is.


Signed-off-by: Peter Lieven 
---
  qemu-img.c | 197 +
  1 file changed, 81 insertions(+), 116 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index caa76a7..f271167 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
  int min_sparse;
  size_t cluster_sectors;
  size_t buf_sectors;
-int num_coroutines;
+long num_coroutines;

Why is this change needed? A hint in the commit message may be
appropriate (I'm especially wary of 'long', as it differs between 32-
and 64-bit platforms).


This is long because it is passed as a pointer to qemu_strtol. num_coroutines
was also long.

Peter

Re: [Qemu-block] [PATCH] qemu-img: simplify img_convert

[Eric Blake]

On 02/28/2017 07:35 AM, Peter Lieven wrote:
> img_convert has been around before there was an ImgConvertState or
> a block backend, but it has never been modified to directly use
> these structs. Change this by parsing parameters directly into
> the ImgConvertState and directly use BlockBackend where possible.
> Futhermore variable initalization has been reworked and sorted.

s/Futhermore/Furthermore/
s/initalization/initialization/

I almost wonder if this should be split into multiple patches for ease
of review.  But since you've already submitted it, I'll review it as-is.

> 
> Signed-off-by: Peter Lieven 
> ---
>  qemu-img.c | 197 
> +
>  1 file changed, 81 insertions(+), 116 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index caa76a7..f271167 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1488,7 +1488,7 @@ typedef struct ImgConvertState {
>  int min_sparse;
>  size_t cluster_sectors;
>  size_t buf_sectors;
> -int num_coroutines;
> +long num_coroutines;

Why is this change needed? A hint in the commit message may be
appropriate (I'm especially wary of 'long', as it differs between 32-
and 64-bit platforms).

> @@ -2012,19 +2002,18 @@ static int img_convert(int argc, char **argv)
>  quiet = true;
>  break;
>  case 'n':
> -skip_create = 1;
> +skip_create = true;
>  break;
>  case 'm':
> -if (qemu_strtol(optarg, NULL, 0, _coroutines) ||
> -num_coroutines < 1 || num_coroutines > MAX_COROUTINES) {
> +if (qemu_strtol(optarg, NULL, 0, _coroutines) ||
> +s.num_coroutines < 1 || s.num_coroutines > MAX_COROUTINES) {

At any rate, it looks like you clamp it, so the change in type doesn't
matter too much.

I didn't spot any problems in the conversion, so:
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] commit: Fix error handling

[Philippe Mathieu-Daudé]

On 03/06/2017 01:21 PM, Kevin Wolf wrote:

Apparently some kind of mismerge happened in commit 8dfba279, which
broke the error handling without any real reason by removing the
assignment of the return value to ret in a blk_insert_bs() call.

Signed-off-by: Kevin Wolf 


Reviewed-by: Philippe Mathieu-Daudé 


---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 22a0a4d..e57c1cf 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -364,7 +364,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,

 /* Required permissions are already taken with block_job_add_bdrv() */
 s->top = blk_new(0, BLK_PERM_ALL);
-blk_insert_bs(s->top, top, errp);
+ret = blk_insert_bs(s->top, top, errp);
 if (ret < 0) {
 goto fail;
 }

[Qemu-block] [PATCH 05/10] block: Fix blockdev-snapshot error handling

[Kevin Wolf]

For blockdev-snapshot, external_snapshot_prepare() accepts an arbitrary
node reference at first and only checks later whether it already has a
backing file. Between those places, other errors can occur.

Therefore checking in external_snapshot_abort() whether state->new_bs
has a backing file is not sufficient to tell whether bdrv_append() was
already completed or not. Trying to undo the bdrv_append() when it
wasn't even executed is wrong.

Introduce a new boolean flag in the state to fix this.

Signed-off-by: Kevin Wolf 
---
 blockdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 8eb4e84..af67ce4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1614,6 +1614,7 @@ typedef struct ExternalSnapshotState {
 BlockDriverState *old_bs;
 BlockDriverState *new_bs;
 AioContext *aio_context;
+bool overlay_appended;
 } ExternalSnapshotState;
 
 static void external_snapshot_prepare(BlkActionState *common,
@@ -1780,6 +1781,7 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 error_propagate(errp, local_err);
 return;
 }
+state->overlay_appended = true;
 }
 
 static void external_snapshot_commit(BlkActionState *common)
@@ -1803,7 +1805,7 @@ static void external_snapshot_abort(BlkActionState 
*common)
 ExternalSnapshotState *state =
  DO_UPCAST(ExternalSnapshotState, common, common);
 if (state->new_bs) {
-if (state->new_bs->backing) {
+if (state->overlay_appended) {
 bdrv_replace_in_backing_chain(state->new_bs, state->old_bs);
 }
 }
-- 
1.8.3.1

Re: [Qemu-block] [PATCH 1/2] block: Don't use error_abort in blk_new_open

[Kevin Wolf]

Am 03.03.2017 um 14:38 hat Fam Zheng geschrieben:
> We have an errp and bdrv_root_attach_child can fail permission check,
> error_abort is not the best choice here.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/block-backend.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index daa7908..4eab68f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -213,7 +213,11 @@ BlockBackend *blk_new_open(const char *filename, const 
> char *reference,
>  }
>  
>  blk->root = bdrv_root_attach_child(bs, "root", _root,
> -   perm, BLK_PERM_ALL, blk, 
> _abort);
> +   perm, BLK_PERM_ALL, blk, errp);
> +if (!blk->root) {
> +blk_unref(blk);
> +return NULL;
> +}

Shouldn't we bdrv_unref(bs), too?

Kevin

[Qemu-block] [PATCH 06/10] block: Factor out should_update_child()

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 block.c | 42 +++---
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index f293ccb..6dc02b8 100644
--- a/block.c
+++ b/block.c
@@ -2886,28 +2886,40 @@ void bdrv_close_all(void)
 assert(QTAILQ_EMPTY(_bdrv_states));
 }
 
+static bool should_update_child(BdrvChild *c, BlockDriverState *to)
+{
+BdrvChild *to_c;
+
+if (c->role->stay_at_node) {
+return false;
+}
+
+if (c->role == _backing) {
+/* If @from is a backing file of @to, ignore the child to avoid
+ * creating a loop. We only want to change the pointer of other
+ * parents. */
+QLIST_FOREACH(to_c, >children, next) {
+if (to_c == c) {
+break;
+}
+}
+if (to_c) {
+return false;
+}
+}
+
+return true;
+}
+
 static void change_parent_backing_link(BlockDriverState *from,
BlockDriverState *to)
 {
-BdrvChild *c, *next, *to_c;
+BdrvChild *c, *next;
 
 QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
-if (c->role->stay_at_node) {
+if (!should_update_child(c, to)) {
 continue;
 }
-if (c->role == _backing) {
-/* If @from is a backing file of @to, ignore the child to avoid
- * creating a loop. We only want to change the pointer of other
- * parents. */
-QLIST_FOREACH(to_c, >children, next) {
-if (to_c == c) {
-break;
-}
-}
-if (to_c) {
-continue;
-}
-}
 
 bdrv_ref(to);
 /* FIXME Are we sure that bdrv_replace_child() can't run into
-- 
1.8.3.1

[Qemu-block] [PATCH 10/10] block: Fix error handling in bdrv_replace_in_backing_chain()

[Kevin Wolf]

When adding an Error parameter, bdrv_replace_in_backing_chain() would
become nothing more than a wrapper around change_parent_backing_link().
So make the latter public, renamed as bdrv_replace_node(), and remove
bdrv_replace_in_backing_chain().

Most of the callers just remove a node from the graph that they just
inserted, so they can use _abort, but completion of a mirror job
with 'replaces' set can actually fail.

Signed-off-by: Kevin Wolf 
---
 block.c   | 25 ++---
 block/mirror.c| 15 +--
 blockdev.c|  2 +-
 include/block/block.h |  4 ++--
 include/block/block_int.h |  4 ++--
 5 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index a310132..dd9ded8 100644
--- a/block.c
+++ b/block.c
@@ -2932,8 +2932,8 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
 return true;
 }
 
-static void change_parent_backing_link(BlockDriverState *from,
-   BlockDriverState *to, Error **errp)
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+   Error **errp)
 {
 BdrvChild *c, *next;
 GSList *list = NULL, *p;
@@ -2941,6 +2941,9 @@ static void change_parent_backing_link(BlockDriverState 
*from,
 uint64_t perm = 0, shared = BLK_PERM_ALL;
 int ret;
 
+assert(!atomic_read(>in_flight));
+assert(!atomic_read(>in_flight));
+
 /* Make sure that @from doesn't go away until we have successfully attached
  * all of its parents to @to. */
 bdrv_ref(from);
@@ -3003,16 +3006,13 @@ void bdrv_append(BlockDriverState *bs_new, 
BlockDriverState *bs_top,
 {
 Error *local_err = NULL;
 
-assert(!atomic_read(_top->in_flight));
-assert(!atomic_read(_new->in_flight));
-
 bdrv_set_backing_hd(bs_new, bs_top, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
 }
 
-change_parent_backing_link(bs_top, bs_new, _err);
+bdrv_replace_node(bs_top, bs_new, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 bdrv_set_backing_hd(bs_new, NULL, _abort);
@@ -3025,19 +3025,6 @@ out:
 bdrv_unref(bs_new);
 }
 
-void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState 
*new)
-{
-assert(!bdrv_requests_pending(old));
-assert(!bdrv_requests_pending(new));
-
-bdrv_ref(old);
-
-/* FIXME Proper error handling */
-change_parent_backing_link(old, new, _abort);
-
-bdrv_unref(old);
-}
-
 static void bdrv_delete(BlockDriverState *bs)
 {
 assert(!bs->job);
diff --git a/block/mirror.c b/block/mirror.c
index f24dc51..a5d30ee 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -550,8 +550,12 @@ static void mirror_exit(BlockJob *job, void *opaque)
 /* The mirror job has no requests in flight any more, but we need to
  * drain potential other users of the BDS before changing the graph. */
 bdrv_drained_begin(target_bs);
-bdrv_replace_in_backing_chain(to_replace, target_bs);
+bdrv_replace_node(to_replace, target_bs, _err);
 bdrv_drained_end(target_bs);
+if (local_err) {
+error_report_err(local_err);
+data->ret = -EPERM;
+}
 }
 if (s->to_replace) {
 bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
@@ -570,12 +574,11 @@ static void mirror_exit(BlockJob *job, void *opaque)
  * block the removal. */
 block_job_remove_all_bdrv(job);
 bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), _abort);
 
 /* We just changed the BDS the job BB refers to (with either or both of the
- * bdrv_replace_in_backing_chain() calls), so switch the BB back so the
- * cleanup does the right thing. We don't need any permissions any more
- * now. */
+ * bdrv_replace_node() calls), so switch the BB back so the cleanup does
+ * the right thing. We don't need any permissions any more now. */
 blk_remove_bs(job->blk);
 blk_set_perm(job->blk, 0, BLK_PERM_ALL, _abort);
 blk_insert_bs(job->blk, mirror_top_bs, _abort);
@@ -1234,7 +1237,7 @@ fail:
 }
 
 bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
-bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
+bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), _abort);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index af67ce4..f1f49bd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1806,7 +1806,7 @@ static void external_snapshot_abort(BlkActionState 
*common)
  DO_UPCAST(ExternalSnapshotState, common, common);
 if (state->new_bs) {
 if (state->overlay_appended) {
-

[Qemu-block] [PATCH 09/10] block: Handle permission errors in change_parent_backing_link()

[Kevin Wolf]

Instead of just trying to change parents by parent over to reference @to
instead of @from, and abort()ing whenever the permissions don't allow
this, do proper permission checking beforehand and pass any error to the
callers.

Signed-off-by: Kevin Wolf 
---
 block.c | 50 --
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a7b09d3..a310132 100644
--- a/block.c
+++ b/block.c
@@ -2933,21 +2933,53 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
-   BlockDriverState *to)
+   BlockDriverState *to, Error **errp)
 {
 BdrvChild *c, *next;
+GSList *list = NULL, *p;
+uint64_t old_perm, old_shared;
+uint64_t perm = 0, shared = BLK_PERM_ALL;
+int ret;
+
+/* Make sure that @from doesn't go away until we have successfully attached
+ * all of its parents to @to. */
+bdrv_ref(from);
 
+/* Put all parents into @list and calculate their cumulative permissions */
 QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
 if (!should_update_child(c, to)) {
 continue;
 }
+list = g_slist_prepend(list, c);
+perm |= c->perm;
+shared &= c->shared_perm;
+}
+
+/* Check whether the required permissions can be granted on @to, ignoring
+ * all BdrvChild in @list so that they can't block themselves. */
+ret = bdrv_check_update_perm(to, perm, shared, list, errp);
+if (ret < 0) {
+bdrv_abort_perm_update(to);
+goto out;
+}
+
+/* Now actually perform the change. We performed the permission check for
+ * all elements of @list at once, so set the permissions all at once at the
+ * very end. */
+for (p = list; p != NULL; p = p->next) {
+c = p->data;
 
 bdrv_ref(to);
-/* FIXME Are we sure that bdrv_replace_child() can't run into
- * _abort because of permissions? */
-bdrv_replace_child(c, to, true);
+bdrv_replace_child_noperm(c, to);
 bdrv_unref(from);
 }
+
+bdrv_get_cumulative_perm(to, _perm, _shared);
+bdrv_set_perm(to, old_perm | perm, old_shared | shared);
+
+out:
+g_slist_free(list);
+bdrv_unref(from);
 }
 
 /*
@@ -2980,7 +3012,12 @@ void bdrv_append(BlockDriverState *bs_new, 
BlockDriverState *bs_top,
 goto out;
 }
 
-change_parent_backing_link(bs_top, bs_new);
+change_parent_backing_link(bs_top, bs_new, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+bdrv_set_backing_hd(bs_new, NULL, _abort);
+goto out;
+}
 
 /* bs_new is now referenced by its new parents, we don't need the
  * additional reference any more. */
@@ -2995,7 +3032,8 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, 
BlockDriverState *new)
 
 bdrv_ref(old);
 
-change_parent_backing_link(old, new);
+/* FIXME Proper error handling */
+change_parent_backing_link(old, new, _abort);
 
 bdrv_unref(old);
 }
-- 
1.8.3.1

[Qemu-block] [PATCH 02/10] mirror: Fix permission problem with 'replaces'

[Kevin Wolf]

The 'replaces' option of drive-mirror can be used to mirror a Quorum
node to a new image and then let the target image replace one of the
Quorum children. In order for this graph modification to succeed, the
mirror job needs to lift its restrictions on the target node first
before actually replacing the child.

Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 57f26c3..c9185b3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -509,6 +509,13 @@ static void mirror_exit(BlockJob *job, void *opaque)
  * block_job_completed(). */
 bdrv_ref(src);
 bdrv_ref(mirror_top_bs);
+bdrv_ref(target_bs);
+
+/* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
+ * inserting target_bs at s->to_replace, where we might not be able to get
+ * these permissions. */
+blk_unref(s->target);
+s->target = NULL;
 
 /* We don't access the source any more. Dropping any WRITE/RESIZE is
  * required before it could become a backing file of target_bs. */
@@ -555,8 +562,7 @@ static void mirror_exit(BlockJob *job, void *opaque)
 aio_context_release(replace_aio_context);
 }
 g_free(s->replaces);
-blk_unref(s->target);
-s->target = NULL;
+bdrv_unref(target_bs);
 
 /* Remove the mirror filter driver from the graph. Before this, get rid of
  * the blockers on the intermediate nodes so that the resulting state is
-- 
1.8.3.1

[Qemu-block] [PATCH 07/10] block: Factor out bdrv_replace_child_noperm()

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
---
 block.c | 38 +-
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 6dc02b8..d4570c8 100644
--- a/block.c
+++ b/block.c
@@ -1713,11 +1713,10 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
BdrvChild *c,
 *nshared = shared;
 }
 
-static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
-   bool check_new_perm)
+static void bdrv_replace_child_noperm(BdrvChild *child,
+  BlockDriverState *new_bs)
 {
 BlockDriverState *old_bs = child->bs;
-uint64_t perm, shared_perm;
 
 if (old_bs) {
 if (old_bs->quiesce_counter && child->role->drained_end) {
@@ -1727,7 +1726,29 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs,
 child->role->detach(child);
 }
 QLIST_REMOVE(child, next_parent);
+}
+
+child->bs = new_bs;
+
+if (new_bs) {
+QLIST_INSERT_HEAD(_bs->parents, child, next_parent);
+if (new_bs->quiesce_counter && child->role->drained_begin) {
+child->role->drained_begin(child);
+}
+
+if (child->role->attach) {
+child->role->attach(child);
+}
+}
+}
 
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
+   bool check_new_perm)
+{
+BlockDriverState *old_bs = child->bs;
+uint64_t perm, shared_perm;
+
+if (old_bs) {
 /* Update permissions for old node. This is guaranteed to succeed
  * because we're just taking a parent away, so we're loosening
  * restrictions. */
@@ -1736,23 +1757,14 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs,
 bdrv_set_perm(old_bs, perm, shared_perm);
 }
 
-child->bs = new_bs;
+bdrv_replace_child_noperm(child, new_bs);
 
 if (new_bs) {
-QLIST_INSERT_HEAD(_bs->parents, child, next_parent);
-if (new_bs->quiesce_counter && child->role->drained_begin) {
-child->role->drained_begin(child);
-}
-
 bdrv_get_cumulative_perm(new_bs, , _perm);
 if (check_new_perm) {
 bdrv_check_perm(new_bs, perm, shared_perm, _abort);
 }
 bdrv_set_perm(new_bs, perm, shared_perm);
-
-if (child->role->attach) {
-child->role->attach(child);
-}
 }
 }
 
-- 
1.8.3.1

[Qemu-block] [PATCH 03/10] mirror: Fix permissions for removing mirror_top_bs

[Kevin Wolf]

mirror_top_bs takes write permissions on its backing file, which can
make it impossible to attach that backing file node to another parent.
However, this is exactly what needs to be done in order to remove
mirror_top_bs from the backing chain. So give up the write permission
first.

Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9185b3..001b5f0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -566,8 +566,10 @@ static void mirror_exit(BlockJob *job, void *opaque)
 
 /* Remove the mirror filter driver from the graph. Before this, get rid of
  * the blockers on the intermediate nodes so that the resulting state is
- * valid. */
+ * valid. Also give up permissions on mirror_top_bs->backing, which might
+ * block the removal. */
 block_job_remove_all_bdrv(job);
+bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
 bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 
 /* We just changed the BDS the job BB refers to (with either or both of the
@@ -1234,6 +1236,7 @@ fail:
 block_job_unref(>common);
 }
 
+bdrv_child_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL);
 bdrv_replace_in_backing_chain(mirror_top_bs, backing_bs(mirror_top_bs));
 }
 
-- 
1.8.3.1

[Qemu-block] [PATCH 04/10] mirror: Fix error path for dirty bitmap creation

[Kevin Wolf]

mirror_top_bs must be removed from the graph again when creating the
dirty bitmap fails.

Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 001b5f0..f24dc51 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1197,10 +1197,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 
 s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
 if (!s->dirty_bitmap) {
-g_free(s->replaces);
-blk_unref(s->target);
-block_job_unref(>common);
-return;
+goto fail;
 }
 
 /* Required permissions are already taken with blk_new() */
-- 
1.8.3.1

[Qemu-block] [PATCH 01/10] commit: Fix error handling

[Kevin Wolf]

Apparently some kind of mismerge happened in commit 8dfba279, which
broke the error handling without any real reason by removing the
assignment of the return value to ret in a blk_insert_bs() call.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 22a0a4d..e57c1cf 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -364,7 +364,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 
 /* Required permissions are already taken with block_job_add_bdrv() */
 s->top = blk_new(0, BLK_PERM_ALL);
-blk_insert_bs(s->top, top, errp);
+ret = blk_insert_bs(s->top, top, errp);
 if (ret < 0) {
 goto fail;
 }
-- 
1.8.3.1

[Qemu-block] [PATCH 00/10] block: Op blocker fixes

[Kevin Wolf]

This series fixes a few problems introduced recently with the op blocker
series. It includes mainly fix for cases where qemu would abort()
instead of doing proper error handling previously. These changes also
happen to result in more complete and correct permission checking.

Kevin Wolf (10):
  commit: Fix error handling
  mirror: Fix permission problem with 'replaces'
  mirror: Fix permissions for removing mirror_top_bs
  mirror: Fix error path for dirty bitmap creation
  block: Fix blockdev-snapshot error handling
  block: Factor out should_update_child()
  block: Factor out bdrv_replace_child_noperm()
  block: Ignore multiple children in bdrv_check_update_perm()
  block: Handle permission errors in change_parent_backing_link()
  block: Fix error handling in bdrv_replace_in_backing_chain()

 block.c   | 182 ++
 block/commit.c|   2 +-
 block/mirror.c|  35 +
 blockdev.c|   6 +-
 include/block/block.h |   4 +-
 include/block/block_int.h |   6 +-
 6 files changed, 152 insertions(+), 83 deletions(-)

-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH 14/15] qapi-schema: Rename SocketAddressFlat's variant tcp to inet

[Markus Armbruster]

Markus Armbruster  writes:

> Eric Blake  writes:
>
>> On 03/02/2017 03:44 PM, Markus Armbruster wrote:
>>> QAPI type SocketAddressFlat differs from SocketAddress pointlessly:
>>> the discriminator value for variant InetSocketAddress is 'tcp' instead
>>> of 'inet'.  Rename.
>>> 
>>> The type is far only used by the Gluster block drivers.  Take care to
>>> keep 'tcp' working there.
>>
>> The old name was visible in QMP in 2.8, but only by blockdev-add, which
>> we've already argued was not stable (and where we've already made other
>> non-back-compat changes to it).  But that means this HAS to go into 2.9,
>> if we're declaring blockdev-add stable for 2.9.
>
> Yes.
>
> Note that the command line pseudo-filename's URI syntax stays the same
> (file=gluster+tcp://), and the command line's dotted key syntax keeps
> accepting tcp for compatiblity (file.server.0.type=tcp works in addition
> to =inet).
>
>> It wouldn't hurt to mention that additional information in the commit
>> message.
>
> I'll cook something up.

Replacing the second paragraph by

The type is so far only used by the Gluster block drivers.  Take care
to keep 'tcp' working in things like -drive's file.server.0.type=tcp.
The "gluster+tcp" URI scheme in pseudo-filenames stays the same.
blockdev-add changes, but it has changed incompatibly since 2.8
already.

[...]