[pacman-dev] [PATCH] Support reading package args from stdin

[Dan McGee]

On Thu, Nov 4, 2010 at 10:27 AM, Dave Reisner <d at 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 at 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 at 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