diff mbox

[RFC,v1,2/4] util/oslib-win32: Remove invalid check

Message ID 6955f1684346baf57b619ca407cd71363027307a.1498607452.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis June 27, 2017, 11:57 p.m. UTC 
There is no way nhandles can be zero in this section so that part of the
if statement will always be false. Let's just remove it to make the code
easier to read.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---

 util/oslib-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé via June 28, 2017, 4:12 a.m. UTC | #1 
On Tue, Jun 27, 2017 at 8:57 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>          /* If we have a timeout, or no handles to poll, be satisfied
>           * with just noticing we have messages waiting.
>           */
> -        if (timeout != 0 || nhandles == 0) {
> +        if (timeout != 0) {
>              return 1;
>          }
>
> --
> 2.11.0
>
>
Fam Zheng June 29, 2017, 9:25 a.m. UTC | #2 
On Tue, 06/27 16:57, Alistair Francis wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> 
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>          /* If we have a timeout, or no handles to poll, be satisfied
>           * with just noticing we have messages waiting.
>           */
> -        if (timeout != 0 || nhandles == 0) {
> +        if (timeout != 0) {
>              return 1;
>          }
>  
> -- 
> 2.11.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>
Paolo Bonzini June 29, 2017, 1:32 p.m. UTC | #3 
On 28/06/2017 01:57, Alistair Francis wrote:
> There is no way nhandles can be zero in this section so that part of the
> if statement will always be false. Let's just remove it to make the code
> easier to read.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
> 
>  util/oslib-win32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 80e4668935..7ec0f8e083 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>          /* If we have a timeout, or no handles to poll, be satisfied
>           * with just noticing we have messages waiting.
>           */
> -        if (timeout != 0 || nhandles == 0) {
> +        if (timeout != 0) {
>              return 1;
>          }
>  
> 

Hmm, I think it's possible, poll_msgs is true here.

Paolo
Alistair Francis June 29, 2017, 4:37 p.m. UTC | #4 
On Thu, Jun 29, 2017 at 6:32 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 28/06/2017 01:57, Alistair Francis wrote:
>> There is no way nhandles can be zero in this section so that part of the
>> if statement will always be false. Let's just remove it to make the code
>> easier to read.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Acked-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>> ---
>>
>>  util/oslib-win32.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index 80e4668935..7ec0f8e083 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -414,7 +414,7 @@ static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
>>          /* If we have a timeout, or no handles to poll, be satisfied
>>           * with just noticing we have messages waiting.
>>           */
>> -        if (timeout != 0 || nhandles == 0) {
>> +        if (timeout != 0) {
>>              return 1;
>>          }
>>
>>
>
> Hmm, I think it's possible, poll_msgs is true here.

poll_msgs?

If nhandles is 0 then we have already entered an earlier if statement
and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
can't enter this part of the if statement.

Thanks,
Alistair

>
> Paolo
Paolo Bonzini June 30, 2017, 10:37 a.m. UTC | #5 
On 29/06/2017 18:37, Alistair Francis wrote:
>> Hmm, I think it's possible, poll_msgs is true here.
> poll_msgs?
> 
> If nhandles is 0 then we have already entered an earlier if statement
> and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
> can't enter this part of the if statement.

No, that's not correct.  The code is:

    if (poll_msgs) {
        /* Wait for either messages or handles
         * -> Use MsgWaitForMultipleObjectsEx
         */
        ready = MsgWaitForMultipleObjectsEx(nhandles, handles, timeout,
                                            QS_ALLINPUT, MWMO_ALERTABLE);

        if (ready == WAIT_FAILED) {
            gchar *emsg = g_win32_error_message(GetLastError());
            g_warning("MsgWaitForMultipleObjectsEx failed: %s", emsg);
            g_free(emsg);
        }
    } else if (nhandles == 0) {
        /* No handles to wait for, just the timeout */
        if (timeout == INFINITE) {
            ready = WAIT_FAILED;
        } else {
            SleepEx(timeout, TRUE);
            ready = WAIT_TIMEOUT;
        }

You can have poll_msgs == TRUE && nhandles == 0.  This happens for

   GPollFD fds[1] = { .fd = G_WIN32_MSG_HANDLE, .events = G_IO_IN };
   g_poll(fds, 1, timeout);

Thanks,

Paolo
Alistair Francis July 5, 2017, 3:44 p.m. UTC | #6 
On Fri, Jun 30, 2017 at 3:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 29/06/2017 18:37, Alistair Francis wrote:
>>> Hmm, I think it's possible, poll_msgs is true here.
>> poll_msgs?
>>
>> If nhandles is 0 then we have already entered an earlier if statement
>> and set ready to either WAIT_FAILED or WAIT_TIMEOUT in which case we
>> can't enter this part of the if statement.
>
> No, that's not correct.  The code is:
>
>     if (poll_msgs) {
>         /* Wait for either messages or handles
>          * -> Use MsgWaitForMultipleObjectsEx
>          */
>         ready = MsgWaitForMultipleObjectsEx(nhandles, handles, timeout,
>                                             QS_ALLINPUT, MWMO_ALERTABLE);
>
>         if (ready == WAIT_FAILED) {
>             gchar *emsg = g_win32_error_message(GetLastError());
>             g_warning("MsgWaitForMultipleObjectsEx failed: %s", emsg);
>             g_free(emsg);
>         }
>     } else if (nhandles == 0) {
>         /* No handles to wait for, just the timeout */
>         if (timeout == INFINITE) {
>             ready = WAIT_FAILED;
>         } else {
>             SleepEx(timeout, TRUE);
>             ready = WAIT_TIMEOUT;
>         }
>
> You can have poll_msgs == TRUE && nhandles == 0.  This happens for
>
>    GPollFD fds[1] = { .fd = G_WIN32_MSG_HANDLE, .events = G_IO_IN };
>    g_poll(fds, 1, timeout);

Ah. Yeah good point.

Ok, I'll respin the series without this patch then.

Thanks,
Alistair

>
> Thanks,
>
> Paolo
>
diff mbox

Patch

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 80e4668935..7ec0f8e083 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -414,7 +414,7 @@  static int poll_rest(gboolean poll_msgs, HANDLE *handles, gint nhandles,
         /* If we have a timeout, or no handles to poll, be satisfied
          * with just noticing we have messages waiting.
          */
-        if (timeout != 0 || nhandles == 0) {
+        if (timeout != 0) {
             return 1;
         }