Re: [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable

[Eduardo Habkost]

On Wed, Apr 17, 2019 at 07:26:14AM +0200, Markus Armbruster wrote:
> Like Xu <address@hidden> writes:
> 
> > This patch makes the remaining dozen or so uses of the global
> > current_machine outside vl.c use qdev_get_machine() instead,
> > and then make current_machine local to vl.c instead of global.
> >
> > Suggested-by: Peter Maydell <address@hidden>
> > Signed-off-by: Like Xu <address@hidden>
> 
> I'm afraid I dislike this one, too.
> 
> The patch does not reduce global state, it's merely MICAHI (make it
> complicated and hide it).
> 
> It does not improve safety, it merely turns dereferences of null
> current_machine into unwanted creation of "/machine" as container (ugh),
> which the next patch then fixes up to assertion failure.
> 
> The only benefit I can see is you can't assign to current_machine
> outside vl.c anymore.  Nobody ever did, thus complete non-issue.

The benefit I see is that we now have a single way to access the
existing global state instead of two.

> 
> If you want to hide global state without actually reducing it, create an
> accessor function.  You can then use that to replace qdev_get_machine(),
> getting rid of its surprising side effect.  *That* would be an
> improvement I could get behind.
> 
> Better that *hiding* use of global state would be *eliminating* use of
> global state: pass current_machine around.  This isn't always practical.
> But where it is, the dependence on "machine created" becomes obvious in
> the code.

I agree qdev_get_machine() has many issues.  I dislike
qdev_get_machine() a lot, and I think I had suggested we stop
using it and use current_machine instead.

But at least now we have one imperfect API instead of two
imperfect APIs.  I do think this makes future clean up work
easier.

-- 
Eduardo