[2018-11-04 16:21] markus schnalke <mei...@marmaro.de>
> part text/plain 4116
> Hoi.
Hello;
> [2018-11-04 15:32] c_14 <g...@c-14.de>
> > [2018-11-04 14:33] markus schnalke <mei...@marmaro.de>
> >
> > [..]
> > > part 2 text/x-diff 1523 format_size.patch
> > >
> > > diff --git a/uip/mhlistsbr.c b/uip/mhlistsbr.c
> > > index dd6264b..8d4799c 100644
> > > --- a/uip/mhlistsbr.c
> > > +++ b/uip/mhlistsbr.c
> > > @@ -53,6 +53,27 @@ static int list_encoding(CT);
> > > #define LSTFMT2d2 "\t %-65.65s\n"
> > >
> > >
> > > +char *
> > > +format_size(long unsigned int size)
> > > +{
> > > + static char ret[BUFSIZ];
> > > + char *cp;
> > > +
> > > + /* find correct scale for size (Kilo/Mega/Giga/Tera) */
> > > + for (cp = " KMGT"; size > 9999; size /= 1024) {
> > > + if (!*++cp) {
> > > + return "huge";
> > > + }
> > > + }
> > > + if (*cp == ' ') {
> > > + snprintf(ret, sizeof(ret), "%5lu", size);
> > > + } else {
> > > + snprintf(ret, sizeof(ret), "%4lu%c", size, *cp);
> > > + }
> > > + return ret;
> > > +}
> >
> > Please don't ever return an array you allocated on the stack in a
> > function. As soon as the function returns, any references to the array
> > become invalid and all accesses to that array become undefined behavior.
> > Although accessing the array and elements thereof might still work on
> > certain systems with certain compilers at certain optimization levels,
> > relying on this behavior is foolhardy at best (this exact issue has
> > already caused problems, see e.g. commit 5edd0). Please either allocate a
> > char* on the heap, or if you really want to keep it on the stack create
> > the array on the stack in the parent function and pass a pointer to the
> > array to format_size. (Note that both of these solutions will preclude
> > your ability to `return "huge";' which would need to be replaced with a
> > call to copy that string into the array/char * in either case.)
>
> Thanks a lot for the comment. The big problem, as in the mentioned
> commit, is not existent here, as far as I can tell, because the
> array, to which I return a pointer is *static*. It produces defined
> behavior. You probably missed the keyword `static'.
Yeah, I missed that in my first read-through, I also completely forgot
about static for functions even though I should know better. Guess I
wasn't as awake as I thought I was.
> The only technical problem here, is that there is only one return
> space for the function, thus each further function call overwrites
> the previous result. If you want to save it, you have to copy the
> result. (See the manpage of strtok(3) for a standard example of
> this scheme. Or grep for /^.static/ in sbr/* for more mmh examples.)
>
> Nonetheless, I agree that this approach might not be the best in
> this case. I'm not yet sure. The advantage is that it creates the
> least overhead. The disadvantage is that we cannot do:
>
> printf("%s %s\n", format_size(x), format_size(y));
>
> But if we don't want to do that, but still use this function
> primarily for outputting (which I assume is the case), then the
> static result space inside the function might be the best way to
> go.
I did just try and think of other possible solutions, such as replacing
the format_size function with a printf-wrapper that instead of returning
a char* prints the formatted size directly, but all the solutions I came
up with were either less flexible than your approach or much more
complicated. Your solution is probably the best for ease-of-use and
minimal code complexity. Maybe just add a comment above the format_size
function stating that you can't call the function twice in a single
printf() because subsequent calls will overwrite data returned by the
first call to the function? (Just in case somebody in future also
doesn't notice the static)
[..]
> Am I missing something out?
Nothing I can see right now anyways.
> To be clear: I am hugely thankful that you had a look at the patch.
> Explaining that much and ``thinking myself through the topic'' is
> my way of checking it. Doing that by email forces me to not take
> shortcuts in my thoughts.
Yeah, no problem. I really should have been paying more attention
anyways.
--
c_14
