>
> 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@xxxxxxxxxx>
> >> ---
> >> 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 ?
>
They are already deprecated in spice-server.h, just retained for
backward compatibility.
Maybe we could just return -1 ??
> >
> > 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.
>
Still racing, probably would be better to use shutdown instead
or use dup2 on a /dev/null to atomically replace the file descriptor
or call some other functions.
Currently is called by spice_server_destroy which is never called.
And being spice_server_destroy a final "destructor" call would perhaps
better to just free all resources (memory, sockets and so on).
> Thanks,
> Uri.
>
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel