Re: [PATCH] qemu_alias: Fix backcompat console alias generation
On Wed, Mar 15, 2023 at 08:51:33AM +0100, Michal Prívozník wrote:
On 3/14/23 20:33, Andrea Bolognani wrote:
> On Tue, Mar 14, 2023 at 11:20:49AM +0100, Michal Privoznik wrote:
>> 2) virDomainDefAddConsoleCompat() - which adds a copy of
>> <serial/> or <console/> into virDomainDef in post parse.
>
> Since we add the missing elements in this function, shouldn't we be
> able to remove the hacky code from the format part?
That's what I wanted to do. But A LOT of tests started failing. But I
blame my code for that. Let me see if I can do better today.
Any progress at all towards making that part of libvirt more
reasonable would be much appreciated. If you feel like spending some
time looking into this, I'll be happy to review the results :)
BTW: even then we still would need to keep this patch (or its
variant),
because the problem would still be there. I mean, the backcompat console
handling is done in PostParse. Alias assignment is done after PostParse
(when starting the domain). So regardless of what we do in PostParse -
the aliased console would still get different alias then the serial
device. Simply because that's how qemuAssignDeviceChrAlias() is written.
But okay, we could work around that - we have qemuProcessPrepareDomain()
where we could fix the aliases. But what about other drivers? So far,
the CH driver is the only one where this crazy backcompat is not
happening (as of v9.1.0-rc1~63). But whatever we do, we would need to
just push the code from src/conf/ into the drivers.
Yeah, I hadn't considered the fact that moving the handling down to
the driver effectively means having to make multiple copies of it.
Perhaps there's a way we can organize the code so that most of the
logic still lives in common code, but it does make your solution look
more appealing.
> The only reason why I'm not ACKing it right away is that
I'm
> concerned we're piling hacks on top of hacks, and in particular that
> allowing multiple devices to have the same alias will cause grief
> further down the line.
Yeah, I was worried about non-unique alias too. Until I realized that
after the daemon is restarted, the status XML is parsed and the aliases
ARE the same. IOW, we're exactly at the same position as after this patch.
Right, the fact that we need to successfully parse back a status XML
(or an incoming migration XML) where the alias is the same means that
we're never going to be able to add a sanity check for the absence of
duplicated (non user-provided) aliases - unless of course we add even
more hacks :)
> If you have already considered this and convinced yourself that
we're
> okay, then say so and I'll gladly ACK the patch.
Yeah, I'm not proud of this patch either. It's not something I'd be
showing to others at parties. But at the same time, I think this is the
only sensible solution. MAYBE, there's another one - writing similar
hack into qemuDomainDetachDeviceChr(). But that fixes just the
device-detach case. I don't know if there's another place where this
aliasing is causing problems, so maybe we could do just that and not use
this big hammer?
Another way to look at it is that the two devices having the same
alias sort of make sense because they *do* represent the same device,
and not just at the QEMU driver level.
All in all, handling these devices is always going to suck. After
your changes at least the internal consistency is improved, which I
guess is the best we can realistically hope for.
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization