On Tuesday 05 February 2008, Tilman Schmidt wrote:
> [Originally sent on Sun, 3 Feb 2008 14:02:17 +0100 (CET). Resending
> because it hasn't appeared on the list yet.]
>
> From: Tilman Schmidt <tilman@xxxxxxx>
>
> Add an ldattach(8) utility program similar to the one in OpenBSD.
>
> Signed-off-by: Tilman Schmidt <tilman@xxxxxxx>
i really dont have an opinion on the utility itself ... never heard of it.
i'd wonder though if it'd be a better fit if it were integrated into the
setserial package ? (setserial.sf.net)
> --- a/sys-utils/ldattach.8 1970-01-01 01:00:00.000000000 +0100
> +++ b/sys-utils/ldattach.8 2008-02-03 13:00:56.000000000 +0100
is there really nothing you could cram under SEE ALSO ?
> --- a/sys-utils/ldattach.c 1970-01-01 01:00:00.000000000 +0100
> +++ b/sys-utils/ldattach.c 2008-02-03 13:00:56.000000000 +0100
> + while (ple->s && strcasecmp(ple->s, s))
> + ple++;
> + return ple->v;
seems like a for loop via ARRAY_SIZE(ld_table) would be nicer:
size_t i;
for (i = 0; i < ARRAY_SIZE(ld_table); ++i)
if (strcasecmp(ple[i].s, s))
return ple + i;
> +static speed_t lookup_speed(const char *s)
hrm, is there seriously no other function (either in the libc or util-linux)
we could reuse ? if not, the same comment as above with ARRAY_SIZE()
applies ...
> +static void usage(const char *prog)
> +{
> + fprintf(stderr,
> + "Usage: %s [ -d78neo12 ] [ -s <speed> ] <ldisc> <device>\n", prog);
> + exit(EXIT_FAILURE);
> +}
i feel like "prog" should be a program-level static rather than passing around
all the time. you should also pass in the exit status instead of assuming
EXIT_FAILURE ... that way you can have -h (for help) exit with 0 while
everything else exits with 1. and mark the function as noreturn.
> +int main(int argc, char **argv)
char *argv[]
> + case 's':
> + if ((speed = lookup_speed(optarg)) == B0) {
> + fprintf(stderr, "%s: bad speed: %s\n", prog, optarg);
> + exit(EXIT_FAILURE);
> + }
hmm, this doesnt seem right ... this does not allow for arbitrary baud rates
which newer versions of linux now supports. no point in starting off with a
hobbled program.
> + fprintf(stderr, "%s: bad line discipline: %s\n", prog, optarg);
> + exit(EXIT_FAILURE);
> ...
> + fprintf(stderr, "%s: cannot open %s: %s\n", prog, dev, strerror(errno));
> + exit(EXIT_FAILURE);
> ...
> + fprintf(stderr, "%s: %s is not a serial line\n", prog, dev);
> + exit(EXIT_FAILURE);
> ...
> + fprintf(stderr, "%s: cannot set %s to exclusive mode: %s\n",
> + prog, dev, strerror(errno));
> + exit(EXIT_FAILURE);
> ...
> + fprintf(stderr, "%s: cannot get terminal attributes for %s: %s\n",
> + prog, dev, strerror(errno));
> + exit(EXIT_FAILURE);
> ...
> <many more>
could do with writing an error function and/or macro to make things cleaner
> + /* Go into background if not in debug mode. */
> + if (!debug) {
> + /* Fork once to go into the background. */
> + switch (fork()) {
> + case 0: /* child: continue */
> + break;
> + case -1: /* failure: abort */
> + fprintf(stderr, "%s: cannot fork: %s\n", prog, strerror(errno));
> + exit(EXIT_FAILURE);
> + default: /* parent: done */
> + exit(EXIT_SUCCESS);
> + }
> +
> + /* Create new session. */
> + if (setsid() < 0) {
> + fprintf(stderr, "%s: cannot setsid: %s\n", prog, strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + /* Fork again to isolate from parent. */
> + switch (fork()) {
> + case 0: /* child: continue */
> + break;
> + case -1: /* failure: abort */
> + fprintf(stderr, "%s: cannot refork: %s\n", prog, strerror(errno));
> + exit(EXIT_FAILURE);
> + default: /* parent: done */
> + exit(EXIT_SUCCESS);
> + }
> +
> + /* Close unneeded files. */
> + chdir("/");
> + close(0);
> + close(1);
> + close(2);
> + }
isnt this simply duplicating daemon() functionality ?
-mike
Attachment:
signature.asc
Description: This is a digitally signed message part.
