Re: [libvirt] [PATCH] qemu: add virQEMUBuildBufferEscapeComma in qemu_command.c

On 03/16/2018 01:02 PM, Sukrit Bhatnagar wrote: Try to keep shorter lines in your commit message (generally <=70 ish characters) and the link to the bite size task should have gone under the --- below since it's likely that once this task is complete (and code pushed) that someone will remove the entry from the bite size task page and we don't really want a knowingly dead link to live in git history forever. In any case, consider the following (or something even shorter): qemu: Add comma escape for more command line options This patch adds virQEMUBuildBufferEscapeComma to qemu command line processing for data fields which are user provided in order to ensure that if a comma (',') is provided that it'll be properly handled by the qemu monitor which expects a double comma as a form of escaping. BTW: Since this was a followup/adjustment of what you submitted: https://www.redhat.com/archives/libvir-list/2018-March/msg00940.html then you should prefix with a "v2" via something like '--subject-prefix="PATCH v2"' on your 'git send-email' where you can place a link to the v1 in this section (below the ---) and a description of what changed or was fixed since v1. BTW: Your next patch will be v3, so you'll have a shot to try it! Providing a link to the previous version is generally "nice" so that the reviewer doesn't have to search for it and it gives a bit more history. Sure; however, you could use virBuffer* API's and then use virBufferContentAndReset in order to get the escaped string you'd want for the final result. I'll admit the src->configFile processing in qemuBuildNetworkDriveStr appears awful due to the need to escape the ':' for ":conf=", but I think you can get away with the EscapeComma on just the src->configFile. There is a test for adding the ":conf=%s", so if you do get it wrong, you'll know... If you wanted to be sure the right thing was done with the comma, then alter the XML file and see that the ARGS output gets the double comma. Whether that becomes part of the commit isn't necessary, but at least it proves to yourself that things were done right. hint... git log -p src/qemu/qemu_command.c <search for qemuBuildChrArgStr> and you'd see that processing was moved by commit id '426dc5eb' into qemuBuildChrChardevStr and qemuBuildChrChardevFileStr which were handled in this patch. True, since it's not built into the qemu command line but rather as part of some other command... Perhaps the request here was to remove the check and failure for "," in the name/path and use EscapeComma instead... Another one that wasn't listed is "rendernode" - although nothing tells us "how" that field is populated in the XML, I suppose someone could add a "," into the provided entry and that'd be problematic. Also "throttling.group" and "group_name" - although that's already escaped, someone conceivably could put in a "," into the name too as it's not checked. FWIW: After applying your patch I did a: grep "=%s" src/qemu/qemu_command.c | \ grep -v "bus=%s" | grep -v "fd=%s" | grep -v "id=%s" and that reduced the number things to specifically look at for needing the comma escaping. You may want to make the same check - I didn't complete looking at the output... [...] Below here there's a few more "%s" for net->data.socket.address that weren't addressed in different case's (SERVER, MCAST, UDP) [...] The rest seemed fine to my eyes. John