* [Qemu-devel] Is the use of bdrv_getlength() in quorum_co_flush() kosher?
@ 2017-08-04 12:48 Markus Armbruster
2017-08-04 13:38 ` Alberto Garcia
0 siblings, 1 reply; 2+ messages in thread
From: Markus Armbruster @ 2017-08-04 12:48 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-devel, qemu-block, Kevin Wolf
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?
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] Is the use of bdrv_getlength() in quorum_co_flush() kosher?
2017-08-04 12:48 [Qemu-devel] Is the use of bdrv_getlength() in quorum_co_flush() kosher? Markus Armbruster
@ 2017-08-04 13:38 ` Alberto Garcia
0 siblings, 0 replies; 2+ messages in thread
From: Alberto Garcia @ 2017-08-04 13:38 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, qemu-block, Kevin Wolf
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-08-04 13:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-04 12:48 [Qemu-devel] Is the use of bdrv_getlength() in quorum_co_flush() kosher? Markus Armbruster
2017-08-04 13:38 ` Alberto Garcia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.