Re: LC_MESSAGES in xargs(1)
Hi Martijn, Martijn van Duren wrote on Thu, Jul 16, 2020 at 11:13:31PM +0200: > I gave a quick look at replacing prompt with readpassphrase(3), but that > would be more trouble than it's worth. (adjusting pledge, semantics > change in where the "?..." would be printed). > > Minor nits inline, but either way OK martijn@ Thanks to all three of you for checking, i just committed the patch. > On Thu, 2020-07-16 at 21:49 +0200, Ingo Schwarze wrote: [...] >> * There is no need to put a marker "return value ignored on >>purpose" where ignoring the return value almost never indicates >>a bug, like for fprintf, fflush, fclose. > If you're going to change this, please also remove them from run() Done. >> -(void)fflush(stderr); >> +fflush(stderr); > According to setvbuf(3): > The standard error stream stderr is initially unbuffered. > And since setvbuf is not called in xargs this seems superfluous to me. > If this is correct, there's a second fflush that can also be removed. I didn't mix that into this diff because i didn't want to have two unrelated, potentially non-trivial changes in the same commit. But if you think this is worth cleaning up, by all means, send a diff for it. I'm not very experienced with stdio buffering, so i'd hope you might get an OK from another developer. Yours, Ingo
Re: LC_MESSAGES in xargs(1)
I gave a quick look at replacing prompt with readpassphrase(3), but that
would be more trouble than it's worth. (adjusting pledge, semantics
change in where the "?..." would be printed).
Minor nits inline, but either way OK martijn@
On Thu, 2020-07-16 at 21:49 +0200, Ingo Schwarze wrote:
> Hi Jan,
>
> Jan Stary wrote on Thu, Jul 16, 2020 at 09:45:44AM +0200:
>
> > Does xargs need to set LC_MESSAGES?
>
> As it stands, your patch doesn't make much sense. It is true that
> it doesn't change behaviour, but it leaves more cruft behund than
> it tidies up.
>
> That said, i agree that we will never implement LC_MESSAGES.
>
> That allows a nice cleanup, simplifying the code and getting rid
> of several headers and several calls to complicated functions.
>
> Remarks:
> * .Tn was deprecated years ago.
> * On OpenBSD, as the manual page says, nl_langinfo(3) is really
>only useful for CODESET.
> * There is no need to put a marker "return value ignored on
>purpose" where ignoring the return value almost never indicates
>a bug, like for fprintf, fflush, fclose.
If you're going to change this, please also remove them from run()
> * Usually, we prefer getline(3) over fgetln(3), but none of the
>arguments making it better really applies here, so we can as
>well leave it. The only downside is that ttyfp needs to remain
>open until the stdio input buffer has been inspected, so we need
>another automatic variable, "doit", but it's still simpler than
>the malloc(3) dance that getline(3) would require.
Another option would be fgets, but probably still more hassle then
it's worth.
> OK?
> Ingo
>
>
> Index: xargs.1
> ===
> RCS file: /cvs/src/usr.bin/xargs/xargs.1,v
> retrieving revision 1.28
> diff -u -p -r1.28 xargs.1
> --- xargs.1 4 Jun 2014 06:48:33 - 1.28
> +++ xargs.1 16 Jul 2020 19:29:52 -
> @@ -213,11 +213,11 @@ at once.
> .It Fl p
> Echo each command to be executed and ask the user whether it should be
> executed.
> -An affirmative response,
> +If the answer starts with
> .Ql y
> -in the POSIX locale,
> -causes the command to be executed, any other response causes it to be
> -skipped.
> +or
> +.Ql Y ,
> +the command is executed; otherwise it is skipped.
> No commands are executed if the process is not attached to a terminal.
> .It Fl R Ar replacements
> Specify the maximum number of arguments that
> @@ -330,8 +330,7 @@ The flags
> are extensions to
> .St -p1003.1-2008 .
> .Pp
> -The meanings of the 123, 124, and 125 exit values were taken from
> -.Tn GNU
> +The meanings of the 123, 124, and 125 exit values were taken from GNU
> .Nm xargs .
> .Sh HISTORY
> The
> Index: xargs.c
> ===
> RCS file: /cvs/src/usr.bin/xargs/xargs.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 xargs.c
> --- xargs.c 12 Jun 2018 15:24:31 - 1.34
> +++ xargs.c 16 Jul 2020 19:29:52 -
> @@ -41,10 +41,7 @@
> #include
> #include
> #include
> -#include
> -#include
> #include
> -#include
> #include
> #include
> #include
> @@ -86,8 +83,6 @@ main(int argc, char *argv[])
> eofstr = "";
> Jflag = nflag = 0;
>
> - (void)setlocale(LC_MESSAGES, "");
> -
> /*
>* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
>* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
> @@ -607,26 +602,19 @@ waitchildren(const char *name, int waita
> static int
> prompt(void)
> {
> - regex_t cre;
> size_t rsize;
> - int match;
> char *response;
> FILE *ttyfp;
> + int doit = 0;
>
> if ((ttyfp = fopen(_PATH_TTY, "r")) == NULL)
> return (2); /* Indicate that the TTY failed to open. */
> - (void)fprintf(stderr, "?...");
> - (void)fflush(stderr);
> - if ((response = fgetln(ttyfp, )) == NULL ||
> - regcomp(, nl_langinfo(YESEXPR), REG_BASIC) != 0) {
> - (void)fclose(ttyfp);
> - return (0);
> - }
> - response[rsize - 1] = '\0';
> - match = regexec(, response, 0, NULL, 0);
> - (void)fclose(ttyfp);
> - regfree();
> - return (match == 0);
> + fprintf(stderr, "?...");
> + fflush(stderr);
According to setvbuf(3):
The standard error stream stderr is initially unbuffered.
And since setvbuf is not called in xargs this seems superfluous to me.
If this is correct, there's a second fflush that can also be removed.
> + response = fgetln(ttyfp, );
> + doit = response != NULL && (*response == 'y' || *response == 'Y');
> + fclose(ttyfp);
> + return (doit);
> }
>
> static void
>
Re: LC_MESSAGES in xargs(1)
On Thu, Jul 16, 2020 at 09:49:21PM +0200, Ingo Schwarze wrote: > That allows a nice cleanup, simplifying the code and getting rid > of several headers and several calls to complicated functions. OK kn
Re: LC_MESSAGES in xargs(1)
Hi Jan,
Jan Stary wrote on Thu, Jul 16, 2020 at 09:45:44AM +0200:
> Does xargs need to set LC_MESSAGES?
As it stands, your patch doesn't make much sense. It is true that
it doesn't change behaviour, but it leaves more cruft behund than
it tidies up.
That said, i agree that we will never implement LC_MESSAGES.
That allows a nice cleanup, simplifying the code and getting rid
of several headers and several calls to complicated functions.
Remarks:
* .Tn was deprecated years ago.
* On OpenBSD, as the manual page says, nl_langinfo(3) is really
only useful for CODESET.
* There is no need to put a marker "return value ignored on
purpose" where ignoring the return value almost never indicates
a bug, like for fprintf, fflush, fclose.
* Usually, we prefer getline(3) over fgetln(3), but none of the
arguments making it better really applies here, so we can as
well leave it. The only downside is that ttyfp needs to remain
open until the stdio input buffer has been inspected, so we need
another automatic variable, "doit", but it's still simpler than
the malloc(3) dance that getline(3) would require.
OK?
Ingo
Index: xargs.1
===
RCS file: /cvs/src/usr.bin/xargs/xargs.1,v
retrieving revision 1.28
diff -u -p -r1.28 xargs.1
--- xargs.1 4 Jun 2014 06:48:33 - 1.28
+++ xargs.1 16 Jul 2020 19:29:52 -
@@ -213,11 +213,11 @@ at once.
.It Fl p
Echo each command to be executed and ask the user whether it should be
executed.
-An affirmative response,
+If the answer starts with
.Ql y
-in the POSIX locale,
-causes the command to be executed, any other response causes it to be
-skipped.
+or
+.Ql Y ,
+the command is executed; otherwise it is skipped.
No commands are executed if the process is not attached to a terminal.
.It Fl R Ar replacements
Specify the maximum number of arguments that
@@ -330,8 +330,7 @@ The flags
are extensions to
.St -p1003.1-2008 .
.Pp
-The meanings of the 123, 124, and 125 exit values were taken from
-.Tn GNU
+The meanings of the 123, 124, and 125 exit values were taken from GNU
.Nm xargs .
.Sh HISTORY
The
Index: xargs.c
===
RCS file: /cvs/src/usr.bin/xargs/xargs.c,v
retrieving revision 1.34
diff -u -p -r1.34 xargs.c
--- xargs.c 12 Jun 2018 15:24:31 - 1.34
+++ xargs.c 16 Jul 2020 19:29:52 -
@@ -41,10 +41,7 @@
#include
#include
#include
-#include
-#include
#include
-#include
#include
#include
#include
@@ -86,8 +83,6 @@ main(int argc, char *argv[])
eofstr = "";
Jflag = nflag = 0;
- (void)setlocale(LC_MESSAGES, "");
-
/*
* POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
* caused some E2BIG errors, so it was changed to ARG_MAX - 4K. Given
@@ -607,26 +602,19 @@ waitchildren(const char *name, int waita
static int
prompt(void)
{
- regex_t cre;
size_t rsize;
- int match;
char *response;
FILE *ttyfp;
+ int doit = 0;
if ((ttyfp = fopen(_PATH_TTY, "r")) == NULL)
return (2); /* Indicate that the TTY failed to open. */
- (void)fprintf(stderr, "?...");
- (void)fflush(stderr);
- if ((response = fgetln(ttyfp, )) == NULL ||
- regcomp(, nl_langinfo(YESEXPR), REG_BASIC) != 0) {
- (void)fclose(ttyfp);
- return (0);
- }
- response[rsize - 1] = '\0';
- match = regexec(, response, 0, NULL, 0);
- (void)fclose(ttyfp);
- regfree();
- return (match == 0);
+ fprintf(stderr, "?...");
+ fflush(stderr);
+ response = fgetln(ttyfp, );
+ doit = response != NULL && (*response == 'y' || *response == 'Y');
+ fclose(ttyfp);
+ return (doit);
}
static void
LC_MESSAGES in xargs(1)
Does xargs need to set LC_MESSAGES? Jan Index: usr.bin/xargs/xargs.c === RCS file: /cvs/src/usr.bin/xargs/xargs.c,v retrieving revision 1.34 diff -u -p -r1.34 xargs.c --- usr.bin/xargs/xargs.c 12 Jun 2018 15:24:31 - 1.34 +++ usr.bin/xargs/xargs.c 16 Jul 2020 07:40:26 - @@ -42,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -85,8 +84,6 @@ main(int argc, char *argv[]) ep = environ; eofstr = ""; Jflag = nflag = 0; - - (void)setlocale(LC_MESSAGES, ""); /* * POSIX.2 limits the exec line length to ARG_MAX - 2K. Running that
