[v1] Error reporting cleanup, a fix, and &error_warn removal

[PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Markus Armbruster 4 months, 1 week ago

qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
passing NULL specifies that errors are to be ignored.  If warnings are
wanted, the caller must pass &error_warn.

I'm not familiar with the calling code, so I can't say whether it will
work after WSAEventSelect() failure.  If it doesn't, then this should
be an error.  If it does, then why bother the user with a warning that
isn't actionable, and likely confusing?

The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
qemu_socket_select() helper).  Before that commit, the error was
ignored, as indicated by passing a null @errp.  Revert to that
behavior.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/oslib-win32.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b7351634ec..136a8fe118 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
 {
     SOCKET s = _get_osfhandle(sockfd);
 
-    if (errp == NULL) {
-        errp = &error_warn;
-    }
-
     if (s == INVALID_SOCKET) {
         error_setg(errp, "invalid socket fd=%d", sockfd);
         return false;
-- 
2.49.0

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Daniel P. Berrangé 3 months, 3 weeks ago

On Fri, Aug 08, 2025 at 10:08:19AM +0200, Markus Armbruster wrote:
> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
> NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
> passing NULL specifies that errors are to be ignored.  If warnings are
> wanted, the caller must pass &error_warn.
> 
> I'm not familiar with the calling code, so I can't say whether it will
> work after WSAEventSelect() failure.  If it doesn't, then this should
> be an error.  If it does, then why bother the user with a warning that
> isn't actionable, and likely confusing?
> 
> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> qemu_socket_select() helper).  Before that commit, the error was
> ignored, as indicated by passing a null @errp.  Revert to that
> behavior.
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/oslib-win32.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ec..136a8fe118 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>  {
>      SOCKET s = _get_osfhandle(sockfd);
>  
> -    if (errp == NULL) {
> -        errp = &error_warn;
> -    }

This makes sense, but I'd want the callers to be using warn_report
instead. Ideally some (but not all) of the callers would propagate
the error, but this isn't practical with the QIOChannel create
watch function usage. I'd want to keep Error *errp on this function
though, and have warn_report as a sign to our future selves that
this is still not ideal.

> -
>      if (s == INVALID_SOCKET) {
>          error_setg(errp, "invalid socket fd=%d", sockfd);
>          return false;
> -- 
> 2.49.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Markus Armbruster 3 months ago

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 10:08:19AM +0200, Markus Armbruster wrote:
>> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
>> NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
>> passing NULL specifies that errors are to be ignored.  If warnings are
>> wanted, the caller must pass &error_warn.
>> 
>> I'm not familiar with the calling code, so I can't say whether it will
>> work after WSAEventSelect() failure.  If it doesn't, then this should
>> be an error.  If it does, then why bother the user with a warning that
>> isn't actionable, and likely confusing?
>> 
>> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
>> qemu_socket_select() helper).  Before that commit, the error was
>> ignored, as indicated by passing a null @errp.  Revert to that
>> behavior.
>> 
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/oslib-win32.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index b7351634ec..136a8fe118 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>>  {
>>      SOCKET s = _get_osfhandle(sockfd);
>>  
>> -    if (errp == NULL) {
>> -        errp = &error_warn;
>> -    }
>
> This makes sense, but I'd want the callers to be using warn_report
> instead. Ideally some (but not all) of the callers would propagate
> the error, but this isn't practical with the QIOChannel create
> watch function usage. I'd want to keep Error *errp on this function
> though, and have warn_report as a sign to our future selves that
> this is still not ideal.

The direct callers are qio_channel_create_socket_watch(),
aio_set_fd_handler().  Callers via qemu_socket_unselect() are
qio_channel_socket_finalize(), qio_channel_socket_close(),
qemu_socket_set_block().

All but qio_channel_socket_close() cannot fail.  Would you like me to
make them pass &error_warn, because warning is less bad than silence
there?

qio_channel_socket_close() can fail, but it ignores
qemu_socket_unselect() failure.  What do you want me to do there?

>> -
>>      if (s == INVALID_SOCKET) {
>>          error_setg(errp, "invalid socket fd=%d", sockfd);
>>          return false;
>> -- 
>> 2.49.0
>> 
>> 
>
> With regards,
> Daniel

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Daniel P. Berrangé 3 months ago

On Tue, Sep 09, 2025 at 01:50:56PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:19AM +0200, Markus Armbruster wrote:
> >> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
> >> NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
> >> passing NULL specifies that errors are to be ignored.  If warnings are
> >> wanted, the caller must pass &error_warn.
> >> 
> >> I'm not familiar with the calling code, so I can't say whether it will
> >> work after WSAEventSelect() failure.  If it doesn't, then this should
> >> be an error.  If it does, then why bother the user with a warning that
> >> isn't actionable, and likely confusing?
> >> 
> >> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> >> qemu_socket_select() helper).  Before that commit, the error was
> >> ignored, as indicated by passing a null @errp.  Revert to that
> >> behavior.
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  util/oslib-win32.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >> 
> >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> >> index b7351634ec..136a8fe118 100644
> >> --- a/util/oslib-win32.c
> >> +++ b/util/oslib-win32.c
> >> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> >>  {
> >>      SOCKET s = _get_osfhandle(sockfd);
> >>  
> >> -    if (errp == NULL) {
> >> -        errp = &error_warn;
> >> -    }
> >
> > This makes sense, but I'd want the callers to be using warn_report
> > instead. Ideally some (but not all) of the callers would propagate
> > the error, but this isn't practical with the QIOChannel create
> > watch function usage. I'd want to keep Error *errp on this function
> > though, and have warn_report as a sign to our future selves that
> > this is still not ideal.
> 
> The direct callers are qio_channel_create_socket_watch(),
> aio_set_fd_handler().  Callers via qemu_socket_unselect() are
> qio_channel_socket_finalize(), qio_channel_socket_close(),
> qemu_socket_set_block().
> 
> All but qio_channel_socket_close() cannot fail.  Would you like me to
> make them pass &error_warn, because warning is less bad than silence
> there?
> 
> qio_channel_socket_close() can fail, but it ignores
> qemu_socket_unselect() failure.  What do you want me to do there?

I think the overriding important thing is that we /must/ try to
close(), and if close() succeeds claim the whole qio_channel_socket_close
was successful. So I guess I'd say that &error_warn should be passed
from all callers.

> 
> >> -
> >>      if (s == INVALID_SOCKET) {
> >>          error_setg(errp, "invalid socket fd=%d", sockfd);
> >>          return false;
> >> -- 
> >> 2.49.0
> >> 
> >> 
> >
> > With regards,
> > Daniel
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Marc-André Lureau 4 months, 1 week ago

Hi

On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:

> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
> NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
> passing NULL specifies that errors are to be ignored.  If warnings are
> wanted, the caller must pass &error_warn.
>
> I'm not familiar with the calling code, so I can't say whether it will
> work after WSAEventSelect() failure.  If it doesn't, then this should
> be an error.  If it does, then why bother the user with a warning that
> isn't actionable, and likely confusing?
>
> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> qemu_socket_select() helper).  Before that commit, the error was
> ignored, as indicated by passing a null @errp.  Revert to that
> behavior.
>

Yes, the potential errors before introducing the wrapper were simply
ignored. I think we should fix the users or maybe just report the warning
and drop errp from the wrapper function. wdyt?


>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

---
>  util/oslib-win32.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ec..136a8fe118 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT
> hEventObject,
>  {
>      SOCKET s = _get_osfhandle(sockfd);
>
> -    if (errp == NULL) {
> -        errp = &error_warn;
> -    }
> -
>      if (s == INVALID_SOCKET) {
>          error_setg(errp, "invalid socket fd=%d", sockfd);
>          return false;
> --
> 2.49.0
>
>

Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

Posted by Markus Armbruster 4 months, 1 week ago

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
>> NULL @errp as &error_warn.  This is wildly inappropriate.  A caller
>> passing NULL specifies that errors are to be ignored.  If warnings are
>> wanted, the caller must pass &error_warn.
>>
>> I'm not familiar with the calling code, so I can't say whether it will
>> work after WSAEventSelect() failure.  If it doesn't, then this should
>> be an error.  If it does, then why bother the user with a warning that
>> isn't actionable, and likely confusing?
>>
>> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
>> qemu_socket_select() helper).  Before that commit, the error was
>> ignored, as indicated by passing a null @errp.  Revert to that
>> behavior.
>>
>
> Yes, the potential errors before introducing the wrapper were simply
> ignored. I think we should fix the users or maybe just report the warning
> and drop errp from the wrapper function. wdyt?

Phil's "[RFC PATCH 0/2] system/win32: Remove unused Error argument in
qemu_socket_[un]select()" does the latter.

I doubt warnings are the right tool here.  I just posted

    Subject: Abuse of warnings for unhandled errors and programming errors
    Message-ID: <87h5yijh3b.fsf@pond.sub.org>

[...]