Re: [PATCH v2 21/29] tools/xenstored: add early_init() function

[Juergen Gross <jgross@xxxxxxxx>]

On 10.11.23 18:31, Julien Grall wrote:
> Hi Juergen,
>
> On 10/11/2023 16:07, Juergen Gross wrote:
> > Some xenstored initialization needs to be done in the daemon case only,
> > so split it out into a new early_init() function being a stub in the
> > stubdom case.
> It is not entirely clear to me how you decided the split. For example...
>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > ---
> > V2:
> > - rename function
> > - move patch earlier in the series
> > ---
> >   tools/xenstored/core.c   |  6 +-----
> >   tools/xenstored/core.h   |  3 +++
> >   tools/xenstored/minios.c |  3 +++
> >   tools/xenstored/posix.c  | 11 +++++++++++
> >   4 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> > index edd07711db..0c14823fb0 100644
> > --- a/tools/xenstored/core.c
> > +++ b/tools/xenstored/core.c
> > @@ -2933,11 +2933,7 @@ int main(int argc, char *argv[])
> >       if (optind != argc)
> >           barf("%s: No arguments desired", argv[0]);
> > -    reopen_log();
> > -
> > -    /* Make sure xenstored directory exists. */
> > -    /* Errors ignored here, will be reported when we open files */
> > -    mkdir(xenstore_daemon_rundir(), 0755);
> > +    early_init();
> >       if (dofork) {
> >           openlog("xenstored", 0, LOG_DAEMON);
> For stubdom we would not fork, so I would expect the call to openlog() not 
> necessary. Same for the init_pipe() below.
The main goal was to move the "mkdir(xenstore_daemon_rundir(), 0755);" out of
the way for stubdom.

I'll make this more clear in the commit message, ...

> > diff --git a/tools/xenstored/core.h b/tools/xenstored/core.h
> > index 480b0f5f7b..d0ac587f8f 100644
> > --- a/tools/xenstored/core.h
> > +++ b/tools/xenstored/core.h
> > @@ -35,6 +35,8 @@
> >   #include "list.h"
> >   #include "hashtable.h"
> > +#define XENSTORE_LIB_DIR    XEN_LIB_DIR "/xenstore"
> > +
> >   #ifndef O_CLOEXEC
> >   #define O_CLOEXEC 0
> >   /* O_CLOEXEC support is needed for Live Update in the daemon case. */
> > @@ -384,6 +386,7 @@ static inline bool domain_is_unprivileged(const struct 
> > connection *conn)
> >   /* Return the event channel used by xenbus. */
> >   evtchn_port_t get_xenbus_evtchn(void);
> > +void early_init(void);
> >   /* Write out the pidfile */
> >   void write_pidfile(const char *pidfile);
> > diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c
> > index 0779efbf91..0cdec3ae51 100644
> > --- a/tools/xenstored/minios.c
> > +++ b/tools/xenstored/minios.c
> > @@ -54,3 +54,6 @@ void unmap_xenbus(void *interface)
> >       xengnttab_unmap(*xgt_handle, interface, 1);
> >   }
> > +void early_init(void)
> > +{
> > +}
> > diff --git a/tools/xenstored/posix.c b/tools/xenstored/posix.c
> > index 7e03dd982d..fcb7791bd7 100644
> > --- a/tools/xenstored/posix.c
> > +++ b/tools/xenstored/posix.c
> > @@ -22,6 +22,7 @@
> >   #include <fcntl.h>
> >   #include <stdlib.h>
> >   #include <sys/mman.h>
> > +#include <xen-tools/xenstore-common.h>
> >   #include "utils.h"
> >   #include "core.h"
> > @@ -157,3 +158,13 @@ void *xenbus_map(void)
> >       return addr;
> >   }
> > +
> > +void early_init(void)
> > +{
> > +    reopen_log();
> > +
> > +    /* Make sure xenstored directories exist. */
> > +    /* Errors ignored here, will be reported when we open files */
> > +    mkdir(xenstore_daemon_rundir(), 0755);
> > +    mkdir(XENSTORE_LIB_DIR, 0755);
> The addition of the second mkdir() doesn't seem to be explained in the commit 
> message.
... moving this change into context.

I'll look into moving more non-stubdom function calls into early_init() in
posix.c.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature