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

On 04/02/2018 04:17 PM, Sukrit Bhatnagar wrote: Well we all get busy and delayed by other things! It's been a week since you posted and well, I know I'm behind doing reviews! Nice on the using the --- for your adjustments and the issue you discovered. What happened to the changes from your previous version? They don't seem to be included in this patch and they weren't pushed upstream. This patch is all new changes. The "next" step is to attempt to generate patches that make incremental progress towards the end goal. Not all your changes need to go in one pile especially if something is separable - there's a methodology one develops over time. Changes don't need to be "so separated" that you get very large series, but you can create smaller patches, altering single API's/helpers and adjusting anything called by them to handle the changes. Some times it's a changed result and other times it's doing nothing because the patch fixes something. Again, it's one of those over time things. In this case, almost every function could have had it's own patch. That way a reviewer can ACK/Reviewed-by and push part of the series while perhaps asking for respins on other parts. It also allows for a NACK of a specific area. Much easier to change and review smaller things too! Since this is a GSOC type activity I won't "do" the work for you, but I will help "guide" you to the answer. First things first - hopefully you haven't lost your original patch. It's easily recreateable at least. We will start easy/slow... Using that patch as a starting point, create a series of 5 patches patch 1: Changes for qemuBuildRomStr patch 2: Changes for qemuBuildDriveDevStr patch 3: Changes for qemuBuildFSStr and qemuBuildFSDevStr patch 4: Changes for qemuBuildGraphicsVNCCommandLine patch 5: Changes for qemuBuildDomainLoaderCommandLine Hold onto the changes for qemuBuildHostNetStr, qemuBuildChrChardevFileStr, qemuBuildChrChardevStr, and qemuBuildGraphicsSPICECommandLine as they'll be combined with separated patches from this patch. Post the above 5 - I've included patch 1 & 2 for you as an attachment to this reply so you can see the format... It should be fairly easy to copy from there and post as a v4. Once you've done that - work through the rest of this using similar context - although a few of these will be a bit larger and more complicated (especially the first one for build network drive. This is because qemuBuildDriveSourceStr calls qemuGetDriveSourceString which calls qemuBuildNetworkDriveStr returning @source. Then back in qemuBuildDriveSourceStr the @source goes through another transformation: if (source) { virBufferAddLit(buf, "file="); ... virQEMUBuildBufferEscapeComma(buf, source); ... Because the return from qemuBuildNetworkDriveStr is used by callers that don't expect qemuGetDriveSourceString to return a comma escaped string that means adding a bit of logic so that qemuBuildNetworkDriveURI and qemuBuildNetworkDriveStr can escape only when necessary. Returning an escaped string for qemuDomainSnapshotCreateSingleDiskActive would not be a good thing. Still it's a *good thing* you've gone through this exercise *and* made note of what you saw. It makes it clearer what the "right" path is for me at least. Of course if you'd separated out your patches it would make resolving it a bit easier! Also, this exercise has shown there's a bug in how the command line is built for hostdev's. The source is not escaped, although I doubt we'd get an iSCSI tgt/lun with a "rogue" comma - it's just not expected. Still who knows, we still need to handle it. I think a third parameter "bool escape" will be necessary... There is a general preference for one argument per line. This logic needs to be separated into "if (escape)" escape the socket, e.g.: if (src->hosts->socket) { if (escape) { virQEMUBuildBufferEscapeComma(&buf, src->hosts->socket); socket = virBufferContentAndReset(&buf); } if (virAsprintf(&uri->query, "socket=%s", socket ? socket : src->hosts->socket) < 0) goto cleanup; } The virBufferContentAndReset will reinitialize the buffer, so no need for this call, but we would leak @socket possibly, so that does need to be freed.... Also, need for extra blank line here. This then is just: VIR_FREE(socket); I think a third parameter "bool escape" will be necessary... again, one line and we should only need one, e.g. 'tmp' - since we know it's initialized to NULL we can use that when deciding whether we escape or not. if (src->path) { if (escape) { virBufferAddLit(&buf, ":exportname="); virQEMUBuildBufferEscapeComma(&buf, src->path); } else { virBufferAsprintf(&buf, ":exportname=%s", src->path); } } if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->path); tmp = virBufferContentAndReset(&bufTemp); } if (src->nhosts == 0) { if (virAsprintf(&ret, "sheepdog:%s", tmp ? tmp : src->path) < 0) goto cleanup; } else if (src->nhosts == 1) { if (virAsprintf(&ret, "sheepdog:%s:%u:%s", src->hosts->name, src->hosts->port, tmp ? tmp : src->path) < 0) goto cleanup; } else { NB: In your patch - if the .xml/.args file hadn't used a host w/ a path having a comma, then that would have failed. IOW: if tests/qemuxml2argdata/disk-drive-network-sheepdog.xml: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='sheepdog' name='image,with,commas'> <host name='example.org' port='6000'/> </source> <target dev='vda' bus='virtio'/> </disk> was: <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='sheepdog' name='image,with,commas'/> <target dev='vda' bus='virtio'/> </disk> then tests/qemuxml2argvdata/disk-drive-network-sheepdog.args would change from: -drive file=sheepdog:example.org:6000:image,,with,,commas,format=raw,if=none,\ id=drive-virtio-disk0 \ to (something like): -drive file=sheepdog:image,,with,,commas,format=raw,if=none,\ id=drive-virtio-disk0 \ But with your change it would have had the 4 commas (hopefully this makes sense). if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->path); tmp = virBufferContentAndReset(&bufTemp); } virBufferStrcat(&buf, "rbd:", src->volume, "/", tmp ? tmp : src->path, NULL); VIR_FREE(tmp); if (src->configFile) { if (escape) { virQEMUBuildBufferEscapeComma(&bufTemp, src->configFile); tmp = virBufferContentAndReset(&bufTemp); } virBufferEscape(&buf, '\\', ":", ":conf=%s", tmp ? tmp : src->configFile); } Again, each of the callers used virBufferContentAndReset, so the @path and @file arguments would have been needed to be VIR_FREE'd instead. However, if you take my suggestion w/ a tmp variable, then you just VIR_FREE(tmp) instead. When you post your next patch - use this opportunity to separate out these two functions into their own patch (along with adjustments to callers to pass the escape bool. This will be the most complex patch out of them all. These can move to ... ... here (IOW: localize them more). VIR_FREE(name); NB: virBufferContentAndReset will be all you need for bufTemp, but @name is leaked, but that's adjusted above The changes for qemuBuildDiskThrottling should be in one patch. This has alignment issues w/ the previous line. It also points out something I'm not sure whether it's a bug or not. If net->data.socket.address == NULL, then the command line would be (from net-vhostuser.args): -netdev socket,listen=:2015, But that looks strange to me, I guess I'd expect: -netdev socket,listen="":2015, Still the former syntax works so I suppose it's OK. Still changes could be rewritten as: virBufferAsprintf(&buf, "socket%clisten=", type_sep); virQEMUBuildBufferEscapeComma(&buf, net->data.socket.address); virBufferAsprintf(&buf, ":%d,", net->data.socket.port); BTW: Your previous patch added a couple of changes to this API - they should be moved into here so that we have all the adjustments to qemuBuildHostNetStr in one patch. And like noted before - the above hunk can be it's own patch! One line per argument... path is leaked. And the above is in it's own patch. Here again, I'd combine the changes from your original patch to qemuBuildChrChardevStr into this one. I'd also include the changes for qemuBuildChrChardevFileStr within the same patch since they are "related". The above can be it's own patch and of course include the SPICE change from your original patch too. And this one gets it's own patch too. In the end, probably 11 patches total. Do the easy ones first. It's always good to make some progress and have some success rather than having to redo everything all over again and have this large pile of a singlular change waiting for some part of it to be fixed. Once everything is done we can remove the entry from bite sized tasks. John