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

[Peter Lieven]

Am 23.06.2014 um 17:11 schrieb Eric Blake <address@hidden>:

> On 06/23/2014 08:30 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.
> 
> But not the incorrect usage of atoi() (hint - atoi cannot detect
> overflow; it should NEVER be used on user-provided input; instead
> strtol() should be preferred).

Thanks for that hint I was not aware and somehow everybody
mist it when it first went in. I will respin for that.

> 
>> 
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>> block/nfs.c |   10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/block/nfs.c b/block/nfs.c
>> index ec43201..a5c0577 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)) {
>> +        if (!strcmp(qp->p[i].name, "uid")) {
>>             nfs_set_uid(client->context, atoi(qp->p[i].value));
> 
> Of course, the use of atoi was pre-existing, so it doesn't necessarily
> need to be done in _this_ patch.
> 
>> -        } else if (!strncmp(qp->p[i].name, "gid", 3)) {
>> +        } else if (!strcmp(qp->p[i].name, "gid")) {
>>             nfs_set_gid(client->context, atoi(qp->p[i].value));
>> -        } else if (!strncmp(qp->p[i].name, "tcp-syncnt", 10)) {
>> +        } else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
>>             nfs_set_tcp_syncnt(client->context, atoi(qp->p[i].value));
>> +#ifdef LIBNFS_FEATURE_READAHEAD
>> +        } else if (!strcmp(qp->p[i].name, "readahead")) {
>> +            nfs_set_readahead(client->context, atoi(qp->p[i].value));
>> +#endif
> 
> I'm not a fan of adding even more special-casing to the file-name URI
> without also adding it to the QAPI schema for use in the blockdev-add
> command.  Of course, we don't have structured nfs options for
> blockdev-add yet, but maybe it's time to start thinking about that
> addition before we do this addition.

I would like to leave this for a follow-up and I promise to take care of this.
If you could give me a pointer of an existing driver that does this as a 
reference
I would be grateful.

For the read ahead parameter its likely not needed as (at least in my use case)
the read ahead makes only sense when converting a compressed QCOW2 Template File
which lives on an NFS Share to a RAW Device. So I use it with qemu-img.
For a Container File that is used as a VM hard drive I would likely not use 
read ahead
as the operating system itself will implement some sort of read ahead already.

Peter



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