| Message ID | 1473707034-31147-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 12, 2016 at 08:03:54PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > The command : > migrate tcp:localhost: > > currently segs; fix it so it now says: > > error parsing address 'localhost:' > > and the same for -incoming. > > (We know that errp is non-null; callers use a local_err). I'd still be more comfortable with us using a local Error object here, as it illustrates best practice and protects against future accidents. > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/socket.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/migration/socket.c b/migration/socket.c > index 00de1fe..6e82924 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -113,7 +113,9 @@ void tcp_start_outgoing_migration(MigrationState *s, > Error **errp) > { > SocketAddress *saddr = tcp_build_address(host_port, errp); > - socket_start_outgoing_migration(s, saddr, errp); > + if (!*errp) { > + socket_start_outgoing_migration(s, saddr, errp); > + } > } > > void unix_start_outgoing_migration(MigrationState *s, > @@ -175,7 +177,9 @@ static void socket_start_incoming_migration(SocketAddress *saddr, > void tcp_start_incoming_migration(const char *host_port, Error **errp) > { > SocketAddress *saddr = tcp_build_address(host_port, errp); > - socket_start_incoming_migration(saddr, errp); > + if (!*errp) { > + socket_start_incoming_migration(saddr, errp); > + } > } Yep, makes sense Regards, Daniel
On 09/12/2016 02:11 PM, Daniel P. Berrange wrote: > On Mon, Sep 12, 2016 at 08:03:54PM +0100, Dr. David Alan Gilbert (git) wrote: >> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> The command : >> migrate tcp:localhost: >> >> currently segs; fix it so it now says: >> >> error parsing address 'localhost:' >> >> and the same for -incoming. >> >> (We know that errp is non-null; callers use a local_err). > > I'd still be more comfortable with us using a local Error > object here, as it illustrates best practice and protects > against future accidents. Concur. Having an example contrary to best practice makes it far too easy for that example to be copied somewhere where it will later break. >> +++ b/migration/socket.c >> @@ -113,7 +113,9 @@ void tcp_start_outgoing_migration(MigrationState *s, >> Error **errp) >> { >> SocketAddress *saddr = tcp_build_address(host_port, errp); >> - socket_start_outgoing_migration(s, saddr, errp); >> + if (!*errp) { >> + socket_start_outgoing_migration(s, saddr, errp); >> + } Since we make a decision about code flow based on whether an earlier error occurred, this SHOULD be: { Error *err = NULL; SocketAddress *saddr = tcp_build_address(host_port, &err); if (!err) { socket_start_outgoing_migration(s, saddr, &err); } error_propagate(errp, err); }
On 12/09/2016 21:03, Dr. David Alan Gilbert (git) wrote: > SocketAddress *saddr = tcp_build_address(host_port, errp); > - socket_start_outgoing_migration(s, saddr, errp); > + if (!*errp) { > + socket_start_outgoing_migration(s, saddr, errp); > + } > } > > void unix_start_outgoing_migration(MigrationState *s, > @@ -175,7 +177,9 @@ static void socket_start_incoming_migration(SocketAddress *saddr, > void tcp_start_incoming_migration(const char *host_port, Error **errp) > { > SocketAddress *saddr = tcp_build_address(host_port, errp); > - socket_start_incoming_migration(saddr, errp); > + if (!*errp) { > + socket_start_incoming_migration(saddr, errp); > + } FWIW, this less-boilerplate version was almost fine, just replace "!*errp" with "saddr". Paolo
diff --git a/migration/socket.c b/migration/socket.c index 00de1fe..6e82924 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -113,7 +113,9 @@ void tcp_start_outgoing_migration(MigrationState *s, Error **errp) { SocketAddress *saddr = tcp_build_address(host_port, errp); - socket_start_outgoing_migration(s, saddr, errp); + if (!*errp) { + socket_start_outgoing_migration(s, saddr, errp); + } } void unix_start_outgoing_migration(MigrationState *s, @@ -175,7 +177,9 @@ static void socket_start_incoming_migration(SocketAddress *saddr, void tcp_start_incoming_migration(const char *host_port, Error **errp) { SocketAddress *saddr = tcp_build_address(host_port, errp); - socket_start_incoming_migration(saddr, errp); + if (!*errp) { + socket_start_incoming_migration(saddr, errp); + } } void unix_start_incoming_migration(const char *path, Error **errp)