| Message ID | 20220116083328.26524-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove device specific struct file members | expand |
Juergen Gross, le dim. 16 janv. 2022 09:33:19 +0100, a ecrit: > Allocate the needed file type via alloc_file_type(). > > Instead of directly accessing the files[] array use get_file_from_fd(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > V3: > - switch to struct file * parameter for callbacks > - use __attribute__((constructor)) > --- > include/lib.h | 3 +-- > lib/sys.c | 18 -------------- > lib/xs.c | 65 +++++++++++++++++++++++++++++++++++++------------ > xenbus/xenbus.c | 1 + > 4 files changed, 52 insertions(+), 35 deletions(-) > > diff --git a/include/lib.h b/include/lib.h > index 5838b0d4..c5e47786 100644 > --- a/include/lib.h > +++ b/include/lib.h > @@ -169,8 +169,7 @@ extern struct wait_queue_head event_queue; > #define FTYPE_BLK 9 > #define FTYPE_TPMFRONT 10 > #define FTYPE_TPM_TIS 11 > -#define FTYPE_XENBUS 12 > -#define FTYPE_N 13 > +#define FTYPE_N 12 > #define FTYPE_SPARE 16 > > struct file { > diff --git a/lib/sys.c b/lib/sys.c > index 6ba26294..53870aa4 100644 > --- a/lib/sys.c > +++ b/lib/sys.c > @@ -523,11 +523,6 @@ int close(int fd) > switch (file->type) { > default: > break; > -#ifdef CONFIG_LIBXS > - case FTYPE_XENBUS: > - xs_daemon_close((void*)(intptr_t) fd); > - break; > -#endif > #ifdef HAVE_LWIP > case FTYPE_SOCKET: > res = lwip_close(files[fd].fd); > @@ -759,7 +754,6 @@ int closedir(DIR *dir) > static const char *const file_types[] = { > [FTYPE_NONE] = "none", > [FTYPE_CONSOLE] = "console", > - [FTYPE_XENBUS] = "xenbus", > [FTYPE_SOCKET] = "socket", > [FTYPE_TAP] = "net", > [FTYPE_BLK] = "blk", > @@ -947,18 +941,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce > n++; > FD_CLR(i, exceptfds); > break; > -#ifdef CONFIG_LIBXS > - case FTYPE_XENBUS: > - if (FD_ISSET(i, readfds)) { > - if (files[i].dev) > - n++; > - else > - FD_CLR(i, readfds); > - } > - FD_CLR(i, writefds); > - FD_CLR(i, exceptfds); > - break; > -#endif > case FTYPE_TAP: > case FTYPE_BLK: > case FTYPE_KBD: > diff --git a/lib/xs.c b/lib/xs.c > index 4af0f960..c12341aa 100644 > --- a/lib/xs.c > +++ b/lib/xs.c > @@ -18,23 +18,56 @@ static inline int _xs_fileno(struct xs_handle *h) { > return (intptr_t) h; > } > > +static int xs_close_fd(struct file *file) > +{ > + struct xenbus_event *event, *next; > + > + for (event = file->dev; event; event = next) > + { > + next = event->next; > + free(event); > + } > + > + return 0; > +} > + > +static bool xs_can_read(struct file *file) > +{ > + return file && file->dev; > +} > + > +static const struct file_ops xenbus_ops = { > + .name = "xenbus", > + .close = xs_close_fd, > + .select_rd = xs_can_read, > +}; > + > +static unsigned int ftype_xenbus; > + > +__attribute__((constructor)) > +static void xs_initialize(void) > +{ > + ftype_xenbus = alloc_file_type(&xenbus_ops); > +} > + > struct xs_handle *xs_daemon_open() > { > - int fd = alloc_fd(FTYPE_XENBUS); > - files[fd].dev = NULL; > - printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev); > + int fd; > + struct file *file; > + > + fd = alloc_fd(ftype_xenbus); > + file = get_file_from_fd(fd); > + if ( !file ) > + return NULL; > + > + file->dev = NULL; > + printk("xs_daemon_open -> %d, %p\n", fd, &file->dev); > return (void*)(intptr_t) fd; > } > > void xs_daemon_close(struct xs_handle *h) > { > - int fd = _xs_fileno(h); > - struct xenbus_event *event, *next; > - for (event = files[fd].dev; event; event = next) > - { > - next = event->next; > - free(event); > - } > + close(_xs_fileno(h)); > } > > int xs_fileno(struct xs_handle *h) > @@ -169,18 +202,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t, > > bool xs_watch(struct xs_handle *h, const char *path, const char *token) > { > - int fd = _xs_fileno(h); > + struct file *file = get_file_from_fd(_xs_fileno(h)); > + > printk("xs_watch(%s, %s)\n", path, token); > return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token, > - (xenbus_event_queue *)&files[fd].dev)); > + (xenbus_event_queue *)&file->dev)); > } > > char **xs_read_watch(struct xs_handle *h, unsigned int *num) > { > - int fd = _xs_fileno(h); > struct xenbus_event *event; > - event = files[fd].dev; > - files[fd].dev = event->next; > + struct file *file = get_file_from_fd(_xs_fileno(h)); > + > + event = file->dev; > + file->dev = event->next; > printk("xs_read_watch() -> %s %s\n", event->path, event->token); > *num = 2; > return (char **) &event->path; > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c > index b687678f..785389fb 100644 > --- a/xenbus/xenbus.c > +++ b/xenbus/xenbus.c > @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void) > void init_xenbus(void) > { > int err; > + > DEBUG("init_xenbus called.\n"); > create_thread("xenstore", xenbus_thread_func, NULL); > DEBUG("buf at %p.\n", xenstore_buf); > -- > 2.26.2 >
On 16/01/2022 08:33, Juergen Gross wrote: > diff --git a/lib/xs.c b/lib/xs.c > index 4af0f960..c12341aa 100644 > --- a/lib/xs.c > +++ b/lib/xs.c > @@ -18,23 +18,56 @@ static inline int _xs_fileno(struct xs_handle *h) { > return (intptr_t) h; > } > > +static int xs_close_fd(struct file *file) > +{ > + struct xenbus_event *event, *next; > + > + for (event = file->dev; event; event = next) > + { > + next = event->next; > + free(event); > + } > + > + return 0; > +} > + > +static bool xs_can_read(struct file *file) > +{ > + return file && file->dev; Just 'return file->dev;' ? > @@ -169,18 +202,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t, > > bool xs_watch(struct xs_handle *h, const char *path, const char *token) > { > - int fd = _xs_fileno(h); > + struct file *file = get_file_from_fd(_xs_fileno(h)); > + > printk("xs_watch(%s, %s)\n", path, token); > return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token, > - (xenbus_event_queue *)&files[fd].dev)); > + (xenbus_event_queue *)&file->dev)); This is utterly mad. In particular, close() looks to be very racy with new watches arriving. However, can the indentation at least be fixed here as the line is changing. That's a parameter to xenbus_watch_path_token(), not xs_bool(). > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c > index b687678f..785389fb 100644 > --- a/xenbus/xenbus.c > +++ b/xenbus/xenbus.c > @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void) > void init_xenbus(void) > { > int err; > + Spurious hunk? ~Andrew
On 18.01.22 15:29, Andrew Cooper wrote: > On 16/01/2022 08:33, Juergen Gross wrote: >> diff --git a/lib/xs.c b/lib/xs.c >> index 4af0f960..c12341aa 100644 >> --- a/lib/xs.c >> +++ b/lib/xs.c >> @@ -18,23 +18,56 @@ static inline int _xs_fileno(struct xs_handle *h) { >> return (intptr_t) h; >> } >> >> +static int xs_close_fd(struct file *file) >> +{ >> + struct xenbus_event *event, *next; >> + >> + for (event = file->dev; event; event = next) >> + { >> + next = event->next; >> + free(event); >> + } >> + >> + return 0; >> +} >> + >> +static bool xs_can_read(struct file *file) >> +{ >> + return file && file->dev; > > Just 'return file->dev;' ? Yes. > >> @@ -169,18 +202,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t, >> >> bool xs_watch(struct xs_handle *h, const char *path, const char *token) >> { >> - int fd = _xs_fileno(h); >> + struct file *file = get_file_from_fd(_xs_fileno(h)); >> + >> printk("xs_watch(%s, %s)\n", path, token); >> return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token, >> - (xenbus_event_queue *)&files[fd].dev)); >> + (xenbus_event_queue *)&file->dev)); > > This is utterly mad. In particular, close() looks to be very racy with > new watches arriving. In practice it should be no problem, though (closing the file in one thread while the other one is adding a watch would be rather strange). Additionally close() for xenbus in Mini-OS is called only when stopping the domain today. > However, can the indentation at least be fixed here as the line is > changing. That's a parameter to xenbus_watch_path_token(), not xs_bool(). Yes, that should be done. > >> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c >> index b687678f..785389fb 100644 >> --- a/xenbus/xenbus.c >> +++ b/xenbus/xenbus.c >> @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void) >> void init_xenbus(void) >> { >> int err; >> + > > Spurious hunk? Yes. Juergen
diff --git a/include/lib.h b/include/lib.h index 5838b0d4..c5e47786 100644 --- a/include/lib.h +++ b/include/lib.h @@ -169,8 +169,7 @@ extern struct wait_queue_head event_queue; #define FTYPE_BLK 9 #define FTYPE_TPMFRONT 10 #define FTYPE_TPM_TIS 11 -#define FTYPE_XENBUS 12 -#define FTYPE_N 13 +#define FTYPE_N 12 #define FTYPE_SPARE 16 struct file { diff --git a/lib/sys.c b/lib/sys.c index 6ba26294..53870aa4 100644 --- a/lib/sys.c +++ b/lib/sys.c @@ -523,11 +523,6 @@ int close(int fd) switch (file->type) { default: break; -#ifdef CONFIG_LIBXS - case FTYPE_XENBUS: - xs_daemon_close((void*)(intptr_t) fd); - break; -#endif #ifdef HAVE_LWIP case FTYPE_SOCKET: res = lwip_close(files[fd].fd); @@ -759,7 +754,6 @@ int closedir(DIR *dir) static const char *const file_types[] = { [FTYPE_NONE] = "none", [FTYPE_CONSOLE] = "console", - [FTYPE_XENBUS] = "xenbus", [FTYPE_SOCKET] = "socket", [FTYPE_TAP] = "net", [FTYPE_BLK] = "blk", @@ -947,18 +941,6 @@ static int select_poll(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exce n++; FD_CLR(i, exceptfds); break; -#ifdef CONFIG_LIBXS - case FTYPE_XENBUS: - if (FD_ISSET(i, readfds)) { - if (files[i].dev) - n++; - else - FD_CLR(i, readfds); - } - FD_CLR(i, writefds); - FD_CLR(i, exceptfds); - break; -#endif case FTYPE_TAP: case FTYPE_BLK: case FTYPE_KBD: diff --git a/lib/xs.c b/lib/xs.c index 4af0f960..c12341aa 100644 --- a/lib/xs.c +++ b/lib/xs.c @@ -18,23 +18,56 @@ static inline int _xs_fileno(struct xs_handle *h) { return (intptr_t) h; } +static int xs_close_fd(struct file *file) +{ + struct xenbus_event *event, *next; + + for (event = file->dev; event; event = next) + { + next = event->next; + free(event); + } + + return 0; +} + +static bool xs_can_read(struct file *file) +{ + return file && file->dev; +} + +static const struct file_ops xenbus_ops = { + .name = "xenbus", + .close = xs_close_fd, + .select_rd = xs_can_read, +}; + +static unsigned int ftype_xenbus; + +__attribute__((constructor)) +static void xs_initialize(void) +{ + ftype_xenbus = alloc_file_type(&xenbus_ops); +} + struct xs_handle *xs_daemon_open() { - int fd = alloc_fd(FTYPE_XENBUS); - files[fd].dev = NULL; - printk("xs_daemon_open -> %d, %p\n", fd, &files[fd].dev); + int fd; + struct file *file; + + fd = alloc_fd(ftype_xenbus); + file = get_file_from_fd(fd); + if ( !file ) + return NULL; + + file->dev = NULL; + printk("xs_daemon_open -> %d, %p\n", fd, &file->dev); return (void*)(intptr_t) fd; } void xs_daemon_close(struct xs_handle *h) { - int fd = _xs_fileno(h); - struct xenbus_event *event, *next; - for (event = files[fd].dev; event; event = next) - { - next = event->next; - free(event); - } + close(_xs_fileno(h)); } int xs_fileno(struct xs_handle *h) @@ -169,18 +202,20 @@ char **xs_directory(struct xs_handle *h, xs_transaction_t t, bool xs_watch(struct xs_handle *h, const char *path, const char *token) { - int fd = _xs_fileno(h); + struct file *file = get_file_from_fd(_xs_fileno(h)); + printk("xs_watch(%s, %s)\n", path, token); return xs_bool(xenbus_watch_path_token(XBT_NULL, path, token, - (xenbus_event_queue *)&files[fd].dev)); + (xenbus_event_queue *)&file->dev)); } char **xs_read_watch(struct xs_handle *h, unsigned int *num) { - int fd = _xs_fileno(h); struct xenbus_event *event; - event = files[fd].dev; - files[fd].dev = event->next; + struct file *file = get_file_from_fd(_xs_fileno(h)); + + event = file->dev; + file->dev = event->next; printk("xs_read_watch() -> %s %s\n", event->path, event->token); *num = 2; return (char **) &event->path; diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c index b687678f..785389fb 100644 --- a/xenbus/xenbus.c +++ b/xenbus/xenbus.c @@ -393,6 +393,7 @@ static int allocate_xenbus_id(void) void init_xenbus(void) { int err; + DEBUG("init_xenbus called.\n"); create_thread("xenstore", xenbus_thread_func, NULL); DEBUG("buf at %p.\n", xenstore_buf);
Allocate the needed file type via alloc_file_type(). Instead of directly accessing the files[] array use get_file_from_fd(). Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - switch to struct file * parameter for callbacks - use __attribute__((constructor)) --- include/lib.h | 3 +-- lib/sys.c | 18 -------------- lib/xs.c | 65 +++++++++++++++++++++++++++++++++++++------------ xenbus/xenbus.c | 1 + 4 files changed, 52 insertions(+), 35 deletions(-)