Re: [Patch] Win32 gdbserver new interrupt support, and attach to process fix.

[Lerele <lerele@champenstudios.com>]

Oops. This was my first patch submission directly to the list (last one 
I sent directly to gdbserver maintainer), something had to go wrong, sorry.

>>+#ifdef USE_WIN32API
>>+static int remote_desc=INVALID_SOCKET;
>>+#else
>>+static int remote_desc=-1;
>>+#endif
>>    
>>
>
>I don't like using OS-dependent #define's where a functionality-based
>#define can do the job.  How about
>
>    +#ifndef INVALID_SOCKET
>    +#define INVALID_SOCKET -1
>    +#endif
>
>and then use INVALID_SOCKET everywhere?
>  
>
Isn't INVALID_SOCKET just an OS specific define?
Since this define will be in non OS specific files, wouldn't it "hurt" 
to be reading that portable code with OS specific embedded  words?

Personally I'd prefer something like:
#if USE_WIN32API
#define BADSOCKET INVALID_SOCKET
#else
#define BADSOCKET -1
#endif

However I believe it's natural to use -1 directly to check for bad 
socket in non-win32 code, so maybe using INVALID_SOCKET or BADSOCKET 
everywhere would make code uglier.

Tell me what you prefer.




>  
>
>>@@ -574,7 +584,7 @@
>> 
>>   FreeLibrary (kernel32);
>> 
>>-  return res;
>>+  return res? 0:-1;
>>    
>>
>
>I don't understand the need for this change.  Can you explain?
>child_continue does not promise to return exactly 1 when it fails,
>only non-zero.
>
>
>  
>

That should actually be the simple fix for attach to process (function 
win32_attach). Do you see that line in child_continue function? Strange. 
It should be the last line in win32_attach.

'res' as it is indicates success if it's != 0 (it's a Win32 BOOL), 
however the attach to process function should indicate success returning 
0, or -1 if it cannot attach (see server.c 'attach_inferior' function). 
That's what this line does.
Is it necessary to add a source code comment here to explain this?

Or change the line; maybe this line would be clearer:

return res != FALSE? 0:-1;