On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
> No review, just an observation.
>
> Mao Zhongyi <maozy.f...@cn.fujitsu.com> 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: jasow...@redhat.com, arm...@redhat.com
> > Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
> > ---
> > 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 **.
>
> > 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.
> > }
> > return NULL;
>
> Not reachable. Ugly, but not your patch's concern.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
