| Message ID | 1504212659-9674-3-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: > To avoid clashing with user variables. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > util/util.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/util/util.h b/util/util.h > index cbf0deca2dde7b..30d32ee6b51e24 100644 > --- a/util/util.h > +++ b/util/util.h > @@ -8,8 +8,8 @@ > * error */ > #define check_snprintf(buf, len, fmt, ...) \ > ({ \ > - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > - (rc < len && rc >= 0); \ > + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ It can't clash, because it is declared in the scope {..} of that define and it is local to check_snprintf macro. > + (__rc__ < len && __rc__ >= 0); \ > }) > > /* a CMP b. See also the BSD macro timercmp(). */ > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 01/09/2017 à 06:56, Leon Romanovsky a écrit : > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: >> To avoid clashing with user variables. >> >> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >> --- >> util/util.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/util/util.h b/util/util.h >> index cbf0deca2dde7b..30d32ee6b51e24 100644 >> --- a/util/util.h >> +++ b/util/util.h >> @@ -8,8 +8,8 @@ >> * error */ >> #define check_snprintf(buf, len, fmt, ...) \ >> ({ \ >> - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ >> - (rc < len && rc >= 0); \ >> + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > It can't clash, because it is declared in the scope {..} of that define and > it is local to check_snprintf macro. It does if any of the va-args is called rc. This is "valid" C although not sure what printfs does print here (probably random stack value) { int i = printf("i = %d\n", i); } Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote: > > > Le 01/09/2017 à 06:56, Leon Romanovsky a écrit : > > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: > >> To avoid clashing with user variables. > >> > >> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > >> --- > >> util/util.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/util/util.h b/util/util.h > >> index cbf0deca2dde7b..30d32ee6b51e24 100644 > >> --- a/util/util.h > >> +++ b/util/util.h > >> @@ -8,8 +8,8 @@ > >> * error */ > >> #define check_snprintf(buf, len, fmt, ...) \ > >> ({ \ > >> - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > >> - (rc < len && rc >= 0); \ > >> + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > > It can't clash, because it is declared in the scope {..} of that define and > > it is local to check_snprintf macro. > > It does if any of the va-args is called rc. > This is "valid" C although not sure what printfs does print here (probably random stack value) > { > int i = printf("i = %d\n", i); > } So how does this rename solve the issue? Now, we can clash with __rc__ variable. Thanks > > Nicolas
Le 01/09/2017 à 11:35, Leon Romanovsky a écrit : > On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote: >> >> Le 01/09/2017 à 06:56, Leon Romanovsky a écrit : >>> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: >>>> To avoid clashing with user variables. >>>> >>>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> >>>> --- >>>> util/util.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/util/util.h b/util/util.h >>>> index cbf0deca2dde7b..30d32ee6b51e24 100644 >>>> --- a/util/util.h >>>> +++ b/util/util.h >>>> @@ -8,8 +8,8 @@ >>>> * error */ >>>> #define check_snprintf(buf, len, fmt, ...) \ >>>> ({ \ >>>> - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ >>>> - (rc < len && rc >= 0); \ >>>> + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ >>> It can't clash, because it is declared in the scope {..} of that define and >>> it is local to check_snprintf macro. >> It does if any of the va-args is called rc. >> This is "valid" C although not sure what printfs does print here (probably random stack value) >> { >> int i = printf("i = %d\n", i); >> } > So how does this rename solve the issue? > Now, we can clash with __rc__ variable. > > Thanks There is no safe way to handle that. It can be detected with -Wshadow though. But rc is quite a common name (39 definitions in the code base), __rc__ much less (unused... yet) If you want some real fun, try writing macros with local variables that can be called recursively ;) Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 01, 2017 at 11:46:02AM +0200, Nicolas Morey-Chaisemartin wrote: > > > Le 01/09/2017 à 11:35, Leon Romanovsky a écrit : > > On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote: > >> > >> Le 01/09/2017 à 06:56, Leon Romanovsky a écrit : > >>> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: > >>>> To avoid clashing with user variables. > >>>> > >>>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > >>>> --- > >>>> util/util.h | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/util/util.h b/util/util.h > >>>> index cbf0deca2dde7b..30d32ee6b51e24 100644 > >>>> --- a/util/util.h > >>>> +++ b/util/util.h > >>>> @@ -8,8 +8,8 @@ > >>>> * error */ > >>>> #define check_snprintf(buf, len, fmt, ...) \ > >>>> ({ \ > >>>> - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > >>>> - (rc < len && rc >= 0); \ > >>>> + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > >>> It can't clash, because it is declared in the scope {..} of that define and > >>> it is local to check_snprintf macro. > >> It does if any of the va-args is called rc. > >> This is "valid" C although not sure what printfs does print here (probably random stack value) > >> { > >> int i = printf("i = %d\n", i); > >> } > > So how does this rename solve the issue? > > Now, we can clash with __rc__ variable. > > > > Thanks > > There is no safe way to handle that. It can be detected with -Wshadow though. > But rc is quite a common name (39 definitions in the code base), __rc__ much less (unused... yet) Or commit message should reflect that it is not fix, but attempt to reduce name collision, or the patch should provide real fix. For example, create inline function and convert all 5 callers to use it without ##__VA_ARGS__. > > > If you want some real fun, try writing macros with local variables that can be called recursively ;) > > Nicolas > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 01, 2017 at 07:56:32AM +0300, Leon Romanovsky wrote: > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: > > To avoid clashing with user variables. > > > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > util/util.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/util/util.h b/util/util.h > > index cbf0deca2dde7b..30d32ee6b51e24 100644 > > +++ b/util/util.h > > @@ -8,8 +8,8 @@ > > * error */ > > #define check_snprintf(buf, len, fmt, ...) \ > > ({ \ > > - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > > - (rc < len && rc >= 0); \ > > + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > > It can't clash, because it is declared in the scope {..} of that define and > it is local to check_snprintf macro. This triggers a -Wshadow warning: int rc; check_snprinf(str, sizeof(str), ..) And rc is commonly used in callers, so it shouldn't. Perhaps this addresses all the concerns: /* Return true if the snprintf succeeded, false if there was truncation or * error */ static inline bool __good_snprintf(size_t len, int rc) { return (rc < len && rc >= 0); } #define check_snprintf(buf, len, fmt, ...) \ __good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__)) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2017-09-01 at 09:00 -0600, Jason Gunthorpe wrote: > On Fri, Sep 01, 2017 at 07:56:32AM +0300, Leon Romanovsky wrote: > > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote: > > > To avoid clashing with user variables. > > > > > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > > util/util.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/util/util.h b/util/util.h > > > index cbf0deca2dde7b..30d32ee6b51e24 100644 > > > +++ b/util/util.h > > > @@ -8,8 +8,8 @@ > > > * error */ > > > #define check_snprintf(buf, len, fmt, ...) \ > > > ({ \ > > > - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > > > - (rc < len && rc >= 0); \ > > > + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ > > > > It can't clash, because it is declared in the scope {..} of that define and > > it is local to check_snprintf macro. > > This triggers a -Wshadow warning: > > int rc; > check_snprinf(str, sizeof(str), ..) > > And rc is commonly used in callers, so it shouldn't. > > Perhaps this addresses all the concerns: > > /* Return true if the snprintf succeeded, false if there was truncation or > * error */ > static inline bool __good_snprintf(size_t len, int rc) > { > return (rc < len && rc >= 0); > } > > #define check_snprintf(buf, len, fmt, ...) \ > __good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__)) If an inline function will be introduced anyway, why not to make check_snprintf() itself an inline function and use vsnprintf() instead of snprintf()? Bart.
On Fri, Sep 01, 2017 at 03:13:20PM +0000, Bart Van Assche wrote: > > #define check_snprintf(buf, len, fmt, ...) \ > > __good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__)) > > If an inline function will be introduced anyway, why not to make check_snprintf() > itself an inline function and use vsnprintf() instead of snprintf()? Well.. Generally speaking I prefer to avoid wrappering 'magic' functions like snprintf with inlines because you loose the 'magic'. eg, however it is done, modern gcc triggers -Wformat-length analysis for snprintf, and if you wrapper it with an inline it appears that analysis is lost. This is separate from the well known attribute((printf)). Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/util/util.h b/util/util.h index cbf0deca2dde7b..30d32ee6b51e24 100644 --- a/util/util.h +++ b/util/util.h @@ -8,8 +8,8 @@ * error */ #define check_snprintf(buf, len, fmt, ...) \ ({ \ - int rc = snprintf(buf, len, fmt, ##__VA_ARGS__); \ - (rc < len && rc >= 0); \ + int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__); \ + (__rc__ < len && __rc__ >= 0); \ }) /* a CMP b. See also the BSD macro timercmp(). */
To avoid clashing with user variables. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- util/util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)