[Spice-devel] [PATCH spice-gtk 2/3] usb-device-manager: Don't show an error dialog on a cancelled device open

[Hans de Goede]

Hi,

On 11/21/2011 12:09 AM, Marc-André Lureau wrote:
> On Sat, Nov 19, 2011 at 4:29 PM, Hans de Goede<hdegoede at redhat.com>  wrote:
>> If for example the user plugs in a new device, then gets the policykit agent
>> authentication dialog and then unplugs the device, spice-gtk will cancel
>> the acl-helper request, which in turn will dismiss the policykit agent
>> authentication dialog. Which is all a nice and smooth user experience,
>> except that when this happens spice-gtk throws a dialog with an error
>
> s/spice-gtk/spicy
>

Agreed, will fix in the next revision (which I'll do when you respond to
my comments to your review of the 1st patch in this set).

>> that the open was cancelled. Since a cancel usually is done deliberately
>> (such as on the user unpluging the device) no error dialog should be thrown
>> for it.
>
> But then the client looses track of this action and error. If we
> imagine a special client asking you to plug a smartcard reader, and
> the user unplugged it immediately we may still want to show an
> appropriate error dialog, such as keep the reader plugged (perhaps the
> example isn't well chosen)
>
> Anyway, it's easy to filter an error from the client side, and I think
> it could be interesting for the client to decide what to do / to show
> in such situation.
>
>>
>> Signed-off-by: Hans de Goede<hdegoede at redhat.com>
>> ---
>>   gtk/usb-device-manager.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
>> index b1d6c95..24f9ad1 100644
>> --- a/gtk/usb-device-manager.c
>> +++ b/gtk/usb-device-manager.c
>> @@ -432,15 +432,17 @@ static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
>>      GError *err = NULL;
>>
>>      spice_usb_device_manager_connect_device_finish(self, res,&err);
>> -    if (err) {
>> +    if (err&&  !(err->domain == G_IO_ERROR&&
>> +                 err->code   == G_IO_ERROR_CANCELLED)) {
>
> So perhaps this filtering should be done in spicy.

Sounds good, will modify in next revision.

>
>>          gchar *desc = spice_usb_device_get_description(device);
>>          g_prefix_error(&err, "Could not auto-redirect %s: ", desc);
>>          g_free(desc);
>>
>>          g_warning("%s", err->message);
>>          g_signal_emit(self, signals[AUTO_CONNECT_FAILED], 0, device, err);
>> -        g_error_free(err);
>>      }
>> +    if (err)
>> +        g_error_free(err);
>
> In general, we should favor g_clear_error(), which is also NULL tolerant.

Hmm, I checked the docs before doing things this way and the docs say:

void                g_clear_error                       (GError **err);

If err is NULL, does nothing. If err is non-NULL, calls g_error_free() on *err and sets *err to NULL.

So this means that **err may by NULL, but *err will get passed to
g_error_free(), which is not NULL tolerant (I did check that).

I also just checked the actual code for g_clear_error and the documentation
is correct, so using g_clear_error(&err) instead of:
     if (err)
         g_error_free(err);
Will cause g_error_free to get called with a NULL, which it does not like.

Regards,

Hans