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

[Dave Reisner]

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?

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