[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.