| Message ID | 20231110160804.29021-25-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: > 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@suse.com> > --- > 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,
On 10.11.23 19:05, Julien Grall wrote: > 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? Good point. I think it should be removed. I don't think it can work at all. > >> >> 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@suse.com> >> --- >> 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()'? Yes, good idea. I wasn't very happy with the name either. Juergen
On 13.11.23 09:58, Juergen Gross wrote: > On 10.11.23 19:05, Julien Grall wrote: >> 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? > > Good point. > > I think it should be removed. I don't think it can work at all. I even think the "-N" and "-P" should be removed, too. "-N" isn't really _that_ helpful when doing tests. Attaching gdb to the running xenstored daemon is by far superior. And if you are testing any changes in the init code you can easily patch xenstored not to daemonize. "-P" has no real purpose at all. Why would we want to show the PID, if we can just look into the pidfile? Thoughts? Juergen
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(); + /* 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);
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. 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@suse.com> --- tools/xenstored/core.c | 2 ++ tools/xenstored/domain.c | 12 ++++++------ tools/xenstored/domain.h | 1 + 3 files changed, 9 insertions(+), 6 deletions(-)