[libcacard,1/2] vscclient: Catch write errors

Submitted by Jason Andryuk on July 24, 2018, 6:34 p.m.

Details

Message ID 20180724183459.18965-2-jandryuk@gmail.com
State New
Headers show
Series "Error handling improvements" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Jason Andryuk July 24, 2018, 6:34 p.m.
The GIOStatus return value indicates errors, so catch it and check it to
know if we should return an error.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Truth be told, I can't remember exactly why I had to write this.  Could
status be G_IO_STATUS_ERROR, but err still be NULL?  Since I added
debugging output with those value, it maybe the case.

 src/vscclient.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/vscclient.c b/src/vscclient.c
index fa60162..56e2ced 100644
--- a/src/vscclient.c
+++ b/src/vscclient.c
@@ -69,12 +69,21 @@  do_socket_send(GIOChannel *source,
 {
     gsize bw;
     GError *err = NULL;
+    GIOStatus status;
 
     g_return_val_if_fail(socket_to_send->len != 0, FALSE);
     g_return_val_if_fail(condition & G_IO_OUT, FALSE);
 
-    g_io_channel_write_chars(channel_socket,
+    status = g_io_channel_write_chars(channel_socket,
         (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
+    if (verbose) {
+        printf("status: %d bytes written: %d err: %p\n", status, bw, err);
+    }
+
+    if (status == G_IO_STATUS_ERROR) {
+        return FALSE;
+    }
+
     if (err != NULL) {
         g_error("Error while sending socket %s", err->message);
         return FALSE;

Comments

> 
> The GIOStatus return value indicates errors, so catch it and check it to
> know if we should return an error.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> Truth be told, I can't remember exactly why I had to write this.  Could
> status be G_IO_STATUS_ERROR, but err still be NULL?  Since I added
> debugging output with those value, it maybe the case.
> 

Looking at GLib source code there are different paths leading to
status == G_IO_STATUS_ERROR and err == NULL.
For instance trying to mix read and write and encoding.
Or for some parameter checks.

>  src/vscclient.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/vscclient.c b/src/vscclient.c
> index fa60162..56e2ced 100644
> --- a/src/vscclient.c
> +++ b/src/vscclient.c
> @@ -69,12 +69,21 @@ do_socket_send(GIOChannel *source,
>  {
>      gsize bw;
>      GError *err = NULL;
> +    GIOStatus status;
>  
>      g_return_val_if_fail(socket_to_send->len != 0, FALSE);
>      g_return_val_if_fail(condition & G_IO_OUT, FALSE);
>  
> -    g_io_channel_write_chars(channel_socket,
> +    status = g_io_channel_write_chars(channel_socket,
>          (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
> +    if (verbose) {
> +        printf("status: %d bytes written: %d err: %p\n", status, bw, err);

This looks like more a debug thing than a verbose one (don't know
much about libcacard).

> +    }
> +
> +    if (status == G_IO_STATUS_ERROR) {

No logs?

> +        return FALSE;
> +    }
> +
>      if (err != NULL) {
>          g_error("Error while sending socket %s", err->message);
>          return FALSE;

Frediano
On Wed, Jul 25, 2018 at 5:53 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>
>> The GIOStatus return value indicates errors, so catch it and check it to
>> know if we should return an error.
>>
>> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>> ---
>> Truth be told, I can't remember exactly why I had to write this.  Could
>> status be G_IO_STATUS_ERROR, but err still be NULL?  Since I added
>> debugging output with those value, it maybe the case.
>>
>
> Looking at GLib source code there are different paths leading to
> status == G_IO_STATUS_ERROR and err == NULL.
> For instance trying to mix read and write and encoding.
> Or for some parameter checks.
>
>>  src/vscclient.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/vscclient.c b/src/vscclient.c
>> index fa60162..56e2ced 100644
>> --- a/src/vscclient.c
>> +++ b/src/vscclient.c
>> @@ -69,12 +69,21 @@ do_socket_send(GIOChannel *source,
>>  {
>>      gsize bw;
>>      GError *err = NULL;
>> +    GIOStatus status;
>>
>>      g_return_val_if_fail(socket_to_send->len != 0, FALSE);
>>      g_return_val_if_fail(condition & G_IO_OUT, FALSE);
>>
>> -    g_io_channel_write_chars(channel_socket,
>> +    status = g_io_channel_write_chars(channel_socket,
>>          (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
>> +    if (verbose) {
>> +        printf("status: %d bytes written: %d err: %p\n", status, bw, err);
>
> This looks like more a debug thing than a verbose one (don't know
> much about libcacard).

vscclient doesn't have a debug option, but it does have some verbose >
10 checks for debug-like messages.  I can bump it up to that.

>> +    }
>> +
>> +    if (status == G_IO_STATUS_ERROR) {
>
> No logs?

I can add one.  Also, I'm testing out re-ordering this test after the
err != NULL check.  In either case we'll abort from the function, but
if err is set, then we can print a more informative message.

>> +        return FALSE;
>> +    }
>> +
>>      if (err != NULL) {
>>          g_error("Error while sending socket %s", err->message);
>>          return FALSE;
>
> Frediano

Thanks for the review.

Jason
> 
> On Wed, Jul 25, 2018 at 5:53 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> >>
> >> The GIOStatus return value indicates errors, so catch it and check it to
> >> know if we should return an error.
> >>
> >> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> >> ---
> >> Truth be told, I can't remember exactly why I had to write this.  Could
> >> status be G_IO_STATUS_ERROR, but err still be NULL?  Since I added
> >> debugging output with those value, it maybe the case.
> >>
> >
> > Looking at GLib source code there are different paths leading to
> > status == G_IO_STATUS_ERROR and err == NULL.
> > For instance trying to mix read and write and encoding.
> > Or for some parameter checks.
> >
> >>  src/vscclient.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/vscclient.c b/src/vscclient.c
> >> index fa60162..56e2ced 100644
> >> --- a/src/vscclient.c
> >> +++ b/src/vscclient.c
> >> @@ -69,12 +69,21 @@ do_socket_send(GIOChannel *source,
> >>  {
> >>      gsize bw;
> >>      GError *err = NULL;
> >> +    GIOStatus status;
> >>
> >>      g_return_val_if_fail(socket_to_send->len != 0, FALSE);
> >>      g_return_val_if_fail(condition & G_IO_OUT, FALSE);
> >>
> >> -    g_io_channel_write_chars(channel_socket,
> >> +    status = g_io_channel_write_chars(channel_socket,
> >>          (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
> >> +    if (verbose) {
> >> +        printf("status: %d bytes written: %d err: %p\n", status, bw,
> >> err);
> >
> > This looks like more a debug thing than a verbose one (don't know
> > much about libcacard).
> 
> vscclient doesn't have a debug option, but it does have some verbose >
> 10 checks for debug-like messages.  I can bump it up to that.
> 

Looking better libcacard is using GLib, maybe a g_debug would be better.

> >> +    }
> >> +
> >> +    if (status == G_IO_STATUS_ERROR) {
> >
> > No logs?
> 
> I can add one.  Also, I'm testing out re-ordering this test after the
> err != NULL check.  In either case we'll abort from the function, but
> if err is set, then we can print a more informative message.
> 

Make sense.

> >> +        return FALSE;
> >> +    }
> >> +
> >>      if (err != NULL) {
> >>          g_error("Error while sending socket %s", err->message);

So any error here with a message is terminating the program.

> >>          return FALSE;

And this is never executed.

> >
> > Frediano
> 
> Thanks for the review.
> 
> Jason
>
On Fri, Jul 27, 2018 at 4:07 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>
>> On Wed, Jul 25, 2018 at 5:53 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> >>
>> >> The GIOStatus return value indicates errors, so catch it and check it to
>> >> know if we should return an error.
>> >>
>> >> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>> >> ---
>> >> Truth be told, I can't remember exactly why I had to write this.  Could
>> >> status be G_IO_STATUS_ERROR, but err still be NULL?  Since I added
>> >> debugging output with those value, it maybe the case.
>> >>
>> >
>> > Looking at GLib source code there are different paths leading to
>> > status == G_IO_STATUS_ERROR and err == NULL.
>> > For instance trying to mix read and write and encoding.
>> > Or for some parameter checks.
>> >
>> >>  src/vscclient.c | 11 ++++++++++-
>> >>  1 file changed, 10 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/src/vscclient.c b/src/vscclient.c
>> >> index fa60162..56e2ced 100644
>> >> --- a/src/vscclient.c
>> >> +++ b/src/vscclient.c
>> >> @@ -69,12 +69,21 @@ do_socket_send(GIOChannel *source,
>> >>  {
>> >>      gsize bw;
>> >>      GError *err = NULL;
>> >> +    GIOStatus status;
>> >>
>> >>      g_return_val_if_fail(socket_to_send->len != 0, FALSE);
>> >>      g_return_val_if_fail(condition & G_IO_OUT, FALSE);
>> >>
>> >> -    g_io_channel_write_chars(channel_socket,
>> >> +    status = g_io_channel_write_chars(channel_socket,
>> >>          (gchar *)socket_to_send->data, socket_to_send->len, &bw, &err);
>> >> +    if (verbose) {
>> >> +        printf("status: %d bytes written: %d err: %p\n", status, bw,
>> >> err);
>> >
>> > This looks like more a debug thing than a verbose one (don't know
>> > much about libcacard).
>>
>> vscclient doesn't have a debug option, but it does have some verbose >
>> 10 checks for debug-like messages.  I can bump it up to that.
>>
>
> Looking better libcacard is using GLib, maybe a g_debug would be better.
>
>> >> +    }
>> >> +
>> >> +    if (status == G_IO_STATUS_ERROR) {
>> >
>> > No logs?
>>
>> I can add one.  Also, I'm testing out re-ordering this test after the
>> err != NULL check.  In either case we'll abort from the function, but
>> if err is set, then we can print a more informative message.
>>
>
> Make sense.
>
>> >> +        return FALSE;
>> >> +    }
>> >> +
>> >>      if (err != NULL) {
>> >>          g_error("Error while sending socket %s", err->message);
>
> So any error here with a message is terminating the program.
>
>> >>          return FALSE;
>
> And this is never executed.

Sorry for the slow response.  I'll have to look at this again.  I
think I wanted to avoid terminating the program, so the skipping of
g_error was important for that.  But I can't recall, so I'll have to
re-investigate.

Regards,
Jason