Re: [libvirt] [PATCH] Add function that raises error if domain is not active
On 04/13/2018 09:31 AM, Ján Tomko wrote:
On Thu, Apr 12, 2018 at 07:49:15PM +0000, Clementine Hayat wrote:
> Add a function named virDomainObjCheckIsActive in src/conf/domain_conf.c.
> It calls virDomainObjIsActive, raises error and returns.
*raises error if necessary
>
> There is a lot of occurence of this pattern and it will save 3 lines on
> each call. Knowing that there is over 100 occurences, it will remove 300
> lines from the code base.
False advertising.
If you don't want to send them all in one patch, I suggest spliting them
per-subdirectory: conf, qemu, libxl, vz, ... (and use that prefix in the
commit summary)
>
> Signed-off-by: Clementine Hayat <clem(a)lse.epita.fr>
If you have any accents in your name, feel free to use them. Even danpb
recently decided the world is ready for UTF-8:
https://libvirt.org/git/?p=libvirt.git;a=search;s=Daniel+P.+Berrang%C3%A9...
> ---
> Patch proposed for gsoc2018.
>
> src/conf/domain_conf.c | 11 +++++
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 96 +++++++++-------------------------------
> 4 files changed, 34 insertions(+), 76 deletions(-)
>
The changes look good but the patch feels incomplete.
I was just looking at this patch. Yes it is incomplete but I think that
was the point. Give upstream taste what the patch looks like before
diving in and changing all those 108 occurrences (I did change them
locally).
The patch looks good to me, but as Jan suggests, you can break this big
change into smaller (=easier to review) patches. In the first you can
just introduce the function. And then have patch per each folder.
Alternatively, you can write a small semantic patch [1] and use that to
generate the diff. But this is rather advanced stuff.
Michal
1: http://coccinelle.lip6.fr/