[libvirt] [PATCH v3 0/5] virsh: Further improve handling of integer options

[John Ferlan]

On 06/02/2015 05:35 AM, Andrea Bolognani wrote:
> On Sun, 2015-05-31 at 15:19 -0400, John Ferlan wrote:
> 
> First of all, thanks for the review :)
> 
>> Patch 2 had some comments which should be simple to fix
> 
> I've commented in detail in your other mail, should have taken care of
> them all.
> 
>> Patch 4 had a couple of NIT's about going beyond 80 columns on a line,
>> but since it's so pervasive in the rest of the module that can be a
>> future cleanup ;-)...
> 
> Sure, there's always time for cleaning up after the cleanup :)
> 
>> I do note that 'vshCommandOpt' now has an
>> unrelated difference - I assume it had a ctl argument at one point
>> that's since been removed...
> 
> I'm not sure what you mean here, it looks fine to me. Care to explain in
> more detail?
> 

I use gitk when perusing differences and saw the following which
ironically may have taken care of an 80 col thing, but in reality had no
"real" difference other than putting P2 and P3 on a second line. I thus
assumed you had added the *ctl argument to this function at one time -
perhaps as P1 (I think in fact there may have been a review comment on
it)...

 static int
-vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
+vshCommandOpt(const vshCmd *cmd,
+              const char *name, vshCmdOpt **opt,
               bool needData)


>> Patch 5 still seemed to need some sort of error message in
>> vshCommandOptString... Some callers ignore return status, so even adding
>> an error may still be "ignored".
> 
> This series deals with numeric options only. String options are
> something I will probably tackle in the near future :)
> 
>> Overall seems OK to me with some minor cleanups.
> 
> Looks like this series is growing a new commit for every subsequent
> review! I've just posted v4, which should address your comments. Please
> take a look at it and let me know if there's more work to do.
>

I'll look today at the updates...

John