[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 4e4c7af51e..3cdbf44503 100644
--- a/util/async.c
+++ b/util/async.c
@@ -425,7 +425,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 27/09/2019 à 12:32, 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>

[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..9bd173ecda 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, INT32_MAX);
 }
 
 
-- 
2.21.0

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

[Laurent Vivier]

Le 27/09/2019 à 12:32, 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..9bd173ecda 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, INT32_MAX);
>  }
>  

Perhaps it would be cleaner to have always the same return type for
MIN() with:

    MIN(ms, (int64_t) INT32_MAX)

Anyway:

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

Thanks,
Laurent

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

[Frediano Ziglio]

> 
> Le 27/09/2019 à 12:32, 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..9bd173ecda 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, INT32_MAX);
> >  }
> >  
> 
> Perhaps it would be cleaner to have always the same return type for
> MIN() with:
> 
>     MIN(ms, (int64_t) INT32_MAX)
> 

The (int64_t) cast here is useless, ms is signed, C compilers knows how to
convert and compare properly.
The function returns int and it's used in some cases to fill 32bit integer,
changing the return type to int64_t is not trivial.
But I suppose that here you mean instead that the 2 sides of the ternary
operator used by MIN macro are int64_t and (probably) int32_t. In this case
feel free to add the cast with a 

   return (int) MIN(ms, (int64_t) INT32_MAX);

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

Frediano

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

[Laurent Vivier]

Le 27/09/2019 à 13:57, Frediano Ziglio a écrit :
>>
>> Le 27/09/2019 à 12:32, 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..9bd173ecda 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, INT32_MAX);
>>>  }
>>>  
>>
>> Perhaps it would be cleaner to have always the same return type for
>> MIN() with:
>>
>>     MIN(ms, (int64_t) INT32_MAX)
>>
> 
> The (int64_t) cast here is useless, ms is signed, C compilers knows how to
> convert and compare properly.

I agree.

> The function returns int and it's used in some cases to fill 32bit integer,
> changing the return type to int64_t is not trivial.
> But I suppose that here you mean instead that the 2 sides of the ternary
> operator used by MIN macro are int64_t and (probably) int32_t. In this case

Yes, it's what I mean.

Thanks,
Laurent

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

[Laurent Vivier]

Le 27/09/2019 à 12:32, 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 4e4c7af51e..3cdbf44503 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -425,7 +425,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]

Please re-send your series to qemu-devel@nongnu.org if you want to have
it merged.

Thanks,
Laurent

Le 27/09/2019 à 12:32, 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 4e4c7af51e..3cdbf44503 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -425,7 +425,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
> 

[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