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

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature