[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]