Re: [libvirt] [PATCH v3 0/5] virsh: Further improve handling of integer options
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?
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.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c "print('a'.join(['', 'bologn', '@redh',
't.com']))"