| Message ID | 20231110160804.29021-22-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: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@suse.com> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > --- > 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. > 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. Cheers,
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@suse.com> >> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >> --- >> 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
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); 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); +}