Mail Archives: cygwin/2004/04/14/06:16:08
> -----Original Message-----
> From: cygwin-owner On Behalf Of Dmitry Karasik
> Sent: 13 April 2004 23:05
> ChangeLog:
>
> 2004-13-04 Dmitry Karasik <dmitry AT karasik DOT eu DOT 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/
- Raw text -