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