From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56369) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKdDL-0005XL-Fa for qemu-devel@nongnu.org; Mon, 03 Mar 2014 19:28:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKdDG-0001W4-4k for qemu-devel@nongnu.org; Mon, 03 Mar 2014 19:28:07 -0500 Received: from mail-pb0-x234.google.com ([2607:f8b0:400e:c01::234]:46119) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKdDF-0001Vs-Ox for qemu-devel@nongnu.org; Mon, 03 Mar 2014 19:28:02 -0500 Received: by mail-pb0-f52.google.com with SMTP id rr13so4463230pbb.11 for ; Mon, 03 Mar 2014 16:28:00 -0800 (PST) Message-ID: <53151E0B.701@gmail.com> Date: Tue, 04 Mar 2014 08:27:55 +0800 From: Chen Gang MIME-Version: 1.0 References: <52EF68CA.9060604@gmail.com> <20140203103429.GB10408@redhat.com> <52EF71DC.3000309@gmail.com> <52F0C8BA.7020709@gmail.com> <20140204110631.GD5632@redhat.com> <52F0CD67.5070601@gmail.com> <87siry3l7t.fsf@linux.vnet.ibm.com> <52F17B5E.1050602@gmail.com> <52FF3182.9090106@gmail.com> <53097D8E.1030803@gmail.com> <87sir850ho.fsf@blackfin.pond.sub.org> <87ha7o3c5x.fsf@blackfin.pond.sub.org> <530FCBAD.10305@gmail.com> <531219CC.4050505@gmail.com> <53121A12.5050105@gmail.com> <53121A4B.70308@gmail.com> <53121A93.8080301@gmail.com> <87d2i3xne9.fsf@linux.vnet.ibm.com> <871tyjxeqe.fsf@linux.vnet.ibm.com> In-Reply-To: <871tyjxeqe.fsf@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] hw/9pfs: use g_strdup_printf() instead of PATH_MAX limitation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Aneesh Kumar K.V" , Markus Armbruster , aliguori@amazon.com, Eric Blake , "Daniel P. Berrange" Cc: QEMU Developers On 03/04/2014 03:29 AM, Aneesh Kumar K.V wrote: > "Aneesh Kumar K.V" writes: > >> Chen Gang writes: >> Can we keep this as >> v9fs_co_run_in_worker( >> { >> >> buf->data = __readlink(&s->ctx, path); >> >> } >> >> I can do that change for you if you want. I will also have to go through >> the rest of the code to make sure we do free the memory in all error >> path. So the rest of the review is going to take time. Hope that is ok >> > > I ended up with the below diff on top. The resulting tree is pushed to > > https://github.com/kvaneesh/qemu/commits/for-upstream > and pass all the tuxera tests. Let me know what you think. > The 2 improvement (after the patch 3/3) looks fine to me, thanks. And I guess, I need not send patch v2, again (if I still should/need send patch v2 for it, please let me know, thanks). > diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c > index 739bad019e3a..42ee614e27f0 100644 > --- a/hw/9pfs/cofs.c > +++ b/hw/9pfs/cofs.c > @@ -17,42 +17,55 @@ > #include "block/coroutine.h" > #include "virtio-9p-coth.h" > > +static ssize_t __readlink(V9fsState *s, V9fsPath *path, V9fsString *buf) > +{ > + ssize_t len, maxlen = PATH_MAX; > + > + buf->data = g_malloc(PATH_MAX); > + for(;;) { > + len = s->ops->readlink(&s->ctx, path, buf->data, maxlen); > + if (len < 0) { > + g_free(buf->data); > + buf->data = NULL; > + buf->size = 0; > + break; > + } else if (len == maxlen) { > + /* > + * We dodn't have space to put the NULL or we have more > + * to read. Increase the size and try again > + */ > + maxlen *= 2; > + g_free(buf->data); > + buf->data = g_malloc(maxlen); > + continue; > + } > + /* > + * Null terminate the readlink output > + */ > + buf->data[len] = '\0'; > + buf->size = len; > + break; > + } > + return len; > +} > + > int v9fs_co_readlink(V9fsPDU *pdu, V9fsPath *path, V9fsString *buf) > { > int err; > - ssize_t len, maxlen = PATH_MAX; > V9fsState *s = pdu->s; > > if (v9fs_request_cancelled(pdu)) { > return -EINTR; > } > - buf->data = g_malloc(maxlen); > v9fs_path_read_lock(s); > v9fs_co_run_in_worker( > - while (1) { > - len = s->ops->readlink(&s->ctx, path, > - buf->data, maxlen - 1); > - if (len == maxlen - 1) { > - g_free(buf->data); > - maxlen *= 2; > - buf->data = g_malloc(maxlen); > - continue; > - } > - if (len > -1) { > - buf->size = len; > - buf->data[len] = 0; > - err = 0; > - } else { > + { > + err = __readlink(s, path, buf); > + if (err < 0) { > err = -errno; > } > - break; > }); > v9fs_path_unlock(s); > - if (err) { > - g_free(buf->data); > - buf->data = NULL; > - buf->size = 0; > - } > return err; > } > > diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c > index 9e9cc319ec54..56b302c122b6 100644 > --- a/hw/9pfs/virtio-9p-local.c > +++ b/hw/9pfs/virtio-9p-local.c > @@ -125,8 +125,7 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf) > buffer = rpath(fs_ctx, path); > err = lstat(buffer, stbuf); > if (err) { > - g_free(buffer); > - return err; > + goto err_out; > } > if (fs_ctx->export_flags & V9FS_SM_MAPPED) { > /* Actual credentials are part of extended attrs */ > @@ -151,6 +150,7 @@ static int local_lstat(FsContext *fs_ctx, V9fsPath *fs_path, struct stat *stbuf) > local_mapped_file_attr(fs_ctx, path, stbuf); > } > > +err_out: > g_free(buffer); > return err; > } > Thanks. -- Chen Gang Open, share, and attitude like air, water, and life which God blessed