4:42 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org