| Message ID | 20231008060138.517057-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/9p/xattr.c: avoid format-overflow warning | expand |
On Sunday, October 8, 2023 8:01:39 AM CEST Su Hui wrote: > with gcc and W=1 option, there's a warning like this: > > In file included from fs/9p/xattr.c:12: > In function ‘v9fs_xattr_get’, > inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: > include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null > [-Werror=format-overflow=] > 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > use "" replace NULL to silence this warning. > > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > fs/9p/xattr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c > index e00cf8109b3f..d995ee080835 100644 > --- a/fs/9p/xattr.c > +++ b/fs/9p/xattr.c > @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, > > ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > { > - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); > + return v9fs_xattr_get(dentry, "", buffer, buffer_size); > } > > static int v9fs_xattr_handler_get(const struct xattr_handler *handler, > Mmm, that's not the same is it? Have you tested this change? Currently this function causes a 'Txattrwalk' 9p message to be sent to 9p server with its name[s] field being NULL, and the latter being the magical hint to 9p server to not send an attribute, but rather the list of attributes. With your change I would assume that it would rather ask server for one attribute called "". I have not tested myself, just worrying that it might break behaviour. /Christian
On 2023/10/10 02:34, Christian Schoenebeck wrote: > On Sunday, October 8, 2023 8:01:39 AM CEST Su Hui wrote: >> with gcc and W=1 option, there's a warning like this: >> >> In file included from fs/9p/xattr.c:12: >> In function ‘v9fs_xattr_get’, >> inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: >> include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null >> [-Werror=format-overflow=] >> 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> use "" replace NULL to silence this warning. >> >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> fs/9p/xattr.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c >> index e00cf8109b3f..d995ee080835 100644 >> --- a/fs/9p/xattr.c >> +++ b/fs/9p/xattr.c >> @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, >> >> ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) >> { >> - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); >> + return v9fs_xattr_get(dentry, "", buffer, buffer_size); >> } >> >> static int v9fs_xattr_handler_get(const struct xattr_handler *handler, >> > Mmm, that's not the same is it? Have you tested this change? Oh, sorry. That's not the same and I just tested compilation. > > Currently this function causes a 'Txattrwalk' 9p message to be sent to 9p > server with its name[s] field being NULL, and the latter being the magical > hint to 9p server to not send an attribute, but rather the list of attributes. > > With your change I would assume that it would rather ask server for one > attribute called "". I have not tested myself, just worrying that it might > break behaviour. Got it, I made a mistake there. Thanks for your explanation. Sorry for the noise again. Su Hui
Christian Schoenebeck wrote on Mon, Oct 09, 2023 at 08:34:15PM +0200: > > +++ b/fs/9p/xattr.c > > @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, > > > > ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > > { > > - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); > > + return v9fs_xattr_get(dentry, "", buffer, buffer_size); > > } > > > > static int v9fs_xattr_handler_get(const struct xattr_handler *handler, > > > > Mmm, that's not the same is it? Have you tested this change? > > Currently this function causes a 'Txattrwalk' 9p message to be sent to 9p > server with its name[s] field being NULL, and the latter being the magical > hint to 9p server to not send an attribute, but rather the list of attributes. > > With your change I would assume that it would rather ask server for one > attribute called "". I have not tested myself, just worrying that it might > break behaviour. p9pdu_vwritef should output the same (just a 0 length) for both NULL and "" so I think it should be ok, but it definitely needs testing. I'll try to find time to check (getfattr -d should be enough) later this week and add it to the pile
On Sun, Oct 08, 2023 at 02:01:39PM +0800, Su Hui wrote: > with gcc and W=1 option, there's a warning like this: > > In file included from fs/9p/xattr.c:12: > In function ‘v9fs_xattr_get’, > inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: > include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null > [-Werror=format-overflow=] > 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > use "" replace NULL to silence this warning. > > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > fs/9p/xattr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c > index e00cf8109b3f..d995ee080835 100644 > --- a/fs/9p/xattr.c > +++ b/fs/9p/xattr.c > @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, > > ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > { > - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); > + return v9fs_xattr_get(dentry, "", buffer, buffer_size); I'm pretty uncomfortable with this patch... This code is 13 years old so it can't be too huge of a problem. We're doing this for the printks, but now they're going to look weird first of all. Old: "file = (null)" New: "file = " But also this must have some other effects on runtime right? regards, dan carpenter
On Tuesday, October 10, 2023 4:23:06 AM CEST asmadeus@codewreck.org wrote: > Christian Schoenebeck wrote on Mon, Oct 09, 2023 at 08:34:15PM +0200: > > > +++ b/fs/9p/xattr.c > > > @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, > > > > > > ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > > > { > > > - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); > > > + return v9fs_xattr_get(dentry, "", buffer, buffer_size); > > > } > > > > > > static int v9fs_xattr_handler_get(const struct xattr_handler *handler, > > > > > > > Mmm, that's not the same is it? Have you tested this change? > > > > Currently this function causes a 'Txattrwalk' 9p message to be sent to 9p > > server with its name[s] field being NULL, and the latter being the magical > > hint to 9p server to not send an attribute, but rather the list of attributes. > > > > With your change I would assume that it would rather ask server for one > > attribute called "". I have not tested myself, just worrying that it might > > break behaviour. > > p9pdu_vwritef should output the same (just a 0 length) for both NULL and > "" so I think it should be ok, but it definitely needs testing. > > I'll try to find time to check (getfattr -d should be enough) later this > week and add it to the pile Yeah, I think you are right Dominique, it should end up the same as both cases result in a string length of 0. /Christian
On 2023/10/10 15:51, Dan Carpenter wrote: > On Sun, Oct 08, 2023 at 02:01:39PM +0800, Su Hui wrote: >> with gcc and W=1 option, there's a warning like this: >> >> In file included from fs/9p/xattr.c:12: >> In function ‘v9fs_xattr_get’, >> inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: >> include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null >> [-Werror=format-overflow=] >> 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> use "" replace NULL to silence this warning. >> >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> fs/9p/xattr.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c >> index e00cf8109b3f..d995ee080835 100644 >> --- a/fs/9p/xattr.c >> +++ b/fs/9p/xattr.c >> @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, >> >> ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) >> { >> - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); >> + return v9fs_xattr_get(dentry, "", buffer, buffer_size); > I'm pretty uncomfortable with this patch... This code is 13 years old > so it can't be too huge of a problem. We're doing this for the printks, > but now they're going to look weird first of all. > > Old: "file = (null)" > New: "file = " Agreed, this looks really weird and the patch is just for avoiding gcc's warning. Sorry for the noise. Su Hui > > But also this must have some other effects on runtime right? > > regards, > dan carpenter >
diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c index e00cf8109b3f..d995ee080835 100644 --- a/fs/9p/xattr.c +++ b/fs/9p/xattr.c @@ -139,7 +139,7 @@ int v9fs_fid_xattr_set(struct p9_fid *fid, const char *name, ssize_t v9fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) { - return v9fs_xattr_get(dentry, NULL, buffer, buffer_size); + return v9fs_xattr_get(dentry, "", buffer, buffer_size); } static int v9fs_xattr_handler_get(const struct xattr_handler *handler,
with gcc and W=1 option, there's a warning like this: In file included from fs/9p/xattr.c:12: In function ‘v9fs_xattr_get’, inlined from ‘v9fs_listxattr’ at fs/9p/xattr.c:142:9: include/net/9p/9p.h:55:2: error: ‘%s’ directive argument is null [-Werror=format-overflow=] 55 | _p9_debug(level, __func__, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ use "" replace NULL to silence this warning. Signed-off-by: Su Hui <suhui@nfschina.com> --- fs/9p/xattr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)