Hi All, Can someone review the patch please ? Thanks, Shivaprasad On Wed, Mar 15, 2017 at 11:22 AM, Shivaprasad bhat < shivaprasadbhat(a)gmail.com&gt; wrote:

On 01/27/2017 05:59 AM, Shivaprasad G Bhat wrote: Since it's been sitting for a while with a couple of pings... First off - it doesn't apply on top of the current git tree. Secondly, not an area of the code I've worked in, but perhaps a response will at least cause others to chime in... Beyond that, the commit messages noted above are really, really old. Still in looking at them "quickly" they are handling the Save operation and not the Restore operation AFAICT, so I'm not sure your commit message is precise. What this patch is trying to do is apply the same/similar logic for the Restore operation that the Save operation via qemuDomainSaveMemory is doing - is that a fair assessment? But what I'm not clear of is what problem is trying to be solved. What follows is my interpretation from looking at the code... There are 4 callers to SaveImageOpen, two (qemuDomainSaveImageGetXMLDesc and qemuDomainSaveImageDefineXML) can be ruled out quickly because they pass bypass_cache = false and wrapperFd = NULL. That leaves qemuDomainRestoreFlags which will set bypass_cache depending on whether or not the VIR_DOMAIN_SAVE_BYPASS_CACHE flag was set and qemuDomainObjRestore which has a similar setting based on whether qemuDomainObjStart had VIR_DOMAIN_START_BYPASS_CACHE set. All of that seems perfectly reasonable. So, what then does this patch do. Well as long as a valid wrapperFd was passed, it would then always call virFileWrapperFdNew with at least the VIR_FILE_WRAPPER_NON_BLOCKING flag set; however, if one checks the virFileWrapperFdNew comments that should only be set if non-blocking I/O is properly supported. Still nothing is yet done with that flag, so again no big deal (there's a comment about posix_fadvise). At least we know for now that open_write will be false from the two paths in question; otherwise, the other requirement in virFileWrapperFdNew that the fd be O_RDONLY or O_WRONLY wouldn't be met. So the question is does the iohelper really need to always be used on Restore when bypass_cache isn't set? And of course why? IIRC, using iohelper was to avoid blocking other qemu interactions. But Restore is pulling from a file and doesn't have those same interactions - so what advantage is using iohelper for the Restore function that doesn't have bypass-cache set? John BTW: There's another side effect... On a platform where virFileWrapperFdNew isn't supported, Restore wuld fail always. Not that Save would work for the same reason, but one wouldn't be able to Restore.