RE: proposed sync() patch

["Dave Korn" <dk at artimi dot com>]

> -----Original Message-----
> From: cygwin-owner On Behalf Of Dmitry Karasik
> Sent: 13 April 2004 23:05

> ChangeLog:
> 
> 2004-13-04  Dmitry Karasik <dmitry@karasik.eu.org>
> 
> 	* syscalls.cc: Implement sync() for letter-mapped drives

  Just a couple of style suggestions for your consideration:

> +static int 
> +sync_drive( char drive) 

  It's kind of trivial, but you have this somewhat odd asymmetric spacing
style when you use brackets, with a space inside the open bracket but not
the close bracket.  Usual windoze coding style would be 

+static int 
+sync_drive( char drive )

and GNU coding style would be

+static int 
+sync_drive (char drive)

which is probably the more appropriate here.  You do the same thing with a
bunch of your other function calls and most of your if conditions, but then
sometimes you omit the spacing altogether.  This is a tiny nit-picking
point, but it's good to be consistent.

> +{
> +   HANDLE f;
> +   int ret;
> +   char file[7] = "\\\\.\\X:";

  No need to specify the size; let the compiler handle it:

+   char file[] = "\\\\.\\X:";

The compiler will always allocate enough space for the string; the only
advantage of specifying the size is that if you get it off-by-one the
compiler will obligingly not NUL-terminate the string for you...

> +   file[4] = drive;
> +   
> +   f = CreateFile( file, 

  I would have placed a blank line between the declaration of file and the
initialisation of file[4], which is the first real statement of the
function; it's slightly confusing to have a statement obscured amongst the
variable declarations at the top of a function.  In fact, I would have
placed the line setting file[4] hard against the CreateFile call without an
intervening blank, to emphasis that we're setting up a parameter to the
call.

> +		   GENERIC_READ|GENERIC_WRITE, 
> +		   FILE_SHARE_READ|FILE_SHARE_WRITE, 
> +		   NULL, OPEN_EXISTING, 
> +		   0, NULL);
> +   if ( f == INVALID_HANDLE_VALUE)
> +      return -1;
> +
> +   ret = FlushFileBuffers(f) ? 0 : -1;

  If you're going to return -1, shouldn't you be setting errno to some valid
and meaningful value?  Except of course that this return value is discarded
by the sync routine in any case.  I don't know what the policy is about
setting errno in internal library code - does anyone else know?

> +   CloseHandle(f);
> +   return ret;
> +}
> +
> +
>  /* sync: SUSv3 */
>  extern "C" void
>  sync ()
>  {
> +   DWORD map;
> +   char i, drive[4] = "X:\\";
> +
> +   map = GetLogicalDrives();
> +   /* sync all letter-mapped local 'fixed' drives */
> +   for ( i = 0; i < 26; i++) 
> +      if (( map << ( 31 - i)) >> 31) {
> +	 drive[0] = 'A' + i;
> +	 if ( GetDriveType( drive) == DRIVE_FIXED) 
> +	    sync_drive('A' + i);
> +      }
>  }

  That's a slightly funny way to write the loop: your way of testing a bit
needs two shifts and a subtract.  How about this:

 extern "C" void
 sync ()
 {
+   DWORD map;
+   char i, drive[4] = "A:\\";
+
+   map = GetLogicalDrives();
+   /* sync all letter-mapped local 'fixed' drives */
+   for (i = 26; i; i--, map >>=1, drive[0]++) 
+      if ((map & 1) && (GetDriveType (drive) == DRIVE_FIXED))
+	    sync_drive (drive[0]);
 }



    cheers, 
      DaveK
-- 
Can't think of a witty .sigline today....


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/