From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzYZa-0005FW-Pc for qemu-devel@nongnu.org; Tue, 24 Jun 2014 17:48:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WzYZV-0002oN-3s for qemu-devel@nongnu.org; Tue, 24 Jun 2014 17:48:14 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:41336 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WzYZU-0002o4-OV for qemu-devel@nongnu.org; Tue, 24 Jun 2014 17:48:09 -0400 Message-ID: <53A9F212.4020606@kamp.de> Date: Tue, 24 Jun 2014 23:48:02 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1403599971-9232-1-git-send-email-pl@kamp.de> <53A9A189.5050904@redhat.com> In-Reply-To: <53A9A189.5050904@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2] block/nfs: add knob to set readahead List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, ronniesahlberg@gmail.com, stefanha@redhat.com 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 >> --- >> 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