Patchwork [17/34] qerror: drop QERR_SOCKET_CONNECT_IN_PROGRESS

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 2, 2012, 1:02 a.m.
Message ID <1343869374-23417-18-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174642/
State New
Headers show

Comments

Luiz Capitulino - Aug. 2, 2012, 1:02 a.m.
This error is currently returned by inet_connect_opts(), however
it causes the follow spurious message on HMP:

    (qemu) migrate tcp:0:4444
    migrate: Connection can not be completed immediately
    (qemu)

But migration succeeds.

inet_connect_opts() has a 'in_progress' argument that callers can
use to check whether a connection is in progress. The QERR_ macro
is not needed anymore.

PS: I didn't test with QMP, but I guess the migrate command will
    return an error response.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-sockets.c | 2 --
 qerror.c       | 4 ----
 qerror.h       | 3 ---
 3 files changed, 9 deletions(-)
Markus Armbruster - Aug. 2, 2012, 3:58 p.m.
[cc: Amos]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This error is currently returned by inet_connect_opts(), however
> it causes the follow spurious message on HMP:
>
>     (qemu) migrate tcp:0:4444
>     migrate: Connection can not be completed immediately
>     (qemu)
>
> But migration succeeds.

Broken in commit d5c5dacc.

Commit a6ba35b3 earlier in the same series might have broken other users
of inet_connect() and inet_connect_opts() similarly.  Would be nice to
know.  Whatever was brolen, your patch fixes it, too.

> inet_connect_opts() has a 'in_progress' argument that callers can
> use to check whether a connection is in progress. The QERR_ macro
> is not needed anymore.
>
> PS: I didn't test with QMP, but I guess the migrate command will
>     return an error response.

Plausible.

I'd squash this into PATCH 14, because the purpose of the combined patch
will be obvious.  Right now, 14's isn't.

>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-sockets.c | 2 --
>  qerror.c       | 4 ----
>  qerror.h       | 3 ---
>  3 files changed, 9 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 82f4736..7196c5f 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -284,8 +284,6 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>              if (in_progress) {
>                  *in_progress = true;
>              }
> -
> -            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>          } else if (rc < 0) {
>              if (NULL == e->ai_next)
>                  fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> diff --git a/qerror.c b/qerror.c
> index 691d8a8..33b8780 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -309,10 +309,6 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Could not start VNC server on %(target)",
>      },
>      {
> -        .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
> -        .desc      = "Connection can not be completed immediately",
> -    },
> -    {
>          .error_fmt = QERR_SOCKET_CONNECT_FAILED,
>          .desc      = "Failed to connect to socket",
>      },
> diff --git a/qerror.h b/qerror.h
> index de8497d..52ce58d 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -240,9 +240,6 @@ char *qerror_format(const char *fmt, QDict *error);
>  #define QERR_VNC_SERVER_FAILED \
>      "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>  
> -#define QERR_SOCKET_CONNECT_IN_PROGRESS \
> -    "{ 'class': 'SockConnectInprogress', 'data': {} }"
> -
>  #define QERR_SOCKET_CONNECT_FAILED \
>      "{ 'class': 'SockConnectFailed', 'data': {} }"
Michael Roth - Aug. 2, 2012, 4:54 p.m.
On Wed, Aug 01, 2012 at 10:02:37PM -0300, Luiz Capitulino wrote:
> This error is currently returned by inet_connect_opts(), however
> it causes the follow spurious message on HMP:
> 
>     (qemu) migrate tcp:0:4444
>     migrate: Connection can not be completed immediately
>     (qemu)
> 
> But migration succeeds.

I think the core issue is that inet_connect_opts() passes back the
QERR_SOCKET_CONNECT_IN_PROGRESS via Error (which is fine), but that
we have users that erroneous pass this error up the stack, when really,
when specifying blocking=on as one of the options, they should be
expecting and doing specific handling for this error.

So if we fix that (by simply using a local Error when doing the call and
using error_propagate() for non QSCIP errors), I think we can basically
drop patches 14-17 by fixing the callers in that manner and just giving QSCIP
it's own error class.

Relying on the errno result was something these socket errors were
specifically meant to fix, since errno is set multiple times
throughout the function and extracting an errno reliably requires
callers to examine all the possible error paths and errno setters. So I
think it's a regression to go back to the old behavior, and these were
issues found in inet_connect() when we attempted to generalize it's
usage for non-blocking connections.

> 
> inet_connect_opts() has a 'in_progress' argument that callers can
> use to check whether a connection is in progress. The QERR_ macro
> is not needed anymore.
> 
> PS: I didn't test with QMP, but I guess the migrate command will
>     return an error response.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-sockets.c | 2 --
>  qerror.c       | 4 ----
>  qerror.h       | 3 ---
>  3 files changed, 9 deletions(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 82f4736..7196c5f 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -284,8 +284,6 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>              if (in_progress) {
>                  *in_progress = true;
>              }
> -
> -            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
>          } else if (rc < 0) {
>              if (NULL == e->ai_next)
>                  fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> diff --git a/qerror.c b/qerror.c
> index 691d8a8..33b8780 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -309,10 +309,6 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Could not start VNC server on %(target)",
>      },
>      {
> -        .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
> -        .desc      = "Connection can not be completed immediately",
> -    },
> -    {
>          .error_fmt = QERR_SOCKET_CONNECT_FAILED,
>          .desc      = "Failed to connect to socket",
>      },
> diff --git a/qerror.h b/qerror.h
> index de8497d..52ce58d 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -240,9 +240,6 @@ char *qerror_format(const char *fmt, QDict *error);
>  #define QERR_VNC_SERVER_FAILED \
>      "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
> 
> -#define QERR_SOCKET_CONNECT_IN_PROGRESS \
> -    "{ 'class': 'SockConnectInprogress', 'data': {} }"
> -
>  #define QERR_SOCKET_CONNECT_FAILED \
>      "{ 'class': 'SockConnectFailed', 'data': {} }"
> 
> -- 
> 1.7.11.2.249.g31c7954.dirty
>
Luiz Capitulino - Aug. 2, 2012, 5:08 p.m.
On Thu, 2 Aug 2012 11:54:11 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Aug 01, 2012 at 10:02:37PM -0300, Luiz Capitulino wrote:
> > This error is currently returned by inet_connect_opts(), however
> > it causes the follow spurious message on HMP:
> > 
> >     (qemu) migrate tcp:0:4444
> >     migrate: Connection can not be completed immediately
> >     (qemu)
> > 
> > But migration succeeds.
> 
> I think the core issue is that inet_connect_opts() passes back the
> QERR_SOCKET_CONNECT_IN_PROGRESS via Error (which is fine), but that
> we have users that erroneous pass this error up the stack, when really,
> when specifying blocking=on as one of the options, they should be
> expecting and doing specific handling for this error.

You're right here.

> So if we fix that (by simply using a local Error when doing the call and
> using error_propagate() for non QSCIP errors), I think we can basically
> drop patches 14-17 by fixing the callers in that manner and just giving QSCIP
> it's own error class.

I don't think QSCIP errors is something we should report to QMP clients, at
least not for the use-case this patch is about, hence we should not have
a specific error class for this.

As pointed out by Markus in his review, keeping the in_progress flag introduced
by patch 14/34 should be enough to drop patches 15 and 16.

> Relying on the errno result was something these socket errors were
> specifically meant to fix, since errno is set multiple times
> throughout the function and extracting an errno reliably requires
> callers to examine all the possible error paths and errno setters. So I
> think it's a regression to go back to the old behavior, and these were
> issues found in inet_connect() when we attempted to generalize it's
> usage for non-blocking connections.

I'm not completely sure I agree because the new error format doesn't allow
callers to programatically know the cause of an failure. That's what errno
is for, though.

But I'll drop the patch that changes inet_connect() to return errno,
so it's not worth it to discuss this specific case.
Michael Roth - Aug. 3, 2012, 6:26 p.m.
On Thu, Aug 02, 2012 at 02:08:48PM -0300, Luiz Capitulino wrote:
> On Thu, 2 Aug 2012 11:54:11 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Aug 01, 2012 at 10:02:37PM -0300, Luiz Capitulino wrote:
> > > This error is currently returned by inet_connect_opts(), however
> > > it causes the follow spurious message on HMP:
> > > 
> > >     (qemu) migrate tcp:0:4444
> > >     migrate: Connection can not be completed immediately
> > >     (qemu)
> > > 
> > > But migration succeeds.
> > 
> > I think the core issue is that inet_connect_opts() passes back the
> > QERR_SOCKET_CONNECT_IN_PROGRESS via Error (which is fine), but that
> > we have users that erroneous pass this error up the stack, when really,
> > when specifying blocking=on as one of the options, they should be
> > expecting and doing specific handling for this error.
> 
> You're right here.
> 
> > So if we fix that (by simply using a local Error when doing the call and
> > using error_propagate() for non QSCIP errors), I think we can basically
> > drop patches 14-17 by fixing the callers in that manner and just giving QSCIP
> > it's own error class.
> 
> I don't think QSCIP errors is something we should report to QMP clients, at
> least not for the use-case this patch is about, hence we should not have
> a specific error class for this.

But we do have internal users besides QMP, and in this case they're
interested in a specific error. What if we generalized it to EAGAIN or
something? It's seems to me a fairly reasonable exception since it's one
of the few errno-style errors that we don't generally propagate up the
stack and need to check for explicitly...

> 
> As pointed out by Markus in his review, keeping the in_progress flag introduced
> by patch 14/34 should be enough to drop patches 15 and 16.

Although, being an exceptional case I guess having an "in_progress" field
to functions would use it is reasonable...

I think I'd still prefer a class for QSCIP/EAGAIN that we could use for
socket utility functions, but I'm okay with an in_progress param.

> 
> > Relying on the errno result was something these socket errors were
> > specifically meant to fix, since errno is set multiple times
> > throughout the function and extracting an errno reliably requires
> > callers to examine all the possible error paths and errno setters. So I
> > think it's a regression to go back to the old behavior, and these were
> > issues found in inet_connect() when we attempted to generalize it's
> > usage for non-blocking connections.
> 
> I'm not completely sure I agree because the new error format doesn't allow
> callers to programatically know the cause of an failure. That's what errno

Is error_get_class() not to be used for this purpose? It seems like a
good thing to allow for in the odd circumstances where we do end up
adding new error classes (unless the notion of error classes is purely
legacy support for ones that libvirt is dependent on, and new ones will
never be added?)

> callers to programatically know the cause of an failure. That's what errno
> is for, though.

But it's just simply unusable when calling into a function that has
multiple paths that can set it (or clobber it). Errno values frequently
require the context of the function that set it to do anything intelligent,
which is why QSCIP was added to remove that burden from users of
inet_connect_opts() and friends.

It's good that errors are no longer tethered to the errors
descriptions/parameters and that that has amounted to a big reduction
in the number of error classes we have, but that doesn't mean we shouldn't
be open to added new error classes in the future, where it makes sense.

But, again, an in_progress param seems like a workable compromise here, I
just think prefering this approach over new error classes may lead to
unecessary code churn in the future.

> 
> But I'll drop the patch that changes inet_connect() to return errno,
> so it's not worth it to discuss this specific case.
>
Luiz Capitulino - Aug. 3, 2012, 8:31 p.m.
On Fri, 3 Aug 2012 13:26:27 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Thu, Aug 02, 2012 at 02:08:48PM -0300, Luiz Capitulino wrote:
> > On Thu, 2 Aug 2012 11:54:11 -0500
> > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > 
> > > On Wed, Aug 01, 2012 at 10:02:37PM -0300, Luiz Capitulino wrote:
> > > > This error is currently returned by inet_connect_opts(), however
> > > > it causes the follow spurious message on HMP:
> > > > 
> > > >     (qemu) migrate tcp:0:4444
> > > >     migrate: Connection can not be completed immediately
> > > >     (qemu)
> > > > 
> > > > But migration succeeds.
> > > 
> > > I think the core issue is that inet_connect_opts() passes back the
> > > QERR_SOCKET_CONNECT_IN_PROGRESS via Error (which is fine), but that
> > > we have users that erroneous pass this error up the stack, when really,
> > > when specifying blocking=on as one of the options, they should be
> > > expecting and doing specific handling for this error.
> > 
> > You're right here.
> > 
> > > So if we fix that (by simply using a local Error when doing the call and
> > > using error_propagate() for non QSCIP errors), I think we can basically
> > > drop patches 14-17 by fixing the callers in that manner and just giving QSCIP
> > > it's own error class.
> > 
> > I don't think QSCIP errors is something we should report to QMP clients, at
> > least not for the use-case this patch is about, hence we should not have
> > a specific error class for this.
> 
> But we do have internal users besides QMP, and in this case they're
> interested in a specific error. What if we generalized it to EAGAIN or
> something? It's seems to me a fairly reasonable exception since it's one
> of the few errno-style errors that we don't generally propagate up the
> stack and need to check for explicitly...

The fact that we don't propagate it up and that this is used only for
communication between two functions are the reasons for not creating an
error class for it.

For this specific case, my in_progress change is a better solution IMO.

> > As pointed out by Markus in his review, keeping the in_progress flag introduced
> > by patch 14/34 should be enough to drop patches 15 and 16.
> 
> Although, being an exceptional case I guess having an "in_progress" field
> to functions would use it is reasonable...
> 
> I think I'd still prefer a class for QSCIP/EAGAIN that we could use for
> socket utility functions, but I'm okay with an in_progress param.
> 
> > 
> > > Relying on the errno result was something these socket errors were
> > > specifically meant to fix, since errno is set multiple times
> > > throughout the function and extracting an errno reliably requires
> > > callers to examine all the possible error paths and errno setters. So I
> > > think it's a regression to go back to the old behavior, and these were
> > > issues found in inet_connect() when we attempted to generalize it's
> > > usage for non-blocking connections.
> > 
> > I'm not completely sure I agree because the new error format doesn't allow
> > callers to programatically know the cause of an failure. That's what errno
> 
> Is error_get_class() not to be used for this purpose? It seems like a
> good thing to allow for in the odd circumstances where we do end up
> adding new error classes (unless the notion of error classes is purely
> legacy support for ones that libvirt is dependent on, and new ones will
> never be added?)

I think I didn't express myself correctly here.

Yes, it's possible to use error_get_class() to programatically check for an
error class. There's nothing wrong with that.

The problem though, is that this series shrinks the number of errors classes
from 70+ to a few. So, most errors causes will be GenericError.

Take the block layer for example. Several block layer functions check for
errno and take different actions depending on the error cause. When we
Errorify the block layer (and we'll do it to propagate the error cause to
users) we won't be able to drop errno in favor of error_get_class(), otherwise
we'll be back to dozens of error classes (worse, most of them will only
be useful for the block layer or any subsystem doing similar things).

For the block layer case, I think we'll need to introduce error_sete() and
error_get_errno().

> > callers to programatically know the cause of an failure. That's what errno
> > is for, though.
> 
> But it's just simply unusable when calling into a function that has
> multiple paths that can set it (or clobber it). Errno values frequently
> require the context of the function that set it to do anything intelligent,
> which is why QSCIP was added to remove that burden from users of
> inet_connect_opts() and friends.

That's case by case. If there's a case where adding a new class make sense,
then we can do it. I don't think this is the case, though. Besides, the current
usage of QSCIP is buggy.

> It's good that errors are no longer tethered to the errors
> descriptions/parameters and that that has amounted to a big reduction
> in the number of error classes we have, but that doesn't mean we shouldn't
> be open to added new error classes in the future, where it makes sense.
> 
> But, again, an in_progress param seems like a workable compromise here, I
> just think prefering this approach over new error classes may lead to
> unecessary code churn in the future.
> 
> > 
> > But I'll drop the patch that changes inet_connect() to return errno,
> > so it's not worth it to discuss this specific case.
> > 
>
Amos Kong - Aug. 6, 2012, 7:04 a.m.
----- Original Message -----
> [cc: Amos]
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This error is currently returned by inet_connect_opts(), however
> > it causes the follow spurious message on HMP:
> >
> >     (qemu) migrate tcp:0:4444
> >     migrate: Connection can not be completed immediately

We use non-block socket for migration, connect() might return
before connection complete.

However, I agree with remove this note.


> >     (qemu)
> >
> > But migration succeeds.
>
> Broken in commit d5c5dacc.
> 
> Commit a6ba35b3 earlier in the same series might have broken other
> users
> of inet_connect() and inet_connect_opts() similarly.  Would be nice
> to
> know.  Whatever was brolen, your patch fixes it, too.


INPROGRESS 'error' will only be returned for non-block socket.
Other users(vnc, nbd) use block socket, and they pass a 'NULL'
to second argument of inet_connect(QemuOpts *opts, Error **errp),
so this Error doesn't effect them.


> > inet_connect_opts() has a 'in_progress' argument that callers can
> > use to check whether a connection is in progress. The QERR_ macro
> > is not needed anymore.
> >
> > PS: I didn't test with QMP, but I guess the migrate command will
> >     return an error response.
> 
> Plausible.
> 
> I'd squash this into PATCH 14, because the purpose of the combined
> patch
> will be obvious.  Right now, 14's isn't.
> 
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-sockets.c | 2 --
> >  qerror.c       | 4 ----
> >  qerror.h       | 3 ---
> >  3 files changed, 9 deletions(-)
> >
> > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > index 82f4736..7196c5f 100644
> > --- a/qemu-sockets.c
> > +++ b/qemu-sockets.c
> > @@ -284,8 +284,6 @@ int inet_connect_opts(QemuOpts *opts, bool
> > *in_progress, Error **errp)
> >              if (in_progress) {
> >                  *in_progress = true;
> >              }
> > -
> > -            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
> >          } else if (rc < 0) {
> >              if (NULL == e->ai_next)
> >                  fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n",
> >                  __FUNCTION__,
> > diff --git a/qerror.c b/qerror.c
> > index 691d8a8..33b8780 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -309,10 +309,6 @@ static const QErrorStringTable qerror_table[]
> > = {
> >          .desc      = "Could not start VNC server on %(target)",
> >      },
> >      {
> > -        .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
> > -        .desc      = "Connection can not be completed
> > immediately",
> > -    },
> > -    {
> >          .error_fmt = QERR_SOCKET_CONNECT_FAILED,
> >          .desc      = "Failed to connect to socket",
> >      },
> > diff --git a/qerror.h b/qerror.h
> > index de8497d..52ce58d 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -240,9 +240,6 @@ char *qerror_format(const char *fmt, QDict
> > *error);
> >  #define QERR_VNC_SERVER_FAILED \
> >      "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
> >  
> > -#define QERR_SOCKET_CONNECT_IN_PROGRESS \
> > -    "{ 'class': 'SockConnectInprogress', 'data': {} }"
> > -
> >  #define QERR_SOCKET_CONNECT_FAILED \
> >      "{ 'class': 'SockConnectFailed', 'data': {} }"
>

Patch

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 82f4736..7196c5f 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -284,8 +284,6 @@  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
             if (in_progress) {
                 *in_progress = true;
             }
-
-            error_set(errp, QERR_SOCKET_CONNECT_IN_PROGRESS);
         } else if (rc < 0) {
             if (NULL == e->ai_next)
                 fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
diff --git a/qerror.c b/qerror.c
index 691d8a8..33b8780 100644
--- a/qerror.c
+++ b/qerror.c
@@ -309,10 +309,6 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not start VNC server on %(target)",
     },
     {
-        .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
-        .desc      = "Connection can not be completed immediately",
-    },
-    {
         .error_fmt = QERR_SOCKET_CONNECT_FAILED,
         .desc      = "Failed to connect to socket",
     },
diff --git a/qerror.h b/qerror.h
index de8497d..52ce58d 100644
--- a/qerror.h
+++ b/qerror.h
@@ -240,9 +240,6 @@  char *qerror_format(const char *fmt, QDict *error);
 #define QERR_VNC_SERVER_FAILED \
     "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
 
-#define QERR_SOCKET_CONNECT_IN_PROGRESS \
-    "{ 'class': 'SockConnectInprogress', 'data': {} }"
-
 #define QERR_SOCKET_CONNECT_FAILED \
     "{ 'class': 'SockConnectFailed', 'data': {} }"