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

[Jiri Denemark]

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