Re: [Qemu-devel] Is the use of bdrv_getlength() in quorum_co_flush() kosher?

[Alberto Garcia]

On Fri 04 Aug 2017 02:48:03 PM CEST, Markus Armbruster wrote:
> Have a look at quorum_co_flush():
>
>             quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
>                               bdrv_getlength(s->children[i]->bs),
>                               s->children[i]->bs->node_name, result);
>
> bdrv_getlength() can fail.  Does it do the right thing then?

If it fails then it returns -errno, but then quorum_report_bad() turns
into uint64_t and assumes it's valid.

Since that number is then rounded up to the next multiple of
BDRV_SECTOR_SIZE in order to calculate end_sector, I think that what
happens in practice is that the user sees a QUORUM_REPORT_BAD event with
sectors-count = 0 (in most cases) or with a very high value in
sectors-count (if errno > BDRV_SECTOR_SIZE).

The result of bdrv_getlength() is only used to report the number of
affected sectors in the QUORUM_REPORT_BAD event, so there are no other
consequences.

Anyway I think it's a good idea not to make assumptions, detect the
error and pass 0 instead.

--- a/block/quorum.c
+++ b/block/quorum.c
@@ -785,8 +785,9 @@ static coroutine_fn int
quorum_co_flush(BlockDriverState *bs)
     for (i = 0; i < s->num_children; i++) {
         result = bdrv_co_flush(s->children[i]->bs);
         if (result) {
+            int64_t length = bdrv_getlength(s->children[i]->bs);
             quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0,
-                              bdrv_getlength(s->children[i]->bs),
+                              length > 0 ? length : 0,
                               s->children[i]->bs->node_name, result);
             result_value.l = result;
             quorum_count_vote(&error_votes, &result_value, i);

Berto