| Message ID | 20231110160804.29021-27-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: enable xenstore-stubdom to use 9pfs | expand |
Hi Juergen, On 10/11/2023 16:08, Juergen Gross wrote: > diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c > index 162b87b460..4263c1360f 100644 > --- a/tools/xenstored/domain.c > +++ b/tools/xenstored/domain.c > @@ -1236,6 +1236,8 @@ void stubdom_init(void) > barf_perror("Failed to initialize stubdom"); > > xenevtchn_notify(xce_handle, stubdom->port); > + > + mount_9pfs(); > #endif > } > > diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c > index 202d70387a..fddbede869 100644 > --- a/tools/xenstored/minios.c > +++ b/tools/xenstored/minios.c > @@ -19,8 +19,16 @@ > #include <sys/mman.h> > #include "core.h" > #include <xen/grant_table.h> > +#include <mini-os/9pfront.h> > #include <mini-os/events.h> > #include <mini-os/gnttab.h> > +#include <mini-os/sched.h> > +#include <mini-os/xenbus.h> > +#include <mini-os/xmalloc.h> > + > +#define P9_STATE_PATH "device/9pfs/0/state" > + > +static void *p9_device; > > void write_pidfile(const char *pidfile) > { > @@ -62,3 +70,31 @@ void early_init(void) > if (stub_domid == DOMID_INVALID) > barf("could not get own domid"); > } > + > +static void mount_thread(void *p) > +{ > + xenbus_event_queue events = NULL; > + char *err; > + char *dummy; > + > + free(xenbus_watch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs", &events)); AFAICT, xenbus_watch_path_token() can fail. I agree this is unlikely, but if it fails, then it would be useful to get some logs. Otherwise... > + > + for (;;) { ... this loop would be infinite. > + xenbus_wait_for_watch(&events); > + err = xenbus_read(XBT_NIL, P9_STATE_PATH, &dummy); Can you explain why don't care about the value of the node? > + if (!err) > + break; > + free(err); > + } > + > + free(dummy); > + > + free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); xenbus_unwatch_path_token() could technically fails. It would be helpful to print a message. > + > + p9_device = init_9pfront(0, XENSTORE_LIB_DIR); > +} > + > +void mount_9pfs(void) > +{ > + create_thread("mount-9pfs", mount_thread, NULL); > +} Cheers,
On 13.11.23 23:09, Julien Grall wrote: > Hi Juergen, > > On 10/11/2023 16:08, Juergen Gross wrote: >> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c >> index 162b87b460..4263c1360f 100644 >> --- a/tools/xenstored/domain.c >> +++ b/tools/xenstored/domain.c >> @@ -1236,6 +1236,8 @@ void stubdom_init(void) >> barf_perror("Failed to initialize stubdom"); >> xenevtchn_notify(xce_handle, stubdom->port); >> + >> + mount_9pfs(); >> #endif >> } >> diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c >> index 202d70387a..fddbede869 100644 >> --- a/tools/xenstored/minios.c >> +++ b/tools/xenstored/minios.c >> @@ -19,8 +19,16 @@ >> #include <sys/mman.h> >> #include "core.h" >> #include <xen/grant_table.h> >> +#include <mini-os/9pfront.h> >> #include <mini-os/events.h> >> #include <mini-os/gnttab.h> >> +#include <mini-os/sched.h> >> +#include <mini-os/xenbus.h> >> +#include <mini-os/xmalloc.h> >> + >> +#define P9_STATE_PATH "device/9pfs/0/state" >> + >> +static void *p9_device; >> void write_pidfile(const char *pidfile) >> { >> @@ -62,3 +70,31 @@ void early_init(void) >> if (stub_domid == DOMID_INVALID) >> barf("could not get own domid"); >> } >> + >> +static void mount_thread(void *p) >> +{ >> + xenbus_event_queue events = NULL; >> + char *err; >> + char *dummy; >> + >> + free(xenbus_watch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs", &events)); > > AFAICT, xenbus_watch_path_token() can fail. I agree this is unlikely, but if it > fails, then it would be useful to get some logs. Otherwise... > >> + >> + for (;;) { > > ... this loop would be infinite. Okay, will add logging. > >> + xenbus_wait_for_watch(&events); >> + err = xenbus_read(XBT_NIL, P9_STATE_PATH, &dummy); > > Can you explain why don't care about the value of the node? I only care about the presence of the "state" node. All real state changes will be handled in init_9pfront(). > >> + if (!err) >> + break; >> + free(err); >> + } >> + >> + free(dummy); >> + >> + free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); > > xenbus_unwatch_path_token() could technically fails. It would be helpful to > print a message. I can add that, but do we really care? This is a common pattern in Mini-OS. Juergen
Hi Juergen, On 14/11/2023 06:40, Juergen Gross wrote: > On 13.11.23 23:09, Julien Grall wrote: >> Hi Juergen, >> >> On 10/11/2023 16:08, Juergen Gross wrote: >>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c >>> index 162b87b460..4263c1360f 100644 >>> --- a/tools/xenstored/domain.c >>> +++ b/tools/xenstored/domain.c >>> @@ -1236,6 +1236,8 @@ void stubdom_init(void) >>> barf_perror("Failed to initialize stubdom"); >>> xenevtchn_notify(xce_handle, stubdom->port); >>> + >>> + mount_9pfs(); >>> #endif >>> } >>> diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c >>> index 202d70387a..fddbede869 100644 >>> --- a/tools/xenstored/minios.c >>> +++ b/tools/xenstored/minios.c >>> @@ -19,8 +19,16 @@ >>> #include <sys/mman.h> >>> #include "core.h" >>> #include <xen/grant_table.h> >>> +#include <mini-os/9pfront.h> >>> #include <mini-os/events.h> >>> #include <mini-os/gnttab.h> >>> +#include <mini-os/sched.h> >>> +#include <mini-os/xenbus.h> >>> +#include <mini-os/xmalloc.h> >>> + >>> +#define P9_STATE_PATH "device/9pfs/0/state" >>> + >>> +static void *p9_device; >>> void write_pidfile(const char *pidfile) >>> { >>> @@ -62,3 +70,31 @@ void early_init(void) >>> if (stub_domid == DOMID_INVALID) >>> barf("could not get own domid"); >>> } >>> + >>> +static void mount_thread(void *p) >>> +{ >>> + xenbus_event_queue events = NULL; >>> + char *err; >>> + char *dummy; >>> + >>> + free(xenbus_watch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs", >>> &events)); >> >> AFAICT, xenbus_watch_path_token() can fail. I agree this is unlikely, >> but if it fails, then it would be useful to get some logs. Otherwise... >> >>> + >>> + for (;;) { >> >> ... this loop would be infinite. > > Okay, will add logging. > >> >>> + xenbus_wait_for_watch(&events); >>> + err = xenbus_read(XBT_NIL, P9_STATE_PATH, &dummy); >> >> Can you explain why don't care about the value of the node? > > I only care about the presence of the "state" node. All real state changes > will be handled in init_9pfront(). Ok. Can this be documented in the code? > >> >>> + if (!err) >>> + break; >>> + free(err); >>> + } >>> + >>> + free(dummy); >>> + >>> + free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); >> >> xenbus_unwatch_path_token() could technically fails. It would be >> helpful to print a message. > > I can add that, but do we really care? Well... Such approach is ok until the day all the watches are exhausted. At this point, the developper who hit the bug will likely wish there were some debugging. > This is a common pattern in Mini-OS. You are not selling Mini-OS :). Really all the callers should check error return and print errors. Let's not spread that mistake in Xenstored. Cheers,
On 14.11.23 10:00, Julien Grall wrote: > Hi Juergen, > > On 14/11/2023 06:40, Juergen Gross wrote: >> On 13.11.23 23:09, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 10/11/2023 16:08, Juergen Gross wrote: >>>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c >>>> index 162b87b460..4263c1360f 100644 >>>> --- a/tools/xenstored/domain.c >>>> +++ b/tools/xenstored/domain.c >>>> @@ -1236,6 +1236,8 @@ void stubdom_init(void) >>>> barf_perror("Failed to initialize stubdom"); >>>> xenevtchn_notify(xce_handle, stubdom->port); >>>> + >>>> + mount_9pfs(); >>>> #endif >>>> } >>>> diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c >>>> index 202d70387a..fddbede869 100644 >>>> --- a/tools/xenstored/minios.c >>>> +++ b/tools/xenstored/minios.c >>>> @@ -19,8 +19,16 @@ >>>> #include <sys/mman.h> >>>> #include "core.h" >>>> #include <xen/grant_table.h> >>>> +#include <mini-os/9pfront.h> >>>> #include <mini-os/events.h> >>>> #include <mini-os/gnttab.h> >>>> +#include <mini-os/sched.h> >>>> +#include <mini-os/xenbus.h> >>>> +#include <mini-os/xmalloc.h> >>>> + >>>> +#define P9_STATE_PATH "device/9pfs/0/state" >>>> + >>>> +static void *p9_device; >>>> void write_pidfile(const char *pidfile) >>>> { >>>> @@ -62,3 +70,31 @@ void early_init(void) >>>> if (stub_domid == DOMID_INVALID) >>>> barf("could not get own domid"); >>>> } >>>> + >>>> +static void mount_thread(void *p) >>>> +{ >>>> + xenbus_event_queue events = NULL; >>>> + char *err; >>>> + char *dummy; >>>> + >>>> + free(xenbus_watch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs", &events)); >>> >>> AFAICT, xenbus_watch_path_token() can fail. I agree this is unlikely, but if >>> it fails, then it would be useful to get some logs. Otherwise... >>> >>>> + >>>> + for (;;) { >>> >>> ... this loop would be infinite. >> >> Okay, will add logging. >> >>> >>>> + xenbus_wait_for_watch(&events); >>>> + err = xenbus_read(XBT_NIL, P9_STATE_PATH, &dummy); >>> >>> Can you explain why don't care about the value of the node? >> >> I only care about the presence of the "state" node. All real state changes >> will be handled in init_9pfront(). > > Ok. Can this be documented in the code? Fine with me. > >> >>> >>>> + if (!err) >>>> + break; >>>> + free(err); >>>> + } >>>> + >>>> + free(dummy); >>>> + >>>> + free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); >>> >>> xenbus_unwatch_path_token() could technically fails. It would be helpful to >>> print a message. >> >> I can add that, but do we really care? > > Well... Such approach is ok until the day all the watches are exhausted. At this > point, the developper who hit the bug will likely wish there were some debugging. > >> This is a common pattern in Mini-OS. > > You are not selling Mini-OS :). Really all the callers should check error return > and print errors. Let's not spread that mistake in Xenstored. Okay. :-) Juergen
diff --git a/tools/xenstored/core.h b/tools/xenstored/core.h index 3c28007d11..a0d3592961 100644 --- a/tools/xenstored/core.h +++ b/tools/xenstored/core.h @@ -389,6 +389,10 @@ static inline bool domain_is_unprivileged(const struct connection *conn) evtchn_port_t get_xenbus_evtchn(void); void early_init(void); +#ifdef __MINIOS__ +void mount_9pfs(void); +#endif + /* Write out the pidfile */ void write_pidfile(const char *pidfile); diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c index 162b87b460..4263c1360f 100644 --- a/tools/xenstored/domain.c +++ b/tools/xenstored/domain.c @@ -1236,6 +1236,8 @@ void stubdom_init(void) barf_perror("Failed to initialize stubdom"); xenevtchn_notify(xce_handle, stubdom->port); + + mount_9pfs(); #endif } diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c index 202d70387a..fddbede869 100644 --- a/tools/xenstored/minios.c +++ b/tools/xenstored/minios.c @@ -19,8 +19,16 @@ #include <sys/mman.h> #include "core.h" #include <xen/grant_table.h> +#include <mini-os/9pfront.h> #include <mini-os/events.h> #include <mini-os/gnttab.h> +#include <mini-os/sched.h> +#include <mini-os/xenbus.h> +#include <mini-os/xmalloc.h> + +#define P9_STATE_PATH "device/9pfs/0/state" + +static void *p9_device; void write_pidfile(const char *pidfile) { @@ -62,3 +70,31 @@ void early_init(void) if (stub_domid == DOMID_INVALID) barf("could not get own domid"); } + +static void mount_thread(void *p) +{ + xenbus_event_queue events = NULL; + char *err; + char *dummy; + + free(xenbus_watch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs", &events)); + + for (;;) { + xenbus_wait_for_watch(&events); + err = xenbus_read(XBT_NIL, P9_STATE_PATH, &dummy); + if (!err) + break; + free(err); + } + + free(dummy); + + free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); + + p9_device = init_9pfront(0, XENSTORE_LIB_DIR); +} + +void mount_9pfs(void) +{ + create_thread("mount-9pfs", mount_thread, NULL); +}