[libvirt] [PATCH 1/2] conf: Add oldlabel field to virSecurityDeviceLabelDef

[Martin Kletzander]

On 02/18/2013 12:38 PM, Daniel P. Berrange wrote:
> On Mon, Feb 18, 2013 at 12:29:03PM +0100, Michal Privoznik wrote:
>> The field is there to store the original label of device,
>> so we can restore it when domain is shutting down.
>> ---
>>  src/conf/domain_conf.c | 20 +++++++++++++++-----
>>  src/conf/domain_conf.h |  1 +
>>  2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 7a2b012..d83330a 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -957,6 +957,7 @@ virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def)
>>          return;
>>      VIR_FREE(def->model);
>>      VIR_FREE(def->label);
>> +    VIR_FREE(def->oldlabel);
>>      VIR_FREE(def);
>>  }
>>  
>> @@ -3639,14 +3640,15 @@ static int
>>  virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
>>                                    size_t *nseclabels_rtn,
>>                                    virSecurityLabelDefPtr *vmSeclabels,
>> -                                  int nvmSeclabels, xmlXPathContextPtr ctxt)
>> +                                  int nvmSeclabels, xmlXPathContextPtr ctxt,
>> +                                  unsigned int flags)
>>  {
>>      virSecurityDeviceLabelDefPtr *seclabels;
>>      size_t nseclabels = 0;
>>      int n, i, j;
>>      xmlNodePtr *list = NULL;
>>      virSecurityLabelDefPtr vmDef = NULL;
>> -    char *model, *relabel, *label;
>> +    char *model, *relabel, *label, *oldlabel;
>>  
>>      if ((n = virXPathNodeSet("./seclabel", ctxt, &list)) < 0)
>>          goto error;
>> @@ -3717,6 +3719,13 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn,
>>                               NULLSTR(seclabels[i]->model));
>>              goto error;
>>          }
>> +
>> +        /* only parse oldlabel when parsing domain status XML */
>> +        if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS) {
>> +            oldlabel = virXPathStringLimit("string(./oldlabel)",
>> +                                           VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
>> +            seclabels[i]->oldlabel = oldlabel;
>> +        }
>>      }
>>      VIR_FREE(list);
>>  
>> @@ -4299,7 +4308,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>>                                                &def->nseclabels,
>>                                                vmSeclabels,
>>                                                nvmSeclabels,
>> -                                              ctxt) < 0)
>> +                                              ctxt, flags) < 0)
>>              goto error;
>>          ctxt->node = saved_node;
>>      }
>> @@ -5926,7 +5935,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>>                                                            &chr_def->nseclabels,
>>                                                            vmSeclabels,
>>                                                            nvmSeclabels,
>> -                                                          ctxt) < 0) {
>> +                                                          ctxt, flags) < 0) {
>>                          ctxt->node = saved_node;
>>                          goto error;
>>                      }
>> @@ -12344,10 +12353,11 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf,
>>  
>>      virBufferAsprintf(buf, " relabel='%s'", def->norelabel ? "no" : "yes");
>>  
>> -    if (def->label) {
>> +    if (def->label || def->oldlabel) {
>>          virBufferAddLit(buf, ">\n");
>>          virBufferEscapeString(buf, "  <label>%s</label>\n",
>>                                def->label);

Unnecessary label when def->label == NULL but def->oldlabel is specified
(is this the case when seclabel is not specified, but we relabel some
files so we need to store the old seclabel?).

>> +        virBufferEscapeString(buf, "  <oldlabel>%s</oldlabel>\n", def->oldlabel);
>>          virBufferAddLit(buf, "</seclabel>\n");
>>      } else {
>>          virBufferAddLit(buf, "/>\n");
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 9232ff9..b7f4b38 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -327,6 +327,7 @@ typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr;
>>  struct _virSecurityDeviceLabelDef {
>>      char *model;
>>      char *label;        /* image label string */
>> +    char *oldlabel;     /* the original label to return to */
>>      bool norelabel;
>>  };
> 
> This is storing driver specific state in the XML configuration
> description which is bad practice in general. Any such state
> should be maintained inside the DAC driver itself.
> 

I'm not sure how Michal wants to continue with this, but I'd say it's
pretty reasonable if all drivers understand this element (even though it
will not get to another driver thanks to the 'model' attribute) as I was
under the impression that Michal wants to continue with selinux and
other labels as well.

Since this is saved only in the state XML and not parsed when not
understood, it solves the problem with libvirt being restarted and
doesn't make any other libvirt versions fail with such XML.  And it gets
transferred when migrating.

Martin