[libvirt] [PATCH 2/4] conf: Fix parsing python style triple quotes

[Cole Robinson]

On 11/19/2010 03:38 PM, Eric Blake wrote:
> On 11/19/2010 09:15 AM, Cole Robinson wrote:
>> An incorrect check broke matching the closing set of quotes. Update
>> tests to cover this case for XM config files, and update the domain schema
>> to allow more path characters.
>>
>> -      <param name="pattern">/[a-zA-Z0-9_\.\+\-&/%]*</param>
>> +      <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]*</param>
> 
> So far, so good...
> 
>>      </data>
>>    </define>
>>    <define name="devicePath">
>>      <data type="string">
>> -      <param name="pattern">/[a-zA-Z0-9_\+\-/%]+</param>
>> +      <param name="pattern">/[a-zA-Z0-9_\+\-\\&"'<>/%]+</param>
> 
> but given that a devicePath can't have '.', should it really be allowed
> to have other characters like &, ", ', <, or >?
> 

I didn't notice the lack of '.'  but should probably also be added. From
the XML point of view, a devicePath could really just be any old FS path.

Looking again, deviceName/Path are used in very different ways in the
schema, so this might need additional cleanup anyways. But anything that
represents a path should probably allow all valid path characters,
device or not.

>>      </data>
>>    </define>
>>    <define name="deviceName">
>>      <data type="string">
>> -      <param name="pattern">[a-zA-Z0-9_\.\-:/]+</param>
>> +      <param name="pattern">[a-zA-Z0-9_\.\-\\&"'<>:/]+</param>
> 
> Likewise for deviceName.
> 
>> +++ b/src/util/conf.c
>> @@ -400,8 +400,9 @@ virConfParseString(virConfParserCtxtPtr ctxt)
>>          ctxt->cur += 3;
>>          base = ctxt->cur;
>>  
>> +        /* Find the ending triple quotes */
>>          while ((ctxt->cur + 2 < ctxt->end) &&
>> -               (STRPREFIX(ctxt->cur, "\"\"\""))) {
>> +               !(STRPREFIX(ctxt->cur, "\"\"\""))) {
> 
> Ah, the bug fix for patch 1.  ACK to patch 1, then.
> 
>> +++ b/tests/xmconfigdata/test-escape-paths.cfg
>> @@ -19,7 +19,7 @@ vnc = 1
>>  vncunused = 1
>>  vnclisten = "127.0.0.1"
>>  vncpasswd = "123poi"
>> -disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso&test,hdc:cdrom,r" ]
>> +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso&test,hdc:cdrom,r", """phy:/dev/HostVG/XenGuest'",hdb,w""" ]
>>  vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ]
>>  parallel = "none"
>>  serial = "none"
>> diff --git a/tests/xmconfigdata/test-escape-paths.xml b/tests/xmconfigdata/test-escape-paths.xml
>> index dabf492..13e6e29 100644
>> --- a/tests/xmconfigdata/test-escape-paths.xml
>> +++ b/tests/xmconfigdata/test-escape-paths.xml
>> @@ -31,6 +31,11 @@
>>        <target dev='hdc' bus='ide'/>
>>        <readonly/>
>>      </disk>
>> +    <disk type='block' device='disk'>
>> +      <driver name='phy'/>
>> +      <source dev='/dev/HostVG/XenGuest'"'/>
> 
> Hmm; this really is a case of deviceName in the domain.rng schema.  Are
> there really devices named with ' or " in the name?
> 

Probably not, but someone could always create a symbolic link with
whatever valid pathname they want.

> If so, then ACK to patch 2.  If not, then it would be better to use
> <disk type='file'><source file='/.../XenGuest'"'/>, since that
> would pick up on absFilePath, which is more likely to match reality of
> having ' or " in the name.
> 

Having a whacky named file is probably more likely, but behind the
scenes it's all the same code here that is being tested (xm disk block
generation). The restrictions in RNG were fairly arbitrary anyways.

Thanks,
Cole