| Message ID | 20220107103544.9271-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/libs: decouple more from mini-os | expand |
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
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
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
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
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; } }
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(-)