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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

-- 
Doug Goldstein

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]