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

On Thu, Nov 09, 2017 at 04:57:54PM +0100, Michal Privoznik wrote: > 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. > How about setting a boolean in @ctl instead? If you don't like that either, then keep what you have, I don't care that much about it.