[libvirt] [PATCH 2/2] qemu: Escape commas for qemuBuildSCSIiSCSIHostdevDrvStr

[Anya Harter]

On 06/21/2018 06:55 PM, John Ferlan wrote:
> 
> 
> On 06/20/2018 09:17 AM, Anya Harter wrote:
>> Add comma escaping for netsource
>>
>> Signed-off-by: Anya Harter <aharter at redhat.com>
>> ---
>>  src/qemu/qemu_command.c                 | 4 +++-
>>  tests/qemuxml2argvdata/name-escape.args | 6 +++++-
>>  tests/qemuxml2argvdata/name-escape.xml  | 7 +++++++
>>  tests/qemuxml2argvtest.c                | 5 ++++-
>>  4 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index a99240992a..96c6c08c2d 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -4677,7 +4677,9 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
>>          if (!(netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
>>                                                     srcPriv->secinfo : NULL)))
>>              goto cleanup;
>> -        virBufferAsprintf(&buf, "file=%s,if=none,format=raw", netsource);
>> +        virBufferAddLit(&buf, "file=");
>> +        virQEMUBuildBufferEscapeComma(&buf, netsource);
>> +        virBufferAddLit(&buf, ",if=none,format=raw");
>>      }
>>  
>>      if (virBufferCheckError(&buf) < 0)
>> diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args
>> index aef7c238ca..1cbb1efd66 100644
>> --- a/tests/qemuxml2argvdata/name-escape.args
>> +++ b/tests/qemuxml2argvdata/name-escape.args
>> @@ -22,6 +22,7 @@ bar=2/monitor.sock,server,nowait \
>>  -no-shutdown \
>>  -no-acpi \
>>  -boot c \
>> +-device lsi,id=scsi0,bus=pci.0,addr=0x3 \
> 
> [1]
> 
>>  -device usb-ccid,id=ccid0,bus=usb.0,port=1 \
>>  -usb \
>>  -drive file=/dev/HostVG/QEMUGuest1,format=qcow2,if=none,id=drive-ide0-0-0,\
>> @@ -41,4 +42,7 @@ cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \
>>  -spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\
>>  rendernode=/dev/dri/foo,,bar \
>>  -device cirrus-vga,id=video0,bus=pci.0,addr=0x2 \
>> --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>> +-drive file=iscsi://example,,foo.org:3260/iqn.1992-01.com.example/0,if=none,\
>> +format=raw,id=drive-hostdev0 \
>> +-device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev0,id=hostdev0 \
>> +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
>> diff --git a/tests/qemuxml2argvdata/name-escape.xml b/tests/qemuxml2argvdata/name-escape.xml
>> index 70a1ce09d3..24dd248842 100644
>> --- a/tests/qemuxml2argvdata/name-escape.xml
>> +++ b/tests/qemuxml2argvdata/name-escape.xml
>> @@ -27,6 +27,13 @@
>>        </iotune>
>>        <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>>      </disk>
>> +    <controller type='scsi' index='0'/>
> 
> [1] If you change this to:
> 
> <controller type='scsi' index='0' model='virtio-scsi'/>
> 
> Then the lsi doesn't show up...
> 
>> +    <hostdev mode='subsystem' type='scsi' managed='yes'>
>> +      <source protocol='iscsi' name='iqn.1992-01.com.example'>
>> +        <host name='example,foo.org' port='3260'/>
>> +      </source>
> 
> Something is not quite right about seeing "example,foo.org" as a host
> name. I don't see that as a valid host name regardless of comma
> escaping. Do we escape commas in other "<host name=%s...>" string
> fields? I don't see it being done for qemuBuildChrChardevStr (search on
> TCP or "host.*name" in qemu_command.c)>
> Now the one thing that perhaps *could* be escaped is the <source> name
> attribute. Currently it's set to "iqn.1992-01.com.example", but the spec
> defines a structure of that name field to have a field that is a "String
> defined by the naming authority" (shorter version, see
> https://en.wikipedia.org/wiki/ISCSI)
> 
> So if the name is : 'iqn.1992-01.com.example:storage/2', then the
> "storage/2" piece is the string defined by the naming authority. Not
> that I know whether it would work, but that's where I suppose a comma
> could go.

If you could give me a string with a comma in it, I would be happy to
put it into the test instead of in the host name field. However, if no
one can think of a realistic case where comma escaping would be
necessary, we can just remove the test altogether. I only put it in as a
test that implementation was correct and that the commas were actually
being escaped correctly.

> 
>> +      <address type='drive' controller='0' bus='0' target='0' unit='4'/>
>> +    </hostdev>
>>      <graphics type='vnc'>
>>        <listen type='socket'/>
>>      </graphics>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 582a9de7bb..2e891a0bea 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2772,7 +2772,10 @@ mymain(void)
>>              QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
>>              QEMU_CAPS_DEVICE_ISA_SERIAL,
>>              QEMU_CAPS_CHARDEV_FILE_APPEND,
>> -            QEMU_CAPS_CCID_EMULATED);
>> +            QEMU_CAPS_CCID_EMULATED,
>> +            QEMU_CAPS_VIRTIO_SCSI,
>> +            QEMU_CAPS_SCSI_LSI,
> 
> [1] and the LSI wouldn't be needed here.
> 
> John
> 
> first patch looks fine, but I'm not so convinced on this one only
> because of the host name w/ a comma.
> 
>> +            QEMU_CAPS_DEVICE_SCSI_GENERIC);
>>      DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
>>  
>>      DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
>>