Re: [PATCH] libxl: implement virDomainObjCheckIsActive

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

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux