Re: [libvirt] [PATCH 1/2] init qemu_driver's qemuImgBinary field
On 11/14/2012 01:57 AM, li guang wrote:
在 2012-11-14三的 02:28 -0600，Doug Goldstein写道：
> On Wed, Nov 14, 2012 at 1:56 AM, li guang <lig.fnst(a)cn.fujitsu.com> 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>
wrote:
>>>> Signed-off-by: liguang <lig.fnst(a)cn.fujitsu.com>
>>>> ---
Your commit message needs justification why you are moving from a lazy
cache to a guaranteed initialization.
>
> 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.
I don't mind accessing the member variable directly, but only on
condition that we completely delete qemuFindQemuImgBinary(), and instead
inline its initialization code into the one-time initialization. As
long as the function call remains, then code should use the function
call. If you are trying to avoid the caching function call, then you
should avoid it everywhere, and this patch didn't go far enough.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)