* Re: [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface.
2009-04-06 7:56 [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface Andre Prendel
@ 2009-04-19 7:39 ` Jean Delvare
2009-04-21 7:35 ` Andre Prendel
2009-04-21 8:45 ` Jean Delvare
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2009-04-19 7:39 UTC (permalink / raw)
To: lm-sensors
On Mon, 6 Apr 2009 09:56:18 +0200, Andre Prendel wrote:
> Remove the hidden commandline interface of sensord.
>
> Sensord can be invoked as an console application. Therefor a link
> named sensors pointing to sensord is needed. That is very
> intransparent and IMO useless. Printing sensor values to console is
> done by the sensors tool.
I agree. I guess the original idea was that "sensord" would possibly
ultimately supersede "sensors" but that did not happen and I don't
think this will ever happen due to the extra dependencies sensord
requires (librrd).
> ---
>
> args.c | 38 +++++++++-----------------------------
> 1 file changed, 9 insertions(+), 29 deletions(-)
>
> --- quilt-sensors.orig/prog/sensord/args.c 2009-03-24 21:53:35.000000000 +0100
> +++ quilt-sensors/prog/sensord/args.c 2009-03-24 21:54:17.000000000 +0100
> @@ -127,19 +127,6 @@
> "the RRD file configuration must EXACTLY match the sensors that are used. If\n"
> "your configuration changes, delete the old RRD file and restart sensord.\n";
>
> -static const char *appSyntax > - " -a, --alarm-scan -- only scan for alarms\n"
> - " -s, --set -- execute set statements (root only)\n"
> - " -r, --rrd-file <file> -- only update RRD file\n"
> - " -c, --config-file <file> -- configuration file\n"
> - " -d, --debug -- display some debug information\n"
> - " -v, --version -- display version and exit\n"
> - " -h, --help -- display help and exit\n"
> - "\n"
> - "Specify the filename `-' to read the config file from stdin.\n"
> - "\n"
> - "If no chips are specified, all chip info will be printed.\n";
> -
> static const char *daemonShortOptions = "i:l:t:Tf:r:c:p:advhg:";
>
> static const struct option daemonLongOptions[] = {
> @@ -159,19 +146,6 @@
> { NULL, 0, NULL, 0 }
> };
>
> -static const char *appShortOptions = "asr:c:dvh";
> -
> -static const struct option appLongOptions[] = {
> - { "alarm-scan", no_argument, NULL, 'a' },
> - { "set", no_argument, NULL, 's' },
> - { "rrd-file", required_argument, NULL, 'r' },
> - { "config-file", required_argument, NULL, 'c' },
> - { "debug", no_argument, NULL, 'd' },
> - { "version", no_argument, NULL, 'v' },
> - { "help", no_argument, NULL, 'h' },
> - { NULL, 0, NULL, 0 }
> -};
> -
> int parseArgs(int argc, char **argv)
> {
> int c;
> @@ -179,8 +153,14 @@
> const struct option *longOptions;
>
> isDaemon = (argv[0][strlen (argv[0]) - 1] = 'd');
> - shortOptions = isDaemon ? daemonShortOptions : appShortOptions;
> - longOptions = isDaemon ? daemonLongOptions : appLongOptions;
> + if (!isDaemon) {
> + fprintf(stderr, "Sensord no longer runs as an commandline"
> + " application.\n");
> + return -1;
> + }
> +
> + shortOptions = daemonShortOptions;
> + longOptions = daemonLongOptions;
I presume we could go one step further and rename daemonShortOptions
and daemonLongOptions to shortOptions and longOptions, respectively?
Or, alternatively, use daemonShortOptions and daemonLongOptions in the
code below.
>
> while ((c = getopt_long(argc, argv, shortOptions, longOptions, NULL))
> != EOF) {
> @@ -235,7 +215,7 @@
> break;
> case 'h':
> printf("Syntax: %s {options} {chips}\n%s", argv[0],
> - isDaemon ? daemonSyntax : appSyntax);
> + daemonSyntax);
> exit(EXIT_SUCCESS);
> break;
> case ':':
There are many other references to isDaemon left in the code, which
suggests that there is room for many additional cleanups. Are you going
to do this?
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface.
2009-04-06 7:56 [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface Andre Prendel
2009-04-19 7:39 ` Jean Delvare
@ 2009-04-21 7:35 ` Andre Prendel
2009-04-21 8:45 ` Jean Delvare
2 siblings, 0 replies; 4+ messages in thread
From: Andre Prendel @ 2009-04-21 7:35 UTC (permalink / raw)
To: lm-sensors
On Sun, Apr 19, 2009 at 09:39:54AM +0200, Jean Delvare wrote:
> On Mon, 6 Apr 2009 09:56:18 +0200, Andre Prendel wrote:
> > Remove the hidden commandline interface of sensord.
> >
> > Sensord can be invoked as an console application. Therefor a link
> > named sensors pointing to sensord is needed. That is very
> > intransparent and IMO useless. Printing sensor values to console is
> > done by the sensors tool.
>
> I agree. I guess the original idea was that "sensord" would possibly
> ultimately supersede "sensors" but that did not happen and I don't
> think this will ever happen due to the extra dependencies sensord
> requires (librrd).
>
> > ---
> >
> > args.c | 38 +++++++++-----------------------------
> > 1 file changed, 9 insertions(+), 29 deletions(-)
> >
> > --- quilt-sensors.orig/prog/sensord/args.c 2009-03-24 21:53:35.000000000 +0100
> > +++ quilt-sensors/prog/sensord/args.c 2009-03-24 21:54:17.000000000 +0100
> > @@ -127,19 +127,6 @@
> > "the RRD file configuration must EXACTLY match the sensors that are used. If\n"
> > "your configuration changes, delete the old RRD file and restart sensord.\n";
> >
> > -static const char *appSyntax > > - " -a, --alarm-scan -- only scan for alarms\n"
> > - " -s, --set -- execute set statements (root only)\n"
> > - " -r, --rrd-file <file> -- only update RRD file\n"
> > - " -c, --config-file <file> -- configuration file\n"
> > - " -d, --debug -- display some debug information\n"
> > - " -v, --version -- display version and exit\n"
> > - " -h, --help -- display help and exit\n"
> > - "\n"
> > - "Specify the filename `-' to read the config file from stdin.\n"
> > - "\n"
> > - "If no chips are specified, all chip info will be printed.\n";
> > -
> > static const char *daemonShortOptions = "i:l:t:Tf:r:c:p:advhg:";
> >
> > static const struct option daemonLongOptions[] = {
> > @@ -159,19 +146,6 @@
> > { NULL, 0, NULL, 0 }
> > };
> >
> > -static const char *appShortOptions = "asr:c:dvh";
> > -
> > -static const struct option appLongOptions[] = {
> > - { "alarm-scan", no_argument, NULL, 'a' },
> > - { "set", no_argument, NULL, 's' },
> > - { "rrd-file", required_argument, NULL, 'r' },
> > - { "config-file", required_argument, NULL, 'c' },
> > - { "debug", no_argument, NULL, 'd' },
> > - { "version", no_argument, NULL, 'v' },
> > - { "help", no_argument, NULL, 'h' },
> > - { NULL, 0, NULL, 0 }
> > -};
> > -
> > int parseArgs(int argc, char **argv)
> > {
> > int c;
> > @@ -179,8 +153,14 @@
> > const struct option *longOptions;
> >
> > isDaemon = (argv[0][strlen (argv[0]) - 1] = 'd');
> > - shortOptions = isDaemon ? daemonShortOptions : appShortOptions;
> > - longOptions = isDaemon ? daemonLongOptions : appLongOptions;
> > + if (!isDaemon) {
> > + fprintf(stderr, "Sensord no longer runs as an commandline"
> > + " application.\n");
> > + return -1;
> > + }
> > +
> > + shortOptions = daemonShortOptions;
> > + longOptions = daemonLongOptions;
>
> I presume we could go one step further and rename daemonShortOptions
> and daemonLongOptions to shortOptions and longOptions, respectively?
> Or, alternatively, use daemonShortOptions and daemonLongOptions in the
> code below.
That's right. I will send you an updated patch series.
>
> >
> > while ((c = getopt_long(argc, argv, shortOptions, longOptions, NULL))
> > != EOF) {
> > @@ -235,7 +215,7 @@
> > break;
> > case 'h':
> > printf("Syntax: %s {options} {chips}\n%s", argv[0],
> > - isDaemon ? daemonSyntax : appSyntax);
> > + daemonSyntax);
> > exit(EXIT_SUCCESS);
> > break;
> > case ':':
>
> There are many other references to isDaemon left in the code, which
> suggests that there is room for many additional cleanups. Are you going
> to do this?
Will fix this in v2 too. What about the two occurrences at the
beginning of parseArgs? I would prefer leaving that to inform the user
about the change of the behaviour.
Andre
> Thanks,
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface.
2009-04-06 7:56 [lm-sensors] [PATCH 1/8] sensord: Remove commandline interface Andre Prendel
2009-04-19 7:39 ` Jean Delvare
2009-04-21 7:35 ` Andre Prendel
@ 2009-04-21 8:45 ` Jean Delvare
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2009-04-21 8:45 UTC (permalink / raw)
To: lm-sensors
On Tue, 21 Apr 2009 09:35:33 +0200, Andre Prendel wrote:
> On Sun, Apr 19, 2009 at 09:39:54AM +0200, Jean Delvare wrote:
> > There are many other references to isDaemon left in the code, which
> > suggests that there is room for many additional cleanups. Are you going
> > to do this?
>
> Will fix this in v2 too. What about the two occurrences at the
> beginning of parseArgs? I would prefer leaving that to inform the user
> about the change of the behaviour.
Yes, this is fine with me. We can remove them later, in a year or two.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread