Patchwork [RFC,V2,05/10] quorum: Add quorum_getlength().

login
register
mail settings
Submitter Benoit Canet
Date Aug. 7, 2012, 1:44 p.m.
Message ID <1344347073-7773-6-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/175645/
State New
Headers show

Comments

Benoit Canet - Aug. 7, 2012, 1:44 p.m.
Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/quorum.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Stefan Hajnoczi - Aug. 8, 2012, 3:30 p.m.
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.
Benoît Canet - Aug. 9, 2012, 9:07 a.m.
> > +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
Stefan Hajnoczi - Aug. 9, 2012, 9:14 a.m.
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

Patch

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,
 };