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 > > > >> > >> 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) > > > >> > >> 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). > > 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"... Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
