Patchwork [RFC,V2,03/10] quorum: Add quorum_open().

login
register
mail settings
Submitter Benoit Canet
Date Aug. 7, 2012, 1:44 p.m.
Message ID <1344347073-7773-4-git-send-email-benoit@irqsave.net>
Download mbox | patch
Permalink /patch/175632/
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 |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
Blue Swirl - Aug. 7, 2012, 8:30 p.m.
On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index e0405b6..de58ab8 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
>      int vote_ret;
>  };
>
> +/* Valid quorum filenames look like
> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image

This syntax would mean that stacking for example curl or other network
paths would not be possible. How about comma as separator?

> + */
> +static int quorum_open(BlockDriverState *bs, const char *filename, int flags)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret, i;
> +    char *a, *b, *c, *filenames[3];
> +
> +    /* Parse the quorum: prefix */
> +    if (strncmp(filename, "quorum:", strlen("quorum:"))) {
> +        return -EINVAL;
> +    }
> +    a = g_strdup(filename + strlen("quorum:"));
> +
> +    /* Find separators */
> +    b = strchr(a, ':');
> +    if (b == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    c = strrchr(a, ':');
> +    if (c == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    /* Check that filename contains two separate ':' */
> +    if (b == c) {
> +        return -EINVAL;
> +    }
> +
> +    /* Split string */
> +    *b = '\0';
> +    *c = '\0';
> +
> +    filenames[0] = a;
> +    filenames[1] = b + 1;
> +    filenames[2] = c + 1;
> +
> +    /* Open files */
> +    for (i = 0; i <= 2; i++) {
> +        s->bs[i] = bdrv_new("");
> +        ret = bdrv_open(s->bs[i], filenames[i], flags, NULL);
> +        if (ret < 0) {
> +            goto error_exit;

Successfully opening two out of three should be enough, but maybe it
does not make much sense.

> +        }
> +    }
> +
> +    goto clean_exit;
> +
> +error_exit:
> +    for (; i >= 0; i--) {
> +        bdrv_delete(s->bs[i]);

bdrv_close() instead?

> +        s->bs[i] = NULL;
> +    }
> +clean_exit:
> +    g_free(a);
> +    return ret;
> +}
> +
>  static BlockDriver bdrv_quorum = {
>      .format_name        = "quorum",
>      .protocol_name      = "quorum",
>
>      .instance_size      = sizeof(BDRVQuorumState),
> +
> +    .bdrv_file_open     = quorum_open,
>  };
>
>  static void bdrv_quorum_init(void)
> --
> 1.7.9.5
>
Eric Blake - Aug. 7, 2012, 8:40 p.m.
On 08/07/2012 02:30 PM, Blue Swirl wrote:
> On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>> ---
>>  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 62 insertions(+)
>>
>> diff --git a/block/quorum.c b/block/quorum.c
>> index e0405b6..de58ab8 100644
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
>>      int vote_ret;
>>  };
>>
>> +/* Valid quorum filenames look like
>> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
> 
> This syntax would mean that stacking for example curl or other network
> paths would not be possible. How about comma as separator?

Also, what escaping mechanism is in place for allowing a file containing
the same character as the separator (whether you end up with : or , as
the separator)?  If you support a larger quorum (whether always n/(2n-1)
or whether fully configurable n/m), rather than hard-coded 2/3, then you
also need a way to specify how many quorum members will follow.
Stefan Hajnoczi - Aug. 8, 2012, 3:04 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 |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index e0405b6..de58ab8 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
>      int vote_ret;
>  };
>
> +/* Valid quorum filenames look like
> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
> + */
> +static int quorum_open(BlockDriverState *bs, const char *filename, int flags)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret, i;
> +    char *a, *b, *c, *filenames[3];
> +
> +    /* Parse the quorum: prefix */
> +    if (strncmp(filename, "quorum:", strlen("quorum:"))) {
> +        return -EINVAL;
> +    }
> +    a = g_strdup(filename + strlen("quorum:"));
> +
> +    /* Find separators */
> +    b = strchr(a, ':');
> +    if (b == NULL) {
> +        return -EINVAL;

Leaks a.  Same for returns below.

>  static BlockDriver bdrv_quorum = {
>      .format_name        = "quorum",
>      .protocol_name      = "quorum",
>
>      .instance_size      = sizeof(BDRVQuorumState),
> +
> +    .bdrv_file_open     = quorum_open,

Please squash quorum_close() into this commit so the g_strdup()
filesnames[] aren't leaked on close.
Benoît Canet - Aug. 10, 2012, 5:48 p.m.
Le Tuesday 07 Aug 2012 à 20:30:09 (+0000), Blue Swirl a écrit :
> On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/block/quorum.c b/block/quorum.c
> > index e0405b6..de58ab8 100644
> > --- a/block/quorum.c
> > +++ b/block/quorum.c
> > @@ -47,11 +47,73 @@ struct QuorumAIOCB {
> >      int vote_ret;
> >  };
> >
> > +/* Valid quorum filenames look like
> > + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
> 
> This syntax would mean that stacking for example curl or other network
> paths would not be possible. How about comma as separator?

I just tried comma but it fail because the qemu command line parsing
catch it believing that the string next to the coma is another "file="
like options.

Is there any other separator we can use ?

Benoît
Kevin Wolf - Aug. 13, 2012, 7:41 a.m.
Am 10.08.2012 19:48, schrieb Benoît Canet:
> Le Tuesday 07 Aug 2012 à 20:30:09 (+0000), Blue Swirl a écrit :
>> On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>> ---
>>>  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 62 insertions(+)
>>>
>>> diff --git a/block/quorum.c b/block/quorum.c
>>> index e0405b6..de58ab8 100644
>>> --- a/block/quorum.c
>>> +++ b/block/quorum.c
>>> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
>>>      int vote_ret;
>>>  };
>>>
>>> +/* Valid quorum filenames look like
>>> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
>>
>> This syntax would mean that stacking for example curl or other network
>> paths would not be possible. How about comma as separator?
> 
> I just tried comma but it fail because the qemu command line parsing
> catch it believing that the string next to the coma is another "file="
> like options.
> 
> Is there any other separator we can use ?

I would ignore that for now, or you can introduce your own escaping of
colons. The real solution is, once again, -blockdev.

Kevin
Benoît Canet - Aug. 14, 2012, 4:56 p.m.
Le Monday 13 Aug 2012 à 09:41:00 (+0200), Kevin Wolf a écrit :
> Am 10.08.2012 19:48, schrieb Benoît Canet:
> > Le Tuesday 07 Aug 2012 à 20:30:09 (+0000), Blue Swirl a écrit :
> >> On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
> >>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>> ---
> >>>  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 62 insertions(+)
> >>>
> >>> diff --git a/block/quorum.c b/block/quorum.c
> >>> index e0405b6..de58ab8 100644
> >>> --- a/block/quorum.c
> >>> +++ b/block/quorum.c
> >>> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
> >>>      int vote_ret;
> >>>  };
> >>>
> >>> +/* Valid quorum filenames look like
> >>> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
> >>
> >> This syntax would mean that stacking for example curl or other network
> >> paths would not be possible. How about comma as separator?
> > 
> > I just tried comma but it fail because the qemu command line parsing
> > catch it believing that the string next to the coma is another "file="
> > like options.
> > 
> > Is there any other separator we can use ?
> 
> I would ignore that for now, or you can introduce your own escaping of
> colons. The real solution is, once again, -blockdev.

Is -blockdev related to BlockBackend ?

Benoît

> 
> Kevin
>
Kevin Wolf - Aug. 15, 2012, 10:40 a.m.
Am 14.08.2012 18:56, schrieb Benoît Canet:
> Le Monday 13 Aug 2012 à 09:41:00 (+0200), Kevin Wolf a écrit :
>> Am 10.08.2012 19:48, schrieb Benoît Canet:
>>> Le Tuesday 07 Aug 2012 à 20:30:09 (+0000), Blue Swirl a écrit :
>>>> On Tue, Aug 7, 2012 at 1:44 PM, Benoît Canet <benoit.canet@gmail.com> wrote:
>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>> ---
>>>>>  block/quorum.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 62 insertions(+)
>>>>>
>>>>> diff --git a/block/quorum.c b/block/quorum.c
>>>>> index e0405b6..de58ab8 100644
>>>>> --- a/block/quorum.c
>>>>> +++ b/block/quorum.c
>>>>> @@ -47,11 +47,73 @@ struct QuorumAIOCB {
>>>>>      int vote_ret;
>>>>>  };
>>>>>
>>>>> +/* Valid quorum filenames look like
>>>>> + * quorum:path/to/a_image:path/to/b_image:path/to/c_image
>>>>
>>>> This syntax would mean that stacking for example curl or other network
>>>> paths would not be possible. How about comma as separator?
>>>
>>> I just tried comma but it fail because the qemu command line parsing
>>> catch it believing that the string next to the coma is another "file="
>>> like options.
>>>
>>> Is there any other separator we can use ?
>>
>> I would ignore that for now, or you can introduce your own escaping of
>> colons. The real solution is, once again, -blockdev.
> 
> Is -blockdev related to BlockBackend ?

BlockBackend is a prerequisite for it.

One of the goals with -blockdev is to have not only a filename for
bdrv_open from which you can parse options, but that you can get
structured and driver-specific options.

Kevin

Patch

diff --git a/block/quorum.c b/block/quorum.c
index e0405b6..de58ab8 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -47,11 +47,73 @@  struct QuorumAIOCB {
     int vote_ret;
 };
 
+/* Valid quorum filenames look like
+ * quorum:path/to/a_image:path/to/b_image:path/to/c_image
+ */
+static int quorum_open(BlockDriverState *bs, const char *filename, int flags)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int ret, i;
+    char *a, *b, *c, *filenames[3];
+
+    /* Parse the quorum: prefix */
+    if (strncmp(filename, "quorum:", strlen("quorum:"))) {
+        return -EINVAL;
+    }
+    a = g_strdup(filename + strlen("quorum:"));
+
+    /* Find separators */
+    b = strchr(a, ':');
+    if (b == NULL) {
+        return -EINVAL;
+    }
+
+    c = strrchr(a, ':');
+    if (c == NULL) {
+        return -EINVAL;
+    }
+
+    /* Check that filename contains two separate ':' */
+    if (b == c) {
+        return -EINVAL;
+    }
+
+    /* Split string */
+    *b = '\0';
+    *c = '\0';
+
+    filenames[0] = a;
+    filenames[1] = b + 1;
+    filenames[2] = c + 1;
+
+    /* Open files */
+    for (i = 0; i <= 2; i++) {
+        s->bs[i] = bdrv_new("");
+        ret = bdrv_open(s->bs[i], filenames[i], flags, NULL);
+        if (ret < 0) {
+            goto error_exit;
+        }
+    }
+
+    goto clean_exit;
+
+error_exit:
+    for (; i >= 0; i--) {
+        bdrv_delete(s->bs[i]);
+        s->bs[i] = NULL;
+    }
+clean_exit:
+    g_free(a);
+    return ret;
+}
+
 static BlockDriver bdrv_quorum = {
     .format_name        = "quorum",
     .protocol_name      = "quorum",
 
     .instance_size      = sizeof(BDRVQuorumState),
+
+    .bdrv_file_open     = quorum_open,
 };
 
 static void bdrv_quorum_init(void)