Re: [PATCH v2 26/29] tools/xenstored: mount 9pfs device in stubdom

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall