On 08/07/2021 20:39, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
On 08/07/2021 20:18, BALATON Zoltan wrote:
On Thu, 8 Jul 2021, Alexey Kardashevskiy wrote:
This addresses the comments from v22.
The functional changes are (the VOF ones need retesting with Pegasos2):
(VOF) setprop will start failing if the machine class callback
did not handle it;
I'll try this later but I think I've seen guests using setprop (Linux
also does that for some property). How should I allow that? Do I need a
new callback for this? Could it be allower unless there's a callback that
could deby it? But that was the previous way I think.
A simple defined callback which always returns "true" should do.
Yes but what's the point? That would just effectiverly disable this change
so if we need that, we could just as well keep the previous behaviour which
is to allow setprop unless there's a callback that can decide otherwise.
The spapr machine has such a callback so it already does not allow all
setprop and if I'll have a callback in pegasos2 returning true that will
allow what's allowed now so this part of this patch does nothing indeed.
Since guests could do all kinds of things that we don't know without trying
them restricting setprop is a good way to run into problems with guests
that were not tested that could otherwise just work. Then we'll need
another patch to enable that guest adding some more properties to the list
of allowed ones. Why it it a problem to allow this by default in the first
place and only reject changes for machines that have a callback? Then I
would not need more empty callbacks in pegasos2.
From here:
https://patchwork.ozlabs.org/project/qemu-devel/patch/20210625055155.2252896-1-aik@ozlabs.ru/#2714158
===
+ if (vmo) {
+ VofMachineIfClass *vmc = VOF_MACHINE_GET_CLASS(vmo);
+
+ if (vmc->setprop &&
+ !vmc->setprop(ms, nodepath, propname, val, vallen)) {
+ goto trace_exit;
This defaults to allowing the setprop if the machine doesn't provide a
setprop callback. I think it would be safer to default to prohibiting
all setprops except those the machine explicitly allows.
Mmmm... I can imagine the client using the device tree as a temporary
storage. I'd rather add a trace for such cases.
If they do, I think that's something we'll need to consider and
account for that platform, rather than something we want to allow to
begin with.