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

[li guang]

在 2012-11-14三的 02:28 -0600，Doug Goldstein写道：
> On Wed, Nov 14, 2012 at 1:56 AM, li guang <lig.fnst at cn.fujitsu.com> wrote:
> > 在 2012-11-14三的 01:38 -0600，Doug Goldstein写道：
> >> On Tue, Nov 13, 2012 at 9:03 PM, liguang <lig.fnst at cn.fujitsu.com> wrote:
> >> > Signed-off-by: liguang <lig.fnst at cn.fujitsu.com>
> >> > ---
> >> >  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 at 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.

well, quite conceptually,
but, qemu_driver is global variable,
every member can't be hidden at all.
e.g. driver->snapshotDir was used directly.

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

I just initialize it like other variables of qemu_driver,
e.g. libDir, cacheDir, saveDir, snapshotDir...

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

-- 
li guang  lig.fnst at cn.fujitsu.com
linux kernel team at FNST, china