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

[Juergen Gross <jgross@xxxxxxxx>]

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