在 2012-11-14三的 02:28 -0600，Doug Goldstein写道：
> On Wed, Nov 14, 2012 at 1:56 AM, li guang <lig.fnst@xxxxxxxxxxxxxx> wrote:
> > 在 2012-11-14三的 01:38 -0600，Doug Goldstein写道：
> >> On Tue, Nov 13, 2012 at 9:03 PM, liguang <lig.fnst@xxxxxxxxxxxxxx> wrote:
> >> > Signed-off-by: liguang <lig.fnst@xxxxxxxxxxxxxx>
> >> > ---
> >> > 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@xxxxxxxxxx
> >> > 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@xxxxxxxxxxxxxx
linux kernel team at FNST, china
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list