[libvirt] [PATCH] conf: Fix parsing of seclabels without model

[Jiri Denemark]

On Thu, Aug 30, 2012 at 21:32:32 +0200, Jiri Denemark wrote:
> On Thu, Aug 30, 2012 at 15:01:34 -0300, Marcelo Cerri wrote:
> > On 08/30/2012 02:12 PM, Jiri Denemark wrote:
> > > On Thu, Aug 30, 2012 at 13:19:31 -0300, Marcelo Cerri wrote:
> > >> With this patch libvirt tries to assign a model to seclabels when model
> > >> is missing. Libvirt will look up at host's capabilities and assign a
> > >> model in order to each seclabel that doesn't have a model assigned.
> > >>
> > >> This patch fixes:
> > >>
> > >> 1. The problem with existing guests that have a seclabel defined in its XML.
> > >> 2. A XML parse error when a guest is restored.
> > >>
> > >> Signed-off-by: Marcelo Cerri <mhcerri at linux.vnet.ibm.com>
> > >> ---
> > >>   src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++------------------------
> > >>   1 file changed, 29 insertions(+), 27 deletions(-)
> > >
> > > I think this is trying to fix the issue at a wrong place. It's not that XML
> > > generated by older libvirtd is not correctly parsed by current libvirtd. The
> > > problem is that *current* libvirtd creates an XML that it cannot parse back.
> > > Thus we should rather fix the code that formats the XML.
> > >
> > 
> > I don't agree. If you save a domain using the latest libvirt and using 
> > an earlier version (I used v0.9.10) and then check the XML included in 
> > each save file, you'll see something similar to this for the latest 
> > libvirt version:
> > 
> >    ...
> >    </devices>
> >    <seclabel type='dynamic' model='selinux' relabel='yes'>
> >      <label>unconfined_u:system_r:svirt_t:s0:c323,c995</label>
> >  
> > <imagelabel>unconfined_u:object_r:svirt_image_t:s0:c323,c995</imagelabel>
> >    </seclabel>
> >    <seclabel type='dynamic' model='dac' relabel='yes'>
> >      <label>0:0</label>
> >      <imagelabel>0:0</imagelabel>
> >    </seclabel>
> > </domain>
> > 
> > And this for v0.9.10:
> > 
> >    ...
> >    </devices>
> >    <seclabel type='dynamic' model='selinux' relabel='yes'>
> >      <label>system_u:system_r:svirt_t:s0:c175,c437</label>
> >      <imagelabel>system_u:object_r:svirt_image_t:s0:c175,c437</imagelabel>
> >    </seclabel>
> > </domain>
> > 
> > The biggest difference is the seclabel for DAC.
> 
> Exactly. But while latest libvirt can happily parse the XML generated with
> 0.9.13 (or older), it will fail to load the XML the latest libvirt itself
> generated. Thus, if the generated XML is missing something and you would need
> to guess that info when parsing the XML, it's the formatting code that needs
> to be fixed to output what is needed by the parsing code. But that's mostly
> commenting the code that I don't quite understand why it is needed; the code
> that fills in missing seclabel models, while all seclabel elements in both
> XMLs contain model attributes.

OK, now I think I understand the issue. The problem is that in certain cases,
where model is not required for compatibility reasons, we would just ignore it
instead of parsing it anyway. Thus we end up in a situation that should never
happen because model is always required when multiple seclabels are used. With
this understanding, I commented on your original patch.

Jirka