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

[Markus Armbruster]

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?

>          return net_socket_fd_init_stream(peer, model, name, fd, 
> is_connected);
>      }
>      return NULL;

Not reachable.  Ugly, but not your patch's concern.

[...]