[pacman-dev] [PATCH] Support reading package args from stdin
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.: pacman -Qdtq | pacman -Rs This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman. Revised from the original 7/24 submission at Allan's request to check the return value of freopen(). Signed-off-by: Dave Reisner <d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif +#include <ctype.h> /* isspace */ #include <stdlib.h> /* atoi */ #include <stdio.h> #include <limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); } + /* read package arguments from stdin if we have none yet */ + if(!pm_targets && !isatty(0)) { + char line[BUFSIZ]; + int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { + line[i] = 0; + /* avoid adding zero length arg when multiple spaces separate args */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; + } + } + /* end of stream -- check for data still in line buffer */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) { + pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + /* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) { -- 1.7.3.2
On Thu, Nov 4, 2010 at 9:20 AM, Dave Reisner <d@falconindy.com> wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
Signed-off-by: Dave Reisner <d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include <ctype.h> /* isspace */ #include <stdlib.h> /* atoi */ #include <stdio.h> #include <limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets && !isatty(0)) { isatty(fileno(stdin)) is technically more correct, no? It also would handle the case where someone reassigned stdin.
+ char line[BUFSIZ]; Another constant for a buffer size? If there is one in our codebase I'd rather we use that, and of course it would be even better to not have an arbitrary limit on this at all. See later comment.
+ int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { + line[i] = 0; Use '\0' please, this is a char, not an int.
+ /* avoid adding zero length arg when multiple spaces separate args */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; i++; we don't use the pre-increment operator anywhere and it makes no difference so we should style-match.
+ } + } Uhhhhh- buffer overflow condition?
+ /* end of stream -- check for data still in line buffer */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) { + pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + So the other thing I'm thinking here is it breaks anyone that might have piped into pacman before. I'm not sure if we care, but using /usr/bin/yes did work before. Is it worth guarding this functionality behind an option (--use-stdin or something) instead of the isatty() check (which would take place on the freopen() call instead)?
/* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) { -- 1.7.3.2
On Thu, Nov 04, 2010 at 09:40:46AM -0500, Dan McGee wrote:
On Thu, Nov 4, 2010 at 9:20 AM, Dave Reisner <d@falconindy.com> wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
Signed-off-by: Dave Reisner <d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include <ctype.h> /* isspace */ #include <stdlib.h> /* atoi */ #include <stdio.h> #include <limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets && !isatty(0)) { isatty(fileno(stdin)) is technically more correct, no? It also would handle the case where someone reassigned stdin.
+ char line[BUFSIZ]; Another constant for a buffer size? If there is one in our codebase I'd rather we use that, and of course it would be even better to not have an arbitrary limit on this at all. See later comment.
BUFSIZ is provided by stdlib.h and is supposed to be tuned to be architecture specific. I believe POSIX guarnatees it to be a minimum of 256. PATH_MAX seems to make a fairly common appearance throughout the code -- would you prefer I use that instead?
+ int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { + line[i] = 0; Use '\0' please, this is a char, not an int.
+ /* avoid adding zero length arg when multiple spaces separate args */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; i++; we don't use the pre-increment operator anywhere and it makes no difference so we should style-match.
+ } + } Uhhhhh- buffer overflow condition?
What should the behavior be if an overflow is detected? Truncate the argument and fast forward to the next delimeter? Restart parsing right after the truncated argument? Fail entirely?
+ /* end of stream -- check for data still in line buffer */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) { + pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + So the other thing I'm thinking here is it breaks anyone that might have piped into pacman before. I'm not sure if we care, but using /usr/bin/yes did work before. Is it worth guarding this functionality behind an option (--use-stdin or something) instead of the isatty() check (which would take place on the freopen() call instead)?
I hadn't considered that, but it's certainly a valid concern. My opinion is that having to specify an extra flag to read from stdin makes this a lot less appealing. I'd be personally patching that _out_ if that were deemed to be the desirable path. Just an opinion, though.
/* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) { -- 1.7.3.2
On Thu, Nov 4, 2010 at 10:27 AM, Dave Reisner <d@falconindy.com> wrote:
On Thu, Nov 04, 2010 at 09:40:46AM -0500, Dan McGee wrote:
On Thu, Nov 4, 2010 at 9:20 AM, Dave Reisner <d@falconindy.com> wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
Signed-off-by: Dave Reisner <d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include <ctype.h> /* isspace */ #include <stdlib.h> /* atoi */ #include <stdio.h> #include <limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets && !isatty(0)) { isatty(fileno(stdin)) is technically more correct, no? It also would handle the case where someone reassigned stdin.
+ char line[BUFSIZ]; Another constant for a buffer size? If there is one in our codebase I'd rather we use that, and of course it would be even better to not have an arbitrary limit on this at all. See later comment.
BUFSIZ is provided by stdlib.h and is supposed to be tuned to be architecture specific. I believe POSIX guarnatees it to be a minimum of 256. PATH_MAX seems to make a fairly common appearance throughout the code -- would you prefer I use that instead?
Best of two evils? Yeah, just for consistency that might make more sense. We could really use some dynamic buffer code, but that isn't really where you wanted to go with this patch. :)
+ int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { I also forgot to mention- we do some (unsigned char) cast stuff in util.c in the isspace() calls; you might want to track down why that is there and see if we should/need to do it here to.
+ line[i] = 0; Use '\0' please, this is a char, not an int.
+ /* avoid adding zero length arg when multiple spaces separate args */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; i++; we don't use the pre-increment operator anywhere and it makes no difference so we should style-match.
+ } + } Uhhhhh- buffer overflow condition?
What should the behavior be if an overflow is detected? Truncate the argument and fast forward to the next delimeter? Restart parsing right after the truncated argument? Fail entirely?
As a "user", I would find the truncate/fast-forward idea slightly broken- it would throw away part of my input but not all of it? I'm leaning toward fail entirely, but could be swayed if someone else has a good reason.
+ /* end of stream -- check for data still in line buffer */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) { + pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + So the other thing I'm thinking here is it breaks anyone that might have piped into pacman before. I'm not sure if we care, but using /usr/bin/yes did work before. Is it worth guarding this functionality behind an option (--use-stdin or something) instead of the isatty() check (which would take place on the freopen() call instead)?
I hadn't considered that, but it's certainly a valid concern. My opinion is that having to specify an extra flag to read from stdin makes this a lot less appealing. I'd be personally patching that _out_ if that were deemed to be the desirable path. Just an opinion, though.
/* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) { -- 1.7.3.2
On Thu, Nov 04, 2010 at 10:34:29AM -0500, Dan McGee wrote:
On Thu, Nov 4, 2010 at 10:27 AM, Dave Reisner <d@falconindy.com> wrote:
On Thu, Nov 04, 2010 at 09:40:46AM -0500, Dan McGee wrote:
On Thu, Nov 4, 2010 at 9:20 AM, Dave Reisner <d@falconindy.com> wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
Signed-off-by: Dave Reisner <d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include <ctype.h> /* isspace */ #include <stdlib.h> /* atoi */ #include <stdio.h> #include <limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets && !isatty(0)) { isatty(fileno(stdin)) is technically more correct, no? It also would handle the case where someone reassigned stdin.
+ char line[BUFSIZ]; Another constant for a buffer size? If there is one in our codebase I'd rather we use that, and of course it would be even better to not have an arbitrary limit on this at all. See later comment.
BUFSIZ is provided by stdlib.h and is supposed to be tuned to be architecture specific. I believe POSIX guarnatees it to be a minimum of 256. PATH_MAX seems to make a fairly common appearance throughout the code -- would you prefer I use that instead?
Best of two evils? Yeah, just for consistency that might make more sense. We could really use some dynamic buffer code, but that isn't really where you wanted to go with this patch. :)
+ int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { I also forgot to mention- we do some (unsigned char) cast stuff in util.c in the isspace() calls; you might want to track down why that is there and see if we should/need to do it here to.
pacman does this correctly. isspace(3) states: [isspace] check[s] whether c, which must have the value of an unsigned char or EOF, falls into a certain character class according to the current locale.
+ line[i] = 0; Use '\0' please, this is a char, not an int.
+ /* avoid adding zero length arg when multiple spaces separate args */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; i++; we don't use the pre-increment operator anywhere and it makes no difference so we should style-match.
+ } + } Uhhhhh- buffer overflow condition?
What should the behavior be if an overflow is detected? Truncate the argument and fast forward to the next delimeter? Restart parsing right after the truncated argument? Fail entirely?
As a "user", I would find the truncate/fast-forward idea slightly broken- it would throw away part of my input but not all of it? I'm leaning toward fail entirely, but could be swayed if someone else has a good reason.
I agree. The user has done something wrong and pacman shouldn't try to figure it out. I'm a little uncertain about the best way to handle this -- I'm sure you'll have something to say about my next submission. =P
+ /* end of stream -- check for data still in line buffer */ + if(i > 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) { + pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + So the other thing I'm thinking here is it breaks anyone that might have piped into pacman before. I'm not sure if we care, but using /usr/bin/yes did work before. Is it worth guarding this functionality behind an option (--use-stdin or something) instead of the isatty() check (which would take place on the freopen() call instead)?
I hadn't considered that, but it's certainly a valid concern. My opinion is that having to specify an extra flag to read from stdin makes this a lot less appealing. I'd be personally patching that _out_ if that were deemed to be the desirable path. Just an opinion, though.
/* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) { -- 1.7.3.2
On 05/11/10 00:20, Dave Reisner wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Dan: this sold the patch to me.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
FYI, comments like this can go below the "---" two lines down. That way they do not get included in the actual git commit.
Signed-off-by: Dave Reisner<d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include<ctype.h> /* isspace */ #include<stdlib.h> /* atoi */ #include<stdio.h> #include<limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets&& !isatty(0)) { + char line[BUFSIZ]; + int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { + line[i] = 0; + /* avoid adding zero length arg when multiple spaces separate args */ + if(i> 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; + } + } + /* end of stream -- check for data still in line buffer */ + if(i> 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) {
Two things here. The HACKING file says to use if(foo() == 0) rather than if(!foo()). But freopen returns a FILE*. So we really should be checking if that == NULL? Also, does the FILE* that freopen returns need closed here or is that fine as is? I get slight confused...
+ pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + /* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) {
On Fri, Nov 05, 2010 at 12:42:54AM +1000, Allan McRae wrote:
On 05/11/10 00:20, Dave Reisner wrote:
Only occurs if no arguments were provided directly. Arguments can be separated by any amount of valid whitespace. This allows for piping into pacman from other programs or from itself, e.g.:
pacman -Qdtq | pacman -Rs
This is better than using xargs, as xargs will not reconnect stdin to the terminal. The above operation performed using xargs would require the --noconfirm flag to be passed to pacman.
Dan: this sold the patch to me.
Revised from the original 7/24 submission at Allan's request to check the return value of freopen().
FYI, comments like this can go below the "---" two lines down. That way they do not get included in the actual git commit.
Cool beans. I should really look at the man page for git-send-email one of these days.
Signed-off-by: Dave Reisner<d@falconindy.com> --- src/pacman/pacman.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 15abecc..6f40a97 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -26,6 +26,7 @@ #define PACKAGE_VERSION GIT_VERSION #endif
+#include<ctype.h> /* isspace */ #include<stdlib.h> /* atoi */ #include<stdio.h> #include<limits.h> @@ -1305,6 +1306,32 @@ int main(int argc, char *argv[]) cleanup(ret); }
+ /* read package arguments from stdin if we have none yet */ + if(!pm_targets&& !isatty(0)) { + char line[BUFSIZ]; + int i = 0; + while((line[i] = fgetc(stdin)) != EOF) { + if(isspace(line[i])) { + line[i] = 0; + /* avoid adding zero length arg when multiple spaces separate args */ + if(i> 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + i = 0; + } + } else { + ++i; + } + } + /* end of stream -- check for data still in line buffer */ + if(i> 0) { + pm_targets = alpm_list_add(pm_targets, strdup(line)); + } + if (!freopen(ctermid(NULL), "r", stdin)) {
Two things here. The HACKING file says to use if(foo() == 0) rather than if(!foo()). But freopen returns a FILE*. So we really should be checking if that == NULL? Also, does the FILE* that freopen returns need closed here or is that fine as is? I get slight confused...
Just above where my patch falls in, is this chunk (line 1275): if(!isatty()) { config->noprogressbar = 1; } The reopened stdin should be closed as usual by the kernel on exit. We only need to check for the presence of a FILE* being returned. When successful, the function just returns the value of the third argument -- stdin in this case.
+ pm_printf(PM_LOG_ERROR, _("failed to reopen stdin for reading: (%s)\n"), + strerror(errno)); + } + } + /* parse the config file */ ret = parseconfig(config->configfile); if(ret != 0) {
participants (3)
-
Allan McRae
-
Dan McGee
-
Dave Reisner