On 21/06/18 18:20, Aleksander Morgado wrote: >>> >>> Why not change the logic and instead of not closing when there is no >>> iochannel, we just remove the g_io_channel_shutdown() calls? We know >>> that g_io_channel_unref() won't do any implicit close, so we can >>> safely destroy the iochannel regardless of whether we leave the >>> underlying FD open, and then close the FD with close() ourselves >>> unconditionally. That would probably be easier to understand as well? >>> Instead of the shutdown() call we could place a big fat /* comment */ >>> explaining why we don't call shutdown. > >>> What do you think? >>> >> >> If we directly close() instead of using g_io_channel_shutdown() then >> (1) the channel buffer won't be flushed before closing (which is >> probably fine?), and > > Isn't there an explicit buffer flush using the fd when closing the > channel just a bit before? I believe I saw that when I checked the > code.
I meant flushing the GIOChannel's buffer, but I looked back and it's disabled with g_io_channel_set_buffered() after creation anyway. So not a problem. > >> (2) if there are other refs to the channel and the "closing" >> g_io_channel_unref() doesn't destroy it, then the channel's internal >> record keeping will think that the closed fd is still open and >> readable/writable. >> I know (2) is not a problem today because there are no other >> g_io_channel_ref() calls, but it could be overlooked if one was added in >> future. >> >> Perhaps a better way would be to only use g_io_channel_shutdown() and >> replace priv->fd with g_io_channel_unix_get_fd() where required for the >> tcgetattr/tcsetattr/ioctl? >> Removing priv->fd would mean the fd is tracked in only one place. The >> "self->priv->fd >= 0" checks would become the "self->priv->iochannel" >> check so the fd being open would be tied directly to the iochannel being >> present. >> > > I also thought about that yesterday, but then the shutdown removal > seemed like a very quick and simple solution for the issue anyway. If > you want to suggest a patch with the change to use > g_io_channel_unix_get_fd() instead of self->priv->fd, I'll gladly > review it, though. :) > Considering (1) is not a problem and (2) is paranoia of a non-existent problem, I think I'll stick to your suggestion of shutdown removal :) I'll send a V2 patch soon. Sam _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
