Re: [libvirt] [PATCH] Fix parsing of vendor_id

On 12/17/12 09:05, Ken ICHIKAWA wrote: > I have a problem about parsing vendor_id of domain XML. > When define vendor_id attribute, > why definition of fallback attribute is needed? > I explain below for example. > (I used virsh edit.) > > Current domain xml state is like below, > <domain> > <cpu> > </cpu> > </domain> > > And I redefine domain xml like below, > <domain> > <cpu> > <model vendor_id='aaaabbbbcccc'>core2duo</model> > </cpu> > </domain> > > Then, do dumpxml, > vendor_id is not reflected like below. > <domain> > <cpu mode='custom' match='exact'> > <model fallback='allow'>core2duo</model> > </cpu> > </domain> > > I think this is not right behavior. It should be defined like > below. > <domain> > <cpu mode='custom' match='exact'> > <model fallback='allow' vendor_id='aaaabbbbcccc'>core2duo</model> > </cpu> > </domain> > > And if I define fallback attribute and vendor_id attribute at the > same time, or define vendor_id attribute after fallback attribute > is defined, vendor_id attribute is reflected normally. > Is it a bug? or is there some reason? > > I read past mailing list's thread about the patch adding vendor_id > but I could not find the reason. > https://www.redhat.com/archives/libvir-list/2012-June/thread.html#00917 > It seems that patch v1 doesn't need definition of fallback attribute > but v2 needs it. > > If it's a bug, please consider to apply this patch. It is a bug. > > This patch fixes a problem that vendor_id attribute can not be > defined when fallback attribute is not defined. > > Signed-off-by: Ken ICHIKAWA <ichikawa.ken(a)jp.fujitsu.com&gt; > --- > src/conf/cpu_conf.c | 42 +++++++++++++++++++++--------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > ACK to the changes, unfortunately some of the code you touched is really ugly so I will repost your patch along with patches that fix the code after it's applied. I will also simplify your commit message to describe the problem better, please let me know if you're okay with that.