[Top][All Lists]

[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: Egeyar Bagcioglu
Subject: Re: [PATCH] libpoke/ios-dev-stream.c: free buffer only in read mode
Date: Sun, 18 Oct 2020 12:32:11 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

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:

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.

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]