search

wotwot / pdsh

Automatically exported from code.google.com/p/pdsh
GNU General Public License v2.0
0 stars 0 forks source link

[patch] PDSH_SSH_ARGS undocumented behavior change #25

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
``$PDSH_SSH_ARGS`` has new undocumented requirement of
specifying host and username format characters for
expansion in pipecmd().

These format chars appear to be intended for internal
pdsh usage and probably should not be exposed to the
user (and required as they are now), breaking
compatibility: My usual login scripts broke after
upgrading pdsh from 2.11 to 2.23.

The manual seems to say that ``$PDSH_SSH_ARGS`` is
strictly an ssh command line; it does not say anything
about the user needing to supply the special ``%h`` and
``%l`` format characters in there.  After moving from
2.11 to trunk it became a requirement to specify these
format chars if ``$PDSH_SSH_ARGS`` was used.

Looks like this was broken in r1143 during refactor of
ssh module to use pipecmd.

Following patch is intended to restore original
behavior while preserving the changes from r1143.

Please apply, thanks.

-- 
Scott

Original issue reported on code.google.com by scott.m....@gmail.com on 27 Apr 2011 at 2:00

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for the report and the patch.

I have been thinking about this one and unfortunately I'd like to keep
the current functionality of PDSH_SSH_ARGS as it is very useful for testing
and since it crept into a couple of releases.

However, you have very valid points about the change in behavior and the 
incorrect
documentation. (The %u %h %n parameters are documented only in the section
about the rcmd/exec module).

What I'd like to try, if it is ok with you, is to scan PDSH_SSH_ARGS if supplied
for %u and %h, and if they are not found, then append "-l%u" and "%h" to the
ssh arguments. It is not ideal, but in that way we can hopefully preserve both
the new and old behavior. (I would like to allow users some way to override 
*all*
the ssh args)

I'll work on a proposed patch later today. I am sorry, and I really appreciate 
that
you attach patches to your bug reports (I don't want to discourage that!) :-)

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 8:29

GoogleCodeExporter commented 9 years ago 
Oh, I will also update documentation.

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 8:30

GoogleCodeExporter commented 9 years ago 
Yeah, when reading the code revisions I had seen that
the ssh string had been factored out to use the same
code as the new ``-R exec`` module.  That sort of makes
sense except that the code became broken for its
previously intended use :-)

I tried building new packages with it and see the test
suite failure... test 8 relies on::

    OUTPUT=$(PDSH_SSH_ARGS="ssh -p 922 -l%u %h" pdsh \
    -luser -Rssh -wfoo hostname)

but this is again the same string anyways (``-l%u %h``)
so that portion of the string could just be removed
from the test after the reverting to the old behavior.

However, I do agree that not being able to specify the
whole thing in the first place and hardcoding it is bad.

I think scanning the string for format characters may
be a hack; either a new $PDSH_SSH_FMTARGS should be
made or just keep the behavior from trunk and document
it.  It's ok to break stuff if it's better...
downstream can always deal with that :-) We are lucky
after all to have source to begin with.

I can make a try at a PDSH_SSH_FMTARGS if you like that
approach, or I'll just make site-local packages until
I'm ready to update all my shell profiles.

Thanks for looking at this and considering all the
options.

-- 
Scott

Original comment by scott.m....@gmail.com on 27 Apr 2011 at 8:53

GoogleCodeExporter commented 9 years ago 
btw ``PDSH_SSH_ARGS`` as a name does not make sense
anyways for the current behavior; they aren't just
arguments but also include the executable name ``ssh``.
It's actually a whole command string.

They *were* "just arguments" before.  I think just a
new environment variable altogether should be used for
the current behavior, but either way does make sense.

Original comment by scott.m....@gmail.com on 27 Apr 2011 at 9:00

GoogleCodeExporter commented 9 years ago 
oh and also, thinking more about it: scanning for a
format character might actually not be so bad because
if they were not supplied, they would *have* to be
provided by pdsh or the resulting command would be
broken, but if any were supplied then the user would
almost certainly be supplying the whole command line.

btw my patch also makes ``PDSH_SSH_ARGS_APPEND`` be
appended to ``PDSH_SSH_ARGS`` which I don't think was
old behavior but seems to make sense semantically.

Original comment by scott.m....@gmail.com on 27 Apr 2011 at 9:04

GoogleCodeExporter commented 9 years ago 
> btw my patch also makes ``PDSH_SSH_ARGS_APPEND`` be
> appended to ``PDSH_SSH_ARGS`` which I don't think was
> old behavior but seems to make sense semantically.

Ah, yes that is a bug there. I'm going to open a separate issue
on that one.

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 9:05

GoogleCodeExporter commented 9 years ago 
> > btw my patch also makes ``PDSH_SSH_ARGS_APPEND`` be
> > appended to ``PDSH_SSH_ARGS`` which I don't think was
> > old behavior but seems to make sense semantically.

> Ah, yes that is a bug there. I'm going to open a separate issue
> on that one.

Hm, actually do you mean PDSH_SSH_ARGS_APPEND are *prepended*? Yeah, that was
a compromise I had to make at some point. Assuming the argument order doesn't
matter it shouldn't be a problem, but I agree that it is not intuitive for sure.

mark

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 9:08

GoogleCodeExporter commented 9 years ago 
> I think scanning the string for format characters may
> be a hack; either a new $PDSH_SSH_FMTARGS should be
> made or just keep the behavior from trunk and document
> it.  It's ok to break stuff if it's better...
> downstream can always deal with that :-) We are lucky
> after all to have source to begin with.
>
> I can make a try at a PDSH_SSH_FMTARGS if you like that
> approach, or I'll just make site-local packages until
> I'm ready to update all my shell profiles.
>

I kind of agree it is a hack but I'm also not thrilled about adding yet
another environment variable. I'll give the code an attempt and if it doesn't
work out we'll do the alternate env variable.

Future plans for the ssh module is to transition all 'exec' type rcmd
modules to using a config file instead of coding them up as DSOs. The rough
idea is that you'd have a config something like

 exec-module "ssh" {
   cmdline = "ssh -2 -a -x -l%u %h"
 }

so that sites will be able to easily modify the ssh arguments. I might even work
on some way to have exec-modules define new pdsh options. Anyhow, this is why
ssh was transitioned to using the pipecmd code. Sorry that it broke a bunch of
things.

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 9:20

GoogleCodeExporter commented 9 years ago 
Ok, attached is the first attempt at scanning for %u and %h in PDSH_SSH_ARGS.
If they are not found then the code appends them.
(Note this patch breaks part of the t2001-ssh.sh tests -- to be fixed
in a subsequent commit)

mark

Original comment by mark.gro...@gmail.com on 27 Apr 2011 at 10:48

Attachments:

GoogleCodeExporter commented 9 years ago 
like the future direction indicated.  It abstracts out
into kind of a generic threaded popen(3) command
processor.  Maybe a different module could be made to
build ssh support in directly with something like
libssh.org...  that might help for doing stdin copying.
Then again maybe that's useful for exec module too...

Anyways, your patch looks sane, built and ran fine with
no changes to my profiles needed anymore.  Thanks!

Original comment by scott@omnisys.com on 28 Apr 2011 at 10:29

GoogleCodeExporter commented 9 years ago 
This issue was updated by revision r1329.

Tests for optional use of %u and %h in PDSH_SSH_ARGS.

Original comment by mark.gro...@gmail.com on 28 Apr 2011 at 2:34

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r1330.

Original comment by mark.gro...@gmail.com on 28 Apr 2011 at 2:34