[libvirt] Domain XML format using defined storage volume + RFC

[Stefan de Konink]

Daniel P. Berrange schreef:
> On Fri, May 16, 2008 at 01:00:16AM +0200, Stefan de Konink wrote:
>> On Thu, 15 May 2008, Stefan de Konink wrote:
>>
>>> On Thu, 15 May 2008, Stefan de Konink wrote:
>>>
>>>> I almost started crying why it didn't work. But it is fixed, and it works
>>>> as a charm :) See updated patch!
>>> I guess I need to write the 'check' tool and documentation update too?
>> Since there is currently no check build for pools in that directory, I'm
>> passing this to someother person.
> 
> Yes, testing the interaction with pools is actually harder that I anticipated,
> because the libvirtd isn't available in context of the test suite. I'll have to
> think about best way todo it. Might be able to stub out a dummy impl for purposes
> of testing.

Ok.

> 
>> @@ -1309,11 +1311,24 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node,
>>          if (cur->type == XML_ELEMENT_NODE) {
>>              if ((source == NULL) &&
>>                  (xmlStrEqual(cur->name, BAD_CAST "source"))) {
>> -
>>                  if (typ == 0)
>>                      source = xmlGetProp(cur, BAD_CAST "file");
>> -                else
>> +                else if (typ == 1)
>>                      source = xmlGetProp(cur, BAD_CAST "dev");
>> +                else if (typ == 2) {
>> +                    xmlChar *pool   = xmlGetProp(cur, BAD_CAST "pool");
>> +                    xmlChar *volume = xmlGetProp(cur, BAD_CAST "volume");
>> +                    if (pool != NULL && volume != NULL) {
>> +                        virStoragePoolPtr virPool;
>> +                        virPool = virStoragePoolLookupByName(conn, (const char *) pool);
>> +                        if (virPool != NULL) {
>> +                            virStorageVolPtr virVol;
>> +                            virVol = virStorageVolLookupByName(virPool, (const char *) volume);
>> +                            if (virVol != NULL)
>> +                                source = BAD_CAST virStorageVolGetPath(virVol);
> 
> This is leaking the pool and volume objects.

Ok. Appended xmlFree's.
http://kinkrsoftware.nl/contrib/libvirt/pool.patch


> 
>> +                        }
>> +                    }
>> +                }
>>              } else if ((target == NULL) &&
>>                         (xmlStrEqual(cur->name, BAD_CAST "target"))) {
>>                  target = xmlGetProp(cur, BAD_CAST "dev");
>> @@ -1411,7 +1426,8 @@ virDomainParseXMLDiskDesc(virConnectPtr conn, xmlNodePtr node,
>>                  virBufferVSprintf(buf, "(uname 'phy:%s')", source);
>>              else
>>                  virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", source);
>> -        }
>> +        } else if (typ == 2)
>> +            virBufferVSprintf(buf, "(uname 'phy:%s')", source);
> 
> This is leaking the 'source' string, 

It is not leaking imho, since 'down under' there is an xmlFree(source).

> and a volume can be either a file
> or a physical device, so fixing it to be 'phy:' is not correct.

How can we know if the volume is a file or a device?

> We also need to apply the reverse mapping when generating the XML. eg
> do a virStorageVolLookupByPath() to discover the volume/pool. 

Mmm... that sounds not trivial... (I mean duplicate wise).



Stefan