[PATCH 1/3] util/async: avoid useless cast

[PATCH 1/3] util/async: avoid useless cast

[Frediano Ziglio]

event_notifier_dummy_cb is already compatible with EventNotifierHandler.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 util/async.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/async.c b/util/async.c
index ca83e32c7f..b1fa5319e5 100644
--- a/util/async.c
+++ b/util/async.c
@@ -429,7 +429,6 @@ AioContext *aio_context_new(Error **errp)
 
     aio_set_event_notifier(ctx, &ctx->notifier,
                            false,
-                           (EventNotifierHandler *)
                            event_notifier_dummy_cb,
                            event_notifier_poll);
 #ifdef CONFIG_LINUX_AIO
-- 
2.21.0

[PATCH 2/3] event_notifier: avoid dandling file descriptor in event_notifier_cleanup

[Frediano Ziglio]

If rfd is equal to wfd the file descriptor is closed but
rfd will still have the closed value.
The EventNotifier structure should not be used again after calling
event_notifier_cleanup or should be initialized again but make
sure to not have dandling file descriptors around.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 util/event_notifier-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
index 73c4046b58..00d93204f9 100644
--- a/util/event_notifier-posix.c
+++ b/util/event_notifier-posix.c
@@ -80,8 +80,8 @@ void event_notifier_cleanup(EventNotifier *e)
 {
     if (e->rfd != e->wfd) {
         close(e->rfd);
-        e->rfd = -1;
     }
+    e->rfd = -1;
     close(e->wfd);
     e->wfd = -1;
 }
-- 
2.21.0

Re: [PATCH 2/3] event_notifier: avoid dandling file descriptor in event_notifier_cleanup

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> If rfd is equal to wfd the file descriptor is closed but
> rfd will still have the closed value.
> The EventNotifier structure should not be used again after calling
> event_notifier_cleanup or should be initialized again but make
> sure to not have dandling file descriptors around.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/event_notifier-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
> index 73c4046b58..00d93204f9 100644
> --- a/util/event_notifier-posix.c
> +++ b/util/event_notifier-posix.c
> @@ -80,8 +80,8 @@ void event_notifier_cleanup(EventNotifier *e)
>  {
>      if (e->rfd != e->wfd) {
>          close(e->rfd);
> -        e->rfd = -1;
>      }
> +    e->rfd = -1;
>      close(e->wfd);
>      e->wfd = -1;
>  }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 2/3] event_notifier: avoid dandling file descriptor in event_notifier_cleanup

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> If rfd is equal to wfd the file descriptor is closed but
> rfd will still have the closed value.
> The EventNotifier structure should not be used again after calling
> event_notifier_cleanup or should be initialized again but make
> sure to not have dandling file descriptors around.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/event_notifier-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
> index 73c4046b58..00d93204f9 100644
> --- a/util/event_notifier-posix.c
> +++ b/util/event_notifier-posix.c
> @@ -80,8 +80,8 @@ void event_notifier_cleanup(EventNotifier *e)
>  {
>      if (e->rfd != e->wfd) {
>          close(e->rfd);
> -        e->rfd = -1;
>      }
> +    e->rfd = -1;
>      close(e->wfd);
>      e->wfd = -1;
>  }
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

[PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 util/qemu-timer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index d428fec567..094a20a05a 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
     ms = DIV_ROUND_UP(ns, SCALE_MS);
 
     /* To avoid overflow problems, limit this to 2^31, i.e. approx 25 days */
-    if (ms > (int64_t) INT32_MAX) {
-        ms = INT32_MAX;
-    }
-
-    return (int) ms;
+    return (int) MIN(ms, (int64_t) INT32_MAX);
 }
 
 
-- 
2.21.0

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/qemu-timer.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index d428fec567..094a20a05a 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>      ms = DIV_ROUND_UP(ns, SCALE_MS);
>  
>      /* To avoid overflow problems, limit this to 2^31, i.e. approx 25 days */
> -    if (ms > (int64_t) INT32_MAX) {
> -        ms = INT32_MAX;
> -    }
> -
> -    return (int) ms;
> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>  }
>  
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Eric Blake]

On 10/23/19 8:42 AM, Laurent Vivier wrote:
> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>> ---
>>   util/qemu-timer.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
>> index d428fec567..094a20a05a 100644
>> --- a/util/qemu-timer.c
>> +++ b/util/qemu-timer.c
>> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>>       ms = DIV_ROUND_UP(ns, SCALE_MS);
>>   
>>       /* To avoid overflow problems, limit this to 2^31, i.e. approx 25 days */
>> -    if (ms > (int64_t) INT32_MAX) {
>> -        ms = INT32_MAX;
>> -    }
>> -
>> -    return (int) ms;
>> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>>   }

Why so many casts?  It should also work as:

return MIN(ms, INT32_MAX);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Frediano Ziglio]

> 
> On 10/23/19 8:42 AM, Laurent Vivier wrote:
> > Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> >> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> >> ---
> >>   util/qemu-timer.c | 6 +-----
> >>   1 file changed, 1 insertion(+), 5 deletions(-)
> >>
> >> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> >> index d428fec567..094a20a05a 100644
> >> --- a/util/qemu-timer.c
> >> +++ b/util/qemu-timer.c
> >> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
> >>       ms = DIV_ROUND_UP(ns, SCALE_MS);
> >>   
> >>       /* To avoid overflow problems, limit this to 2^31, i.e. approx 25
> >>       days */
> >> -    if (ms > (int64_t) INT32_MAX) {
> >> -        ms = INT32_MAX;
> >> -    }
> >> -
> >> -    return (int) ms;
> >> +    return (int) MIN(ms, (int64_t) INT32_MAX);
> >>   }
> 
> Why so many casts?  It should also work as:
> 
> return MIN(ms, INT32_MAX);
> 

This was former version. Laurent pointed out that MIN macro
is using ternary operator which is expected to find the same time
on second and third part so the cast inside the MIN macro.
The cast before MIN was kept from previous code.

Frediano

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Eric Blake]

On 10/23/19 9:15 AM, Frediano Ziglio wrote:
>>
>> On 10/23/19 8:42 AM, Laurent Vivier wrote:
>>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>>> ---
>>>>    util/qemu-timer.c | 6 +-----
>>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
>>>> index d428fec567..094a20a05a 100644
>>>> --- a/util/qemu-timer.c
>>>> +++ b/util/qemu-timer.c
>>>> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>>>>        ms = DIV_ROUND_UP(ns, SCALE_MS);
>>>>    
>>>>        /* To avoid overflow problems, limit this to 2^31, i.e. approx 25
>>>>        days */
>>>> -    if (ms > (int64_t) INT32_MAX) {
>>>> -        ms = INT32_MAX;
>>>> -    }
>>>> -
>>>> -    return (int) ms;
>>>> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>>>>    }
>>
>> Why so many casts?  It should also work as:
>>
>> return MIN(ms, INT32_MAX);
>>
> 
> This was former version. Laurent pointed out that MIN macro
> is using ternary operator which is expected to find the same time
> on second and third part so the cast inside the MIN macro.
> The cast before MIN was kept from previous code.

The C rules for ternary type promotion guarantee that the MIN macro 
produces the correct type without the cast ('cond ? int64_t : int32_t' 
produces int64_t).  I agree that the (int) cast was pre-existing, but as 
long as we are cleaning up useless casts, we might as well clean up both.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 23/10/2019 à 16:23, Eric Blake a écrit :
> On 10/23/19 9:15 AM, Frediano Ziglio wrote:
>>>
>>> On 10/23/19 8:42 AM, Laurent Vivier wrote:
>>>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>>>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>>>> ---
>>>>>    util/qemu-timer.c | 6 +-----
>>>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
>>>>> index d428fec567..094a20a05a 100644
>>>>> --- a/util/qemu-timer.c
>>>>> +++ b/util/qemu-timer.c
>>>>> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>>>>>        ms = DIV_ROUND_UP(ns, SCALE_MS);
>>>>>           /* To avoid overflow problems, limit this to 2^31, i.e.
>>>>> approx 25
>>>>>        days */
>>>>> -    if (ms > (int64_t) INT32_MAX) {
>>>>> -        ms = INT32_MAX;
>>>>> -    }
>>>>> -
>>>>> -    return (int) ms;
>>>>> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>>>>>    }
>>>
>>> Why so many casts?  It should also work as:
>>>
>>> return MIN(ms, INT32_MAX);
>>>
>>
>> This was former version. Laurent pointed out that MIN macro
>> is using ternary operator which is expected to find the same time
>> on second and third part so the cast inside the MIN macro.
>> The cast before MIN was kept from previous code.
> 
> The C rules for ternary type promotion guarantee that the MIN macro
> produces the correct type without the cast ('cond ? int64_t : int32_t'
> produces int64_t). 

gdb seems to disagree with that:

(gdb) whatis l
type = long long
(gdb) whatis i
type = int
(gdb) whatis 1 ? l : i
type = long long
(gdb) whatis 0 ? l : i
type = int

It's on what I based my previous comment.

But both approaches are correct in the end.

Thanks,
Laurent

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Eric Blake]

On 10/23/19 9:45 AM, Laurent Vivier wrote:

>> The C rules for ternary type promotion guarantee that the MIN macro
>> produces the correct type without the cast ('cond ? int64_t : int32_t'
>> produces int64_t).
> 
> gdb seems to disagree with that:
> 
> (gdb) whatis l
> type = long long
> (gdb) whatis i
> type = int
> (gdb) whatis 1 ? l : i
> type = long long
> (gdb) whatis 0 ? l : i
> type = int

It looks like you've found a gdb bug.

C99 6.5.15 p5 states:
"If both the second and third operands have arithmetic type, the result 
type that would be determined by the usual arithmetic conversions, were 
they applied to those two operands, is the type of the result."

and the usual arithmetic conversion of 'long long OP int' is 'long 
long', per 6.3.1.8.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 23/10/2019 à 16:51, Eric Blake a écrit :
> On 10/23/19 9:45 AM, Laurent Vivier wrote:
> 
>>> The C rules for ternary type promotion guarantee that the MIN macro
>>> produces the correct type without the cast ('cond ? int64_t : int32_t'
>>> produces int64_t).
>>
>> gdb seems to disagree with that:
>>
>> (gdb) whatis l
>> type = long long
>> (gdb) whatis i
>> type = int
>> (gdb) whatis 1 ? l : i
>> type = long long
>> (gdb) whatis 0 ? l : i
>> type = int
> 
> It looks like you've found a gdb bug.
> 
> C99 6.5.15 p5 states:
> "If both the second and third operands have arithmetic type, the result
> type that would be determined by the usual arithmetic conversions, were
> they applied to those two operands, is the type of the result."
> 
> and the usual arithmetic conversion of 'long long OP int' is 'long
> long', per 6.3.1.8.
> 

Ok, you're right [1]

Frediano, sorry for my misleading comment.

Thanks,
Laurent

[1] and gcc agrees:

int main(void)
{
        long long l;
        int i;
        typeof(0 ? l : i) f;
        typeof(1 ? l : i) t;
}

(gdb) whatis l
type = long long
(gdb) whatis i
type = int
(gdb) whatis f
type = long long
(gdb) whatis t
type = long long

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/qemu-timer.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index d428fec567..094a20a05a 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -322,11 +322,7 @@ int qemu_timeout_ns_to_ms(int64_t ns)
>      ms = DIV_ROUND_UP(ns, SCALE_MS);
>  
>      /* To avoid overflow problems, limit this to 2^31, i.e. approx 25 days */
> -    if (ms > (int64_t) INT32_MAX) {
> -        ms = INT32_MAX;
> -    }
> -
> -    return (int) ms;
> +    return (int) MIN(ms, (int64_t) INT32_MAX);
>  }
>  
>  
> 

Applied to my trivial-patches branch.

I've updated the patch to remove the two useless casts.

Eric, if you want to add your R-b, I can add it to the queued patch.

Thanks,
Laurent

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Eric Blake]

On 10/24/19 12:31 PM, Laurent Vivier wrote:
> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>> ---
>>   util/qemu-timer.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>

>>
> 
> Applied to my trivial-patches branch.
> 
> I've updated the patch to remove the two useless casts.
> 
> Eric, if you want to add your R-b, I can add it to the queued patch.

I don't see it queued on https://github.com/vivier/qemu/branches yet, 
but if removing the two casts is the only difference from the original:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Laurent Vivier]

Le 24/10/2019 à 20:03, Eric Blake a écrit :
> On 10/24/19 12:31 PM, Laurent Vivier wrote:
>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>> ---
>>>   util/qemu-timer.c | 6 +-----
>>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>>
> 
>>>
>>
>> Applied to my trivial-patches branch.
>>
>> I've updated the patch to remove the two useless casts.
>>
>> Eric, if you want to add your R-b, I can add it to the queued patch.
> 
> I don't see it queued on https://github.com/vivier/qemu/branches yet,

Sorry, forgot to push it.

> but if removing the two casts is the only difference from the original:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Added and pushed (branch trivial-patches), you can check.

Thanks,
Laurent

Re: [PATCH 3/3] qemu-timer: reuse MIN macro in qemu_timeout_ns_to_ms

[Eric Blake]

On 10/24/19 1:11 PM, Laurent Vivier wrote:
> Le 24/10/2019 à 20:03, Eric Blake a écrit :
>> On 10/24/19 12:31 PM, Laurent Vivier wrote:
>>> Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
>>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>>> ---
>>>>    util/qemu-timer.c | 6 +-----
>>>>    1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>
>>>>
>>>
>>> Applied to my trivial-patches branch.
>>>
>>> I've updated the patch to remove the two useless casts.
>>>
>>> Eric, if you want to add your R-b, I can add it to the queued patch.
>>
>> I don't see it queued on https://github.com/vivier/qemu/branches yet,
> 
> Sorry, forgot to push it.
> 
>> but if removing the two casts is the only difference from the original:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> 
> Added and pushed (branch trivial-patches), you can check.

Looks good.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/3] util/async: avoid useless cast

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> event_notifier_dummy_cb is already compatible with EventNotifierHandler.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/async.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/util/async.c b/util/async.c
> index ca83e32c7f..b1fa5319e5 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -429,7 +429,6 @@ AioContext *aio_context_new(Error **errp)
>  
>      aio_set_event_notifier(ctx, &ctx->notifier,
>                             false,
> -                           (EventNotifierHandler *)
>                             event_notifier_dummy_cb,
>                             event_notifier_poll);
>  #ifdef CONFIG_LINUX_AIO
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 1/3] util/async: avoid useless cast

[Laurent Vivier]

Le 23/10/2019 à 14:26, Frediano Ziglio a écrit :
> event_notifier_dummy_cb is already compatible with EventNotifierHandler.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  util/async.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/util/async.c b/util/async.c
> index ca83e32c7f..b1fa5319e5 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -429,7 +429,6 @@ AioContext *aio_context_new(Error **errp)
>  
>      aio_set_event_notifier(ctx, &ctx->notifier,
>                             false,
> -                           (EventNotifierHandler *)
>                             event_notifier_dummy_cb,
>                             event_notifier_poll);
>  #ifdef CONFIG_LINUX_AIO
> 

Applied to my trivial-patches branch.

Thanks,
Laurent