Re: [libvirt] [PATCH v3 0/5] virsh: Further improve handling of integer options
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