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.
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
