On Wed, Apr 17, 2019 at 07:26:14AM +0200, Markus Armbruster wrote: > Like Xu <like...@linux.intel.com> 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 <peter.mayd...@linaro.org> > > Signed-off-by: Like Xu <like...@linux.intel.com> > > 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
