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