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

[Allison Collins <allison.henderson@oracle.com>]

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
>