[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_ma
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable |
Date: |
Wed, 17 Apr 2019 14:05:49 -0300 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, (continued)
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Eduardo Habkost, 2019/04/16
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Markus Armbruster, 2019/04/17
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Eduardo Habkost, 2019/04/17
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Like Xu, 2019/04/23
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Eduardo Habkost, 2019/04/24
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Like Xu, 2019/04/24
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/2] core/qdev: refactor qdev_get_machine() with type assertion, Eduardo Habkost, 2019/04/25
[Qemu-trivial] [Qemu-devel] [PATCH v3 1/2] vl.c: refactor current_machine as non-global variable, Like Xu, 2019/04/15