[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] conf: Ignore panic device on pSeries.



On Sun, 2015-05-10 at 17:50 -0400, Cole Robinson wrote:

> <devices><panic> is about a specific device though, which qemu ppc doesn't
> support. Even if the firmware provides the logical feature (getting PANICKED
> notifications from qemu), it doesn't support the device or any explicit qemu
> CLI config, so IMO it's correct to reject <panic>. Apps/users will just have
> to take that into account, along with all the other XML differences for x86 vs
> ppc64.
> 
> An alternative could be the unconditionally add <panic> to ppc64 XML since the
> logical feature is always available... but you'd probably have to invent a new
> <address> scheme or something
> 
> Instead I think it's just a documentation patch.

I really like the alternative approach you suggested: it's the same
feature after all, so the XML should be the same regardless of how it's
actually implemented for the specific machine type.

I've reworked my patch accordingly, I'm going to send a v2 in a few
minutes. Please let me know what you think of it.

> Another thing from that bug: The error message 'your QEMU is too old to
> support pvpanic' isn't accurate for non-x86, so it's probably better to change
> it to 'this QEMU binary does not support pvpanic' or similar, maybe look at
> existing error messages to find a better example.

I agree. I feel it belongs to a different commit, though.

> FWIW, since this is hypervisor specific, this type of stuff shouldn't be in
> the (intended to be) generic domain_conf.c. Check out
> qemu_domain.c:qemuDomainDefPostParse instead

My mistake. Thanks both to you and Daniel for pointing that out.

Cheers.

-- 
Andrea Bolognani <abologna redhat com>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]