[libvirt] [PATCH 1/2] util: Generalize virFileDirectFd
Jiri Denemark
jdenemar at redhat.com
Tue Feb 7 12:08:43 UTC 2012
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
More information about the libvir-list
mailing list