Re: [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c

["Maynard, Chris" <Christopher.Maynard () GTECH COM>]

Apologies, I meant to write:

 if (!result || avail > 0 || !result1 || childstatus != STILL_ACTIVE) {

I think I am too accustomed to *nix return values where 0 typically means success, and so I read the documentation too 
quickly thinking the same applied here.

... but since I'm not entirely sure what is needed here, I'll leave it to you (or someone else) to make an appropriate 
change.  I just wanted the builds to succeed again so I could continue with some other stuff I was working on.  And 
don't worry about the buildbot failing; it happens to us all, even to the best among us (and no, that would not be me 
in that category).

- Chris

________________________________________
From: wireshark-dev-bounces () wireshark org [wireshark-dev-bounces () wireshark org] On Behalf Of Evan Huus [eapache 
() gmail com]
Sent: Saturday, September 29, 2012 3:41 PM
To: Developer support list for Wireshark
Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c

On Sat, Sep 29, 2012 at 2:20 PM, Maynard, Chris
<Christopher.Maynard () gtech com> wrote:
> Good point Bill.  I hadn't actually looked too deeply here; I just wanted to appease the buildbot.  See: 
> http://buildbot.wireshark.org/trunk/builders/Windows-XP-x86/builds/2646/steps/nmake%20all/logs/stdio
>
> So maybe a minimal patch such as this is what is needed?  Maybe that was the original intent?
> - Chris
>
> Index: ui/gtk/gui_utils.c
> ===================================================================
> --- ui/gtk/gui_utils.c  (revision 45212)
> +++ ui/gtk/gui_utils.c  (working copy)
> @@ -694,7 +694,7 @@
>  {
>      HANDLE        handle;
>      DWORD         avail      = 0;
> -    gboolean      result;
> +    gboolean      result, result1;
>      DWORD         childstatus;
>      pipe_input_t *pipe_input = data;
>      gint          iterations = 0;
> @@ -710,12 +710,13 @@
>          result = PeekNamedPipe(handle, NULL, 0, NULL, &avail, NULL);
>          /* Get the child process exit status */
> -       GetExitCodeProcess((HANDLE)*(pipe_input->child_process), &childstatus);
> +        result1 = GetExitCodeProcess((HANDLE)*(pipe_input->child_process),
> +                                     &childstatus);
>          /* If the Peek returned an error, or there are bytes to be read
>             or the childwatcher thread has terminated then call the normal
>             callback */
> -        if (!result || avail > 0 || childstatus != STILL_ACTIVE) {
> +        if (!result || avail > 0 || result1 || childstatus != STILL_ACTIVE) {
>              /*g_log(NULL, G_LOG_LEVEL_DEBUG, "pipe_timer_cb: data avail");*/
>
>
> - Chris

Also not a Windows programmer, but this doesn't make sense to me. In
the case that the child process is still running and sane, but doesn't
have any packets then I expect GetExitCodeProcess to return TRUE and
set childstatus to STILL_ACTIVE, in which case there's no reason for
us to do the callback (which I believe we would do given this patch).

My understanding is that we should be g_warning if GetExitCodeProcess
returns FALSE, as that seems to indicate an error internal to win32?
In which case it should probably get its very own if statement before
the existing if(!result...

Evan

P.S. Apologies for the original build-breakage. At least something
interesting seems to have come of it.
___________________________________________________________________________
-- 
CONFIDENTIALITY NOTICE: The information contained in this email message is intended only for use of the intended 
recipient. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, 
distribution or copying of this communication is strictly prohibited. If you have received this communication in error, 
please immediately delete it from your system and notify the sender by replying to this email.  Thank you.
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe