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

[Dave Reisner]

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 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.
> 

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
>