[v6,1/4] net/socket: Don't treat odd socket type as SOCK_STREAM

In net_socket_fd_init(), the 'default' case is odd: it warns,
then continues as if the socket type was SOCK_STREAM. The
comment explains "this could be a eg. a pty", but that makes
no sense. If @fd really was a pty, getsockopt() would fail
with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor
SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.

Turn this case into an Error. If there is a genuine reason to
support something like SOCK_RAW, it should be explicitly
handled.

Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Cc: berrange@redhat.com
Cc: armbru@redhat.com
Suggested-by: Markus Armbruster <armbru@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Wed, Jun 28, 2017 at 09:08:47PM +0800, Mao Zhongyi wrote:
> In net_socket_fd_init(), the 'default' case is odd: it warns,
> then continues as if the socket type was SOCK_STREAM. The
> comment explains "this could be a eg. a pty", but that makes
> no sense. If @fd really was a pty, getsockopt() would fail
> with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor
> SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
> 
> Turn this case into an Error. If there is a genuine reason to
> support something like SOCK_RAW, it should be explicitly
> handled.
> 
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Cc: berrange@redhat.com
> Cc: armbru@redhat.com
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  net/socket.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index dcae1ae..53765bd 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -449,9 +449,9 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
>      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);
> -        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> +        error_report("qemu: error: socket type=%d for fd=%d is not"
> +                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);

Please drop the 'qemu: error: ' prefix on the message

Also, rather than 'is not' I suggest 'must be either'


> +        closesocket(fd);
>      }
>      return NULL;
>  }


Regards,
Daniel

On 06/28/2017 08:18 AM, Daniel P. Berrange wrote:
> On Wed, Jun 28, 2017 at 09:08:47PM +0800, Mao Zhongyi wrote:
>> In net_socket_fd_init(), the 'default' case is odd: it warns,
>> then continues as if the socket type was SOCK_STREAM. The
>> comment explains "this could be a eg. a pty", but that makes
>> no sense. If @fd really was a pty, getsockopt() would fail
>> with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor
>> SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
>>
>> Turn this case into an Error. If there is a genuine reason to
>> support something like SOCK_RAW, it should be explicitly
>> handled.
>>

>>      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);
>> -        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>> +        error_report("qemu: error: socket type=%d for fd=%d is not"
>> +                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
> 
> Please drop the 'qemu: error: ' prefix on the message
> 
> Also, rather than 'is not' I suggest 'must be either'

Indentation is also off; we prefer that the second line starts right
after the ( of the first line, as in:

error_report("part 1"
             "part 2")

Hi, Eric

On 06/28/2017 10:23 PM, Eric Blake wrote:
> On 06/28/2017 08:18 AM, Daniel P. Berrange wrote:
>> On Wed, Jun 28, 2017 at 09:08:47PM +0800, Mao Zhongyi wrote:
>>> In net_socket_fd_init(), the 'default' case is odd: it warns,
>>> then continues as if the socket type was SOCK_STREAM. The
>>> comment explains "this could be a eg. a pty", but that makes
>>> no sense. If @fd really was a pty, getsockopt() would fail
>>> with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor
>>> SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
>>>
>>> Turn this case into an Error. If there is a genuine reason to
>>> support something like SOCK_RAW, it should be explicitly
>>> handled.
>>>
>
>>>      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);
>>> -        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>> +        error_report("qemu: error: socket type=%d for fd=%d is not"
>>> +                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
>>
>> Please drop the 'qemu: error: ' prefix on the message
>>
>> Also, rather than 'is not' I suggest 'must be either'
>
> Indentation is also off; we prefer that the second line starts right
> after the ( of the first line, as in:
>
> error_report("part 1"
>              "part 2")

OK, I will. :)

Thanks,
Mao

>
>