Re: [Qemu-devel] [PATCH v3 2/2] block: Support GlusterFS as a QEMU block backend

[Blue Swirl]

On Sat, Jul 28, 2012 at 4:43 AM, Bharata B Rao
<address@hidden> wrote:
> On Fri, Jul 27, 2012 at 06:44:04PM +0000, Blue Swirl wrote:
>> > +struct GlusterOpts {
>>
>> static
>
> Sure.
>
>>
>> > +    bool optional;
>> > +    char defval[10];
>>
>> const char *defval?
>
> Sure I could.
>
>>
>> > +    char *value;
>> > +} GlusterOpts[] = {
>> > +    {false, "", NULL },
>> > +    {false, "", NULL },
>> > +    {true, "0", NULL },
>> > +    {true, "socket", NULL },
>> > +    {false, "", NULL },
>> > +    {false, "", NULL },
>> > +};
>> > +
>> > +    if (i == GOPT_LAST-1 && strlen(q)) {
>>
>> Spaces around '-'.
>
> checkpatch.pl doesn't enforce this, but I can change.
>
>> > +
>> > +    port = strtoul(GlusterOpts[GOPT_PORT].value, NULL, 0);
>> > +    if (port < 0) {
>>
>> port > 65535 could be bad too.
>
> Actually I am just checking if strtoul gave me a valid integer only
> and depending on gluster to flag an error for invalid port number.
> But I guess no harm in checking for valid port range here. Is there
> a #define equivalent for 65535 ?

I don't think there is. It may also be possible to omit the check if
the connection function checks it and fails. Accidental modular
arithmetic for the port numbers (65537 % 65536 == 1) would not be so
nice if there are no checks.

>
>>
>> > +
>> > +    fd = glfs_creat(glfs, GlusterOpts[GOPT_IMAGE].value,
>> > +        O_WRONLY|O_CREAT|O_TRUNC|O_BINARY, S_IRUSR|S_IWUSR);
>>
>> Spaces around '|'.
>
> Again, checkpatch.pl doesn't enforce this, but I can change.
>
> Thanks for take time to review.
>
> Regards,
> Bharata.
>