Re: [libvirt] [PATCH 1/2] init qemu_driver's qemuImgBinary field

On Wed, Nov 14, 2012 at 1:56 AM, li guang <lig.fnst(a)cn.fujitsu.com&gt; wrote: > 在 2012-11-14三的 01:38 -0600，Doug Goldstein写道： >> On Tue, Nov 13, 2012 at 9:03 PM, liguang <lig.fnst(a)cn.fujitsu.com&gt; wrote: >> > Signed-off-by: liguang <lig.fnst(a)cn.fujitsu.com&gt; >> > --- >> > src/qemu/qemu_domain.c | 2 +- >> > src/qemu/qemu_driver.c | 3 +++ >> > 2 files changed, 4 insertions(+), 1 deletions(-) >> > >> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> > index a5592b9..eb5a3d1 100644 >> > --- a/src/qemu/qemu_domain.c >> > +++ b/src/qemu/qemu_domain.c >> > @@ -1647,7 +1647,7 @@ qemuDomainSnapshotForEachQcow2Raw(struct qemud_driver *driver, >> > int i; >> > bool skipped = false; >> > >> > - qemuimgarg[0] = qemuFindQemuImgBinary(driver); >> > + qemuimgarg[0] = driver->qemuImgBinary; >> > if (qemuimgarg[0] == NULL) { >> > /* qemuFindQemuImgBinary set the error */ >> > return -1; >> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> > index 5f91688..a25fb53 100644 >> > --- a/src/qemu/qemu_driver.c >> > +++ b/src/qemu/qemu_driver.c >> > @@ -626,6 +626,9 @@ qemudStartup(int privileged) { >> > if (!qemu_driver->domainEventState) >> > goto error; >> > >> > + /* find kvm-img or qemu-img */ >> > + qemuFindQemuImgBinary(qemu_driver); >> > + >> > /* read the host sysinfo */ >> > if (privileged) >> > qemu_driver->hostsysinfo = virSysinfoRead(); >> > -- >> > 1.7.1 >> > >> > -- >> > libvir-list mailing list >> > libvir-list(a)redhat.com >> > https://www.redhat.com/mailman/listinfo/libvir-list >> >> Honestly this entire patch can be dropped because its not correct. >> qemuFindQemuImgBinary() only looks up the path to the binary the first >> time and then caches it from there on in. So you should always be >> using qemuFindQemuImgBinary(). As far as the addition to the startup >> of looking it up, it seems like that's really a pre-mature >> optimization for your second patch. >> >> So I'd have to say NACK on this change. >> > > your reason sounds strange, you call qemuFindQemuImgBinary() > to get qemu-img handler every time you want to use it, > I use the handler freely for I've I've initialized before > why can't I use driver->qemuImgBinary instead of > qemuFindQemuImgBinary()? > I would argue that its bad form to do that for at least 2 reasons. 1. The fact that driver->qemuImgBinary is "hidden" behind a function to access it means you should effectively treat it as opaque and not directly access it.

2. Its only found when its actually needed rather than always looking it up when libvirt starts up.

qemuFindQemuImgBinary() is functionally equivalent to driver->qemuImgBinary after the first call.