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