Re: [libvirt] [PATCH] report invalid x86 cpu map error

On Mon, Jan 14, 2019 at 04:15:01PM +0100, Jiri Denemark wrote: > On Mon, Jan 14, 2019 at 15:03:26 +0000, Daniel P. Berrangé wrote: > > On Mon, Jan 14, 2019 at 03:36:38PM +0100, Jiri Denemark wrote: > > > On Mon, Jan 14, 2019 at 14:21:42 +0000, Daniel P. Berrangé wrote: > > > > On Mon, Jan 14, 2019 at 02:56:43PM +0100, Jiri Denemark wrote: > > > > > On Mon, Jan 14, 2019 at 20:07:34 +0800, zhenwei pi wrote: > > > > > > Let libvirtd handle invalid x86 cpu map error, and report the real reason. > > > > > > > > > > > > This issue can be reproduced : > > > > > > 1, rm -rf /share/libvirt/cpu_map > > > > > > 2, start libvirtd > > > > > > 3, virsh create INSTANCE.xml > > > > > > > > > > > > Libvirtd reports error : > > > > > > error: invalid argument: Failed to parse group 'tss' > > > > > > > > > > > > In face, libvirtd gets invalid cpu map. > > > > > > With this patch, libvirtd reports error : > > > > > > error: unsupported configuration: invalid x86 cpu map > > > > > > > > > > > > Signed-off-by: zhenwei pi <pizhenwei(a)bytedance.com&gt; > > > > > > --- > > > > > > src/cpu/cpu_x86.c | 5 ++++- > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c > > > > > > index d3a88da21d..91419d91d4 100644 > > > > > > --- a/src/cpu/cpu_x86.c > > > > > > +++ b/src/cpu/cpu_x86.c > > > > > > @@ -2902,8 +2902,11 @@ virCPUx86ValidateFeatures(virCPUDefPtr cpu) > > > > > > virCPUx86MapPtr map; > > > > > > size_t i; > > > > > > > > > > > > - if (!(map = virCPUx86GetMap())) > > > > > > + if (!(map = virCPUx86GetMap())) { > > > > > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > > > > > + _("invalid x86 cpu map")); > > > > > > return -1; > > > > > > + } > > > > > > > > > > This is definitely not the right place to fix the issue. virCPUx86GetMap > > > > > is called from a lot of places and your patch would only fix one of > > > > > them. Moreover the message itself is not very helpful as it doesn't say > > > > > why the CPU map is invalid. The right solution would be to show the > > > > > error reported from virCPUx86LoadMap. However, it is called inside > > > > > virCPUx86DriverOnceInit, which is only called once per libvirtd life > > > > > time. I think you'd need to store that error and reuse it everytime > > > > > cpuMap is NULL inside virCPUx86GetMap. > > > > > > > > VIR_ONCE_GLOBAL_INIT already keeps a record of errors raised in the > > > > initializer & re-raises them > > > > > > Eh, good to know :-) Anyway, it seems it doesn't really work and mostly > > > some random unrelated error is reported. In my case I see > > > > > > virsh # cpu-models x86_64 > > > error: failed to get CPU model names > > > error: Storage volume not found: no storage vol with matching path > > > '/vm/systemrescuecd-x86-2.5.1.iso' > > > > > > So there's definitely a bug to be fixed somewhere. > > > > Hmm, what steps did you use to get into that situation ? It works as > > expected for me, re-reporting the original error. > > I just moved away the CPU map file. But it appears I have a domain > defined pointing to a non-existent disk image. It seems the problem > shows up when multiple errors are raised when libvirtd is starting up. Can you elaborate on that - what guest & storage pool config do you have. I'm struggling to see how to exercise the codepath that reports that error during startup.