[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] libpoke/ios-dev-stream.c: free buffer only in read mode
From: |
Jose E. Marchesi |
Subject: |
Re: [PATCH] libpoke/ios-dev-stream.c: free buffer only in read mode |
Date: |
Sun, 18 Oct 2020 13:32:45 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> Hi Mohammed-Reza,
>
> Thanks for pointing the issue there.
>
>>> Wouldn't it be better to make sure to initialize sio->buffer to NULL
>>> when it is not used, and then to either:
>>>
>> Currently the `buffer` field is inside an anonymous union (and shares the
>> storage with `uint64_t write_offset`).
>> If we want to check the nullability, we should remove the `union`.
>>
>> Like this:
>>
>> ```c
>> struct ios_dev_stream
>> {
>> char *handler;
>> FILE *file;
>> uint64_t flags;
>> struct ios_buffer *buffer;
>> uint64_t write_offset;
>> };
>> ```
>>
>>> 1. Document (in ios-buffer.h) that ios_buffer_free works as a nop if
>>> NULL is passed, or
>>>
>>> 2. Check for sio->buffer == NULL in ios_dev_stream_close and call (or
>>> not) ios_buffer_free accordingly.
>>>
>> I prefer the option 2 (plus adding an explicit pre-condition that `buffer`
>> must
>> be non-null).
>>
>> But I guess option 1 is more popular among C programmers.
>>
>>
>> WDYT?
>> Should I remove the `union`?
>> Which approach do you think is the best for `ios_buffer_free(NULL)`?
>> NOP or assertion?
>
> I am no maintainer but I'd like to share my opinion on this. I believe
> that ios_dev_stream has a union for a good reason. It makes it clear
> that we either use a buffer or keep the offset ourselves. Never both
> at the same time... Considering this additional bit of info, I like
> your original patch.
Makes sense. I'm ok with that.