Matteo Croce writes: > 2015-09-14 13:19 GMT+02:00 Olaf Meeuwissen <paddy-h...@member.fsf.org>: >> Matteo Croce writes: >> >>> 2015-09-13 7:18 GMT+02:00 Olaf Meeuwissen <paddy-h...@member.fsf.org>: >>>> Matteo Croce writes: >>>> >>>> [snip] >>>> Code's usage: [ -a [ username ] | -d [ n ] | -s [ n ] ] | -h >>>> Manual page : [ -a [ username ] | -d [ n ] | -s [ n ] | -h ] >>>> getopt : [-a username] [-d n] [-s n] [-h] >>>> >>>> An incompatible change like that of course cannot go in. >>>> [snip] >>> >>> It's not an incompatible change, actual use cases will still work the same: >>> - plain `saned` will work as inetd >>> - `saned -a [username]` will work as standalone as user username, with >>> the only difference that now `saned -a -d` works as expected >> >> My main concern here is that the code has not had any testing that the >> SANE project is aware of for these cases. The original code was written >> against the later options being ignored. Now all of a sudden that is no >> longer the case. >> >>> - `sane -d[n]` still works, but an extra space is permitted >> >> This is not problematic of course. >> >>> - same for `sane -s[n]` >>> >>> There is no point in having -a and -s mutually exclusive, syslog >>> debugging level should be configurable even in standalone mode. >>> The same applies to -d and -s, if you specify "-d -s" (but why someone >>> should) with current master it will output to stderr, >>> and the same applies with my patch as 's' is the same case without break. >> >> Logically, combining command-line options may be fine but will the code >> still work as one would expect? >> >> Forgetting about the help option and option arguments for a minute, the >> original code only had four cases to consider. Your code has eight. >> Have the extra four been validated? You only mention three in the >> above. What about -a -d -s? >> >> If they have, then that's fine and I have no objections in this going >> in. We just have to remember to mention that the saned command-line has >> slightly changed in the NEWS file then. >> >> Sorry to be such a nitpick but saned is a server and that makes me extra >> nervous. > > I understand your concern, I did some experiments with the patched > code and I realized that if you give any combination of -a -d or -s > then the last one will be effective and the previous options will be > ignored, because they have as effect to write the same global > "run_mode" variable. The only exception is putting -a at the end, but > it seems more a feature than a bug.
I was afraid something like that might be the case. Thanks for checking whether things would be okay or not. Turns out they aren't and what you (and I initially also) thought was a compatible change wasn't. > So, I think that we should implement getopt without changing anything > in the behaviour at all, that means that -a -d and -s are mutually > exclusive, as already are on master. Either that or modify the rest of the code to do what is expected in the case multiple option are given. > The only issue is that the argument to -a is optional on master, > and with getopt optional arguments can't have any space between the > option and the option argument, > eg. '-ascanuser' instead of '-a scanuser' because the latter will > parse as -a'' followed by a non argument token "scanuser" > -d and -s will still continue to work as expected, and making them > mutually exclusive is simple as adding a bool in the parsing loop. Have you considered using getopt_long(). There's an implementation in lib/getopt1.c (to keep your friendly ANSI C neighbourhood watch happy ;-) with a header in include/lgetopt.h. FYI, scanimage uses that as well. > of course man and help needs to be in sync with current behaviour > (they are out of sync already) On current master it is just a minor discrepancy, without any functional difference, but feel free to fix that. Hope this helps, -- Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27 Support Free Software Support the Free Software Foundation https://my.fsf.org/donate https://my.fsf.org/join -- sane-devel mailing list: sane-devel@lists.alioth.debian.org http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel Unsubscribe: Send mail with subject "unsubscribe your_password" to sane-devel-requ...@lists.alioth.debian.org
