2018-04-13 8:25 GMT+00:00 Erik Skultety <eskultet@xxxxxxxxxx>: > 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@xxxxxxxxxxxx> >> > >> > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
