[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: SUSv3 who for Busybox
From:       "Denys Vlasenko" <vda.linux () googlemail ! com>
Date:       2008-12-18 14:03:36
Message-ID: 1158166a0812180603v449fa371x8f30ce0eaab8e403 () mail ! gmail ! com
[Download RAW message or body]

>>         if (option_mask32 & OPT_m) {
>>                 name = ttyname(STDIN_FILENO);
>>                 if (!name)
>>                         fprintf(stderr, "stdin not a tty\n");
>>                 else
>>                         /* skip leading dev */
>>                 if (!strncmp(name, "/dev/", 5))
>>                         name += 5;
>>         }
>>
>> name == NULL later will result in SEGV.
>
>                if (!name)
>                         fprintf(stderr, "stdin not a tty\n");
> should catch that.

But it does not abort, it continues, with name == NULL.
Thus, later in the code, in if (option_mask32 & OPT_m) {...}
blocks, it uses it and segfaults.

>> function                                             old     new   delta
>> who_main                                             292     954    +662
>> print_ut                                               -     517    +517
>> str                                                    -      40     +40
>> time2str_buf20                                         -      38     +38
>> ------------------------------------------------------------------------------
>> (add/remove: 3/0 grow/shrink: 1/0 up/down: 1257/0)           Total: 1257 bytes
>>
>> This is x4 growth.
>
> it is unfair to compare an full SUSv3 to a simple who like that in the current bb.
> the SUSv3 has 13x the number of options and only grows by 4, thats looks good.

Yes, I'm not saying it should not grow at all, just that x4 seems to be
a bit much. Try to make it grow not so much.

For example. str[] strings all have one leading space: " EMPTY", " RUN_LVL",
" BOOT_TIME",... you can as well add it in the only place you use it:
printf(" %s\t", str[ut->ut_type % ARRAY_SIZE(str)]);
        ^here

Second example (again str[]). It's a vector of char* ptrs. Each ptr is
4 or 8 bytes,
which is good 50% of the strings themselves. nth_string() is to the rescue.
It prints N-th string from "array" formed like this:
"STR1\0STR2\0STR3\0..."
See example in miscutils/adjtimex.c

print_header() function can employ a loop, a-la:

bit = 1;
p = pattern_str;
do {
    if (bit & pattern)
        printf(p);
    p += strlen(p) + 1;
    bit <<= 1;
} while (bit != EXIT);


>> # ./busybox who -a
>>
>> No output! You forgot to do setutent().
>
> strange, i did not notice that i need it. I did testing with glibc maybe a subtle
> difference between ulibc and glibc ? non of the testers reported a problem.

Yes, it seems to be uclibc-related. But manpage says
you do need to call setutent().


>> I'm afraid it needs some polishing.

> what do you expect from me now?

Take who.c with my small changes (corrections wrt statics etc),
and you can work on it and send new patch with more impressive
"make bloatcheck" results.
--
vda
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]