Re: [PATCH v2 24/29] tools/xenstored: split domain_init()

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 10/11/2023 16:07, Juergen Gross wrote:
> Today domain_init() is called either just before calling dom0_init()
> in case no live update is being performed, or it is called after
> reading the global state from read_state_global(), as the event
> channel fd is needed.
>
> Split up domain_init() into a preparation part which can be called
> unconditionally, and in a part setting up the event channel handle.
Looking at the code, domain_init() may not be called if -D is passed to 
Xen. With your change, part of it would not be called unconditionally.
So does -D actually make sense? Did it actually work before your change? 
Should it be removed?
> Note that there is no chance that chk_domain_generation() can be
> called now before xc_handle has been setup, so there is no need for
> the related special case anymore.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   tools/xenstored/core.c   |  2 ++
>   tools/xenstored/domain.c | 12 ++++++------
>   tools/xenstored/domain.h |  1 +
>   3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 8e271e31f9..b9ec50b34c 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -2960,6 +2960,8 @@ int main(int argc, char *argv[])
>   
>   	init_pipe(reopen_log_pipe);
>   
> +	domain_static_init();
NIT: I find 'static' a bit confusing. How about using 'early' instead to 
match 'early_init()'?
> +
>         /* Listen to hypervisor. */
>         if (!no_domain_init && !live_update) {
>                 domain_init(-1);
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index 58b0942043..fa17f68618 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1224,10 +1224,8 @@ static int domeq_fn(const void *key1, const void *key2)
>         return *(const unsigned int *)key1 == *(const unsigned int *)key2;
>   }
>   
> -void domain_init(int evtfd)
> +void domain_static_init(void)
>   {
> -       int rc;
> -
>         /* Start with a random rather low domain count for the hashtable. */
>         domhash = create_hashtable(NULL, "domains", domhash_fn, domeq_fn, 0);
>         if (!domhash)
> @@ -1258,6 +1256,11 @@ void domain_init(int evtfd)
>         xengnttab_set_max_grants(*xgt_handle, DOMID_FIRST_RESERVED);
>   
>   	talloc_set_destructor(xgt_handle, close_xgt_handle);
> +}
> +
> +void domain_init(int evtfd)
> +{
> +       int rc;
>   
>   	if (evtfd < 0)
>                 xce_handle = xenevtchn_open(NULL, XENEVTCHN_NO_CLOEXEC);
> @@ -1291,9 +1294,6 @@ static bool chk_domain_generation(unsigned int domid, 
> uint64_t gen)
>   {
>         struct domain *d;
>   
> -	if (!xc_handle && domid == dom0_domid)
> -               return true;
> -
>         d = find_domain_struct(domid);
>   
>   	return d && d->generation <= gen;
> diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
> index 7625dca8cd..6c00540311 100644
> --- a/tools/xenstored/domain.h
> +++ b/tools/xenstored/domain.h
> @@ -82,6 +82,7 @@ int do_get_domain_path(const void *ctx, struct connection 
> *conn,
>   int do_reset_watches(const void *ctx, struct connection *conn,
>                      struct buffered_data *in);
>   
> +void domain_static_init(void);
>   void domain_init(int evtfd);
>   void dom0_init(void);
>   void domain_deinit(void);
Cheers,

--
Julien Grall