Patch Set 1:
> @Pau: You're not the only one getting confused with it, snprintf()
> is a mess, it's hard to deal with it, hence this macro that retains
> semantics that aims to simplify things... if you find any better,
> let me know I'd be happy to reuse it in my code moving forward :-).
>
> @Holger: Either way, I don't mind if you don't need the snprintf
> semantics, this is a _snprintf() function after all, just explaning
> here. So the intention at least that I can remember was to keep in
> sync with how snprintf() works. Anyway, feel free to simplify this.
>
Even if the intention was to follow snprintf, as far as I can tell it doesn't
seem to be actually working correctly. Implementation previous to this patch
returns offset in all functions. offset var is always calculated using this
part of the macro:
if (ret > len) \
ret = len; \
offset += ret; \
Which means offset is never going to be bigger than the "size" originally
passed as parameter. That's correct as offset marks internally next position to
write in the buffer, but should not be the var being returned as it's always <=
size even if size of buffer is smaller than required size.
In exchange, size (from the variable) should be initialized to 0 at the start
of each function after assigning len = size, then return size in each function.
This could be called "count" instead of "size" to avoid confusion with "size in
parameter. Actually, I don't really understand what's the usage of the "size"
variable as used currently, which starts having size of the buffer (used or
not) and keeps growing.
On top of that, I have the feeling the previous implementation doesn't handle
the case where snprintf returns negative value (error). So even after all those
modifications, we still need to add extra logic to handle that. In nftables
code it seems to be handled by calling abort(), which doesn't seem like a
perfect approach in any case. It's not nice having libraries calling abort().
> Anyway, I just wanted to make sure this patch was really fixing up
> the real issue. I understand you observe a crash, but not clear to
> me why the patch is fixing it.
>
> Cheers.
I try to fix all this corner cases explained above in this patch. I started
looking again today at following a more conservative approach, like fixing step
by step the current code, but as far as I can tell doesn't make sense, as
there's a lot of stuff which needs changes.
So, if there's no more objections to this patch, during following days I'll
push a new version with the description fixes as advised by Holger, and we can
then merge it.
--
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <[email protected]>
Gerrit-Reviewer: Holger Freyther <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <[email protected]>
Gerrit-Reviewer: Pau Espin Pedrol <[email protected]>
Gerrit-HasComments: No
