[libvirt] [PATCH] Add function that raises error if domain is not active

[Erik Skultety]

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
> >
> >>
> >> 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 at 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;st=author
> >
> >
> >> ---
> >> 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.

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"...

Erik