| Message ID | 20200727022608.18535-2-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Fix compiler warnings | expand |
On Sun, Jul 26, 2020 at 07:26:07PM -0700, Allison Collins wrote: > Fix compiler warning for variable 'blk' set but not used in > xfs_attr_node_removename_setup. blk is used only in an ASSERT so only > declare blk when DEBUG is set. > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index d4583a0..5168d32 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( > struct xfs_da_state **state) > { > int error; > +#ifdef DEBUG > struct xfs_da_state_blk *blk; > +#endif But now a non-DEBUG compilation will trip over the assignment to blk: blk = &(*state)->path.blk[(*state)->path.active - 1]; that comes just before the asserts, right? ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == XFS_ATTR_LEAF_MAGIC); In the end you probably just want to encode the accessor logic in the assert body so the whole thing just disappears entirely. --D > > error = xfs_attr_node_hasname(args, state); > if (error != -EEXIST) > -- > 2.7.4 >
On 7/27/20 8:46 AM, Darrick J. Wong wrote: > On Sun, Jul 26, 2020 at 07:26:07PM -0700, Allison Collins wrote: >> Fix compiler warning for variable 'blk' set but not used in >> xfs_attr_node_removename_setup. blk is used only in an ASSERT so only >> declare blk when DEBUG is set. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index d4583a0..5168d32 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( >> struct xfs_da_state **state) >> { >> int error; >> +#ifdef DEBUG >> struct xfs_da_state_blk *blk; >> +#endif > > But now a non-DEBUG compilation will trip over the assignment to blk: > > blk = &(*state)->path.blk[(*state)->path.active - 1]; > > that comes just before the asserts, right? > > ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > XFS_ATTR_LEAF_MAGIC); > > In the end you probably just want to encode the accessor logic in the > assert body so the whole thing just disappears entirely. Alrighty, will fix Allison > > --D > >> >> error = xfs_attr_node_hasname(args, state); >> if (error != -EEXIST) >> -- >> 2.7.4 >>
On 7/27/20 8:46 AM, Darrick J. Wong wrote: > On Sun, Jul 26, 2020 at 07:26:07PM -0700, Allison Collins wrote: >> Fix compiler warning for variable 'blk' set but not used in >> xfs_attr_node_removename_setup. blk is used only in an ASSERT so only >> declare blk when DEBUG is set. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index d4583a0..5168d32 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( >> struct xfs_da_state **state) >> { >> int error; >> +#ifdef DEBUG >> struct xfs_da_state_blk *blk; >> +#endif > > But now a non-DEBUG compilation will trip over the assignment to blk: > > blk = &(*state)->path.blk[(*state)->path.active - 1]; > > that comes just before the asserts, right? > > ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > XFS_ATTR_LEAF_MAGIC); > > In the end you probably just want to encode the accessor logic in the > assert body so the whole thing just disappears entirely. Are you sure you'd rather have it that way, then once up in the declaration? Like this: #ifdef DEBUG struct xfs_da_state_blk *blk = &(*state)->path.blk[(*state)->path.active - 1]; #endif > > --D > >> >> error = xfs_attr_node_hasname(args, state); >> if (error != -EEXIST) >> -- >> 2.7.4 >>
On Mon, Jul 27, 2020 at 09:51:54AM -0700, Allison Collins wrote: > > > On 7/27/20 8:46 AM, Darrick J. Wong wrote: > > On Sun, Jul 26, 2020 at 07:26:07PM -0700, Allison Collins wrote: > > > Fix compiler warning for variable 'blk' set but not used in > > > xfs_attr_node_removename_setup. blk is used only in an ASSERT so only > > > declare blk when DEBUG is set. > > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_attr.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > index d4583a0..5168d32 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( > > > struct xfs_da_state **state) > > > { > > > int error; > > > +#ifdef DEBUG > > > struct xfs_da_state_blk *blk; > > > +#endif > > > > But now a non-DEBUG compilation will trip over the assignment to blk: > > > > blk = &(*state)->path.blk[(*state)->path.active - 1]; > > > > that comes just before the asserts, right? > > > > ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); > > ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == > > XFS_ATTR_LEAF_MAGIC); > > > > In the end you probably just want to encode the accessor logic in the > > assert body so the whole thing just disappears entirely. > Are you sure you'd rather have it that way, then once up in the declaration? > Like this: > > #ifdef DEBUG > struct xfs_da_state_blk *blk = &(*state)->path.blk[(*state)->path.active - > 1]; > #endif I thought xfs_attr_node_hasname could allocate the da state and set *state, which means that we can't dereference *state until after that call? --D > > > > --D > > > > > error = xfs_attr_node_hasname(args, state); > > > if (error != -EEXIST) > > > -- > > > 2.7.4 > > >
On 7/27/20 11:50 AM, Darrick J. Wong wrote: > On Mon, Jul 27, 2020 at 09:51:54AM -0700, Allison Collins wrote: >> >> >> On 7/27/20 8:46 AM, Darrick J. Wong wrote: >>> On Sun, Jul 26, 2020 at 07:26:07PM -0700, Allison Collins wrote: >>>> Fix compiler warning for variable 'blk' set but not used in >>>> xfs_attr_node_removename_setup. blk is used only in an ASSERT so only >>>> declare blk when DEBUG is set. >>>> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >>>> --- >>>> fs/xfs/libxfs/xfs_attr.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>>> index d4583a0..5168d32 100644 >>>> --- a/fs/xfs/libxfs/xfs_attr.c >>>> +++ b/fs/xfs/libxfs/xfs_attr.c >>>> @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( >>>> struct xfs_da_state **state) >>>> { >>>> int error; >>>> +#ifdef DEBUG >>>> struct xfs_da_state_blk *blk; >>>> +#endif >>> >>> But now a non-DEBUG compilation will trip over the assignment to blk: >>> >>> blk = &(*state)->path.blk[(*state)->path.active - 1]; >>> >>> that comes just before the asserts, right? >>> >>> ASSERT((*state)->path.blk[(*state)->path.active - 1].bp != NULL); >>> ASSERT((*state)->path.blk[(*state)->path.active - 1].magic == >>> XFS_ATTR_LEAF_MAGIC); >>> >>> In the end you probably just want to encode the accessor logic in the >>> assert body so the whole thing just disappears entirely. >> Are you sure you'd rather have it that way, then once up in the declaration? >> Like this: >> >> #ifdef DEBUG >> struct xfs_da_state_blk *blk = &(*state)->path.blk[(*state)->path.active - >> 1]; >> #endif > > I thought xfs_attr_node_hasname could allocate the da state and set > *state, which means that we can't dereference *state until after that > call? I see, ok then, will add the deference in the asserts Allison > > --D > >>> >>> --D >>> >>>> error = xfs_attr_node_hasname(args, state); >>>> if (error != -EEXIST) >>>> -- >>>> 2.7.4 >>>>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index d4583a0..5168d32 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1174,7 +1174,9 @@ int xfs_attr_node_removename_setup( struct xfs_da_state **state) { int error; +#ifdef DEBUG struct xfs_da_state_blk *blk; +#endif error = xfs_attr_node_hasname(args, state); if (error != -EEXIST)
Fix compiler warning for variable 'blk' set but not used in xfs_attr_node_removename_setup. blk is used only in an ASSERT so only declare blk when DEBUG is set. Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 2 ++ 1 file changed, 2 insertions(+)