Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm List-Subscribe: List-Archive: List-Post: List-Help: , Sender: cygwin-owner AT cygwin DOT com Mail-Followup-To: cygwin AT cygwin DOT com Delivered-To: mailing list cygwin AT cygwin DOT com From: "Dave Korn" To: Subject: RE: proposed sync() patch Date: Wed, 14 Apr 2004 11:14:41 +0100 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit In-Reply-To: <407C641D.9A727023@karasik.eu.org> Message-ID: X-OriginalArrivalTime: 14 Apr 2004 10:14:41.0625 (UTC) FILETIME=[4D115C90:01C42209] > -----Original Message----- > From: cygwin-owner On Behalf Of Dmitry Karasik > Sent: 13 April 2004 23:05 > ChangeLog: > > 2004-13-04 Dmitry Karasik > > * 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/