From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH 7/7] kvmtool: 9p: refactor rel_to_abs() Date: Tue, 8 Nov 2016 02:38:37 +0000 Message-ID: <20161108023836.GX20591@arm.com> References: <1476806585-9954-1-git-send-email-gcampana+kvm@quarkslab.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, andre.przywara@arm.com To: "G. Campana" Return-path: Received: from foss.arm.com ([217.140.101.70]:49832 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751081AbcKHCif (ORCPT ); Mon, 7 Nov 2016 21:38:35 -0500 Content-Disposition: inline In-Reply-To: <1476806585-9954-1-git-send-email-gcampana+kvm@quarkslab.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Oct 18, 2016 at 06:03:05PM +0200, G. Campana wrote: > --- > virtio/9p.c | 50 +++++++++++++++++++++++++------------------------- > 1 file changed, 25 insertions(+), 25 deletions(-) > > diff --git a/virtio/9p.c b/virtio/9p.c > index 5b2d261..3259b79 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -91,18 +91,6 @@ static struct p9_fid *get_fid(struct p9_dev *p9dev, int fid) > return new; > } > > -static int rel_to_abs(struct p9_dev *p9dev, const char *path, char *abs_path, > - size_t size) > -{ > - int ret; > - > - ret = snprintf(abs_path, size, "%s/%s", p9dev->root_dir, path); > - if (ret >= (int)size) > - return -1; > - > - return 0; > -} Can this be merged with patch 5, where you introduced rel_to_abs? > static void stat2qid(struct stat *st, struct p9_qid *qid) > { > *qid = (struct p9_qid) { > @@ -266,6 +254,28 @@ static int get_full_path(char *full_path, size_t size, struct p9_fid *fid, > return 0; > } > > +static int stat_rel(struct p9_dev *p9dev, const char *path, struct stat *st) > +{ > + int ret; > + char full_path[PATH_MAX]; > + > + ret = snprintf(full_path, sizeof(full_path), "%s/%s", p9dev->root_dir, path); > + if (ret >= (int)sizeof(full_path)) { > + errno = ENAMETOOLONG; > + return -1; > + } > + > + if (path_is_illegal(full_path)) { > + errno = EACCES; > + return -1; > + } Up to this point, you've just reimplemented most of get_full_path. Is it worth giving these two functions a comment "concatenate these two path components and check if the result is legal" backend? > + if (lstat(full_path, st) != 0) > + return -1; > + > + return 0; > +} > + > static void virtio_p9_open(struct p9_dev *p9dev, > struct p9_pdu *pdu, u32 *outlen) > { > @@ -440,7 +450,6 @@ static void virtio_p9_walk(struct p9_dev *p9dev, > for (i = 0; i < nwname; i++) { > struct stat st; > char tmp[PATH_MAX] = {0}; > - char full_path[PATH_MAX]; > char *str; > int ret; > > @@ -455,12 +464,7 @@ static void virtio_p9_walk(struct p9_dev *p9dev, > > free(str); > > - if (rel_to_abs(p9dev, tmp, full_path, sizeof(full_path)) != 0) { > - errno = ENAMETOOLONG; > - goto err_out; > - } > - > - if (lstat(full_path, &st) < 0) > + if (stat_rel(p9dev, tmp, &st) != 0) > goto err_out; > > stat2qid(&st, &wqid); > @@ -614,7 +618,6 @@ static void virtio_p9_readdir(struct p9_dev *p9dev, > struct stat st; > struct p9_fid *fid; > struct dirent *dent; > - char full_path[PATH_MAX]; > u64 offset, old_offset; > > rcount = 0; > @@ -645,11 +648,8 @@ static void virtio_p9_readdir(struct p9_dev *p9dev, > break; > } > old_offset = dent->d_off; > - if (rel_to_abs(p9dev, dent->d_name, full_path, sizeof(full_path)) != 0) { > - errno = ENAMETOOLONG; > - goto err_out; > - } > - lstat(full_path, &st); > + if (stat_rel(p9dev, dent->d_name, &st) != 0) > + memset(&st, -1, sizeof(st)); Why the memset, and not goto err_out? Will