Re: [PATCH v2 08/15] qga: throw an Error in ga_channel_open()

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6789 bytes --]

Hi

On Thu, May 5, 2022 at 3:41 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Allow for a single point of error reporting, and further refactoring.
>
> This sounds like there is no behavioral change intended.  But it looks
> like there is change; see below.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qga/channel-posix.c | 42 +++++++++++++++++-------------------------
> >  1 file changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> > index a996858e2492..0ce594bc36c2 100644
> > --- a/qga/channel-posix.c
> > +++ b/qga/channel-posix.c
> > @@ -119,8 +119,9 @@ static int ga_channel_client_add(GAChannel *c, int
> fd)
> >  }
> >
> >  static gboolean ga_channel_open(GAChannel *c, const gchar *path,
> > -                                GAChannelMethod method, int fd)
> > +                                GAChannelMethod method, int fd, Error
> **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> >      c->method = method;
> >
> > @@ -133,21 +134,20 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> >  #endif
> >                             );
> >          if (fd == -1) {
> > -            g_critical("error opening channel: %s", strerror(errno));
> > +            error_setg_errno(errp, errno, "error opening channel");
> >              return false;
> >          }
> >  #ifdef CONFIG_SOLARIS
> >          ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI);
> >          if (ret == -1) {
> > -            g_critical("error setting event mask for channel: %s",
> > -                       strerror(errno));
> > +            error_setg_errno(errp, errno, "error setting event mask for
> channel");
> >              close(fd);
> >              return false;
> >          }
> >  #endif
> >          ret = ga_channel_client_add(c, fd);
> >          if (ret) {
> > -            g_critical("error adding channel to main loop");
> > +            error_setg(errp, "error adding channel to main loop");
> >              close(fd);
> >              return false;
> >          }
> > @@ -159,7 +159,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >          assert(fd < 0);
> >          fd = qemu_open_old(path, O_RDWR | O_NOCTTY | O_NONBLOCK);
> >          if (fd == -1) {
> > -            g_critical("error opening channel: %s", strerror(errno));
> > +            error_setg_errno(errp, errno, "error opening channel");
> >              return false;
> >          }
> >          tcgetattr(fd, &tio);
> > @@ -180,7 +180,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >          tcsetattr(fd, TCSANOW, &tio);
> >          ret = ga_channel_client_add(c, fd);
> >          if (ret) {
> > -            g_critical("error adding channel to main loop");
> > +            error_setg(errp, "error adding channel to main loop");
> >              close(fd);
> >              return false;
> >          }
> > @@ -188,12 +188,8 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >      }
> >      case GA_CHANNEL_UNIX_LISTEN: {
> >          if (fd < 0) {
> > -            Error *local_err = NULL;
> > -
> > -            fd = unix_listen(path, &local_err);
> > -            if (local_err != NULL) {
> > -                g_critical("%s", error_get_pretty(local_err));
> > -                error_free(local_err);
> > +            fd = unix_listen(path, errp);
> > +            if (fd < 0) {
> >                  return false;
> >              }
> >          }
> > @@ -202,24 +198,19 @@ static gboolean ga_channel_open(GAChannel *c,
> const gchar *path,
> >      }
> >      case GA_CHANNEL_VSOCK_LISTEN: {
> >          if (fd < 0) {
> > -            Error *local_err = NULL;
> >              SocketAddress *addr;
> >              char *addr_str;
> >
> >              addr_str = g_strdup_printf("vsock:%s", path);
> > -            addr = socket_parse(addr_str, &local_err);
> > +            addr = socket_parse(addr_str, errp);
> >              g_free(addr_str);
> > -            if (local_err != NULL) {
> > -                g_critical("%s", error_get_pretty(local_err));
> > -                error_free(local_err);
> > +            if (*errp) {
>
> Recommend
>
>                if (!addr) {
>
>
ok


> >                  return false;
> >              }
> >
> > -            fd = socket_listen(addr, 1, &local_err);
> > +            fd = socket_listen(addr, 1, errp);
> >              qapi_free_SocketAddress(addr);
> > -            if (local_err != NULL) {
> > -                g_critical("%s", error_get_pretty(local_err));
> > -                error_free(local_err);
> > +            if (*errp) {
>
> Recommend
>
>                if (fd < 0) {
>
>
ok


> Do you still need ERRP_GUARD() then?
>

It's more future-proof, but could be dropped then.


>
> >                  return false;
> >              }
> >          }
> > @@ -227,7 +218,7 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
> >          break;
> >      }
> >      default:
> > -        g_critical("error binding/listening to specified socket");
> > +        error_setg(errp, "error binding/listening to specified socket");
> >          return false;
> >      }
> >
> > @@ -272,12 +263,13 @@ GIOStatus ga_channel_read(GAChannel *c, gchar
> *buf, gsize size, gsize *count)
> >  GAChannel *ga_channel_new(GAChannelMethod method, const gchar *path,
> >                            int listen_fd, GAChannelCallback cb, gpointer
> opaque)
> >  {
> > +    Error *err = NULL;
> >      GAChannel *c = g_new0(GAChannel, 1);
> >      c->event_cb = cb;
> >      c->user_data = opaque;
> >
> > -    if (!ga_channel_open(c, path, method, listen_fd)) {
> > -        g_critical("error opening channel");
> > +    if (!ga_channel_open(c, path, method, listen_fd, &err)) {
> > +        error_report_err(err);
> >          ga_channel_free(c);
> >          return NULL;
> >      }
>
> This changes error reporting from g_critical() (which doesn't count as
> error reporting in my book) to error_report_err().
>
> How does this affect the program's behavior?
>

Hmm, indeed. I thought because qga uses Error it would have integrated
error_report usage, I am wrong!  (so annoying to have both GError and Error)

qga logging is done via glib API, which Error will simply print to stderr.
I will change it to g_critical again then.

(tbh, I prefer v1 with g_auto(Error), but I don't want to debate for such
thing..)

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 9485 bytes --]