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

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

[Peter Lieven]

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 <pl@kamp.de>
---
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));
+        } else if (!strcmp(qp->p[i].name, "gid")) {
+            nfs_set_gid(client->context, strtol(qp->p[i].value, NULL, 0));
+        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
+            nfs_set_tcp_syncnt(client->context, strtol(qp->p[i].value, NULL, 0));
+#ifdef LIBNFS_FEATURE_READAHEAD
+        } else if (!strcmp(qp->p[i].name, "readahead")) {
+            nfs_set_readahead(client->context, strtol(qp->p[i].value, NULL, 0));
+#endif
         } else {
             error_setg(errp, "Unknown NFS parameter name: %s",
                        qp->p[i].name);
-- 
1.7.9.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

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 <pl@kamp.de>
> ---
> 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.

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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 <pl@kamp.de>
>> ---
>> 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