[sane-devel] [PATCH 1/2] use getopt()

[Olaf Meeuwissen]

Matteo Croce writes:

> 2015-09-14 13:19 GMT+02:00 Olaf Meeuwissen <paddy-hack at member.fsf.org>:
>> Matteo Croce writes:
>>
>>> 2015-09-13 7:18 GMT+02:00 Olaf Meeuwissen <paddy-hack at 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