Re: [PATCH] shred: overwrite inode storage used by some file systems

[Pádraig Brady]

On 04/04/2014 06:49 PM, Paul Eggert wrote:
>> +  else if (S_ISREG (st.st_mode))
>> +    {
>> +      off_t fsize = st.st_size;
>> +      if (fsize > 0 && fsize < ST_BLKSIZE (st) && size > fsize)
>> +        i_size = fsize;
>> +    }
> 
> This can be simplified.  There's no need to worry about checking whether 
> st.st_size == 0 since the code would do the right thing if that test were 
> removed.

> (Or are you worried about st.st_size < 0?  If so, that check should be 
> hoisted out of the previous 'if' and done on all regular files.)

This was the case I was thinking about.
Yes hoisting the check is better.
Always better to validate earlier, so as to simplify later code.

> We generally prefer "<" and "<=" for size comparisons, as it makes code 
> easier to visualize.  Something like this, perhaps:
> 
>    else if (S_ISREG (st.st_mode)
>             && st.st_size < MIN (ST_BLKSIZE (st), size))
>      i_size = st.st_size;

Yes I used that.
I adjusted as per the attached and pushed.

Thanks for the review!
Pádraig.

shred-inode-data-adjustments.diff
Description: Text Data