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

[Andrew Cooper <amc96@xxxxxxxx>]

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.

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.

~Andrew