Re: [Qemu-devel] [PATCHv2] block/nfs: add knob to set readahead

[Peter Lieven]

Am 24.06.2014 18:04, schrieb Eric Blake:
> On 06/24/2014 02:52 AM, Peter Lieven wrote:
>> upcoming libnfs will feature internal readahead support.
>> Add a knob to pass the optional readahead value as a URL
>> parameter.
>>
>> This patch fixes also the incorrect usage of strncmp and
>> atoi.
>>
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>> v1->v2: use strtol instead of atoi [Eric]
>>
>>  block/nfs.c |   16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index ec43201..9783483 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -309,12 +309,16 @@ static int64_t nfs_client_open(NFSClient *client, 
>> const char *filename,
>>                         qp->p[i].name);
>>              goto fail;
>>          }
>> -        if (!strncmp(qp->p[i].name, "uid", 3)) {
>> -            nfs_set_uid(client->context, atoi(qp->p[i].value));
>> -        } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>> -            nfs_set_gid(client->context, atoi(qp->p[i].value));
>> -        } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>> -            nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>> +        if (!strcmp(qp->p[i].name, "uid")) {
>> +            nfs_set_uid(client->context, strtol(qp->p[i].value, NULL, 0));
> If you're going to use strtol, use it correctly.  You have to pre-set
> errno, then check that something got parsed, that errno was not changed,
> and that no unexpected suffix remains.  Better is using a wrapper that
> already makes the parsing sane; such as parse_uint() from util/cutils.c.
>  That is, swapping atoi() with a raw strtol() with no additional error
> checking is not fixing any of the bugs inherent in the fact that atoi()
> cannot detect overflow in user input.

thanks for pointing that out. Actually I was grepping for strtoul in the
qemu source and found it in several places (also with no error checking).
I will change that.

>
> At this point, I'd rather see this split into two patches - one that
> fixes the atoi() usage, and another that adds readahead support - rather
> than trying to cram two things in one commit.
>
Right I will split that up.

Thanks for your comments,
Peter