----- Original Message -----
> From: "Zdenek Styblik" <zdenek.styb...@gmail.com>
> To: "Ales Ledvinka" <aledv...@redhat.com>
> Cc: "ipmitool-devel" <ipmitool-devel@lists.sourceforge.net>
> Sent: Tuesday, January 15, 2013 2:23:51 PM
> Subject: Re: Code Review - ID: 3595612 - ask for password once only if used
>
> On Tue, Jan 15, 2013 at 2:15 PM, Ales Ledvinka <aledv...@redhat.com>
> wrote:
> >
> > ----- Original Message -----
> >> From: "Zdenek Styblik" <zdenek.styb...@gmail.com>
> >> To: "Ales Ledvinka" <aledv...@redhat.com>
> >> Cc: "ipmitool-devel" <ipmitool-devel@lists.sourceforge.net>
> >> Sent: Tuesday, January 15, 2013 1:54:28 PM
> >> Subject: Re: Code Review - ID: 3595612 - ask for password once
> >> only if used
> >>
> >> On Tue, Jan 15, 2013 at 1:27 PM, Ales Ledvinka
> >> <aledv...@redhat.com>
> >> wrote:
> >> > inline(/tail)
> >> > ----- Original Message -----
> >> >> From: "Zdenek Styblik" <zdenek.styb...@gmail.com>
> >> >> To: "Ales Ledvinka" <aledv...@redhat.com>
> >> >
> >> > ...snip...
> >> >> There are two issues with your patch. The first is, as you
> >> >> correctly
> >> >> pointed out, missing kgkey. The second one is user won't be
> >> >> asked
> >> >> about password if, eg. '-P foo' comes in the last, because
> >> >> you're
> >> >> setting 'querypass' variable to zero at places where you
> >> >> shouldn't.
> >> >> Unless, of course, that's on purpose. But later about this one.
> >> >
> >> > ...snip...
> >> >> And don't forget you must ask password/key no matter what. You
> >> >> know,
> >> >> backwards compatibility is a two way street. A bit ironic,
> >> >> isn't
> >> >> it?
> >> >> It seems to me to fix this properly, so called "backwards
> >> >> compatibility" has to be broken.
> >> >
> >> > This seems to be related. By already changing ask twice or more
> >> > to ask once regardless of used/ignored it's already changing
> >> > it's interface.
> >>
> >> What???
> >
> > When expecting the most improbable like: It's wrapped somewhere
> > with duplicate argument and different input with the get* call
> > hijacked by preloaded library. Then it's behaviour is has changed.
> >
>
> Great. I take it as you admit your patch, and any other, breaks so
> called backwards compatibility.
Sure. For the specific corner cases that I overlooked. Unlikely
to be used by anyone. Rather then dropping the compatibility
for almost anyone by reworking and failing on invalid argument
combinations - your proposal.
> >> > Though I do not expect anyone to be really using
> >> > the ask more then once feature.
> >> >
> >> > The "do not ask" if ignored part is intentional in the very
> >> > first patch is not that much conservative. I looks like a bug
> >> > since no matter what the value is ignored.
> >> > Similar case would be -A NONE and asking for password when the
> >> > code
> >> > ignores it.
> >> >
> >> > The password arguments are: -f -E -P -a
> >> >
> >>
> >> And? I thought this was about asking for user input, resp.
> >> getpass(),
> >> -> '-a' and '-Y'.
> >
> > And since the user input is ignored there is no reason to mislead
> > the user by asking for it when the value is going to be obtained
> > from other option method. So the other options are about removing
> > the annoyance of ignored input.
> >
>
> If this is supposed to be argument for your patch, then I find it to
> be weak one.
> I'm just wondering what part of "backwards compatibility" and "not
> asking for password anymore" doesn't get through.
> Backwards compatibility =~ ask for password/kgkey at least *once* if
> '-a' or '-Y' is specified.
As weak as the corner case use.
> >> > The password is used in ipmi_main, ipmi_lanp, ipmi_user
> >> > plugins/ipmi_intf.c plugins/lan/lan.c plugins/lan/auth.c
> >> > The variable from main is copied to intf.
> >> > There is cleanup for -A NONE in ipmi_intf.
> >> > The lanp sets the intf password too from the lan set value.
> >> > The user does a lot of password but nothing relevant besides
> >> > another place of interactive password input.
> >> > The lan checks in the auth capabilities/password presence
> >> > combinations.
> >> > The auth is really a comment only.
> >> >
> >>
> >> I don't see how this is relevant.
> >
> > That is summary of password manipulation showing that there
> > are no: "places where you shouldn't"
> >
>
> No, they do not.
>
> ~~~
> case 'f':
> + querrypass = 0;
> case 'a':
> + querrypass = 1;
> ~~~
>
> If arguments are '-a -f /some/file' then user won't be asked for
> password.
>
case 'a':
...
- password = strdup(tmp);
moved past the getopt loop
case 'f':
...
password = ipmi_password_file_read(optarg);
For the -a -f filename example it is:
Same variable.
Interactive input was dropped. Now not asked for at all.
Last occurred option used as before.
> >>
> >> > So I do not really see where is the: "at places where you
> >> > shouldn't"
> >> > Except that was related to the above backwards compatibility and
> >> > concern
> >> > of misleading ignored user input.
> >> >
> >>
> >> Yes, it was. And since you're obviously blowing backwards
> >> compatibility out of the window, then fix this issue properly and
> >> document such behaviour.
> >> I really don't know what else to add. On one hand you're arguing
> >> with
> >> backwards compatibility, yet on the other you're breaking it
> >> yourself
> >> and with clean consciousness.
> >>
> >
> > I will borrow your words: "It seems to me to fix this properly,
> > so called "backwards compatibility" has to be broken."
> >
>
> Sorry, but I don't consider your patch to be a proper fix, or is it?
Claims to be annoyance removal. Since we do not share the same idea
of the bug in the option parsing there is no common resolution of what
can be proper fix.
> > The change affects interactive users and very few corner cases
> > of preloaded getpass, interactive user can handle that and I expect
> > anyone preloading different getpass to pay close attention to
> > arguments.
> > Therefore I would still consider that as minimum impact.
> >
>
> Is some kind of "automation" wrapped around ipmitool, or any
> interactive, tool corner case as well? I'd say you're quite wrong
> about this one.
>
> Z.
>
> >> Order of arguments matters. If some arguments, namely "ask me for
> >> password", are passed more than once, they may be ignored. End of
> >> story.
> >>
> >> > And I expect to find similar for the kgkey arguments: -k -K -y
> >> > -Y
> >>
> >> Yes, no doubt.
> >>
> >> Z.
> >>
>
------------------------------------------------------------------------------
Master SQL Server Development, Administration, T-SQL, SSAS, SSIS, SSRS
and more. Get SQL Server skills now (including 2012) with LearnDevNow -
200+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
SALE $99.99 this month only - learn more at:
http://p.sf.net/sfu/learnmore_122512
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
