| Message ID | 20240214103741.16189-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix fallout from IVMD/RMRR unification checks | expand |
Hi, On 14/02/2024 10:37, Roger Pau Monne wrote: > It's not obvious from the function itself whether the incremented value will be > stored in the parameter, or returned to the caller. That has leads to bugs in > the past as callers assume the incremented value is stored in the parameter. > > Add the __must_check attribute to the function to easily spot callers that > don't consume the returned value, which signals an error in the caller logic. I like the patch. We should add more __must_check in the code :). I think ECLAIR will also help us to spot any place where we don't use the returned value. > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Julien Grall <jgrall@amazon.com> > --- > xen/include/xen/mm-frame.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h > index 922ae418807a..c25e836f255a 100644 > --- a/xen/include/xen/mm-frame.h > +++ b/xen/include/xen/mm-frame.h > @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn); > #undef mfn_x > #endif > > -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i) > +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i) > { > return _mfn(mfn_x(mfn) + i); > } > @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn); > #undef gfn_x > #endif > > -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i) > +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i) > { > return _gfn(gfn_x(gfn) + i); > } Cheers,
On 14.02.2024 11:37, Roger Pau Monne wrote: > It's not obvious from the function itself whether the incremented value will be s/the function itself/just the function name/ ? > stored in the parameter, or returned to the caller. That has leads to bugs in > the past as callers assume the incremented value is stored in the parameter. ... callers may assume ... ? > --- a/xen/include/xen/mm-frame.h > +++ b/xen/include/xen/mm-frame.h > @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn); > #undef mfn_x > #endif > > -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i) > +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i) > { > return _mfn(mfn_x(mfn) + i); > } > @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn); > #undef gfn_x > #endif > > -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i) > +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i) > { > return _gfn(gfn_x(gfn) + i); > } What about dfn_add() (in xen/iommu.h)? Jan
On Wed, Feb 14, 2024 at 02:42:46PM +0100, Jan Beulich wrote: > On 14.02.2024 11:37, Roger Pau Monne wrote: > > It's not obvious from the function itself whether the incremented value will be > > s/the function itself/just the function name/ ? > > > stored in the parameter, or returned to the caller. That has leads to bugs in > > the past as callers assume the incremented value is stored in the parameter. > > ... callers may assume ... ? > > > --- a/xen/include/xen/mm-frame.h > > +++ b/xen/include/xen/mm-frame.h > > @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn); > > #undef mfn_x > > #endif > > > > -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i) > > +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i) > > { > > return _mfn(mfn_x(mfn) + i); > > } > > @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn); > > #undef gfn_x > > #endif > > > > -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i) > > +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i) > > { > > return _gfn(gfn_x(gfn) + i); > > } > > What about dfn_add() (in xen/iommu.h)? Oh, didn't spot that one. Thanks, Roger.
On Wed, Feb 14, 2024 at 7:42 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > It's not obvious from the function itself whether the incremented value will be > stored in the parameter, or returned to the caller. That has leads to bugs in > the past as callers assume the incremented value is stored in the parameter. > > Add the __must_check attribute to the function to easily spot callers that > don't consume the returned value, which signals an error in the caller logic. > > No functional change intended. Just thinking out loud here... I wonder if "mfn_plus()" would be less likely to be misunderstood. Catching this during compile is def better than catching it w/ Coverity or Eclair, but even better to help people not make the mistake in the first place. -George
On 19.02.2024 06:07, George Dunlap wrote: > On Wed, Feb 14, 2024 at 7:42 PM Roger Pau Monne <roger.pau@citrix.com> wrote: >> >> It's not obvious from the function itself whether the incremented value will be >> stored in the parameter, or returned to the caller. That has leads to bugs in >> the past as callers assume the incremented value is stored in the parameter. >> >> Add the __must_check attribute to the function to easily spot callers that >> don't consume the returned value, which signals an error in the caller logic. >> >> No functional change intended. > > Just thinking out loud here... I wonder if "mfn_plus()" would be less > likely to be misunderstood. Catching this during compile is def > better than catching it w/ Coverity or Eclair, but even better to help > people not make the mistake in the first place. To me while mfn_plus() would indeed be somewhat less likely to cause this kind of confusion, I don't think that naming would entirely eliminate the risk. I was actually considering to add mfn_inc() for the common case of incrementing by 1, yet I can't see an alternative name avoiding the issue there. Jan
diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h index 922ae418807a..c25e836f255a 100644 --- a/xen/include/xen/mm-frame.h +++ b/xen/include/xen/mm-frame.h @@ -23,7 +23,7 @@ TYPE_SAFE(unsigned long, mfn); #undef mfn_x #endif -static inline mfn_t mfn_add(mfn_t mfn, unsigned long i) +static inline mfn_t __must_check mfn_add(mfn_t mfn, unsigned long i) { return _mfn(mfn_x(mfn) + i); } @@ -62,7 +62,7 @@ TYPE_SAFE(unsigned long, gfn); #undef gfn_x #endif -static inline gfn_t gfn_add(gfn_t gfn, unsigned long i) +static inline gfn_t __must_check gfn_add(gfn_t gfn, unsigned long i) { return _gfn(gfn_x(gfn) + i); }
It's not obvious from the function itself whether the incremented value will be stored in the parameter, or returned to the caller. That has leads to bugs in the past as callers assume the incremented value is stored in the parameter. Add the __must_check attribute to the function to easily spot callers that don't consume the returned value, which signals an error in the caller logic. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/include/xen/mm-frame.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)