| Message ID | 1403533836-28589-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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). > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > 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.
Am 23.06.2014 um 17:11 schrieb Eric Blake <eblake@redhat.com>: > 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 <pl@kamp.de> >> --- >> 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 >
On 06/23/2014 02:47 PM, Peter Lieven wrote: >>> +#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. A good example of a recent conversion to structured options would be the blkdebug and blkverify backends (look around commit 1bf20b82), or proposed addition of new backends (such as archipelego, https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05309.html). It may be a bigger task than you anticipate, and certainly won't make the 2.1 timeframe, but it's worth doing so if you're up to the task, more power to you :) > > 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. Even if qemu-img is likely to be the only one ever specifying the option, it still makes sense to expose it via QMP, as we are gradually trying to move qemu-img over to more exclusive use of QMP (or at least the underlying functions reached by QMP) rather than ad-hoc code. For example, https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01822.html).
On 23.06.2014 23:05, Eric Blake wrote: > On 06/23/2014 02:47 PM, Peter Lieven wrote: > >>>> +#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. > A good example of a recent conversion to structured options would be the > blkdebug and blkverify backends (look around commit 1bf20b82), or > proposed addition of new backends (such as archipelego, > https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg05309.html). > It may be a bigger task than you anticipate, and certainly won't make > the 2.1 timeframe, but it's worth doing so if you're up to the task, > more power to you :) > >> 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. > Even if qemu-img is likely to be the only one ever specifying the > option, it still makes sense to expose it via QMP, as we are gradually > trying to move qemu-img over to more exclusive use of QMP (or at least > the underlying functions reached by QMP) rather than ad-hoc code. For > example, > https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01822.html). > Ok, it looks like a bit of work. I will put it on my list for 2.2. Thanks for the pointers, Peter
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)); - } 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 } else { error_setg(errp, "Unknown NFS parameter name: %s", qp->p[i].name);
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. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/nfs.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)