[Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd

[Uri Lublin]

On 10/17/2016 01:38 PM, Frediano Ziglio wrote:
>>
>> I'm not sure that needed as it seems getpeername/getsockname
>> accept int fd and return -1 for fd=-1
>>
>> Signed-off-by: Uri Lublin <uril at redhat.com>
>> ---
>>  server/main-channel.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/server/main-channel.c b/server/main-channel.c
>> index bf84694..9ff4dcd 100644
>> --- a/server/main-channel.c
>> +++ b/server/main-channel.c
>> @@ -281,12 +281,24 @@ MainChannelClient *main_channel_link(MainChannel
>> *channel, RedClient *client,
>>
>>  int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa,
>>  socklen_t *salen)
>>  {
>> -    return main_chan ?
>> getsockname(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
>> +    int socketfd;
>> +
>> +    if (!main_chan || ((socketfd =
>> red_channel_get_first_socket(&main_chan->base)) < 0)) {
>> +        return -1;
>> +    }
>> +
>> +    return getsockname(socketfd, sa, salen);
>>  }
>>
>>  int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa,
>>  socklen_t *salen)
>>  {
>> -    return main_chan ?
>> getpeername(red_channel_get_first_socket(&main_chan->base), sa, salen) : -1;
>> +    int socketfd;
>> +
>> +    if (!main_chan || ((socketfd =
>> red_channel_get_first_socket(&main_chan->base)) < 0)) {
>> +        return -1;
>> +    }
>> +
>> +    return getpeername(socketfd, sa, salen);
>>  }
>>
>>  // TODO: ? shouldn't it disonnect all clients? or shutdown all
>>  main_channels?
>
> Mumble... I don't know why but it does not look that good.
>
> These functions assume that there are only one client.
> They are used only by spice_server_get_sock_info and spice_server_get_peer_info
> which are no more used by recent Qemu so mainly they are dead code.
> Perhaps spice_server_get_sock_info and spice_server_get_peer_info should
> check for the first client and call getpeername/getsockname by themself?
> Or just adding a main_channel_get_socket instead of 2 obsolete main_channel_*
> functions?

Yes, you are correct.
So, should we deprecate those spice_server_get_*_info calls ?

Yes, you are correct.
Currently multi-client is still considered experimental, but we
do have a lot of code to make it almost work.
So, should we deprecate those spice_servet_get_*_info calls ?

>
> OT: red_channel_get_first_socket is used only for obsolete or wrong code.

Due to the assumption there is only one client.

> OT: main_channel_close is causing a dandling file descriptor.

Yes, this function can iterate over all clients and close their
socketfd.

Thanks,
     Uri.