Re: [libvirt] [PATCH v2 0/4] qemu: handle ',' and '=' in VM name

On 05/04/2016 10:56 AM, Cole Robinson wrote: > This series adds qemu cli comma escaping to several places that > are dependent on the VM name, to enable names with embedded commas. > > Patch 4 makes use of qemu -name guest=X value to allow names with > '=' in them. > > https://bugzilla.redhat.com/show_bug.cgi?id=639926 > https://bugzilla.redhat.com/show_bug.cgi?id=1276485 > > Note: There's likely other places that are VM name dependent that need > comma escaping too, but this hits the mandatory ones. I've listed some > more on the BiteSizedTasks page: > > http://wiki.libvirt.org/page/BiteSizedTasks#qemu:_Use_comma_escaping_for_... > > v2: > Rebase to master > > Cole Robinson (4): > qemu: command: escape commas in VM name > qemu: command: escape commas in secret master path > qemu: command: escape commas in chardev socket path > qemu: command: Use -name guest= if available > > src/qemu/qemu_capabilities.c | 2 ++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_command.c | 22 ++++++++++++-------- > tests/qemucapabilitiesdata/caps_2.1.1-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.4.0-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.5.0-1.caps | 1 + > tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + > .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 24 ++++++++++++++++++++++ > .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 18 ++++++++++++++++ > tests/qemuxml2argvtest.c | 2 ++ > 10 files changed, 65 insertions(+), 8 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml > ewww again ;-) While I cannot imagine a reason for someone (other than testing perhaps) to want a "," or an "=" in a vm name, I suppose since it's possible to handle by qemu, then we should 'handle' it. Sorry I dropped the ball after my initial pass - I guess I was wondering if there were any other (similar) feelings!

After reading the bz, I see why you've gone with double commas, although it wasn't apparent without that read... So maybe each commit that adds that double comma escape could explain why the double comma works (or even at points in the code where it occurs?).

Searching qemu_command.c for domainChannelTargetDir and it's seems things are covered (good)... Searching for domainLibDir and I think there's one usage not covered - qemuBuildGraphicsVNCCommandLine uses it to build "$path_with_domainLibDir/vnc.sock".

Of course finding these "one off" type issues makes me think at some point in the future one of those will be used for "something" and the double comma will be missed. So should we generate a local "escaped" names for domainLibDir and domainChannelTargetDir that would include the double comma and that would be passed to the corresponding users?

I think as long as you cover the VNC case and add a few more words why the ",," can be used on the command line, then the series seems reasonable to me, although you may want to wait a bit to push to see if anyone else has angst over the two changes, especially now having "guest=$name" be the default for anyone using qemu 2.1 or later.