Re: [PATCH v2 2/3] tools/libs/gnttab: decouple more from mini-os

[Juergen Gross <jgross@xxxxxxxx>]

On 11.01.22 21:08, Andrew Cooper wrote:
> On 11/01/2022 15:03, Juergen Gross wrote:
> > libgnttab is using implementation details of Mini-OS. Change that by
> > letting libgnttab use the new alloc_file_type() and get_file_from_fd()
> > functions and the generic dev pointer of struct file from Mini-OS.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - add alloc_file_type() support
> > ---
> >   tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++-----------
> >   1 file changed, 48 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
> > index f78caadd30..c19f339c8c 100644
> > --- a/tools/libs/gnttab/minios.c
> > +++ b/tools/libs/gnttab/minios.c
> > @@ -28,18 +28,53 @@
> >   #include <sys/mman.h>
> >   
> >   #include <errno.h>
> > +#include <malloc.h>
> >   #include <unistd.h>
> >   
> >   #include "private.h"
> >   
> > -void minios_gnttab_close_fd(int fd);
> > +int minios_gnttab_close_fd(int fd);
> Again, like evtchn, no need to forward declare.
>
>
> However, I've only just realised...
>
> > +
> > +int minios_gnttab_close_fd(int fd)
> > +{
> > +    struct file *file = get_file_from_fd(fd);
> > +
> > +    gntmap_fini(file->dev);
> > +    free(file->dev);
> > +
> > +    return 0;
> > +}
> The only reason this doesn't break the build is because the declaration
> is not in a header.  After this change, you've got the function
> returning int here, but declared as returning void as far as MiniOS is
> concerned.
>
> Furthermore, we cannot fix this mess atomically now that minios has
> moved into a separate repo.  It's tolerable from an ABI point of view on
> x86, but I don't know for certain on other architectures.
Mini-OS is x86 only right now (well, it has some Arm parts in it, but
it is not in a state to be usable on Arm).

> The least bad way I can think of doing this would be to leave void
> minios_gnttab_close_fd(int fd) exactly as it was, and instead of
> converting it's use here, use a separate static function straight away
> for the file ops.  (Will be necessary anyway if you like my suggestion
> of passing file too).  Then in the whole function in "tools/libs: final
> cleanup making mini-os callbacks static", rather than just making it static.
I can change it if you really want.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature