Re: [libvirt] [PATCH] Add function that raises error if domain is not active
2018-04-13 8:25 GMT+00:00 Erik Skultety <eskultet(a)redhat.com>:
On Fri, Apr 13, 2018 at 09:45:48AM +0200, Michal Privoznik wrote:
> 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
Yes, thank you.
> >> 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)
I'll do that thank you.
> >> 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).
It's right, it's one of the BiteSizedTasks proposed to begin with [1]
and it's asked to do it in two times.
First do a small patch and have it review and then change all the occurences.
I'm sorry I should have mentioned it.
> 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.
I agree with the intention of the patch and the comments, I'd
maybe change the
name slightly --> virDomainObjCheckActive (instead of *IsActive) or even
something that emphasizes a bit more that it will report error on inactive, so
that the caller doesn't have to care about that anymore - from the top of my
head "virDomainObjReportInactive"...
I'm going to do that. I think virDomainObjCheckActive is a good name.
For the virDomainObjReportInactive, I have the feeling that, by
reading the name,
people may expect that the function will return 0 if there was an error.
> Alternatively, you can write a small semantic patch [1] and use
that to
> generate the diff. But this is rather advanced stuff.
I'll take a look into coccinelle. It may take a bit more time thought.
--
Clementine Hayat
[1]
https://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_err...