Re: [libvirt] [PATCH] Add function that raises error if domain is not active

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(a)lse.epita.fr&gt; > > 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... > > >> --- >> 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.