[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] libxl: implement virDomainObjCheckIsActive



On 02/13/2017 01:24 PM, Jim Fehlig wrote:
> Sagar Ghuge wrote:
>> Hi Jim,
>>
>> Thank you for replying. I am planning to participate in GSoC'17 and
>> this is my first patch towards it. This is from Byte sized task which
>> I picked up and trying to solve it.
> 
> Ah, yes, this one
> 
> http://wiki.libvirt.org/page/BiteSizedTasks#Add_function_that_raises_error_if_domain_is_not_active
> 
>> Yes you are right, pattern is used
>> throughout the libxl driver and many others but this is just a
>> rudimentary patch which will help me to get suggestion that am I on
>> right path. if yes then I can I can go ahead and make changes
>> accordingly.
> 
> It looks good to me, although I'm not sure how to best split up the work. Maybe
> a patch that adds virDomainObjCheckIsActive, and then a patch for each driver
> that replaces virDomainObjIsActive with virDomainObjCheckIsActive?
> 
> AFAICT, Cole added that task to the wiki. Perhaps he has a suggestion on the
> best way to organize the work.
> 

Sorry for the late response.

For these types of patches, please link to the BiteSizedTask in the mail after
the -- break generated by git format-patch: this keeps the link out of the
commit message since it's not really relevant there, but gives more info to
potential patch reviewers.

For this item I'd say do a patch series containing more instances of
conversions, to show that it's actually simplifying a common code pattern. So
maybe a patch series like:

1) Add the function
2) Convert src/test/*
3) Convert src/libxl/*
4) Convert src/xen/*

That shouldn't be too much work and will give a bit more to comment on, and if
there's no objections, send the remaining conversions as an additional patch
series.

- Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]