[PATCH] port-serial: don't close tty file descriptor twice

[Brian, Sam]

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