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

[Evan Huus <eapache@xxxxxxxxx>]

On Sat, Sep 29, 2012 at 2:20 PM, Maynard, Chris
<Christopher.Maynard@xxxxxxxxx> 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.