| Message ID | 1425503036-23707-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On Thu, 03/05 00:03, Michael Tokarev wrote:
> + const char *name = basename((char*)path);
checkpatch.pl complained this, s/char*/char */
05.03.2015 04:13, Fam Zheng wrote: > On Thu, 03/05 00:03, Michael Tokarev wrote: >> + const char *name = basename((char*)path); > > checkpatch.pl complained this, s/char*/char */ I've fixed it in git. Thank you for spotting this. /mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > Omit one unnecessary memory allocation for components of the path > and create the resulting path directly given lengths of the components. > > This uses (char*) cast because basename() accepts a char* without const, > for unknown reason. Maybe it is better to use strrchr(), but I'm not > sure for various forms of directory component delimiter. basename(3) says: Both dirname() and basename() may modify the contents of path, so it may be desirable to pass a copy when calling one of these functions > > And according to basename(3) manpage, it might return different strings > for various corner cases, for example for empty argument it returns a > string "." which is obviously at some other address, so both the old > code and new code will do a bad thing here. So maybe it is actually > better to use strrchr() after all. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > hw/9pfs/virtio-9p-local.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c > index d05c917..fddc242 100644 > --- a/hw/9pfs/virtio-9p-local.c > +++ b/hw/9pfs/virtio-9p-local.c > @@ -45,19 +45,10 @@ > > static char *local_mapped_attr_path(FsContext *ctx, const char *path) > { > - char *dir_name; > - char *tmp_path = g_strdup(path); > - char *base_name = basename(tmp_path); > - char *buffer; > - > - /* NULL terminate the directory */ > - dir_name = tmp_path; > - *(base_name - 1) = '\0'; > - > - buffer = g_strdup_printf("%s/%s/%s/%s", > - ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name); > - g_free(tmp_path); > - return buffer; > + const char *name = basename((char*)path); as per the man page basename could end up modifying 'path' and we don't want to modify the input argument of the function. > + int dirlen = name - path - 1; > + return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root, > + dirlen, path, VIRTFS_META_DIR, name); > } > > static FILE *local_fopen(const char *path, const char *mode) I am not sure whether we really need all these cleanups without really fixing anyi specific issue. -aneesh
10.03.2015 20:30, Aneesh Kumar K.V wrote: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> Omit one unnecessary memory allocation for components of the path >> and create the resulting path directly given lengths of the components. >> >> This uses (char*) cast because basename() accepts a char* without const, >> for unknown reason. Maybe it is better to use strrchr(), but I'm not >> sure for various forms of directory component delimiter. > > basename(3) says: > Both dirname() and basename() may modify the contents of path, so it may > be desirable to pass a copy when calling one of these functions The same manpage also says that there are 2 versions of basename(3), one modifies its arg while another doesn't, and they return different results in some corner cases. Which one will be used depends on the compiler flags. So I think it really is better to open-code it here to have guaranteed consistent results. I'll send a v2. [] > I am not sure whether we really need all these cleanups without really > fixing anyi specific issue. This is not a cleanup, it is an optimization: it removes one completely unnecessary memory allocation. Not in a hottest path but I think it is worth this small effor anyway. Thanks, /mjt
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index d05c917..fddc242 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -45,19 +45,10 @@ static char *local_mapped_attr_path(FsContext *ctx, const char *path) { - char *dir_name; - char *tmp_path = g_strdup(path); - char *base_name = basename(tmp_path); - char *buffer; - - /* NULL terminate the directory */ - dir_name = tmp_path; - *(base_name - 1) = '\0'; - - buffer = g_strdup_printf("%s/%s/%s/%s", - ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name); - g_free(tmp_path); - return buffer; + const char *name = basename((char*)path); + int dirlen = name - path - 1; + return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root, + dirlen, path, VIRTFS_META_DIR, name); } static FILE *local_fopen(const char *path, const char *mode)
Omit one unnecessary memory allocation for components of the path and create the resulting path directly given lengths of the components. This uses (char*) cast because basename() accepts a char* without const, for unknown reason. Maybe it is better to use strrchr(), but I'm not sure for various forms of directory component delimiter. And according to basename(3) manpage, it might return different strings for various corner cases, for example for empty argument it returns a string "." which is obviously at some other address, so both the old code and new code will do a bad thing here. So maybe it is actually better to use strrchr() after all. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- hw/9pfs/virtio-9p-local.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)