Re: [Xen-devel] [PATCH RFC 04/23] libxc: add support for FreeBSD

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH RFC 04/23] libxc: add support for FreeBSD"):
> Add the FreeBSD implementation of the privcmd and evtchn devices
> interface.
> 
> The evtchn device interface is the same as the Linux one, while the
> privcmd map interface is simplified because FreeBSD only supports
> IOCTL_PRIVCMD_MMAPBATCH.
...
> +/* Optionally flush file to disk and discard page cache */
> +void discard_file_cache(xc_interface *xch, int fd, int flush) 
> +{

My main criticism of this is that much of this file is a (nearly)
verbatim copy of the Linux functions.  I think we should have a
conversation about other approaches that would allow us to share more
of the code.

Two options I can think of (which could perhaps be combined):
   xc_posix.c containing some "standard" versions
   #ifdeffery


Now, prompted by your patch to look at this function:

> +    off_t cur = 0;
> +    int saved_errno = errno;
> +
> +    if ( flush && (fsync(fd) < 0) )
> +        goto out;

This function has truly demented error behaviour, but this is not
your fault - you're just copying what the Linux one does.

I'm pretty sure that many of the callers expect this function to do
its thing and that if it doesn't work we might get corruption in the
guest.

> +    /* Discard from the buffer cache. */
> +    if ( posix_fadvise(fd, 0, cur, POSIX_FADV_DONTNEED) < 0 )
> +        goto out;

Which also means that the use of posix_fadvise is hardly safe.
fadvise can quite legally be a noop.


> +static xc_osdep_handle freebsd_privcmd_open(xc_interface *xch)
> +{
> +    int flags, saved_errno;
> +    int fd = open(PRIVCMD_DEV, O_RDWR);
> +
> +    if ( fd == -1 )
> +    {
> +        PERROR("Could not obtain handle on privileged command interface "
> +               PRIVCMD_DEV);
> +        return XC_OSDEP_OPEN_ERROR;

I think it would be better to use the "goto out" error handling
style.  Of course this function is a clone so really I'm complaining
about an existing function.

Maybe I should stop looking under this rock before I discover any more
rotting yak hair.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel