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@xxxxxxxxxx> >> --- >> 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); >> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
