Re: [libvirt] [PATCH 02/11] vshCommandOpt: Relax check for valid options

On 11/08/2017 10:54 AM, Martin Kletzander wrote: > On Tue, Nov 07, 2017 at 01:22:50PM +0100, Michal Privoznik wrote: >> When trying to get an opt for command typed on the command line >> we first check if command has such option. Because if it doesn't >> it is a programming error. For instance: vshCommandOptBool(cmd, >> "config") called from say cmdStart() doesn't make sense since >> there's no --config for the start command. However, we will want >> to have generic completers which are going to check if various >> options are set. And so it can happen that we will check for >> non-existent option for given command. Therefore, we need to >> relax the check otherwise we will hit the assert() and don't get >> anywhere. >> > > I would prefer keeping the assert there since it's such an easy check > for that kind of programming error.  Is there no way to distinguish > between calls from the completer?  If not, then I would rename it to > vshCommandOptInternal(), add a bool argument, make vshCommandOpt() call > it with one value and the completer with another one (I don't care if > there's yet another function for that or if completers use the Internal > one). So now that I'm trying to implement what you suggested I came to realize that it would be suboptimal. I mean, we have couple of functions for looking up arguments. For instance: vshCommandOptInt(), vshBlockJobOptionBandwidth(), virshCommandOptDomain() to name a few which eventually call vshCommandOpt(). Now, if I leave the assert() in and make it optional (say based on a boolean arg), I'd need to write alternative functions to all aforementioned so that they call vshCommandOpt() with the boolean arg set to false. For instance: virshCommandOptDomainUnsafe(), vshCommandOptIntUnsafe() or something. That looks like too much work for very little gain. Therefore I'd like to stick with my original proposal and just drop the assert.