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

[PATCH 07/12] ui/spice-core: Clean up error reporting

Posted by Markus Armbruster 4 months, 1 week ago

watch_add() reports _open_osfhandle() failure with
error_setg(&error_warn, ...).

I'm not familiar with Spice, so I can't say whether it will work after
such a 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?

Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
...) are.  Replace by warn_report().

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/spice-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 5992f9daec..97bdd171cd 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -132,7 +132,8 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
 #ifdef WIN32
     fd = _open_osfhandle(fd, _O_BINARY);
     if (fd < 0) {
-        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET");
+        warn_report("Couldn't associate a FD with the SOCKET: %s"
+                    g_win32_error_message(WSAGetLastError()));
         return NULL;
     }
 #endif
-- 
2.49.0

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

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

On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
> watch_add() reports _open_osfhandle() failure with
> error_setg(&error_warn, ...).
> 
> I'm not familiar with Spice, so I can't say whether it will work after
> such a 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?

If watch_add fails, spice is dead. Eventually the NULL returned from
watch_add will bubble up to the spice_server_init function which will
return non-zero and QEMU will do

        error_report("failed to initialize spice server");
        exit(1);

The warning in watch_add gives a better chance of understanding
what failed than this generic error_report() does.

> 
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...) are.  Replace by warn_report().
> 
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/spice-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 5992f9daec..97bdd171cd 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -132,7 +132,8 @@ static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *
>  #ifdef WIN32
>      fd = _open_osfhandle(fd, _O_BINARY);
>      if (fd < 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't associate a FD with the SOCKET");
> +        warn_report("Couldn't associate a FD with the SOCKET: %s"
> +                    g_win32_error_message(WSAGetLastError()));
>          return NULL;
>      }
>  #endif


> -- 
> 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 07/12] ui/spice-core: Clean up error reporting

Posted by Markus Armbruster 3 months ago

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

> On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
>> watch_add() reports _open_osfhandle() failure with
>> error_setg(&error_warn, ...).
>> 
>> I'm not familiar with Spice, so I can't say whether it will work after
>> such a 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?
>
> If watch_add fails, spice is dead. Eventually the NULL returned from
> watch_add will bubble up to the spice_server_init function which will
> return non-zero and QEMU will do
>
>         error_report("failed to initialize spice server");
>         exit(1);
>
> The warning in watch_add gives a better chance of understanding
> what failed than this generic error_report() does.

Would you like me to work this into the commit message?

>> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
>> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
>> ...) are.  Replace by warn_report().
>> 
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  ui/spice-core.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks!

[...]

Re: [PATCH 07/12] ui/spice-core: Clean up error reporting

Posted by Daniel P. Berrangé 3 months ago

On Tue, Sep 09, 2025 at 01:41:51PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:18AM +0200, Markus Armbruster wrote:
> >> watch_add() reports _open_osfhandle() failure with
> >> error_setg(&error_warn, ...).
> >> 
> >> I'm not familiar with Spice, so I can't say whether it will work after
> >> such a 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?
> >
> > If watch_add fails, spice is dead. Eventually the NULL returned from
> > watch_add will bubble up to the spice_server_init function which will
> > return non-zero and QEMU will do
> >
> >         error_report("failed to initialize spice server");
> >         exit(1);
> >
> > The warning in watch_add gives a better chance of understanding
> > what failed than this generic error_report() does.
> 
> Would you like me to work this into the commit message?

Yes, if you're re-spinning might as well add the context info.

> 
> >> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> >> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> >> ...) are.  Replace by warn_report().
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  ui/spice-core.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Thanks!
> 
> [...]
> 

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 07/12] ui/spice-core: Clean up error reporting

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:

> watch_add() reports _open_osfhandle() failure with
> error_setg(&error_warn, ...).
>
> I'm not familiar with Spice, so I can't say whether it will work after
> such a 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?
>
> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> ...) are.  Replace by warn_report().
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>


lgtm, thanks
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  ui/spice-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 5992f9daec..97bdd171cd 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -132,7 +132,8 @@ static SpiceWatch *watch_add(int fd, int event_mask,
> SpiceWatchFunc func, void *
>  #ifdef WIN32
>      fd = _open_osfhandle(fd, _O_BINARY);
>      if (fd < 0) {
> -        error_setg_win32(&error_warn, WSAGetLastError(), "Couldn't
> associate a FD with the SOCKET");
> +        warn_report("Couldn't associate a FD with the SOCKET: %s"
> +                    g_win32_error_message(WSAGetLastError()));
>          return NULL;
>      }
>  #endif
> --
> 2.49.0
>
>