Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Mao Zhongyi]

On 05/03/2017 04:37 PM, Daniel P. Berrange wrote:
> On Wed, May 03, 2017 at 03:09:57PM +0800, Mao Zhongyi wrote:
> > Hi, Markus,Daniel
> >
> > On 04/28/2017 04:02 PM, Markus Armbruster wrote:
> > > "Daniel P. Berrange" <address@hidden> writes:
> > >
> > > > On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
> > > > > No review, just an observation.
> > > > >
> > > > > Mao Zhongyi <address@hidden> writes:
> > > > >
> > > > > > Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> > > > > > net_socket_fd_init() use the function such as fprintf(), perror() to
> > > > > > report an error message.
> > > > > >
> > > > > > Now, convert these functions to Error.
> > > > > >
> > > > > > CC: address@hidden, address@hidden
> > > > > > Signed-off-by: Mao Zhongyi <address@hidden>
> > > > > > ---
> > > > > >  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
> > > > > >  1 file changed, 41 insertions(+), 25 deletions(-)
> > > > > >
> > > > > > diff --git a/net/socket.c b/net/socket.c
> > > > > > index b0decbe..559e09a 100644
> > > > > > --- a/net/socket.c
> > > > > > +++ b/net/socket.c
> > > > > [...]
> > > > > > @@ -433,25 +437,27 @@ static NetSocketState 
> > > > > > *net_socket_fd_init_stream(NetClientState *peer,
> > > > > >
> > > > > >  static NetSocketState *net_socket_fd_init(NetClientState *peer,
> > > > > >                                            const char *model, const char *name,
> > > > > > -                                          int fd, int is_connected)
> > > > > > +                                          int fd, int is_connected,
> > > > > > +                                          Error **errp)
> > > > > >  {
> > > > > >      int so_type = -1, optlen=sizeof(so_type);
> > > > > >
> > > > > >      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> > > > > >          (socklen_t *)&optlen)< 0) {
> > > > > > -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
> > > > > > +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
> > > > > >                  fd);
> > > > > >          closesocket(fd);
> > > > > >          return NULL;
> > > > > >      }
> > > > > >      switch(so_type) {
> > > > > >      case SOCK_DGRAM:
> > > > > > -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
> > > > > > +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, 
> > > > > > errp);
> > > > > >      case SOCK_STREAM:
> > > > > >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> > > > > >      default:
> > > > > >          /* who knows ... this could be a eg. a pty, do warn and continue as 
> > > > > > stream */
> > > > > > -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM 
> > > > > > or SOCK_STREAM\n", so_type, fd);
> > > > > > +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not 
> > > > > > SOCK_DGRAM"
> > > > > > +                   " or SOCK_STREAM", so_type, fd);
> > > > >
> > > > > Not this patches problem: this case is odd, and the comment is bogus.
> > > > > If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
> > > > > it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
> > > > > SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
> > > >
> > > > IMHO it is a problem with this patch. Previously we merely printed
> > > > a warning & carried on, which is conceptually ok in general, though
> > > > dubious here for the reason you say.
> > > >
> > > > Now we are filling an Error **errp object, and carrying on - this is
> > > > conceptually broken anywhere. If an Error ** is filled, we must return.
> > > > If we want to carry on, we shouldn't fill Error **.
> > >
> > > You're right.
> > >
> > > > > >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> > > >
> > > > IMHO, we just kill this and put return NULL here. If there is a genuine
> > > > reason to support something like SOCK_RAW, it should be explicitly
> > > > handled, because this default: case will certainly break SOCK_SEQPACKET
> > > > and SOCK_RDM which can't be treated as streams.
> > >
> > > It's either magic or misguided defensive programming.  Probably the
> > > latter, but I'd like to hear Jason's opinion.
> > >
> > > If it's *necessary* magic, we can't use error_setg().  Else, we should
> > > drop the default, and insert a closesocket(fd) before the final return
> > > NULL.
> >
> > As Daniel said, although the previous printed warning message is
> > dubious, but it is conceptually ok in general. Simply filling it to
> > Error ** is problematic. Of course, as you said drop the default case,
> > there will be no problem. But really to do that?
>
> Yes, please drop that 'default' case since it is broken already.
>
> BTW, drop the default case in a separate patch at the start of
> your series, before changing the error code, so the functional
> change is clear in git history.
>
>
> Regards,
> Daniel

OK，I see.
Thank you very much.

Mao