"Daniel P. Berrange" <berra...@redhat.com> writes:
> 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 **.
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.
>> > }
>> > return NULL;
>>
>> Not reachable. Ugly, but not your patch's concern.
>
>
> Regards,
> Daniel
