Re: [PATCH] libpoke/ios-dev-stream.c: free buffer only in read mode

[Egeyar Bagcioglu]

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.

Having said that, José is the maintainer of poke and he has the last word.

Either way, I think he would ask you to add a ChangeLog regarding your 
changes. If you are not familiar with that, you can check the earlier 
patches to see how to write your change log.

Regards,
Ege