| Message ID | 1403599971-9232-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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);
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(-)