Notes |
(0014704)
vda
10-30-08 16:51
|
I don't know what to do with this report.
There are more than 200 applets now, should I go and check all applets and all options and whether they are accepted but ignored, or implemented not exactly compatibly? How many days (years?) would that take? |
| |
(0014764)
Eero Tamminen
10-31-08 03:25
|
> I don't know what to do with this report.
>
> There are more than 200 applets now, should I go and check all applets and
> all options and whether they are accepted but ignored, or implemented not
> exactly compatibly? How many days (years?) would that take?
First step would be adding the "missing option" warning and fatal config option support for Busbox getopt() style functions. This should fix half the issue and be pretty easy to do (default switch case with ifdeffed printf() & exit(-1)?).
IMHO the warning could be enabled by default as anybody building Busybox himself can easily switch it off. The message could be: "WARNING: option '%s' isn't implemented or enabled in Busybox."
I guess second step would be updating the instructions about the applet development to state that if config option removes handling of options, it should also change the getopt arguments appropriately.
Third step would be fixing the applets. A quick grep for "option", "argument" and " -[a-z]" of the Busybox v1.10 config options reveals some busybox config settings that modify what options are parsed, the list of them is attached. This tool probably would need an update too:
Busybox chat require no options. To make it not fail when used
in place of original chat (which has a bunch of options) turn
this on.
First just the applets which are commonly used from scripts (e.g. in Debian packages) could be fixed. From the attached list those would be:
echo, head, tail, find, grep, pidof, ps, tar, xargs.
Rest could be fixed when noticed later on and have separate bugs. Does this sound reasonable? |
| |
(0014774)
vda
10-31-08 17:15
|
> adding the "missing option" warning and fatal config option support for Busbox getopt() style functions. This should fix half the issue and be pretty easy to do (default switch case with ifdeffed printf() & exit(-1)?).
Can you demonstrate this in a patch form? |
| |
(0014964)
Eero Tamminen
11-03-08 06:51
|
> Can you demonstrate this in a patch form?
I took a look at the BB sources and this seems a bit more complicated to do than my naive proposal. Some of the applets and even Busybox getopt32() call the C-library getopt() (which I had initially thought to be BB implementation) which cannot be modified although BB getop32() could.
So... Even the support for warnings needs some changes to applets too. In the libc getopt case it should be simple:
----------
If getopt() does not recognize an option character, it prints an error
message to stderr, stores the character in optopt, and returns ’?’.
The calling program may prevent the error message by setting opterr to 0.
----------
I.e. code using getopt() should zero opterr only when the new options warnings feature is disabled and with warn=fatal feature, they should check for the '?' getopt() return value.
I'm not sure what should be done with BB getopt32() to have this, but maybe you have some idea?
As to giving different options strings depending on what features are selected, I noticed this in BB getopt32() comments:
--------
If one of the given options is found, a flag value is added to
the return value (an unsigned long).
The flag value is determined by the position of the char in
applet_opts string.
--------
I.e. if applets give different argument strings to getopt32() depending on selected features like I proposed, either the defines need to change, or the additional ones need to be last ones. I think it's easier just to add some extra code for checking the flags getopt32() returns and what getopt() returns.
The warn/fatal code could in a separate libb function, it needs to take only the option name as argument. |
| |
(0014974)
dvv
11-03-08 08:34
|
I almost went crazy while trying to make BB sendmail "compatible". Its flavors managed to use for options almost entire alphabet :) Fantastic "declared" flexibility and ... a couple of really useful options. Why should we follow that way? A swiss-army-knife doesn't mean to be used as drop-in replacement for a steam hammer...
Let us clarify whether we _must_ achieve in applets all the functionality of their big brothers. Or should we _just accept_ all options the latter accept? Or should we keep options people _really_ need? (I recall a thread Walter was starting on the set of options applets should accept...) The resolution should then unconditionally be applied to all applets. FEATURE_DESKTOP leaves too much degrees of freedom for developers...
I think the set of options should not be an #ifdef forest. If we choose to mimic big brothers, _all_ options should be declared as contiguous string. A special char *opt_unsupported should be introduced which would enlist options we don't provide handlers for. That way consistency of option char to option mask value mapping would be kept.
Regards,
--
Vladimir |
| |
(0014984)
Eero Tamminen
11-03-08 09:17
|
> Let us clarify whether we _must_ achieve in applets all the functionality of their big brothers. Or should we _just accept_ all options the latter accept? Or should we keep options people _really_ need?
Because Busybox has such a large number of tools, even just checking for option
compatibility would be a huge job. I've done some work in trying to map Busybox utilities to what Debian has and in which package (using the file search at packages.debian.org), attached is the current list.
The answer to your particular problem should be clear though.
If Busybox tool uses different options or their semantics are different than for a similarly named already existing tool[1], Busybox should really use a different (symlink) name for the tool. For example by adding a common prefix to it like "bb-".
If the tool name is same, then the supported options should be some subset of the all options, and having the same semantics.
[1] For example:
pscan: Busybox version scans ports, Debian C-source
ftpput: Busybox version has regular FTP, Debian camera additions
However, that's separate issue. The problem this bug is about is that Busybox doesn't even tell whether it recognized the option letter. And if it recognized, whether it had an actual implementation for it. |
| |
(0015024)
vda
11-03-08 14:09
|
> Because Busybox has such a large number of tools, even just checking for option
compatibility would be a huge job.
Huge job -> bad idea. Let's do something of a small, manageable step. Apparently you had a problem with one specific applet, with a few specific options being silently ignored.
Can you produce a patch which fixes this problem _for this applet only_? If the solution will look good, it can be extended to other applets, piecemeal. |
| |
(0015054)
Eero Tamminen
11-04-08 04:33
|
> Can you produce a patch which fixes this problem _for this applet only_? If the solution will look good, it can be extended to other applets, piecemeal.
Attached patch for sort. getopt32() needs similar stuff also, but I'll leave that to somebody else as it's more complicated. |
| |
debian-mappings.txt [^] (7,855 bytes) 11-03-08 09:21
option-warn.diff [^] (3,752 bytes) 11-04-08 04:28 
