Bug #3356
closed
xargs needlessly duplicates stdio buffering
100%
Description
For reasons that are unfathomable to me, xargs seems to have its own limited implementation of getchar() and company. It seems that this could be greatly simplified to use the stdio library.
This will help with another issue filed separately, the RFE to add -0 for GNU xargs compatibility.
Files
Updated by Prasad Joshi almost 10 years ago
Hello, Garrett D'Amore would it be okay if I work on this?
Updated by Garrett D'Amore almost 10 years ago
I've long since had the code for this written, but I've not had good luck in getting anyone to code review it.
http://cr.illumos.org/~gdamore/xargs/
I'd welcome a code review. ;-)
Updated by Gary Mills almost 10 years ago
I was unable to find the original review request e-mail message to the developers list. Your changes are complex, although they appear correct. Could you provide us with test results, comparing the two xarg versions? That way we could be confident that the new version works, even in unusual cases.
Updated by Prasad Joshi almost 10 years ago
- File xargs_diff_top_of_gdamore_xargs_bug_3356_changes.patch added
The changes look good to me. There are few (not so important) changes I would like to suggest.
In function parseargs(int ac, char **av)
The av[i][index] is compared or assigned with NULL pointer, though this will work without an error, however in my humble opinion this should have been compared/assigned with '\\0' or 0.
Just above this function, there is typo in the comment, which again is not very important, but may be fixed.
- * -l -> "-i "1"
- * -l10 -> "-i "10"
+ * -l -> "-l "1"
+ * -l10 -> "-l "10"
In main function, I feel it is good to add extra brackets around && and || conditional if statement.- (PER_LINE && N_lines 0 || N_ARGS && N_args 0))
+ ((PER_LINE && N_lines) 0 || (N_ARGS && N_args) 0))
I am attaching a patch with all of these changes. Please check if they are acceptable.
Updated by Garrett D'Amore almost 10 years ago
- Assignee set to Garrett D'Amore
- % Done changed from 0 to 90
Updated by Garrett D'Amore almost 10 years ago
Prasad, you changed the code and broke it with your changes -- it doesn't work at all due to changing the specific logic of the boolean tests. I'm rejecting your diff.
Updated by Prasad Joshi almost 10 years ago
Garrett D'Amore wrote:
Prasad, you changed the code and broke it with your changes -- it doesn't work at all due to changing the specific logic of the boolean tests. I'm rejecting your diff.
Strange it worked for me.
It would be very kind if you can share more details. Can you please, if possible, let me know the testcase which fails?
Updated by Garrett D'Amore almost 10 years ago
Very simple tests, like find . -print0 | xargs -0 echo fail.
Even without -print0 and -0, the output is flat out wrong. You've misinterpreted the boolean expressions and placed grouping operators at the wrong locations, causing the expressions to be mis-evaluated and resulting in early termination of the result.
Try comparing against stock xargs, or gxargs, to verify your test results. And use a non-trivial directory as well. :-)
Updated by Prasad Joshi almost 10 years ago
- File xargs_diff_top_of_gdamore_xargs_bug_3356_changes.patch added
It was my mistake during testing. There are 2 xargs, which I did not know initially. I had modified xargs compiled and installed on my machine. This modified binary is located at /bin/xargs.
However the default xargs which supports all of these options (has support for -0) and is located at /usr/bin/. When I had ran xargs it must have picked /usr/bin/xargs and it worked for me.
Anyways the modified patch is attached and it is test, it works correctly.
Please let me know if it works on your machine. I am sorry for any inconvenience this might have caused.
Updated by Prasad Joshi almost 10 years ago
- File deleted (
xargs_diff_top_of_gdamore_xargs_bug_3356_changes.patch)
Updated by Prasad Joshi almost 10 years ago
Updated by Prasad Joshi almost 10 years ago
- File deleted (
xargs_diff_top_of_gdamore_xargs_bug_3356_changes.patch)
Updated by Garrett D'Amore almost 10 years ago
You probably have GNU xargs as /usr/bin/xargs. I'm guessing. I don't know.
Apart from changing some NULLs to 0s (this was old code, actually), I don't see any functional changes intended. They look like style fixes only (again, NULLs to 0s, and an attempt to make order of precedence explicit by adding parenthesis.)
Updated by Prasad Joshi almost 10 years ago
Garrett D'Amore wrote:
You probably have GNU xargs as /usr/bin/xargs. I'm guessing. I don't know.
I think so.
Apart from changing some NULLs to 0s (this was old code, actually), I don't see any functional changes intended. They look like style fixes only (again, NULLs to 0s, and an attempt to make order of precedence explicit by adding parenthesis.)
It makes sense to change NULL to 0 since, the quantity (or variable) used in comparison is a character and not a pointer. I have dropped the parenthesis fix in new patch since, there are so many places in the code where it is apparent.
Updated by Garrett D'Amore over 9 years ago
- Status changed from New to Resolved
- % Done changed from 90 to 100
commit 03fc868668dd42b1b163d1fb8af3968f7283a7eb
Author: Garrett D'Amore <garrett@dey-sys.com>
Date: Tue Aug 27 13:00:09 2013 -0700
672 xargs doesn't support -0
3356 xargs needlessly duplicates stdio buffering
Reviewed by: Prasad Joshi <prasadjoshi124@gmail.com>
Reviewed by: Gary Mills <gary_mills@fastmail.fm>
Approved by: Robert Mustacchi <rm@joyent.com>
Updated by Garrett D'Amore over 9 years ago
Prasad, if you want to submit your patch with the changes of NULLs to 0s, please open a new bug. That's legacy code anyway.