Jim Meyering wrote:
Hi John,
I tried to apply that, but failed miserably, since all of the following was recently redone to use virBufferVSprintf rather than snprintf.
Yea I suspected the code was likely seeing some motion. Thanks for bringing it forward.
And it's a good thing, because with the changes below, there was potential buffer overrun: nc could end up larger than PATH_MAX. You are correct. I mistook the return value of snprintf() in the case of truncation.
One remaining bit that I haven't done: add tests for this, exercising e.g., - cachemode - !cachemode && (disk->shared && !disk->readonly) - !cachemode && (!disk->shared || disk->readonly)
That logic should be correct as it exists in the patch, namely we'll generate a "cache=*" flag in the case cachemode was specified explicitly in the xml definition or for the preexisting case of (shared && !readonly). Here if both happen to be true the explicit xml cache tag takes precedence. But with the existing code we need to remove the clause of: @@ -1007,18 +1025,34 @@ int qemudBuildCommandLine(virConnectPtr if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) virBufferAddLit(&opt, ",boot=on"); - if (disk->shared && !disk->readonly) - virBufferAddLit(&opt, ",cache=off"); if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType); As this results in generation of a "cache=*" twice on the qemu cmdline, and possibly of different dispositions. With this change I think it is good to go. The attached patch incorporates the above modification. -john -- john.cooper@redhat.com