[PATCH v2 0/2] xfs: Fix compiler warnings

[PATCH v2 0/2] xfs: Fix compiler warnings

[Allison Collins]

Some variables added in the delayed attr series were only used in ASSERT
checks.  This caused some compiler warnings.  This is only declare them when
DEBUG is set

Allison Collins (2):
  xfs: Fix compiler warning in xfs_attr_node_removename_setup
  xfs: Fix compiler warning in xfs_attr_shortform_add

 fs/xfs/libxfs/xfs_attr.c      | 2 ++
 fs/xfs/libxfs/xfs_attr_leaf.c | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Allison Collins]

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
 
 	error = xfs_attr_node_hasname(args, state);
 	if (error != -EEXIST)
-- 
2.7.4

[PATCH v2 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add

[Allison Collins]

Fix compiler warning warning: variable 'error' set but not used in
xfs_attr_shortform_add. error is used only in an ASSERT so only declare
error 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_leaf.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index ad7b351..db985b8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -715,7 +715,10 @@ xfs_attr_shortform_add(
 {
 	struct xfs_attr_shortform	*sf;
 	struct xfs_attr_sf_entry	*sfe;
-	int				offset, size, error;
+	int				offset, size;
+#if DEBUG
+	int				error;
+#endif
 	struct xfs_mount		*mp;
 	struct xfs_inode		*dp;
 	struct xfs_ifork		*ifp;
-- 
2.7.4

Re: [PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Darrick J. Wong]

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
> 

Re: [PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Allison Collins]

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
>>

Re: [PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Allison Collins]

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
>>

Re: [PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Darrick J. Wong]

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
> > > 

Re: [PATCH v2 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup

[Allison Collins]

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
>>>>