Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] xfs: Fix compiler warnings
@ 2020-07-25 23:01 Allison Collins
  2020-07-25 23:01 ` [PATCH 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup Allison Collins
  2020-07-25 23:01 ` [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add Allison Collins
  0 siblings, 2 replies; 7+ messages in thread
From: Allison Collins @ 2020-07-25 23:01 UTC (permalink / raw)
  To: linux-xfs

Some variables added in the delayed attr series were only used in ASSERT checks.
This caused some compiler warnings.  This is a quick set to add some extra
error handling to quiet those warnings.

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      | 10 ++++++----
 fs/xfs/libxfs/xfs_attr_leaf.c | 11 ++++++++---
 fs/xfs/libxfs/xfs_attr_leaf.h |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup
  2020-07-25 23:01 [PATCH 0/2] xfs: Fix compiler warnings Allison Collins
@ 2020-07-25 23:01 ` Allison Collins
  2020-07-25 23:01 ` [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add Allison Collins
  1 sibling, 0 replies; 7+ messages in thread
From: Allison Collins @ 2020-07-25 23:01 UTC (permalink / raw)
  To: linux-xfs

Fix compiler warning for variable 'blk' set but not used in
xfs_attr_node_removename_setup

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index d4583a0..4ef0020 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1181,8 +1181,11 @@ int xfs_attr_node_removename_setup(
 		return error;
 
 	blk = &(*state)->path.blk[(*state)->path.active - 1];
-	ASSERT(blk->bp != NULL);
-	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
+	if (blk->bp == NULL)
+		return -EFSCORRUPTED;
+
+	if (blk->magic != XFS_ATTR_LEAF_MAGIC)
+		return -EFSCORRUPTED;
 
 	if (args->rmtblkno > 0) {
 		error = xfs_attr_leaf_mark_incomplete(args, *state);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add
  2020-07-25 23:01 [PATCH 0/2] xfs: Fix compiler warnings Allison Collins
  2020-07-25 23:01 ` [PATCH 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup Allison Collins
@ 2020-07-25 23:01 ` Allison Collins
  2020-07-26  6:48   ` Eric Sandeen
  1 sibling, 1 reply; 7+ messages in thread
From: Allison Collins @ 2020-07-25 23:01 UTC (permalink / raw)
  To: linux-xfs

Fix compiler warning: variable 'error' set but not used in
xfs_attr_shortform_add

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c      |  3 +--
 fs/xfs/libxfs/xfs_attr_leaf.c | 11 ++++++++---
 fs/xfs/libxfs/xfs_attr_leaf.h |  2 +-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4ef0020..3428f8b 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -562,8 +562,7 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
 	if (!forkoff)
 		return -ENOSPC;
 
-	xfs_attr_shortform_add(args, forkoff);
-	return 0;
+	return xfs_attr_shortform_add(args, forkoff);
 }
 
 
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index ad7b351..d0653bb 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -708,7 +708,7 @@ xfs_attr_sf_findname(
  * Add a name/value pair to the shortform attribute list.
  * Overflow from the inode has already been checked for.
  */
-void
+int
 xfs_attr_shortform_add(
 	struct xfs_da_args		*args,
 	int				forkoff)
@@ -730,7 +730,8 @@ xfs_attr_shortform_add(
 	ASSERT(ifp->if_flags & XFS_IFINLINE);
 	sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
 	error = xfs_attr_sf_findname(args, &sfe, NULL);
-	ASSERT(error != -EEXIST);
+	if (error == -EEXIST)
+		return error;
 
 	offset = (char *)sfe - (char *)sf;
 	size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
@@ -748,6 +749,8 @@ xfs_attr_shortform_add(
 	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE | XFS_ILOG_ADATA);
 
 	xfs_sbversion_add_attr2(mp, args->trans);
+
+	return 0;
 }
 
 /*
@@ -1151,7 +1154,9 @@ xfs_attr3_leaf_to_shortform(
 		nargs.valuelen = be16_to_cpu(name_loc->valuelen);
 		nargs.hashval = be32_to_cpu(entry->hashval);
 		nargs.attr_filter = entry->flags & XFS_ATTR_NSP_ONDISK_MASK;
-		xfs_attr_shortform_add(&nargs, forkoff);
+		error = xfs_attr_shortform_add(&nargs, forkoff);
+		if (error)
+			goto out;
 	}
 	error = 0;
 
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index 9b1c59f..e0027bb 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -46,7 +46,7 @@ struct xfs_attr3_icleaf_hdr {
  * Internal routines when attribute fork size < XFS_LITINO(mp).
  */
 void	xfs_attr_shortform_create(struct xfs_da_args *args);
-void	xfs_attr_shortform_add(struct xfs_da_args *args, int forkoff);
+int	xfs_attr_shortform_add(struct xfs_da_args *args, int forkoff);
 int	xfs_attr_shortform_lookup(struct xfs_da_args *args);
 int	xfs_attr_shortform_getvalue(struct xfs_da_args *args);
 int	xfs_attr_shortform_to_leaf(struct xfs_da_args *args,
-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add
  2020-07-25 23:01 ` [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add Allison Collins
@ 2020-07-26  6:48   ` Eric Sandeen
  2020-07-26 18:10     ` Allison Collins
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2020-07-26  6:48 UTC (permalink / raw)
  To: Allison Collins, linux-xfs

On 7/25/20 4:01 PM, Allison Collins wrote:
> @@ -730,7 +730,8 @@ xfs_attr_shortform_add(
>  	ASSERT(ifp->if_flags & XFS_IFINLINE);
>  	sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
>  	error = xfs_attr_sf_findname(args, &sfe, NULL);
> -	ASSERT(error != -EEXIST);
> +	if (error == -EEXIST)
> +		return error;
>  
>  	offset = (char *)sfe - (char *)sf;
>  	size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);

ASSERTs are normally "this cannot happen unless somebody made a
programming mistake," not an error that can actually happen in normal
use.

So now it's being handled as a normal error. (here and in other places
in these patches)

Is -EEXIST an error that should be handled, or if we get it does that
indicate that somebody made a coding mistake?

I ask because "fix compiler warnings" don't usually turn into
"add a bunch of new error handling" so ... some extra explanation would
be helpful for these changes.

Thanks,
-Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add
  2020-07-26  6:48   ` Eric Sandeen
@ 2020-07-26 18:10     ` Allison Collins
  2020-07-27  0:27       ` Eric Sandeen
  0 siblings, 1 reply; 7+ messages in thread
From: Allison Collins @ 2020-07-26 18:10 UTC (permalink / raw)
  To: Eric Sandeen, linux-xfs



On 7/25/20 11:48 PM, Eric Sandeen wrote:
> On 7/25/20 4:01 PM, Allison Collins wrote:
>> @@ -730,7 +730,8 @@ xfs_attr_shortform_add(
>>   	ASSERT(ifp->if_flags & XFS_IFINLINE);
>>   	sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
>>   	error = xfs_attr_sf_findname(args, &sfe, NULL);
>> -	ASSERT(error != -EEXIST);
>> +	if (error == -EEXIST)
>> +		return error;
>>   
>>   	offset = (char *)sfe - (char *)sf;
>>   	size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
> 
> ASSERTs are normally "this cannot happen unless somebody made a
> programming mistake," not an error that can actually happen in normal
> use.
> 
> So now it's being handled as a normal error. (here and in other places
> in these patches)
> 
> Is -EEXIST an error that should be handled, or if we get it does that
> indicate that somebody made a coding mistake?
> 
> I ask because "fix compiler warnings" don't usually turn into
> "add a bunch of new error handling" so ... some extra explanation would
> be helpful for these changes.
I see. At this point in the attr process, if this error happens, I would 
call it "a programming mistake" of sorts.  This condition of the attr 
already existing is handled much earlier in the code, so this error code 
path really shouldn't ever execute at this point unless something weird 
happened.

Should I have both the assert and the error handling for the compiler 
warning?  I wasn't really sure how concerned people actually were about 
the warnings.  It's not really that the variable is unused, it's just 
only used for the assert.

Thanks!
Allison

> 
> Thanks,
> -Eric
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add
  2020-07-26 18:10     ` Allison Collins
@ 2020-07-27  0:27       ` Eric Sandeen
  2020-07-27  1:57         ` Allison Collins
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Sandeen @ 2020-07-27  0:27 UTC (permalink / raw)
  To: Allison Collins, linux-xfs

On 7/26/20 11:10 AM, Allison Collins wrote:
> 
> 
> On 7/25/20 11:48 PM, Eric Sandeen wrote:
>> On 7/25/20 4:01 PM, Allison Collins wrote:
>>> @@ -730,7 +730,8 @@ xfs_attr_shortform_add(
>>>       ASSERT(ifp->if_flags & XFS_IFINLINE);
>>>       sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
>>>       error = xfs_attr_sf_findname(args, &sfe, NULL);
>>> -    ASSERT(error != -EEXIST);
>>> +    if (error == -EEXIST)
>>> +        return error;
>>>         offset = (char *)sfe - (char *)sf;
>>>       size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
>>
>> ASSERTs are normally "this cannot happen unless somebody made a
>> programming mistake," not an error that can actually happen in normal
>> use.
>>
>> So now it's being handled as a normal error. (here and in other places
>> in these patches)
>>
>> Is -EEXIST an error that should be handled, or if we get it does that
>> indicate that somebody made a coding mistake?
>>
>> I ask because "fix compiler warnings" don't usually turn into
>> "add a bunch of new error handling" so ... some extra explanation would
>> be helpful for these changes.
> I see. At this point in the attr process, if this error happens, I would call it "a programming mistake" of sorts.  This condition of the attr already existing is handled much earlier in the code, so this error code path really shouldn't ever execute at this point unless something weird happened.
> 
> Should I have both the assert and the error handling for the compiler warning?  I wasn't really sure how concerned people actually were about the warnings.  It's not really that the variable is unused, it's just only used for the assert.

hi Allison -

Well, it really is unused if #ifdef DEBUG isn't set.  :)  And we do want to eliminate gcc warnings so you're doing the right thing by addressing them.

If these are typical ASSERTs which are "debug only, should never happen, if it does you broke the code" then I'd say wrap the variable declarations in

#ifdef DEBUG
	int foo;
#endif

it's ugly, but we do it in many places.

if it's a real, possible error that actually needs to be handled at runtime then the way you've done it makes sense, I'd just suggest a commit log that explains the rationale for the change.

Sorry for not being conversant enough in this code to know the difference between the two, it just kind of stuck out at me to see ASSERTs being turned into error handlers as a response to compiler warnings.

Thanks
-Eric

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add
  2020-07-27  0:27       ` Eric Sandeen
@ 2020-07-27  1:57         ` Allison Collins
  0 siblings, 0 replies; 7+ messages in thread
From: Allison Collins @ 2020-07-27  1:57 UTC (permalink / raw)
  To: Eric Sandeen, linux-xfs



On 7/26/20 5:27 PM, Eric Sandeen wrote:
> On 7/26/20 11:10 AM, Allison Collins wrote:
>>
>>
>> On 7/25/20 11:48 PM, Eric Sandeen wrote:
>>> On 7/25/20 4:01 PM, Allison Collins wrote:
>>>> @@ -730,7 +730,8 @@ xfs_attr_shortform_add(
>>>>        ASSERT(ifp->if_flags & XFS_IFINLINE);
>>>>        sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
>>>>        error = xfs_attr_sf_findname(args, &sfe, NULL);
>>>> -    ASSERT(error != -EEXIST);
>>>> +    if (error == -EEXIST)
>>>> +        return error;
>>>>          offset = (char *)sfe - (char *)sf;
>>>>        size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
>>>
>>> ASSERTs are normally "this cannot happen unless somebody made a
>>> programming mistake," not an error that can actually happen in normal
>>> use.
>>>
>>> So now it's being handled as a normal error. (here and in other places
>>> in these patches)
>>>
>>> Is -EEXIST an error that should be handled, or if we get it does that
>>> indicate that somebody made a coding mistake?
>>>
>>> I ask because "fix compiler warnings" don't usually turn into
>>> "add a bunch of new error handling" so ... some extra explanation would
>>> be helpful for these changes.
>> I see. At this point in the attr process, if this error happens, I would call it "a programming mistake" of sorts.  This condition of the attr already existing is handled much earlier in the code, so this error code path really shouldn't ever execute at this point unless something weird happened.
>>
>> Should I have both the assert and the error handling for the compiler warning?  I wasn't really sure how concerned people actually were about the warnings.  It's not really that the variable is unused, it's just only used for the assert.
> 
> hi Allison -
> 
> Well, it really is unused if #ifdef DEBUG isn't set.  :)  And we do want to eliminate gcc warnings so you're doing the right thing by addressing them.
> 
> If these are typical ASSERTs which are "debug only, should never happen, if it does you broke the code" then I'd say wrap the variable declarations in
> 
> #ifdef DEBUG
> 	int foo;
> #endif
> 
> it's ugly, but we do it in many places.
> 
> if it's a real, possible error that actually needs to be handled at runtime then the way you've done it makes sense, I'd just suggest a commit log that explains the rationale for the change.
> 
> Sorry for not being conversant enough in this code to know the difference between the two, it just kind of stuck out at me to see ASSERTs being turned into error handlers as a response to compiler warnings.
No worries, thanks for the feed back.  Ok, will send out a v2.

Allison

> 
> Thanks
> -Eric
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-25 23:01 [PATCH 0/2] xfs: Fix compiler warnings Allison Collins
2020-07-25 23:01 ` [PATCH 1/2] xfs: Fix compiler warning in xfs_attr_node_removename_setup Allison Collins
2020-07-25 23:01 ` [PATCH 2/2] xfs: Fix compiler warning in xfs_attr_shortform_add Allison Collins
2020-07-26  6:48   ` Eric Sandeen
2020-07-26 18:10     ` Allison Collins
2020-07-27  0:27       ` Eric Sandeen
2020-07-27  1:57         ` Allison Collins

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git