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