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.


diff --git a/src/shred.c b/src/shred.c
index 187446f..2793843 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -519,7 +519,7 @@ dopass (int fd, struct stat const *st, char const *qname, off_t *sizep,
                      at all on some (file) systems, or with the current size.
                      I.E. a specified --size that is not aligned, or when
                      dealing with slop at the end of a file with --exact.  */
-                  if (k == 1 && !try_without_directio && errno == EINVAL)
+                  if (! try_without_directio && errno == EINVAL)
                     {
                       direct_mode (fd, false);
                       ssize = 0;
@@ -870,6 +870,11 @@ do_wipefd (int fd, char const *qname, struct randint_source *s,
       error (0, 0, _("%s: invalid file type"), qname);
       return false;
     }
+  else if (S_ISREG (st.st_mode) && st.st_size < 0)
+    {
+      error (0, 0, _("%s: file has negative size"), qname);
+      return false;
+    }
 
   /* Allocate pass array */
   passarray = xnmalloc (flags->n_iterations, sizeof *passarray);
@@ -880,12 +885,6 @@ do_wipefd (int fd, char const *qname, struct randint_source *s,
       if (S_ISREG (st.st_mode))
         {
           size = st.st_size;
-          if (size < 0)
-            {
-              ok = false;
-              error (0, 0, _("%s: file has negative size"), qname);
-              goto wipefd_out;
-            }
 
           if (! flags->exact)
             {
@@ -914,12 +913,9 @@ do_wipefd (int fd, char const *qname, struct randint_source *s,
             }
         }
     }
-  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;
-    }
+  else if (S_ISREG (st.st_mode)
+           && st.st_size < MIN (ST_BLKSIZE (st), size))
+    i_size = st.st_size;
 
   /* Schedule the passes in random order. */
   genpattern (passarray, flags->n_iterations, s);
diff --git a/tests/misc/shred-exact.sh b/tests/misc/shred-exact.sh
index 5f2848e..5434229 100755
--- a/tests/misc/shred-exact.sh
+++ b/tests/misc/shred-exact.sh
@@ -40,6 +40,10 @@ done
 # (i.e. we want to test failed writes not at the start).
 truncate -s1MiB file.slop || framework_failure_
 truncate -s+1 file.slop || framework_failure_
-shred --exact -n1 file.slop || fail=1
+shred --exact -n2 file.slop || fail=1
+
+# make sure direct I/O is handled appropriately at start of file
+truncate -s1 file.slop || framework_failure_
+shred --exact -n2 file.slop || fail=1
 
 Exit $fail