Discussion:
[PATCH 1/3] saned: fix --debug help message (output is stderr)
Add Reply
l***@gmail.com
2017-07-22 08:43:16 UTC
Reply
Permalink
Raw Message
From: Luiz Angelo Daros de Luca <***@gmail.com>

Man page was correct.

Signed-off-by: Luiz Angelo Daros de Luca <***@gmail.com>
---
frontend/saned.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frontend/saned.c b/frontend/saned.c
index 2fc33216..6b97e914 100644
--- a/frontend/saned.c
+++ b/frontend/saned.c
@@ -3300,7 +3300,7 @@ static void usage(char *me, int err)
"Usage: %s [OPTIONS]\n\n"
" Options:\n\n"
" -a, --alone[=user] run standalone and fork in background as `user'\n"
- " -d, --debug[=level] run foreground with output to stdout\n"
+ " -d, --debug[=level] run foreground with output to stderr\n"
" and debug level `level' (default is 2)\n"
" -s, --syslog[=level] run foreground with output to syslog\n"
" and debug level `level' (default is 2)\n"
--
2.11.0
--
sane-devel mailing list: sane-***@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-***@lists.alioth.debian.org
l***@gmail.com
2017-07-22 08:43:17 UTC
Reply
Permalink
Raw Message
From: Luiz Angelo Daros de Luca <***@gmail.com>

---
doc/saned.man | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/doc/saned.man b/doc/saned.man
index 34764f1f..8542d254 100644
--- a/doc/saned.man
+++ b/doc/saned.man
@@ -6,6 +6,9 @@ saned \- SANE network daemon
.B saned
.B [ \-a
.I [ username ]
+.B [ \-b
+.I address
+.B ]
.B | \-d
.I [ n ]
.B | \-s
@@ -59,6 +62,12 @@ is used, the debug messages will be printed to stderr while
.B \-s
requests using syslog.
.PP
+The
+.B \-b
+flag requests that
+.B saned
+bind to a specific address.
+.PP
If
.B saned
is run from inetd, xinetd or systemd, no option can be given.
--
2.11.0
--
sane-devel mailing list: sane-***@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-***@lists.alioth.debian.org
l***@gmail.com
2017-07-22 08:43:18 UTC
Reply
Permalink
Raw Message
From: Luiz Angelo Daros de Luca <***@gmail.com>

Flags like -a, -d and -s have many overlap effects. This patch restricts
the effect of flags to a simple action.

New -u (user) flag replaces -a optional argument for running saned as a different user.
The code that retrieve the user info and drop privileges migrated to runas_user().
As a side effect, PID file can be created even if getting user info fails.

New -l (listen) flag sets run_mode to standalone. It can be cancelled with -i.
New -i (inetd, default) flag sets run_mode to inetd. It is useful only to cancel -l.

New -D (daemonize) flag daemonizes saned after bind. Requires -l and it can be cancelled
by -f.
New -f (foreground) flag for running saned in foreground (useful for procd).
It can be cancelled using new -D flag.

New -o (once) make saned exit after the first client disconnects.

Flag -s (syslog) now only forces output to syslog and does not accept arguments. It can be
cancelled using -e. Previous behavior can be reproduced with '-a -d level -o -f -s'.
New -e (stderr) flag for redirecting output to stderr, instead of syslog. It can be
cancelled using -s.

Flag -d (debug) now only sets the debug level and argument is required. Previous behavior
can be reproduced with '-a -d level -o -f'.

The run_mode SANED_RUN_DEBUG and SANED_RUN_ALONE shared most of its code
path. With the new flags dealing with their difference, SANED_RUN_DEBUG is gone.

Flag '-a' still works as before but it can be replaced by '-l -D -u user'.

Current uses of -d (debug) or -s (syslog) will break.

Signed-off-by: Luiz Angelo Daros de Luca <***@gmail.com>
---
doc/saned.man | 78 ++++++++------
frontend/saned.c | 306 +++++++++++++++++++++++++++++++------------------------
2 files changed, 220 insertions(+), 164 deletions(-)

diff --git a/doc/saned.man b/doc/saned.man
index 8542d254..0bddc58d 100644
--- a/doc/saned.man
+++ b/doc/saned.man
@@ -6,15 +6,21 @@ saned \- SANE network daemon
.B saned
.B [ \-a
.I [ username ]
+.B ]
+.B [ \-u
+.I username
+.B ]
.B [ \-b
.I address
.B ]
-.B | \-d
-.I [ n ]
-.B | \-s
-.I [ n ]
-.B | \-h
+.B [ \-l | \-i ]
+.B [ \-D | \-f ]
+.B [ \-o ]
+.B [ \-d
+.I n
.B ]
+.B [ \-s | \-e ]
+.B [ \-h ]
.SH DESCRIPTION
.B saned
is the SANE (Scanner Access Now Easy) daemon that allows remote clients
@@ -22,51 +28,63 @@ to access image acquisition devices available on the local host.
.SH OPTIONS
.PP
The
-.B \-a
+.B \-l
flag requests that
.B saned
run in standalone daemon mode. In this mode,
.B saned
-will detach from the console and run in the background, listening for incoming
-client connections;
+will listening for incoming client connections;
.B inetd
is not required for
.B saned
-operations in this mode. If the optional
-.B username
-is given after
-.B \-a
-,
+operations in this mode. The
+.B \-b
+flag can control which address
.B saned
-will drop root privileges and run as this user (and group).
+will bind. The
+.B \-u
+.I username
+flag requests that
+.B saned
+drop root privileges and run as this user (and group) after bind.
+The
+.B \-B
+flag will request
+.B saned
+to detach from the console and run in the background, while
+.B \-f
+flag will keep it attached to the console and running foreground.
+The flag
+.B \-a
+is equals to
+.B \-l \-B \-u
+.I username
+.
+.PP
+The
+.B \-e
+flag will request that
+.B saned
+output to stderr while the
+.B \-s
+flag forces the output to syslog.
.PP
The
.B \-d
-and
-.B \-s
-flags request that
+flag sets the debug level of
.B saned
-run in debug mode (as opposed to
-.BR inetd (8)
-daemon mode). In this mode,
-.B saned
-explicitly waits for a connection request. When compiled with
-debugging enabled, these flags may be followed by a number to request
+. When compiled with debugging enabled, these flags may be followed by a number to request
debug info. The larger the number, the more verbose the debug output.
E.g.,
.B \-d128
will request printing of all debug info. Debug level 0 means no debug output
-at all. The default value is 2. If flag
-.B \-d
-is used, the debug messages will be printed to stderr while
-.B \-s
-requests using syslog.
+at all. The default value is 2.
.PP
The
-.B \-b
+.B \-o
flag requests that
.B saned
-bind to a specific address.
+exits after the first client disconnects. Useful for debugging.
.PP
If
.B saned
diff --git a/frontend/saned.c b/frontend/saned.c
index 6b97e914..93afd612 100644
--- a/frontend/saned.c
+++ b/frontend/saned.c
@@ -251,6 +251,8 @@ static Wire wire;
static int num_handles;
static int debug;
static int run_mode;
+static int run_foreground;
+static int run_once;
static Handle *handle;
static char *bind_addr;
static union
@@ -298,9 +300,10 @@ static SANE_Bool log_to_syslog = SANE_TRUE;
static int process_request (Wire * w);

#define SANED_RUN_INETD 0
-#define SANED_RUN_DEBUG 1
-#define SANED_RUN_ALONE 2
+#define SANED_RUN_ALONE 1

+#define SANED_EXEC_FOREGROUND 0
+#define SANED_EXEC_BACKGROUND 1

#define DBG_ERR 1
#define DBG_WARN 2
@@ -2964,6 +2967,114 @@ do_bindings (int *nfds, struct pollfd **fds)


static void
+runas_user (char *user)
+{
+ uid_t runas_uid = 0;
+ gid_t runas_gid = 0;
+ struct passwd *pwent;
+ gid_t *grplist = NULL;
+ struct group *grp;
+ int ngroups = 0;
+ int ret;
+
+ pwent = getpwnam(user);
+
+ if (pwent == NULL)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: user %s not found on system\n", user);
+ bail_out (1);
+ }
+
+ runas_uid = pwent->pw_uid;
+ runas_gid = pwent->pw_gid;
+
+ /* Get group list for runas_uid */
+ grplist = (gid_t *)malloc(sizeof(gid_t));
+
+ if (grplist == NULL)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: cannot allocate memory for group list\n");
+
+ exit (1);
+ }
+
+ ngroups = 1;
+ grplist[0] = runas_gid;
+
+ setgrent();
+ while ((grp = getgrent()) != NULL)
+ {
+ int i = 0;
+
+ /* Already added current group */
+ if (grp->gr_gid == runas_gid)
+ continue;
+
+ while (grp->gr_mem[i])
+ {
+ if (strcmp(grp->gr_mem[i], user) == 0)
+ {
+ int need_to_add = 1, j;
+
+ /* Make sure its not already in list */
+ for (j = 0; j < ngroups; j++)
+ {
+ if (grp->gr_gid == grplist[i])
+ need_to_add = 0;
+ }
+ if (need_to_add)
+ {
+ grplist = (gid_t *)realloc(grplist,
+ sizeof(gid_t)*ngroups+1);
+ if (grplist == NULL)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: cannot reallocate memory for group list\n");
+
+ exit (1);
+ }
+ grplist[ngroups++] = grp->gr_gid;
+ }
+ }
+ i++;
+ }
+ }
+ endgrent();
+
+ /* Drop privileges if requested */
+ if (runas_uid > 0)
+ {
+ ret = setgroups(ngroups, grplist);
+ if (ret < 0)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: could not set group list: %s\n", strerror(errno));
+
+ exit (1);
+ }
+
+ free(grplist);
+
+ ret = setegid (runas_gid);
+ if (ret < 0)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: setegid to gid %d failed: %s\n", runas_gid, strerror (errno));
+
+ exit (1);
+ }
+
+ ret = seteuid (runas_uid);
+ if (ret < 0)
+ {
+ DBG (DBG_ERR, "FATAL ERROR: seteuid to uid %d failed: %s\n", runas_uid, strerror (errno));
+
+ exit (1);
+ }
+
+ DBG (DBG_WARN, "Dropped privileges to uid %d gid %d\n", runas_uid, runas_gid);
+ }
+}
+
+
+static void
run_standalone (char *user)
{
struct pollfd *fds = NULL;
@@ -2973,84 +3084,12 @@ run_standalone (char *user)
int i;
int ret;

- uid_t runas_uid = 0;
- gid_t runas_gid = 0;
- struct passwd *pwent;
- gid_t *grplist = NULL;
- struct group *grp;
- int ngroups = 0;
FILE *pidfile;

do_bindings (&nfds, &fds);

- if (run_mode != SANED_RUN_DEBUG)
+ if (run_foreground == SANE_FALSE)
{
- if (user)
- {
- pwent = getpwnam(user);
-
- if (pwent == NULL)
- {
- DBG (DBG_ERR, "FATAL ERROR: user %s not found on system\n", user);
- bail_out (1);
- }
-
- runas_uid = pwent->pw_uid;
- runas_gid = pwent->pw_gid;
-
- /* Get group list for runas_uid */
- grplist = (gid_t *)malloc(sizeof(gid_t));
-
- if (grplist == NULL)
- {
- DBG (DBG_ERR, "FATAL ERROR: cannot allocate memory for group list\n");
-
- exit (1);
- }
-
- ngroups = 1;
- grplist[0] = runas_gid;
-
- setgrent();
- while ((grp = getgrent()) != NULL)
- {
- int i = 0;
-
- /* Already added current group */
- if (grp->gr_gid == runas_gid)
- continue;
-
- while (grp->gr_mem[i])
- {
- if (strcmp(grp->gr_mem[i], user) == 0)
- {
- int need_to_add = 1, j;
-
- /* Make sure its not already in list */
- for (j = 0; j < ngroups; j++)
- {
- if (grp->gr_gid == grplist[i])
- need_to_add = 0;
- }
- if (need_to_add)
- {
- grplist = (gid_t *)realloc(grplist,
- sizeof(gid_t)*ngroups+1);
- if (grplist == NULL)
- {
- DBG (DBG_ERR, "FATAL ERROR: cannot reallocate memory for group list\n");
-
- exit (1);
- }
- grplist[ngroups++] = grp->gr_gid;
- }
- }
- i++;
- }
- }
- endgrent();
- }
-
DBG (DBG_MSG, "run_standalone: daemonizing now\n");

fd = open ("/dev/null", O_RDWR);
@@ -3093,42 +3132,13 @@ run_standalone (char *user)

setsid ();

- /* Drop privileges if requested */
- if (runas_uid > 0)
- {
- ret = setgroups(ngroups, grplist);
- if (ret < 0)
- {
- DBG (DBG_ERR, "FATAL ERROR: could not set group list: %s\n", strerror(errno));
-
- exit (1);
- }
-
- free(grplist);
-
- ret = setegid (runas_gid);
- if (ret < 0)
- {
- DBG (DBG_ERR, "FATAL ERROR: setegid to gid %d failed: %s\n", runas_gid, strerror (errno));
-
- exit (1);
- }
-
- ret = seteuid (runas_uid);
- if (ret < 0)
- {
- DBG (DBG_ERR, "FATAL ERROR: seteuid to uid %d failed: %s\n", runas_uid, strerror (errno));
-
- exit (1);
- }
-
- DBG (DBG_WARN, "Dropped privileges to uid %d gid %d\n", runas_uid, runas_gid);
- }
-
signal(SIGINT, sig_int_term_handler);
signal(SIGTERM, sig_int_term_handler);
}

+ if (user)
+ runas_user(user);
+
#ifdef WITH_AVAHI
DBG (DBG_INFO, "run_standalone: spawning Avahi process\n");
saned_avahi (fds, nfds);
@@ -3187,13 +3197,13 @@ run_standalone (char *user)
continue;
}

- if (run_mode == SANED_RUN_DEBUG)
- break; /* We have the only connection we're going to handle */
- else
- handle_client (fd);
+ handle_client (fd);
+
+ if (run_once == SANE_TRUE)
+ break; /* We have handled the only connection we're going to handle */
}

- if (run_mode == SANED_RUN_DEBUG)
+ if (run_once == SANE_TRUE)
break;
}

@@ -3201,14 +3211,6 @@ run_standalone (char *user)
close (fdp->fd);

free (fds);
-
- if (run_mode == SANED_RUN_DEBUG)
- {
- if (fd > 0)
- handle_connection (fd);
-
- bail_out(0);
- }
}


@@ -3299,12 +3301,17 @@ static void usage(char *me, int err)
fprintf (stderr,
"Usage: %s [OPTIONS]\n\n"
" Options:\n\n"
- " -a, --alone[=user] run standalone and fork in background as `user'\n"
- " -d, --debug[=level] run foreground with output to stderr\n"
- " and debug level `level' (default is 2)\n"
- " -s, --syslog[=level] run foreground with output to syslog\n"
- " and debug level `level' (default is 2)\n"
- " -b, --bind=addr bind address `addr'\n"
+ " -a, --alone[=user] equals to `-l -D -u user'\n"
+ " -l, --listen run in standalone mode (listen for connection)\n"
+ " -i, --inetd run in inetd mode (default)\n"
+ " -u, --user=user run as `user'\n"
+ " -D, --daemonize run in background\n"
+ " -f, --foreground run in foreground (default)\n"
+ " -o, --once exit after first client disconnects\n"
+ " -d, --debug=level set debug level `level' (default is 2)\n"
+ " -s, --syslog output to syslog (default)\n"
+ " -e, --stderr output to stderr\n"
+ " -b, --bind=addr bind address `addr' (default all interfaces)\n"
" -h, --help show this help message and exit\n", me);

exit(err);
@@ -3317,8 +3324,15 @@ static struct option long_options[] =
/* These options set a flag. */
{"help", no_argument, 0, 'h'},
{"alone", optional_argument, 0, 'a'},
- {"debug", optional_argument, 0, 'd'},
- {"syslog", optional_argument, 0, 's'},
+ {"listen", no_argument, 0, 'l'},
+ {"inetd", no_argument, 0, 'i'},
+ {"user", required_argument, 0, 'u'},
+ {"daemonize", no_argument, 0, 'D'},
+ {"foreground",no_argument, 0, 'f'},
+ {"once", no_argument, 0, 'o'},
+ {"debug", required_argument, 0, 'd'},
+ {"syslog", no_argument, 0, 's'},
+ {"stderr", no_argument, 0, 'e'},
{"bind", required_argument, 0, 'b'},
{0, 0, 0, 0 }
};
@@ -3342,20 +3356,44 @@ main (int argc, char *argv[])

numchildren = 0;
run_mode = SANED_RUN_INETD;
+ run_foreground = SANE_TRUE;
+ run_once = SANE_FALSE;

- while((c = getopt_long(argc, argv,"ha::d::s::b:", long_options, &long_index )) != -1)
+ while((c = getopt_long(argc, argv,"ha::liu:Dfod:seb:", long_options, &long_index )) != -1)
{
switch(c) {
case 'a':
run_mode = SANED_RUN_ALONE;
+ run_foreground = SANE_FALSE;
+ if (optarg)
+ user = optarg;
+ break;
+ case 'l':
+ run_mode = SANED_RUN_ALONE;
+ break;
+ case 'i':
+ run_mode = SANED_RUN_INETD;
+ break;
+ case 'u':
user = optarg;
break;
+ case 'D':
+ run_foreground = SANE_FALSE;
+ break;
+ case 'f':
+ run_foreground = SANE_TRUE;
+ break;
+ case 'o':
+ run_once = SANE_TRUE;
+ break;
case 'd':
- log_to_syslog = SANE_FALSE;
+ debug = atoi(optarg);
+ break;
case 's':
- run_mode = SANED_RUN_DEBUG;
- if(optarg)
- debug = atoi(optarg);
+ log_to_syslog = SANE_TRUE;
+ break;
+ case 'e':
+ log_to_syslog = SANE_FALSE;
break;
case 'b':
bind_addr = optarg;
@@ -3405,7 +3443,7 @@ main (int argc, char *argv[])
DBG (DBG_WARN, "saned from %s ready\n", PACKAGE_STRING);
}

- if ((run_mode == SANED_RUN_ALONE) || (run_mode == SANED_RUN_DEBUG))
+ if (run_mode == SANED_RUN_ALONE)
{
run_standalone(user);
}
--
2.11.0
--
sane-devel mailing list: sane-***@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-***@lists.alioth.debian.org
Luiz Angelo Daros de Luca
2017-07-22 08:45:25 UTC
Reply
Permalink
Raw Message
The two first patches are trivial bugfixes. However, this one proposes a
new organization
on saned options, as shown at:

https://alioth.debian.org/tracker/index.php?func=detail&aid=315747&group_id=30186&atid=410366

All flags now do one thing and normally have an opposite flag that can
deactivate it.
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). If not,
I can introduce new flags for those actions and revert -d/-s previous
behavior.
--
Luiz Angelo Daros de Luca
***@gmail.com
Luiz Angelo Daros de Luca
2017-08-13 06:10:57 UTC
Reply
Permalink
Raw Message
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.
Post by Luiz Angelo Daros de Luca
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?
- 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.
- -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.
Post by Luiz Angelo Daros de Luca
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 ;-)
Post by Luiz Angelo Daros de Luca
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
***@gmail.com
Olaf Meeuwissen
2017-09-09 06:10:51 UTC
Reply
Permalink
Raw Message
Hi Luiz,

Apologies for the late follow-up. Shame on me for pinging you on the
bug report for no follow-up for a long time and then ignoring it when
you promptly send some. :-(
Post by Luiz Angelo Daros de Luca
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.
Post by Luiz Angelo Daros de Luca
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?
- 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.
Let's leave that for the future then.
Post by Luiz Angelo Daros de Luca
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 you don't have any use for it, I'd say let's not bother and focus on
getting every option to do one thing and one thing only.
Post by Luiz Angelo Daros de Luca
If there is no clear use case, perhaps saned should not provide them.
- -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.
Rethinking this, I guess you're right. It's not much effort to manually
background a daemon, simply add an `&` at the end of the command-line.
Post by Luiz Angelo Daros de Luca
- -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.
Correct, so you don't have to do anything to make it the default ;-)
Post by Luiz Angelo Daros de Luca
- -l and make running in stand-alone mode the default because that's
what daemons ought to do anyway
[...] 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. ;-)
Agreed. Let's just get the command-line options untangled.
Post by Luiz Angelo Daros de Luca
I'll remove all options that replicates what is the default behavior. They
-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.
Me neither but I think I can manage (x)inetd.
Post by Luiz Angelo Daros de Luca
# 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.
I "fixed" that in c9356cb.
Post by Luiz Angelo Daros de Luca
I don't think there is any need to keep [...] options backwardly
compatible but it might be in order for saned to warn people that [...]
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?
Eh, I guess the NEWS file would be as good a place as any. Something
like

New with the development version, not yet released:

@Allan> Does that look okay?
Post by Luiz Angelo Daros de Luca
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.
Alright. As a fairly regular user of it, your rsync -a example
convinced me ;-)
Post by Luiz Angelo Daros de Luca
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.
Looks like any user with write access to the directory holding the PID
file can clobber it because

pidfile = fopen (SANED_PID_FILE, "w");

doesn't pass O_EXCL to the underlying open().

On my devuan box, /var/run is a symlink to /run which has 755 perms and
is owned by root. Digging further, /run is actually a mount point for a
tmpfs. But anyway, not all systems are created equally.
Post by Luiz Angelo Daros de Luca
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'll correct them directly (if there are any ;-). It's less overhead
for both of us.
Post by Luiz Angelo Daros de Luca
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,

PS: I'll be travelling a bit in the next two weeks so will probably be
late again in following up.
--
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
--
sane-devel mailing list: sane-***@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-***@lists.alioth.debian.org
Luiz Angelo Daros de Luca
2017-09-10 22:55:34 UTC
Reply
Permalink
Raw Message
Hi Olaf,

Apologies for the late follow-up. Shame on me for pinging you on the
Post by Olaf Meeuwissen
bug report for no follow-up for a long time and then ignoring it when
you promptly send some. :-(
No apologies needed. We know how it works.

I was just waiting for your reply. My changes were already at github (and
just rebased):
https://github.com/luizluca/sane-backends/tree/reorganize-saned-args
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
If there is no clear use case, perhaps saned should not provide them.
- -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
Post by Luiz Angelo Daros de Luca
more traumatic than changing any behavior of options.
Rethinking this, I guess you're right. It's not much effort to manually
background a daemon, simply add an `&` at the end of the command-line.
An `&` is not enough as there are some extra logic when going backgroud.
`-D` do just that.
Post by Olaf Meeuwissen
Something for the Release Notes as Changelog is built from git log. Is
Post by Luiz Angelo Daros de Luca
there any place to save relevant changes for the next release?
Eh, I guess the NEWS file would be as good a place as any. Something
like
@Allan> Does that look okay?
I'll just wait for the "NEWS" answered you asked @Allan in order to update
only that and send patches to ML.
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
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
Post by Luiz Angelo Daros de Luca
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
Post by Luiz Angelo Daros de Luca
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
Post by Luiz Angelo Daros de Luca
requirements) whether a file at PID file exists or not.
Looks like any user with write access to the directory holding the PID
file can clobber it because
pidfile = fopen (SANED_PID_FILE, "w");
doesn't pass O_EXCL to the underlying open().
On my devuan box, /var/run is a symlink to /run which has 755 perms and
is owned by root. Digging further, /run is actually a mount point for a
tmpfs. But anyway, not all systems are created equally.
I'll leave PID_FILE as is for now.
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
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'll correct them directly (if there are any ;-). It's less overhead
for both of us.
OK for me.
Post by Olaf Meeuwissen
PS: I'll be travelling a bit in the next two weeks so will probably be
late again in following up.
Nobody is at a hurry.

Regards,
Post by Olaf Meeuwissen
--
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
***@gmail.com
m. allan noah
2017-09-11 01:15:44 UTC
Reply
Permalink
Raw Message
Feel free to update the NEWS file, I'll incorporate your changes when
we ship 1.0.28

allan

On Sun, Sep 10, 2017 at 6:55 PM, Luiz Angelo Daros de Luca
Post by Luiz Angelo Daros de Luca
Hi Olaf,
Post by Olaf Meeuwissen
Apologies for the late follow-up. Shame on me for pinging you on the
bug report for no follow-up for a long time and then ignoring it when
you promptly send some. :-(
No apologies needed. We know how it works.
I was just waiting for your reply. My changes were already at github (and
https://github.com/luizluca/sane-backends/tree/reorganize-saned-args
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
If there is no clear use case, perhaps saned should not provide them.
- -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.
Rethinking this, I guess you're right. It's not much effort to manually
background a daemon, simply add an `&` at the end of the command-line.
An `&` is not enough as there are some extra logic when going backgroud.
`-D` do just that.
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
Something for the Release Notes as Changelog is built from git log. Is
there any place to save relevant changes for the next release?
Eh, I guess the NEWS file would be as good a place as any. Something
like
@Allan> Does that look okay?
only that and send patches to ML.
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
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.
Looks like any user with write access to the directory holding the PID
file can clobber it because
pidfile = fopen (SANED_PID_FILE, "w");
doesn't pass O_EXCL to the underlying open().
On my devuan box, /var/run is a symlink to /run which has 755 perms and
is owned by root. Digging further, /run is actually a mount point for a
tmpfs. But anyway, not all systems are created equally.
I'll leave PID_FILE as is for now.
Post by Olaf Meeuwissen
Post by Luiz Angelo Daros de Luca
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'll correct them directly (if there are any ;-). It's less overhead
for both of us.
OK for me.
Post by Olaf Meeuwissen
PS: I'll be travelling a bit in the next two weeks so will probably be
late again in following up.
Nobody is at a hurry.
Regards,
Post by Olaf Meeuwissen
--
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
--
"well, I stand up next to a mountain- and I chop it down with the edge
of my hand"
--
sane-devel mailing list: sane-***@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-***@lists.alioth.debian.org
Loading...