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

[Anya Harter]

On 06/26/2018 08:52 AM, John Ferlan wrote:
> 
> 
> On 06/22/2018 09:06 AM, Anya Harter wrote:
> [...]
> 
>>>
>>> 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.
>>
> 
> Sorry for the delay - I stuck my head into the checkpoint/backup
> quicksand and it's been "difficult" to remove it ;-)
> 
> So going back to your analysis from the previous series:
> 
> https://www.redhat.com/archives/libvir-list/2018-June/msg01467.html
> 
> Now I see where/why you got here. The myopia I have is that host->name
> shouldn't have a comma in it as it's an invalid address format.
> 
> So, rather than:
> 
>   <host name='example,foo.org' port='3260'/>
> 
> if I instead use:
> 
>   <source protocol='iscsi' name='iqn.1992-01.com.example:my,storage/1'>
> 
> then with a couple of adjustments to input:
> 
> -    <controller type='scsi' index='0'/>
> +    <controller type='scsi' index='0' model='virtio-scsi'/>
> 
> ...
> 
> -      <source protocol='iscsi' name='iqn.1992-01.com.example'>
> -        <host name='example,foo.org' port='3260'/>
> +        <source protocol='iscsi' name='iqn.1992-01.com.example:my,storage/1'>
> +        <host name='example.foo.org' port='3260'/>
> 
> qemuxml2argvtest.c:
> 
> -            QEMU_CAPS_SCSI_LSI,
> 
> and output:
> 
> --device lsi,id=scsi0,bus=pci.0,addr=0x3 \
> +-device virtio-scsi-pci,id=scsi0,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 \
> +-drive file=iscsi://example.foo.org:3260/iqn.1992-01.com.example%3Amy,,\
> +storage/1,if=none,format=raw,id=drive-hostdev0 \
> +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=0,lun=4,\
> +drive=drive-hostdev0,id=hostdev0 \
> 
> Then, I think we cover this oddball case.
> 
> I can make those changes for you before pushing as long as you
> agree that's fine. In the long run, it's similar, it's just avoiding
> the invalid hostname/uri string.

I think that sounds good.

Thanks,

Anya

> 
> John
> 
> and yes, once this is pushed, that item comes off the wiki list.
> 
> [...]
>