Hi Jan and Michal, > On Sep 4, 2023, at 23:13, Jan Beulich <[email protected]> wrote: > > On 04.09.2023 16:03, Michal Orzel wrote: >> On 04/09/2023 15:28, Jan Beulich wrote: >>> On 04.09.2023 11:14, Michal Orzel wrote: >>>> --- a/xen/include/xen/bitops.h >>>> +++ b/xen/include/xen/bitops.h >>>> @@ -51,7 +51,7 @@ static inline int generic_ffs(int x) >>>> * fls: find last bit set. >>>> */ >>>> >>>> -static __inline__ int generic_fls(int x) >>>> +static __inline__ int generic_fls(unsigned int x) >>>> { >>>> int r = 32; >>>> >>> >>> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care >>> of as well, imo. If additionally you switch __inline__ to inline, things >>> will become nicely symmetric with generic_f{f,l}sl(). >> If others agree, I can switch generic_ffs() to take unsigned as well >> (together with s/__inline__/inline/). >> However, such change would no longer fall in fixes category (i.e. nothing >> wrong with ffs() taking int): >> - is it ok at this stage of release? (not sure but no problem for me to do >> this) > > I'd say yes, but the release manager would have the ultimate say.
>From the release aspect, I am fine with doing the above-mentioned extra changes, as we are not in the code freeze stage. From the development point of view, I think whether doing such extra changes or not is supposed to be an agreement between the developer and the maintainer. Also this patch itself looks good to me so: Reviewed-by: Henry Wang <[email protected]> Kind regards, Henry > >> - is it ok to mix a fix with non-fix change? (I always prefer fixes to be >> done as standalone changes) > > Imo it is, to avoid introducing an inconsistency. While such may > not themselves be bugs, they increase the risk of introducing some. > > Jan >
