[sane-devel] [PATCH 3/3] saned: reorganize flags, remove run_mode SANED_RUN_DEBUG

Sun Aug 13 06:10:57 UTC 2017

Hi Olaf,

>
> Doing the saned options the Unix way?  Great, "Do just one thing" but
> let's also have a look at the "and do it well" part.  This is not to
> imply that your patch is not doing things well, btw, just a little
> reminder that that is also part of the Unix way.
>
> > All flags now do one thing and normally have an opposite flag that can
> > deactivate it.
>
> I am not sure what you are trying to achieve with the opposite flags.
> What use case do you have in mind for them?
>
> I can think of:
>  - negating an option set in a configuration file
>  - negating an earlier option on the command-line
>  - signalling a running saned to toggle its behaviour
> but none look overly compelling to me.  I even think that the first and
> last ones are not supported by saned anyway.
>

I cannot "negating an option set in a configuration file" as there is
(currently) no intersection between that options and saned.conf can define.
Also, saned reads its configuration after options were processed. Maybe it
can change in the future.

I was thinking "negating an earlier option on the command-line". It's
easier when you are building the command arguments from an external
configuration. However, I don't really have use for it yet.

>
> If there is no clear use case, perhaps saned should not provide them.
> In that case, I'd remove:
>  - -D and make running in the background the default because saned is
>    normally meant to run as a daemon
>

I beg to disagree. It's not true for (x)inet, systemd nor procd. They do
expect process in foreground. Only running saned standalone might need it
to go background. Also, I think that changing the default behavior is even
more traumatic than changing any behavior of options.

>  - -s and make logging to syslog the default because daemon processes
>    shouldn't scribble on anyone's terminal unless asked to
>

Isn't it already the default? log_to_syslog is statically initialized as
SANE_TRUE.

>  - -l and make running in stand-alone mode the default because that's
>    what daemons ought to do anyway
>

As systemd is the most used init, it is someway secure to say that
stand-alone will not be the most used "mode" of saned. (x)inetd also
requires inetd mode. procd do need stand-alone (but foreground) mode.

You may be right that a daemon (<xxx>d) server should daemonize and run
background when called without options. The default behavior would only
save time when a user is calling it directly in a terminal. Extra arguments
for a init/superserver really does not make a difference. However, I really
don't like the idea of changing the default behavior that will surely break
all current uses without a really strong argument. Maybe we should save it
for the saned-ng, written from scratch. ;-)

I'll remove all options that replicates what is the default behavior. They
are:

-i : run inetd mode
-D: run foreground
-s: log to syslog

# Hmm, looks like the manual page needs more changes to update the
> # configuration sections for (x)inetd and systemd then.
>
>
Indeed. I'm not an expert on that subject.

> > The flag -a was kept compatible but, flags -d and -s now have a
> > different behavior.  -d only sets the debug level and -s only forces
> > syslog. I guess this breakage does little harm as those flags are only
> > used for dev (as they quits saned after first client).
>
> I had some doubts about -s being "only used for dev" but upon reading
> the manual page and the code, you're right.  The -s option behaves as
> the -d option.  Their only difference is in where the output goes.
>
> With that cleared up, I agree it is safe to say that no-one will be
> using these options when running their saned service as a background
> process.  If running saned on demand, e.g. via (x)inetd, you could use
> them but I would frown upon doing so for "production" use.
>
> # BTW, the saned manual page says you cannot give options when running
> # from (x)inetd but that is incorrect or at least outdated.
>

Yeah, options are processed anyway. They will be accepted. I never really
used inetd (only xinetd). Maybe when this doc was written, inetd used to
not accept command line arguments. There is no reason for not accepting
options and, in fact, it does accept. I would consider it just a doc
error/outdate.

> Systemd?  No clue how that works and very little intent to find out.
> If anyone in the know wants to chime in re running saned with these
> options they're welcome.
>

I do use systemd (it's kind of difficult to not to). I do not have a strong
option of it. I just accepted it. From what I know, it can also replace
(x)inetd calling saned whenever a client connects to a socket. The current
default behavior is enough for systemd.

>
> # FWIW, I switched to Devuan in December 2016 after using Debian for
> # about 19 years to regain init freedom ;-)  Alternative init system
> # approaches are welcome!

> Procd?  I'd think you know ;-)
>

> > If not, I can introduce new flags for those actions and revert -d/-s
> > previous behavior.
>
> I don't think there is any need to keep these options backwardly
> compatible but it might be in order for saned to warn people that these
> options have changed since 1.0.27, show the new invocation for the old
> behaviour and a pointer to the manual page for details on the changed
> options.
>

Something for the Release Notes as Changelog is built from git log. Is
there any place to save relevant changes for the next release?

>
> Something similar could be done for -a, warning users that this option
> is deprecated and will be removed in the future (without any explicit
> mention of when exactly).
>

I still think that it is not worth it. It's a problem when you does not
have an option for changing only a specific behavior. However, a "combo
option" that aggregates a bundle of options normally used together is good
(just like rsync -a). My suggestion is to just keep it.

>
> You mention something about creating a PID file for the -u option.  That
> made me think a -p option to specify where you want that file might be a
> nice addition.  The current location, /var/run/saned.pid, is hard-coded.
> It's not a bad location but one may want to change it.
>

I'll take a look. Normally PID file location is something for a
configuration file.
Sometimes init would like to take care of the PID file life cycle. Besides
being hard-coded, if a previous PID file exists, saned should do some
checks, and abort on failure. Today, if someone manage to run two instances
of a stand-alone saned, the last one would simply overwrite its own PID
inside the PID file. Also it should replace PID file instead of simply
rewriting it. At least it would avoid different code paths (and permission
requirements) whether a file at PID file exists or not.

>
> Oh, about the code changes, there are a few places in the manual page
> I'd change to improve the English but I can do that for you.  There is
> one mistake though, you document a -B option (as if it were -D).
>

Yeah, english skill aren't really my "expertise". :-) I do know my
limitations. Feel free to point me or correct them directly.

>
> I'd drop the SANED_EXEC_* defines you add because they're not used and
> apart from a minor English nitpick in the option descriptions, the
> saned.c changes look fine.
>

Sure. I overlooked that. Thanks.

>
> Hope this helps,
>

It do really helps. Thanks a lot for the review.

> --
> Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
>  GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
>  Support Free Software                        https://my.fsf.org/donate
>  Join the Free Software Foundation              https://my.fsf.org/join
>
-- 

Luiz Angelo Daros de Luca
luizluca at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/sane-devel/attachments/20170813/9af181e4/attachment-0001.html>