[PATCH 0/2] tools/libs: decouple more from mini-os

[PATCH 0/2] tools/libs: decouple more from mini-os

[Juergen Gross]

This small series removes some hard coupling of the Xen build with some
Mini-OS internals, especially the struct file layout and the internal
organization of the file handling.

This series depends on the Mini-OS series posted recently:

https://lists.xen.org/archives/html/xen-devel/2022-01/threads.html#00110

The main idea is to no longer have Xen library specific structures
inside struct file, or to let struct file layout depend on the
configuration of Mini-OS.

All Xen libraries needing a hook in struct file should use the now
available generic dev pointer and allocate the needed data dynamically.

Additionally Xen libraries should get the pointer of struct file via
the new get_file_from_fd() function instead of accessing directly the
files[] array, which might go away in future (e.g. in order to support
dynamic allocation of struct file as needed).

Juergen Gross (2):
  tools/libs/evtchn: decouple more from mini-os
  tools/libs/gnttab: decouple more from mini-os

 tools/libs/evtchn/minios.c | 82 +++++++++++++++++++++++++++-----------
 tools/libs/gnttab/minios.c | 48 +++++++++++++++-------
 2 files changed, 91 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Juergen Gross]

Mini-OS and libevtchn are using implementation details of each other.
Change that by letting libevtchn use the new get_file_from_fd()
function and the generic dev pointer of struct file from Mini-OS.

By using private struct declarations Mini-OS will be able to drop the
libevtchn specific definitions of struct evtchn_port_info and
evtchn_port_list in future.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/evtchn/minios.c | 82 +++++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 24 deletions(-)

diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
index e5dfdc5ef5..3305102f22 100644
--- a/tools/libs/evtchn/minios.c
+++ b/tools/libs/evtchn/minios.c
@@ -38,16 +38,27 @@
 
 #include "private.h"
 
+LIST_HEAD(port_list, port_info);
+
+struct port_info {
+    LIST_ENTRY(port_info) list;
+    evtchn_port_t port;
+    unsigned long pending;
+    int bound;
+};
+
 extern void minios_evtchn_close_fd(int fd);
 
 extern struct wait_queue_head event_queue;
 
 /* XXX Note: This is not threadsafe */
-static struct evtchn_port_info *port_alloc(int fd)
+static struct port_info *port_alloc(int fd)
 {
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
+    struct file *file = get_file_from_fd(fd);
+    struct port_list *port_list = file->dev;
 
-    port_info = malloc(sizeof(struct evtchn_port_info));
+    port_info = malloc(sizeof(struct port_info));
     if ( port_info == NULL )
         return NULL;
 
@@ -55,12 +66,12 @@ static struct evtchn_port_info *port_alloc(int fd)
     port_info->port = -1;
     port_info->bound = 0;
 
-    LIST_INSERT_HEAD(&files[fd].evtchn.ports, port_info, list);
+    LIST_INSERT_HEAD(port_list, port_info, list);
 
     return port_info;
 }
 
-static void port_dealloc(struct evtchn_port_info *port_info)
+static void port_dealloc(struct port_info *port_info)
 {
     if ( port_info->bound )
         unbind_evtchn(port_info->port);
@@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info *port_info)
  */
 int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
 {
-    int fd = alloc_fd(FTYPE_EVTCHN);
+    int fd;
+    struct file *file;
+    struct port_list *list;
+
+    list = malloc(sizeof(*list));
+    if ( !list )
+        return -1;
+
+    fd = alloc_fd(FTYPE_EVTCHN);
+    file = get_file_from_fd(fd);
 
-    if ( fd == -1 )
+    if ( !file )
+    {
+        free(list);
         return -1;
+    }
 
-    LIST_INIT(&files[fd].evtchn.ports);
+    file->dev = list;
+    LIST_INIT(list);
     xce->fd = fd;
     printf("evtchn_open() -> %d\n", fd);
 
@@ -104,12 +128,15 @@ int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
 
 void minios_evtchn_close_fd(int fd)
 {
-    struct evtchn_port_info *port_info, *tmp;
+    struct port_info *port_info, *tmp;
+    struct file *file = get_file_from_fd(fd);
+    struct port_list *port_list = file->dev;
 
-    LIST_FOREACH_SAFE(port_info, &files[fd].evtchn.ports, list, tmp)
+    LIST_FOREACH_SAFE(port_info, port_list, list, tmp)
         port_dealloc(port_info);
+    free(port_list);
 
-    files[fd].type = FTYPE_NONE;
+    file->type = FTYPE_NONE;
 }
 
 int xenevtchn_fd(xenevtchn_handle *xce)
@@ -135,11 +162,14 @@ int xenevtchn_notify(xenevtchn_handle *xce, evtchn_port_t port)
 static void evtchn_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 {
     int fd = (int)(intptr_t)data;
-    struct evtchn_port_info *port_info;
+    struct file *file = get_file_from_fd(fd);
+    struct port_info *port_info;
+    struct port_list *port_list;
 
-    assert(files[fd].type == FTYPE_EVTCHN);
+    assert(file && file->type == FTYPE_EVTCHN);
+    port_list = file->dev;
     mask_evtchn(port);
-    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
+    LIST_FOREACH(port_info, port_list, list)
     {
         if ( port_info->port == port )
             goto found;
@@ -150,7 +180,7 @@ static void evtchn_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
 
  found:
     port_info->pending = 1;
-    files[fd].read = 1;
+    file->read = 1;
     wake_up(&event_queue);
 }
 
@@ -158,7 +188,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
                                                       uint32_t domid)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     int ret;
     evtchn_port_t port;
 
@@ -191,7 +221,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
                                                      evtchn_port_t remote_port)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     evtchn_port_t local_port;
     int ret;
 
@@ -222,9 +252,11 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
 int xenevtchn_unbind(xenevtchn_handle *xce, evtchn_port_t port)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct file *file = get_file_from_fd(fd);
+    struct port_info *port_info;
+    struct port_list *port_list = file->dev;
 
-    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
+    LIST_FOREACH(port_info, port_list, list)
     {
         if ( port_info->port == port )
         {
@@ -244,7 +276,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
                                               unsigned int virq)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     evtchn_port_t port;
 
     assert(get_current() == main_thread);
@@ -273,15 +305,17 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
 xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct file *file = get_file_from_fd(fd);
+    struct port_info *port_info;
+    struct port_list *port_list = file->dev;
     unsigned long flags;
     evtchn_port_t ret = -1;
 
     local_irq_save(flags);
 
-    files[fd].read = 0;
+    file->read = 0;
 
-    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
+    LIST_FOREACH(port_info, port_list, list)
     {
         if ( port_info->port != -1 && port_info->pending )
         {
@@ -292,7 +326,7 @@ xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
             }
             else
             {
-                files[fd].read = 1;
+                file->read = 1;
                 break;
             }
         }
-- 
2.26.2

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

[Juergen Gross]

libgnttab is using implementation details of Mini-OS. Change that by
letting libgnttab use the new get_file_from_fd() function and the
generic dev pointer of struct file from Mini-OS.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/gnttab/minios.c | 48 ++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index f78caadd30..64601db085 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -28,6 +28,7 @@
 #include <sys/mman.h>
 
 #include <errno.h>
+#include <malloc.h>
 #include <unistd.h>
 
 #include "private.h"
@@ -36,10 +37,25 @@ void minios_gnttab_close_fd(int fd);
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
 {
-    int fd = alloc_fd(FTYPE_GNTMAP);
-    if ( fd == -1 )
+    int fd;
+    struct file *file;
+    struct gntmap *gntmap;
+
+    gntmap = malloc(sizeof(*gntmap));
+    if ( !gntmap )
         return -1;
-    gntmap_init(&files[fd].gntmap);
+
+    fd = alloc_fd(FTYPE_GNTMAP);
+    file = get_file_from_fd(fd);
+
+    if ( !file )
+    {
+        free(gntmap);
+        return -1;
+    }
+
+    file->dev = gntmap;
+    gntmap_init(gntmap);
     xgt->fd = fd;
     return 0;
 }
@@ -54,8 +70,11 @@ int osdep_gnttab_close(xengnttab_handle *xgt)
 
 void minios_gnttab_close_fd(int fd)
 {
-    gntmap_fini(&files[fd].gntmap);
-    files[fd].type = FTYPE_NONE;
+    struct file *file = get_file_from_fd(fd);
+
+    gntmap_fini(file->dev);
+    free(file->dev);
+    file->type = FTYPE_NONE;
 }
 
 void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
@@ -64,16 +83,16 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
                              uint32_t notify_offset,
                              evtchn_port_t notify_port)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int stride = 1;
+
     if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN)
         stride = 0;
     if (notify_offset != -1 || notify_port != -1) {
         errno = ENOSYS;
         return NULL;
     }
-    return gntmap_map_grant_refs(&files[fd].gntmap,
-                                 count, domids, stride,
+    return gntmap_map_grant_refs(file->dev, count, domids, stride,
                                  refs, prot & PROT_WRITE);
 }
 
@@ -81,11 +100,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
                        void *start_address,
                        uint32_t count)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int ret;
-    ret = gntmap_munmap(&files[fd].gntmap,
-                        (unsigned long) start_address,
-                        count);
+
+    ret = gntmap_munmap(file->dev, (unsigned long) start_address, count);
     if (ret < 0) {
         errno = -ret;
         return -1;
@@ -95,10 +113,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 
 int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int ret;
-    ret = gntmap_set_max_grants(&files[fd].gntmap,
-                                count);
+
+    ret = gntmap_set_max_grants(file->dev, count);
     if (ret < 0) {
         errno = -ret;
         return -1;
-- 
2.26.2

Re: [PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Andrew Cooper]

On 07/01/2022 10:35, Juergen Gross wrote:
> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> index e5dfdc5ef5..3305102f22 100644
> --- a/tools/libs/evtchn/minios.c
> +++ b/tools/libs/evtchn/minios.c
> @@ -38,16 +38,27 @@
>  
>  #include "private.h"
>  
> +LIST_HEAD(port_list, port_info);
> +
> +struct port_info {
> +    LIST_ENTRY(port_info) list;
> +    evtchn_port_t port;
> +    unsigned long pending;
> +    int bound;

Now this is private, it's even more clear that pending and bound are bool's.

Making this adjustment drops 16 bytes from the structure.

> +};
> +
>  extern void minios_evtchn_close_fd(int fd);
>  
>  extern struct wait_queue_head event_queue;
>  
>  /* XXX Note: This is not threadsafe */
> -static struct evtchn_port_info *port_alloc(int fd)
> +static struct port_info *port_alloc(int fd)
>  {
> -    struct evtchn_port_info *port_info;
> +    struct port_info *port_info;
> +    struct file *file = get_file_from_fd(fd);
> +    struct port_list *port_list = file->dev;

This would be rather more obviously correct if port_alloc() took xce
rather than fd.

It is reasonable to assume that xce->fd is good, and that
get_file_from_fd(xce->fd) will be non-null, but the current logic makes
this very opaque.

> @@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info *port_info)
>   */
>  int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
>  {
> -    int fd = alloc_fd(FTYPE_EVTCHN);
> +    int fd;
> +    struct file *file;
> +    struct port_list *list;
> +
> +    list = malloc(sizeof(*list));
> +    if ( !list )
> +        return -1;
> +
> +    fd = alloc_fd(FTYPE_EVTCHN);
> +    file = get_file_from_fd(fd);
>  
> -    if ( fd == -1 )
> +    if ( !file )
> +    {
> +        free(list);
>          return -1;
> +    }

This wants rearranging to keep alloc_fd() ahead of malloc().  With that,
there is no need for free(list) in this error path.

>  
> -    LIST_INIT(&files[fd].evtchn.ports);
> +    file->dev = list;
> +    LIST_INIT(list);
>      xce->fd = fd;
>      printf("evtchn_open() -> %d\n", fd);
>  
> @@ -104,12 +128,15 @@ int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
>  
>  void minios_evtchn_close_fd(int fd)

Something very broken is going on here.  The top of evtchn.c declares:

extern void minios_evtchn_close_fd(int fd);

I'm surprised that the compiler doesn't object to instantiating a
function which has previously been declared extern.


Furthermore, in minios itself.

lib/sys.c:91:extern void minios_evtchn_close_fd(int fd);
lib/sys.c:447:      minios_evtchn_close_fd(fd);

So lib/sys.c locally extern's this function too.  It needs to be in the
public API if it is used like this, but surely the better thing is to
wire up xenevtchn_close() properly.

>  {
> -    struct evtchn_port_info *port_info, *tmp;
> +    struct port_info *port_info, *tmp;
> +    struct file *file = get_file_from_fd(fd);
> +    struct port_list *port_list = file->dev;

Is it safe to assume that fd is good here?

> @@ -273,15 +305,17 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
>  xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
>  {
>      int fd = xce->fd;
> -    struct evtchn_port_info *port_info;
> +    struct file *file = get_file_from_fd(fd);

You've dropped all uses of 'fd' from this function, so why not drop the
local variable and use xce->fd here?

~Andrew

Re: [PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3950 bytes --]

On 10.01.22 13:25, Andrew Cooper wrote:
> On 07/01/2022 10:35, Juergen Gross wrote:
>> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
>> index e5dfdc5ef5..3305102f22 100644
>> --- a/tools/libs/evtchn/minios.c
>> +++ b/tools/libs/evtchn/minios.c
>> @@ -38,16 +38,27 @@
>>   
>>   #include "private.h"
>>   
>> +LIST_HEAD(port_list, port_info);
>> +
>> +struct port_info {
>> +    LIST_ENTRY(port_info) list;
>> +    evtchn_port_t port;
>> +    unsigned long pending;
>> +    int bound;
> 
> Now this is private, it's even more clear that pending and bound are bool's.
> 
> Making this adjustment drops 16 bytes from the structure.

Already done in V2. :-)

> 
>> +};
>> +
>>   extern void minios_evtchn_close_fd(int fd);
>>   
>>   extern struct wait_queue_head event_queue;
>>   
>>   /* XXX Note: This is not threadsafe */
>> -static struct evtchn_port_info *port_alloc(int fd)
>> +static struct port_info *port_alloc(int fd)
>>   {
>> -    struct evtchn_port_info *port_info;
>> +    struct port_info *port_info;
>> +    struct file *file = get_file_from_fd(fd);
>> +    struct port_list *port_list = file->dev;
> 
> This would be rather more obviously correct if port_alloc() took xce
> rather than fd.
> 
> It is reasonable to assume that xce->fd is good, and that
> get_file_from_fd(xce->fd) will be non-null, but the current logic makes
> this very opaque.

Good point. Will change.

> 
>> @@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info *port_info)
>>    */
>>   int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
>>   {
>> -    int fd = alloc_fd(FTYPE_EVTCHN);
>> +    int fd;
>> +    struct file *file;
>> +    struct port_list *list;
>> +
>> +    list = malloc(sizeof(*list));
>> +    if ( !list )
>> +        return -1;
>> +
>> +    fd = alloc_fd(FTYPE_EVTCHN);
>> +    file = get_file_from_fd(fd);
>>   
>> -    if ( fd == -1 )
>> +    if ( !file )
>> +    {
>> +        free(list);
>>           return -1;
>> +    }
> 
> This wants rearranging to keep alloc_fd() ahead of malloc().  With that,
> there is no need for free(list) in this error path.

Yeah, but the error path of malloc() having failed is quite nasty then.

>> -    LIST_INIT(&files[fd].evtchn.ports);
>> +    file->dev = list;
>> +    LIST_INIT(list);
>>       xce->fd = fd;
>>       printf("evtchn_open() -> %d\n", fd);
>>   
>> @@ -104,12 +128,15 @@ int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
>>   
>>   void minios_evtchn_close_fd(int fd)
> 
> Something very broken is going on here.  The top of evtchn.c declares:
> 
> extern void minios_evtchn_close_fd(int fd);
> 
> I'm surprised that the compiler doesn't object to instantiating a
> function which has previously been declared extern.

Will be gone in V2, by making it static.

> Furthermore, in minios itself.
> 
> lib/sys.c:91:extern void minios_evtchn_close_fd(int fd);
> lib/sys.c:447:      minios_evtchn_close_fd(fd);
> 
> So lib/sys.c locally extern's this function too.  It needs to be in the
> public API if it is used like this, but surely the better thing is to
> wire up xenevtchn_close() properly.
> 
>>   {
>> -    struct evtchn_port_info *port_info, *tmp;
>> +    struct port_info *port_info, *tmp;
>> +    struct file *file = get_file_from_fd(fd);
>> +    struct port_list *port_list = file->dev;
> 
> Is it safe to assume that fd is good here?

Yes.

> 
>> @@ -273,15 +305,17 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
>>   xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
>>   {
>>       int fd = xce->fd;
>> -    struct evtchn_port_info *port_info;
>> +    struct file *file = get_file_from_fd(fd);
> 
> You've dropped all uses of 'fd' from this function, so why not drop the
> local variable and use xce->fd here?

Yes.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Andrew Cooper]

On 10/01/2022 12:49, Juergen Gross wrote:
> On 10.01.22 13:25, Andrew Cooper wrote:
>> On 07/01/2022 10:35, Juergen Gross wrote:
>>> @@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info
>>> *port_info)
>>>    */
>>>   int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
>>>   {
>>> -    int fd = alloc_fd(FTYPE_EVTCHN);
>>> +    int fd;
>>> +    struct file *file;
>>> +    struct port_list *list;
>>> +
>>> +    list = malloc(sizeof(*list));
>>> +    if ( !list )
>>> +        return -1;
>>> +
>>> +    fd = alloc_fd(FTYPE_EVTCHN);
>>> +    file = get_file_from_fd(fd);
>>>   -    if ( fd == -1 )
>>> +    if ( !file )
>>> +    {
>>> +        free(list);
>>>           return -1;
>>> +    }
>>
>> This wants rearranging to keep alloc_fd() ahead of malloc().  With that,
>> there is no need for free(list) in this error path.
>
> Yeah, but the error path of malloc() having failed is quite nasty then.

Oh yeah.  This is ugly, but I guess it is better this way around.

~Andrew

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

[Andrew Cooper]

On 07/01/2022 10:35, Juergen Gross wrote:
> @@ -54,8 +70,11 @@ int osdep_gnttab_close(xengnttab_handle *xgt)
>  
>  void minios_gnttab_close_fd(int fd)
>  {
> -    gntmap_fini(&files[fd].gntmap);
> -    files[fd].type = FTYPE_NONE;
> +    struct file *file = get_file_from_fd(fd);
> +
> +    gntmap_fini(file->dev);
> +    free(file->dev);

file->dev = NULL ?

Particularly as this is a pointer in a global files[] array.

Otherwise, LGTM.

~Andrew

Re: [PATCH 1/2] tools/libs/evtchn: decouple more from mini-os

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1556 bytes --]

On 10.01.22 19:51, Andrew Cooper wrote:
> On 10/01/2022 12:49, Juergen Gross wrote:
>> On 10.01.22 13:25, Andrew Cooper wrote:
>>> On 07/01/2022 10:35, Juergen Gross wrote:
>>>> @@ -75,12 +86,25 @@ static void port_dealloc(struct evtchn_port_info
>>>> *port_info)
>>>>     */
>>>>    int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
>>>>    {
>>>> -    int fd = alloc_fd(FTYPE_EVTCHN);
>>>> +    int fd;
>>>> +    struct file *file;
>>>> +    struct port_list *list;
>>>> +
>>>> +    list = malloc(sizeof(*list));
>>>> +    if ( !list )
>>>> +        return -1;
>>>> +
>>>> +    fd = alloc_fd(FTYPE_EVTCHN);
>>>> +    file = get_file_from_fd(fd);
>>>>    -    if ( fd == -1 )
>>>> +    if ( !file )
>>>> +    {
>>>> +        free(list);
>>>>            return -1;
>>>> +    }
>>>
>>> This wants rearranging to keep alloc_fd() ahead of malloc().  With that,
>>> there is no need for free(list) in this error path.
>>
>> Yeah, but the error path of malloc() having failed is quite nasty then.
> 
> Oh yeah.  This is ugly, but I guess it is better this way around.

Please define "this way around". Do you mean like it is in my patch, or
with the malloc() after alloc_fd()?

With your suggestion I'm basically having an error path with close() in
it, while with my variant I'm having one with free() in it. I'd rather
have a local handling doing free(), than to use another external call to
close() for a half opened file.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 588 bytes --]

On 10.01.22 19:56, Andrew Cooper wrote:
> On 07/01/2022 10:35, Juergen Gross wrote:
>> @@ -54,8 +70,11 @@ int osdep_gnttab_close(xengnttab_handle *xgt)
>>   
>>   void minios_gnttab_close_fd(int fd)
>>   {
>> -    gntmap_fini(&files[fd].gntmap);
>> -    files[fd].type = FTYPE_NONE;
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    gntmap_fini(file->dev);
>> +    free(file->dev);
> 
> file->dev = NULL ?
> 
> Particularly as this is a pointer in a global files[] array.

I'll reset all the struct file contents in close() for all file types.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]