| Message ID | ZG1d51tGG4c97qqb@work (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] nfsd: Replace one-element array with flexible-array member | expand |
On Tue, May 23, 2023 at 06:44:23PM -0600, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace a one-element array > with a flexible-arrayº member in struct vbi_anc_data and refactor I don't know what "struct vbi_anc_data" is. Is the patch description correct? > the rest of the code, accordingly. > > This results in no differences in binary output. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/298 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > fs/nfsd/nfs4callback.c | 2 +- > fs/nfsd/xdr4.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 4039ffcf90ba..2c688d51135d 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -353,7 +353,7 @@ encode_cb_recallany4args(struct xdr_stream *xdr, > { > encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY); > encode_uint32(xdr, ra->ra_keep); > - encode_bitmap4(xdr, ra->ra_bmval, ARRAY_SIZE(ra->ra_bmval)); > + encode_bitmap4(xdr, ra->ra_bmval, 1); I find the new code less self-documenting. > hdr->nops++; > } > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index 510978e602da..68072170eac8 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -899,7 +899,7 @@ struct nfsd4_operation { > struct nfsd4_cb_recall_any { > struct nfsd4_callback ra_cb; > u32 ra_keep; > - u32 ra_bmval[1]; > + u32 ra_bmval[]; This is not a placeholder for "1 or more elements". We actually want just a single u32 element in this array. Doesn't this change the sizeof(struct nfsd4_cb_recall_any) ? > }; > > #endif > -- > 2.34.1 >
On 5/23/23 19:01, Chuck Lever wrote: > On Tue, May 23, 2023 at 06:44:23PM -0600, Gustavo A. R. Silva wrote: >> One-element arrays are deprecated, and we are replacing them with >> flexible array members instead. So, replace a one-element array >> with a flexible-arrayº member in struct vbi_anc_data and refactor > > I don't know what "struct vbi_anc_data" is. Is the patch description > correct? Oops, copy/paste error. I'll fix it up. :) > > >> the rest of the code, accordingly. >> >> This results in no differences in binary output. >> >> Link: https://github.com/KSPP/linux/issues/79 >> Link: https://github.com/KSPP/linux/issues/298 >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > >> --- >> fs/nfsd/nfs4callback.c | 2 +- >> fs/nfsd/xdr4.h | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >> index 4039ffcf90ba..2c688d51135d 100644 >> --- a/fs/nfsd/nfs4callback.c >> +++ b/fs/nfsd/nfs4callback.c >> @@ -353,7 +353,7 @@ encode_cb_recallany4args(struct xdr_stream *xdr, >> { >> encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY); >> encode_uint32(xdr, ra->ra_keep); >> - encode_bitmap4(xdr, ra->ra_bmval, ARRAY_SIZE(ra->ra_bmval)); >> + encode_bitmap4(xdr, ra->ra_bmval, 1); > > I find the new code less self-documenting. > > >> hdr->nops++; >> } >> >> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h >> index 510978e602da..68072170eac8 100644 >> --- a/fs/nfsd/xdr4.h >> +++ b/fs/nfsd/xdr4.h >> @@ -899,7 +899,7 @@ struct nfsd4_operation { >> struct nfsd4_cb_recall_any { >> struct nfsd4_callback ra_cb; >> u32 ra_keep; >> - u32 ra_bmval[1]; >> + u32 ra_bmval[]; > > This is not a placeholder for "1 or more elements". We actually want > just a single u32 element in this array. Doesn't this change the > sizeof(struct nfsd4_cb_recall_any) ? I see. Yes, it does change the size. Can we replace it with a simple object of type u32? or do you actually need this to stay an array? Thanks -- Gustav > > >> }; >> >> #endif >> -- >> 2.34.1 >>
On Tue, May 23, 2023 at 07:11:37PM -0600, Gustavo A. R. Silva wrote: > > On 5/23/23 19:01, Chuck Lever wrote: > > On Tue, May 23, 2023 at 06:44:23PM -0600, Gustavo A. R. Silva wrote: > > > One-element arrays are deprecated, and we are replacing them with > > > flexible array members instead. So, replace a one-element array > > > with a flexible-arrayº member in struct vbi_anc_data and refactor > > > > I don't know what "struct vbi_anc_data" is. Is the patch description > > correct? > > Oops, copy/paste error. I'll fix it up. :) > > > > > > > > the rest of the code, accordingly. > > > > > > This results in no differences in binary output. > > > > > > Link: https://github.com/KSPP/linux/issues/79 > > > Link: https://github.com/KSPP/linux/issues/298 > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > > > > --- > > > fs/nfsd/nfs4callback.c | 2 +- > > > fs/nfsd/xdr4.h | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > > index 4039ffcf90ba..2c688d51135d 100644 > > > --- a/fs/nfsd/nfs4callback.c > > > +++ b/fs/nfsd/nfs4callback.c > > > @@ -353,7 +353,7 @@ encode_cb_recallany4args(struct xdr_stream *xdr, > > > { > > > encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY); > > > encode_uint32(xdr, ra->ra_keep); > > > - encode_bitmap4(xdr, ra->ra_bmval, ARRAY_SIZE(ra->ra_bmval)); > > > + encode_bitmap4(xdr, ra->ra_bmval, 1); > > > > I find the new code less self-documenting. > > > > > > > hdr->nops++; > > > } > > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > > index 510978e602da..68072170eac8 100644 > > > --- a/fs/nfsd/xdr4.h > > > +++ b/fs/nfsd/xdr4.h > > > @@ -899,7 +899,7 @@ struct nfsd4_operation { > > > struct nfsd4_cb_recall_any { > > > struct nfsd4_callback ra_cb; > > > u32 ra_keep; > > > - u32 ra_bmval[1]; > > > + u32 ra_bmval[]; > > > > This is not a placeholder for "1 or more elements". We actually want > > just a single u32 element in this array. Doesn't this change the > > sizeof(struct nfsd4_cb_recall_any) ? > > I see. Yes, it does change the size. Can we replace it with a simple > object of type u32? or do you actually need this to stay an array? It's not impossible to make it a scalar u32, however... In this area of code, *_bmval is always a bitmap -- an array of u32s. Helpers like encode_bitmap4() assume an array. I think it would be less confusing overall to human readers if it remained an array. In this case, it is a single element array because CB_RECALL_ANY doesn't happen to use bits above the first 32-bit word of the bitmap. We could make it a 2-element array, I think, without harm. Send a patch for that, and Dai can test it to make sure there are no unexpected interoperability consequences. I hope that would avoid suspicious-looking array definitions. > > > }; > > > #endif > > > -- > > > 2.34.1 > > >
On 5/23/23 19:31, Chuck Lever wrote: > On Tue, May 23, 2023 at 07:11:37PM -0600, Gustavo A. R. Silva wrote: >> >> On 5/23/23 19:01, Chuck Lever wrote: >>> On Tue, May 23, 2023 at 06:44:23PM -0600, Gustavo A. R. Silva wrote: >>>> One-element arrays are deprecated, and we are replacing them with >>>> flexible array members instead. So, replace a one-element array >>>> with a flexible-arrayº member in struct vbi_anc_data and refactor >>> >>> I don't know what "struct vbi_anc_data" is. Is the patch description >>> correct? >> >> Oops, copy/paste error. I'll fix it up. :) >> >>> >>> >>>> the rest of the code, accordingly. >>>> >>>> This results in no differences in binary output. >>>> >>>> Link: https://github.com/KSPP/linux/issues/79 >>>> Link: https://github.com/KSPP/linux/issues/298 >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>> >>>> --- >>>> fs/nfsd/nfs4callback.c | 2 +- >>>> fs/nfsd/xdr4.h | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>>> index 4039ffcf90ba..2c688d51135d 100644 >>>> --- a/fs/nfsd/nfs4callback.c >>>> +++ b/fs/nfsd/nfs4callback.c >>>> @@ -353,7 +353,7 @@ encode_cb_recallany4args(struct xdr_stream *xdr, >>>> { >>>> encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY); >>>> encode_uint32(xdr, ra->ra_keep); >>>> - encode_bitmap4(xdr, ra->ra_bmval, ARRAY_SIZE(ra->ra_bmval)); >>>> + encode_bitmap4(xdr, ra->ra_bmval, 1); >>> >>> I find the new code less self-documenting. >>> >>> >>>> hdr->nops++; >>>> } >>>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h >>>> index 510978e602da..68072170eac8 100644 >>>> --- a/fs/nfsd/xdr4.h >>>> +++ b/fs/nfsd/xdr4.h >>>> @@ -899,7 +899,7 @@ struct nfsd4_operation { >>>> struct nfsd4_cb_recall_any { >>>> struct nfsd4_callback ra_cb; >>>> u32 ra_keep; >>>> - u32 ra_bmval[1]; >>>> + u32 ra_bmval[]; >>> >>> This is not a placeholder for "1 or more elements". We actually want >>> just a single u32 element in this array. Doesn't this change the >>> sizeof(struct nfsd4_cb_recall_any) ? >> >> I see. Yes, it does change the size. Can we replace it with a simple >> object of type u32? or do you actually need this to stay an array? > > It's not impossible to make it a scalar u32, however... > > In this area of code, *_bmval is always a bitmap -- an array of u32s. > Helpers like encode_bitmap4() assume an array. I think it would be > less confusing overall to human readers if it remained an array. > > In this case, it is a single element array because CB_RECALL_ANY > doesn't happen to use bits above the first 32-bit word of the > bitmap. I see. If this is never going to be treated as a flexible array, then it can stay as is. -fstrict-flex-arrays=3 should not warn about this because the array will never ever be tried to be accessed beyond element 1. :) Thanks for the feedback! -- Gustavo > > We could make it a 2-element array, I think, without harm. Send a > patch for that, and Dai can test it to make sure there are no > unexpected interoperability consequences. > > I hope that would avoid suspicious-looking array definitions. > > >>>> }; >>>> #endif >>>> -- >>>> 2.34.1 >>>>
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 4039ffcf90ba..2c688d51135d 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -353,7 +353,7 @@ encode_cb_recallany4args(struct xdr_stream *xdr, { encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY); encode_uint32(xdr, ra->ra_keep); - encode_bitmap4(xdr, ra->ra_bmval, ARRAY_SIZE(ra->ra_bmval)); + encode_bitmap4(xdr, ra->ra_bmval, 1); hdr->nops++; } diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index 510978e602da..68072170eac8 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -899,7 +899,7 @@ struct nfsd4_operation { struct nfsd4_cb_recall_any { struct nfsd4_callback ra_cb; u32 ra_keep; - u32 ra_bmval[1]; + u32 ra_bmval[]; }; #endif
One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace a one-element array with a flexible-arrayº member in struct vbi_anc_data and refactor the rest of the code, accordingly. This results in no differences in binary output. Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/298 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- fs/nfsd/nfs4callback.c | 2 +- fs/nfsd/xdr4.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)