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

[Clementine Hayat]

2018-04-13 8:25 GMT+00:00 Erik Skultety <eskultet at 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 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).

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_error_if_domain_is_not_active