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).
/** - * virFileDirectFdNew: + * virFileWrapperFdNew: * @fd: pointer to fd to wrap + * @bypassCache: bypass system cache
Here, I'd add: * @nonBlock: ensure non-blocking I/O
* @name: name of fd, for diagnostics * - * Update *FD (created with virFileDirectFdFlag() among the flags to - * open()) to ensure that all I/O to that file will bypass the system - * cache. This must be called after open() and optional fchown() or - * fchmod(), but before any seek or I/O, and only on seekable fd. The - * file must be O_RDONLY (to read the entire existing file) or - * O_WRONLY (to write to an empty file). In some cases, *FD is - * changed to a non-seekable pipe; in this case, the caller must not + * Update @fd (possibly created with virFileDirectFdFlag() among the flags + * to open()) to ensure it properly supports non-blocking I/O, i.e., it will + * report EAGAIN. Iff @direct is true (in which case fd must have been + * created with virFileDirectFdFlag()), it will also be ensured that all + * I/O to that file will bypass the system cache. This must be called after + * open() and optional fchown() or fchmod(), but before any seek or I/O, and + * only on seekable fd. The file must be O_RDONLY (to read the entire + * existing file) or O_WRONLY (to write to an empty file). In some cases, + * @fd is changed to a non-seekable pipe; in this case, the caller must not * do anything further with the original fd.
and here, I'd clarify what "In some cases" means: If @nonBlock is true, or in some cases when @bypassCache is true, @fd is changed to a non-seekable pipe; ...
+virFileWrapperFdPtr +virFileWrapperFdNew(int *fd, bool bypassCache, const char *name)
add the bool nonBlock parameter here,
{ - virFileDirectFdPtr ret = NULL; + virFileWrapperFdPtr ret = NULL; bool output = false; int pipefd[2] = { -1, -1 }; int mode = -1;
- /* XXX support posix_fadvise rather than spawning a child, if the + /* XXX support posix_fadvise rather than O_DIRECT, if the * kernel support for that is decent enough. */
This is why I'm suggesting the fourth parameter. If we ever get a fixed kernel, then it is conceivable to want bypassCache but not nonBlock (maybe not for qemu, but for other uses). Right now, use of iohelper happens to have a side-effect of conversion to a pipe, and thus conversion to non-blocking, but if we find a way to avoid iohelper while still implementing cache bypass, then we'd need a second parameter stating whether we also need the iohelper effect of non-blocking I/O.
- if (!O_DIRECT) { + if (bypassCache && !O_DIRECT) { virFileError(VIR_ERR_INTERNAL_ERROR, "%s", _("O_DIRECT unsupported on this platform")); return NULL; @@ -156,12 +160,7 @@ virFileDirectFdNew(int *fd, const char *name) return NULL; }
-#ifdef F_GETFL - /* Mingw lacks F_GETFL, but it also lacks O_DIRECT so didn't get - * here in the first place. All other platforms reach this - * line. */ mode = fcntl(*fd, F_GETFL); -#endif
if (mode < 0) { virFileError(VIR_ERR_INTERNAL_ERROR, _("invalid fd %d for %s"), @@ -208,48 +207,59 @@ virFileDirectFdNew(int *fd, const char *name) error: VIR_FORCE_CLOSE(pipefd[0]); VIR_FORCE_CLOSE(pipefd[1]); - virFileDirectFdFree(ret); + virFileWrapperFdFree(ret); return NULL; } +#else +virFileWrapperFdPtr +virFileWrapperFdNew(int *fd ATTRIBUTE_UNUSED, + bool bypassCache ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED) +{ + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", + _("virFileWrapperFd unsupported on this platform"));
Don't forget to tweak this signature, too. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org