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

[Julien Grall <julien@xxxxxxx>]

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.
> 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,

--
Julien Grall