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

[Eric Sandeen <sandeen@sandeen.net>]

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