Re: Patch that adds a new test to find

[James Youngman]

diff -Nur findutils/find/cmp.c findutils-rreale/find/cmp.c
--- findutils/find/cmp.c        1970-01-01 01:00:00.000000000 +0100
+++ findutils-rreale/find/cmp.c 2010-04-05 22:29:19.000000000 +0200
@@ -0,0 +1,224 @@
+/* Buffer primitives for comparison operations.  Adapted from the
+   following files:
+
+   - src/cmp.c in GNU diffutils 2.9
+
+   Copyright (C) 1990-1996, 1998, 2001-2002, 2004, 2006-2007, 2009-2010 Free
+   Software Foundation, Inc.
+
+   - lib/cmpbuf.c in GNU diffutils 2.9
+
+   Copyright (C) 1993, 1995, 1998, 2001-2002, 2006, 2009-2010 Free Software
+   Foundation, Inc.  */

Thanks for moving this into a separate file, I think that will work much better.


+#ifndef word
+# define word uintmax_t

If you want to use uintmax_t, you should probably #include <stdint.h>.

+#endif
+
+#undef MIN
+#define MIN(a, b) ((a) <= (b) ? (a) : (b))
+
+/* Read NBYTES bytes from descriptor FD into BUF.
+   NBYTES must not be SIZE_MAX.
+   Return the number of characters successfully read.
+   On error, return SIZE_MAX, setting errno.
+   The number returned is always NBYTES unless end-of-file or error.  */
+
+size_t
+block_read (int fd, char *buf, size_t nbytes)

It only just occurred to me that a somewhat easier way to do this is
to use the safe_read module from gnulib.   Just add "safe-read" to the
list of modules in import-gnulib.config and re-run import-gnulib.sh.
Making this change will also allow you to avoid the need for some of
the definitions above, too.

Also though, disk reads are not usually interrupted by signals, I'm
not sure what you're defending the code from.




+boolean
+cmp (int fd0, int fd1, char *ref_pathname, char *pathname, struct
predicate *pred_ptr)
+{
[...]
+  lseek (fd0, 0, SEEK_SET);
+  lseek (fd1, 0, SEEK_SET);

Some error checking is a good idea there.



+/* cmp.c */
+boolean cmp (int fd0, int fd1, char *ref_pathname, char *pathname,
struct predicate *pred_ptr);
+
 enum DebugOption
   {
     DebugNone             = 0,
@@ -547,7 +560,7 @@
   /* If true, -depth was EXPLICITLY set (as opposed to having been turned
    * on by -delete, for example).
    */
-   boolean explicit_depth;
+  boolean explicit_depth;

I guess you didn't plan to change that line.


diff -Nur findutils/find/pred.c findutils-rreale/find/pred.c
--- findutils/find/pred.c       2010-04-05 13:18:59.000000000 +0200
+++ findutils-rreale/find/pred.c        2010-04-05 19:54:25.000000000 +0200
@@ -234,6 +234,7 @@
   {pred_writable, "writable "},
   {pred_xtype, "xtype   "},
   {pred_context, "context"},
+  {pred_samecontent, "samecontent"},
   {0, "none    "}
 };
 #endif
@@ -1905,6 +1906,30 @@
   return (pred_type (pathname, &sbuf, pred_ptr));
 }

+boolean
+pred_samecontent (const char *pathname, struct stat *stat_buf, struct
predicate *pred_ptr)
+{
+  struct stat *st = &pred_ptr->args.samecontentargs.st;
+  char *ref_pathname = pred_ptr->args.samecontentargs.ref_pathname;
+  int fd0, fd1;
+  boolean exit_status = false;
+
+  if (!S_ISREG (stat_buf->st_mode) || !S_ISREG (st->st_mode))
+    return false;

While I think it's possible to define semantics for "-samecontent" for
a directory, it's possible for someone to get a surprise about what
they should be.  Probably better to reject reference files which
aren't plain regular files in the parsing stage.

+  fd0 = open (ref_pathname, O_RDONLY | O_BINARY, 0);

Another suggestion for efficiency is to open the file in
parse_samefile (and fstat the file descriptor to figure out if it is a
regular file) and keep the reference file open from then on.

This will also simplify the error handling for these two lines (i.e.
we want to not call close(-1) if we failed to open "pathname").

+  fd1 = open (pathname, O_RDONLY | O_BINARY, 0);
+
+  exit_status = cmp (fd0, fd1, ref_pathname, pathname, pred_ptr);
+
+  close (fd0);
+  close (fd1);
+
+  return exit_status;
+}

 boolean
 pred_context (const char *pathname, struct stat *stat_buf,



Thanks,
James.