Re: [libvirt] [PATCH 1/2] util: Generalize virFileDirectFd

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.

> @@ -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)

> @@ -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).