| Message ID | 1344347073-7773-6-git-send-email-benoit@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 7, 2012 at 2:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote: > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block/quorum.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/block/quorum.c b/block/quorum.c > index 9da0432..5cd7083 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -118,12 +118,29 @@ static void quorum_close(BlockDriverState *bs) > } > } > > +static int64_t quorum_getlength(BlockDriverState *bs) > +{ > + BDRVQuorumState *s = bs->opaque; > + int i; > + int64_t ret; > + > + /* return the length of the first available quorum file */ > + for (i = 0, ret = bdrv_getlength(s->bs[i]); > + ret == -ENOMEDIUM && i <= 2; > + i++, ret = bdrv_getlength(s->bs[i])) { > + } Why is -ENOMEDIUM an expected error value? IMO a for loop with a body or a do while loop would make this easier to read.
> > +static int64_t quorum_getlength(BlockDriverState *bs) > > +{ > > + BDRVQuorumState *s = bs->opaque; > > + int i; > > + int64_t ret; > > + > > + /* return the length of the first available quorum file */ > > + for (i = 0, ret = bdrv_getlength(s->bs[i]); > > + ret == -ENOMEDIUM && i <= 2; > > + i++, ret = bdrv_getlength(s->bs[i])) { > > + } > > Why is -ENOMEDIUM an expected error value? I put the -ENOMEDIUM test because of the following piece of code. /** * Length of a file in bytes. Return < 0 if error or unknown. */ int64_t bdrv_getlength(BlockDriverState *bs) { BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; Still I am not sure it's needed. What is your stance on this ? > > IMO a for loop with a body or a do while loop would make this easier to read. > Ok
On Thu, Aug 9, 2012 at 10:07 AM, Benoît Canet <benoit.canet@irqsave.net> wrote: >> > +static int64_t quorum_getlength(BlockDriverState *bs) >> > +{ >> > + BDRVQuorumState *s = bs->opaque; >> > + int i; >> > + int64_t ret; >> > + >> > + /* return the length of the first available quorum file */ >> > + for (i = 0, ret = bdrv_getlength(s->bs[i]); >> > + ret == -ENOMEDIUM && i <= 2; >> > + i++, ret = bdrv_getlength(s->bs[i])) { >> > + } >> >> Why is -ENOMEDIUM an expected error value? > > I put the -ENOMEDIUM test because of the following piece of code. > /** > * Length of a file in bytes. Return < 0 if error or unknown. > */ > int64_t bdrv_getlength(BlockDriverState *bs) > { > BlockDriver *drv = bs->drv; > if (!drv) > return -ENOMEDIUM; > > Still I am not sure it's needed. What is your stance on this ? A BlockDriverState has more than just block filter or image format state, it also has some guest-visible state unfortunately. The BlockDriverState attached to the guest's storage controller (e.g. IDE CD-ROM) can be closed by blockdev.c:eject_device() and left as an empty BlockDriverState with ->drv == NULL. I think we don't need to worry about this in a block filter like quorum because all child BlockDriverStates will not be ejected. Stefan
diff --git a/block/quorum.c b/block/quorum.c index 9da0432..5cd7083 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -118,12 +118,29 @@ static void quorum_close(BlockDriverState *bs) } } +static int64_t quorum_getlength(BlockDriverState *bs) +{ + BDRVQuorumState *s = bs->opaque; + int i; + int64_t ret; + + /* return the length of the first available quorum file */ + for (i = 0, ret = bdrv_getlength(s->bs[i]); + ret == -ENOMEDIUM && i <= 2; + i++, ret = bdrv_getlength(s->bs[i])) { + } + + return ret; +} + static BlockDriver bdrv_quorum = { .format_name = "quorum", .protocol_name = "quorum", .instance_size = sizeof(BDRVQuorumState), + .bdrv_getlength = quorum_getlength, + .bdrv_file_open = quorum_open, .bdrv_close = quorum_close, };
Signed-off-by: Benoit Canet <benoit@irqsave.net> --- block/quorum.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)