On Mon, Feb 06, 2012 at 15:42:43 -0700, Eric Blake wrote: > On 02/06/2012 07:51 AM, Jiri Denemark wrote: > > virFileDirectFd was used for accessing files opened with O_DIRECT using > > libvirt_iohelper. We will want to use the helper for accessing files > > regardless on O_DIRECT and thus virFileDirectFd was generalized and > > renamed to virFileWrapperFd. > > --- > > src/qemu/qemu_driver.c | 42 +++++++++++--------- > > src/util/virfile.c | 98 ++++++++++++++++++++++++++--------------------- > > src/util/virfile.h | 18 +++++---- > > 3 files changed, 87 insertions(+), 71 deletions(-) > > > > Everything you've got is a mechanical rename, so I have no objection to > th is as-is. But if we ever get better kernel support for > posix_fadvise, we have a problem - use of posix_fadvise won't magically > make an fd report EAGAIN. I think it might be worth a v2 with some > slight tweaks by adding a second bool parameter to virFileWrapperFdNew. OK, good point. However, I added a single int flags instead of two bool parameters since I consider combining named bits more readable than several true/false arguments for which you always need to go to the header to get the meaning. > > > @@ -2625,7 +2625,8 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, > > &needUnlink, &bypassSecurityDriver); > > if (fd < 0) > > goto endjob; > > - if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) > > + if (bypass_cache && > > + (wrapperFd = virFileWrapperFdNew(&fd, true, path)) == NULL) > > Here, call virFileWrapperFdNew(&fd, true, true, path) > > and in patch 2/2, it would change to: > > wrapperFd = virFileWrapperFdNew(&fd, bypass_cache, true, path) There's no need to pass a proper non-blocking fd to qemu here since save is not live... > > > @@ -2959,7 +2960,8 @@ doCoreDump(struct qemud_driver *driver, > > NULL, NULL)) < 0) > > goto cleanup; > > > > - if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) > > + if (bypass_cache && > > + (wrapperFd = virFileWrapperFdNew(&fd, true, path)) == NULL) > > Likewise (oops, you missed this one in 2/2). While here core dump can be live so we need non-blocking. And this place was the only place I touched in 2/2 so I'm not quite sure why you say I missed it... Jirka -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list