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@xxxxxxxxxxxxx [wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Evan Huus [eapache@xxxxxxxxx]
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@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.
___________________________________________________________________________
--
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.