Re: [Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd
> > 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> >> --- > >> 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(_chan->base), sa, salen) : > >> -1; > >> +int socketfd; > >> + > >> +if (!main_chan || ((socketfd = > >> red_channel_get_first_socket(_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(_chan->base), sa, salen) : > >> -1; > >> +int socketfd; > >> + > >> +if (!main_chan || ((socketfd = > >> red_channel_get_first_socket(_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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd
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--- 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(_chan->base), sa, salen) : -1; +int socketfd; + +if (!main_chan || ((socketfd = red_channel_get_first_socket(_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(_chan->base), sa, salen) : -1; +int socketfd; + +if (!main_chan || ((socketfd = red_channel_get_first_socket(_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. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server PATCH 4/8] main-channel: getpeername/getsockname return early if no sockfd
> > 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> --- > 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(_chan->base), sa, salen) : -1; > +int socketfd; > + > +if (!main_chan || ((socketfd = > red_channel_get_first_socket(_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(_chan->base), sa, salen) : -1; > +int socketfd; > + > +if (!main_chan || ((socketfd = > red_channel_get_first_socket(_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? OT: red_channel_get_first_socket is used only for obsolete or wrong code. OT: main_channel_close is causing a dandling file descriptor. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel