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