| Message ID | 20231110160804.29021-29-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:08, Juergen Gross wrote: > With 9pfs being fully available in Xenstore-stubdom now, there is no > reason to not fully support all logging capabilities in stubdom. > > Open the logfile on stubdom only after the 9pfs file system has been > mounted. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com> > --- > tools/hotplug/Linux/launch-xenstore.in | 1 + > tools/xenstored/control.c | 30 +++++++++++++------------- > tools/xenstored/minios.c | 3 +++ > 3 files changed, 19 insertions(+), 15 deletions(-) > > diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > index e854ca1eb8..da4eeca7c5 100644 > --- a/tools/hotplug/Linux/launch-xenstore.in > +++ b/tools/hotplug/Linux/launch-xenstore.in > @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF > [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 > XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE" > [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE" > + [ -z "$XENSTORED_TRACE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" I am probably missing something, but any reason to not use @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? Cheers,
On 13.11.23 23:40, Julien Grall wrote: > Hi Juergen, > > On 10/11/2023 16:08, Juergen Gross wrote: >> With 9pfs being fully available in Xenstore-stubdom now, there is no >> reason to not fully support all logging capabilities in stubdom. >> >> Open the logfile on stubdom only after the 9pfs file system has been >> mounted. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >> --- >> tools/hotplug/Linux/launch-xenstore.in | 1 + >> tools/xenstored/control.c | 30 +++++++++++++------------- >> tools/xenstored/minios.c | 3 +++ >> 3 files changed, 19 insertions(+), 15 deletions(-) >> >> diff --git a/tools/hotplug/Linux/launch-xenstore.in >> b/tools/hotplug/Linux/launch-xenstore.in >> index e854ca1eb8..da4eeca7c5 100644 >> --- a/tools/hotplug/Linux/launch-xenstore.in >> +++ b/tools/hotplug/Linux/launch-xenstore.in >> @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . >> @CONFIG_DIR@/@CONFIG_LEAF >> [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 >> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE" >> [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || >> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE" >> + [ -z "$XENSTORED_TRACE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS >> -T xenstored-trace.log" > > I am probably missing something, but any reason to not use > @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? Yes. Stubdom has no access to XEN_LOG_DIR. Juergen
Hi, On 14/11/2023 06:45, Juergen Gross wrote: > On 13.11.23 23:40, Julien Grall wrote: >> Hi Juergen, >> >> On 10/11/2023 16:08, Juergen Gross wrote: >>> With 9pfs being fully available in Xenstore-stubdom now, there is no >>> reason to not fully support all logging capabilities in stubdom. >>> >>> Open the logfile on stubdom only after the 9pfs file system has been >>> mounted. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >>> --- >>> tools/hotplug/Linux/launch-xenstore.in | 1 + >>> tools/xenstored/control.c | 30 +++++++++++++------------- >>> tools/xenstored/minios.c | 3 +++ >>> 3 files changed, 19 insertions(+), 15 deletions(-) >>> >>> diff --git a/tools/hotplug/Linux/launch-xenstore.in >>> b/tools/hotplug/Linux/launch-xenstore.in >>> index e854ca1eb8..da4eeca7c5 100644 >>> --- a/tools/hotplug/Linux/launch-xenstore.in >>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>> @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >>> && . @CONFIG_DIR@/@CONFIG_LEAF >>> [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 >>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory >>> $XENSTORE_DOMAIN_SIZE" >>> [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || >>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem >>> $XENSTORE_MAX_DOMAIN_SIZE" >>> + [ -z "$XENSTORED_TRACE" ] || >>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" >> >> I am probably missing something, but any reason to not use >> @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? > > Yes. Stubdom has no access to XEN_LOG_DIR. Ok. This restriction is not that obvious... The documentation in sysconfig.xencommons.in implies that it will be written in XEN_LOG_DIR: ## Type: string ## Default: "" # # Additional commandline arguments to start xenstored, # like "--trace-file @XEN_LOG_DIR@/xenstored-trace.log" # See "@sbindir@/xenstored --help" for possible options. # Only evaluated if XENSTORETYPE is "daemon". XENSTORED_ARGS= Also, we are saying this is only supported when Xenstored is daemonized. So I think there are some documentation update necessary in this patch. Cheers,
On 14.11.23 10:05, Julien Grall wrote: > Hi, > > On 14/11/2023 06:45, Juergen Gross wrote: >> On 13.11.23 23:40, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 10/11/2023 16:08, Juergen Gross wrote: >>>> With 9pfs being fully available in Xenstore-stubdom now, there is no >>>> reason to not fully support all logging capabilities in stubdom. >>>> >>>> Open the logfile on stubdom only after the 9pfs file system has been >>>> mounted. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >>>> --- >>>> tools/hotplug/Linux/launch-xenstore.in | 1 + >>>> tools/xenstored/control.c | 30 +++++++++++++------------- >>>> tools/xenstored/minios.c | 3 +++ >>>> 3 files changed, 19 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in >>>> b/tools/hotplug/Linux/launch-xenstore.in >>>> index e854ca1eb8..da4eeca7c5 100644 >>>> --- a/tools/hotplug/Linux/launch-xenstore.in >>>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>>> @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . >>>> @CONFIG_DIR@/@CONFIG_LEAF >>>> [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 >>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory >>>> $XENSTORE_DOMAIN_SIZE" >>>> [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || >>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE" >>>> + [ -z "$XENSTORED_TRACE" ] || >>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" >>> >>> I am probably missing something, but any reason to not use >>> @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? >> >> Yes. Stubdom has no access to XEN_LOG_DIR. > > Ok. This restriction is not that obvious... The documentation in > sysconfig.xencommons.in implies that it will be written in XEN_LOG_DIR: > > ## Type: string > ## Default: "" > # > # Additional commandline arguments to start xenstored, > # like "--trace-file @XEN_LOG_DIR@/xenstored-trace.log" > # See "@sbindir@/xenstored --help" for possible options. > # Only evaluated if XENSTORETYPE is "daemon". > XENSTORED_ARGS= > > Also, we are saying this is only supported when Xenstored is daemonized. So I > think there are some documentation update necessary in this patch. This is for the daemon. And the documentation example here is using an absolute filename, so this is very clear where the trace file will be written. For stubdom a related parameter "XENSTORE_DOMAIN_ARGS" is available. I can add a sentence to the explanation of that parameter describing setting of a possible trace file specification. Juergen
Hi Juergen, On 14/11/2023 09:20, Juergen Gross wrote: > On 14.11.23 10:05, Julien Grall wrote: >> Hi, >> >> On 14/11/2023 06:45, Juergen Gross wrote: >>> On 13.11.23 23:40, Julien Grall wrote: >>>> Hi Juergen, >>>> >>>> On 10/11/2023 16:08, Juergen Gross wrote: >>>>> With 9pfs being fully available in Xenstore-stubdom now, there is no >>>>> reason to not fully support all logging capabilities in stubdom. >>>>> >>>>> Open the logfile on stubdom only after the 9pfs file system has been >>>>> mounted. >>>>> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >>>>> --- >>>>> tools/hotplug/Linux/launch-xenstore.in | 1 + >>>>> tools/xenstored/control.c | 30 >>>>> +++++++++++++------------- >>>>> tools/xenstored/minios.c | 3 +++ >>>>> 3 files changed, 19 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in >>>>> b/tools/hotplug/Linux/launch-xenstore.in >>>>> index e854ca1eb8..da4eeca7c5 100644 >>>>> --- a/tools/hotplug/Linux/launch-xenstore.in >>>>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>>>> @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >>>>> && . @CONFIG_DIR@/@CONFIG_LEAF >>>>> [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 >>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory >>>>> $XENSTORE_DOMAIN_SIZE" >>>>> [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || >>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem >>>>> $XENSTORE_MAX_DOMAIN_SIZE" >>>>> + [ -z "$XENSTORED_TRACE" ] || >>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" >>>> >>>> I am probably missing something, but any reason to not use >>>> @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? >>> >>> Yes. Stubdom has no access to XEN_LOG_DIR. >> >> Ok. This restriction is not that obvious... The documentation in >> sysconfig.xencommons.in implies that it will be written in XEN_LOG_DIR: >> >> ## Type: string >> ## Default: "" >> # >> # Additional commandline arguments to start xenstored, >> # like "--trace-file @XEN_LOG_DIR@/xenstored-trace.log" >> # See "@sbindir@/xenstored --help" for possible options. >> # Only evaluated if XENSTORETYPE is "daemon". >> XENSTORED_ARGS= >> >> Also, we are saying this is only supported when Xenstored is >> daemonized. So I think there are some documentation update necessary >> in this patch. > > This is for the daemon. And the documentation example here is using an > absolute filename, so this is very clear where the trace file will be > written. Ah yes. Sorry I got confused. > > For stubdom a related parameter "XENSTORE_DOMAIN_ARGS" is available. I > can add > a sentence to the explanation of that parameter describing setting of a > possible trace file specification. Do you mean in the following comment: ## Type: string ## Default: Not defined, tracing off # # Log xenstored messages # Only evaluated if XENSTORETYPE is "daemon". #XENSTORED_TRACE=[yes|on|1] I think here we need to remove the "Only...". Cheers,
On 14.11.23 21:57, Julien Grall wrote: > Hi Juergen, > > On 14/11/2023 09:20, Juergen Gross wrote: >> On 14.11.23 10:05, Julien Grall wrote: >>> Hi, >>> >>> On 14/11/2023 06:45, Juergen Gross wrote: >>>> On 13.11.23 23:40, Julien Grall wrote: >>>>> Hi Juergen, >>>>> >>>>> On 10/11/2023 16:08, Juergen Gross wrote: >>>>>> With 9pfs being fully available in Xenstore-stubdom now, there is no >>>>>> reason to not fully support all logging capabilities in stubdom. >>>>>> >>>>>> Open the logfile on stubdom only after the 9pfs file system has been >>>>>> mounted. >>>>>> >>>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>>> Reviewed-by: Jason Andryuk <jandryuk@gmail.com> >>>>>> --- >>>>>> tools/hotplug/Linux/launch-xenstore.in | 1 + >>>>>> tools/xenstored/control.c | 30 +++++++++++++------------- >>>>>> tools/xenstored/minios.c | 3 +++ >>>>>> 3 files changed, 19 insertions(+), 15 deletions(-) >>>>>> >>>>>> diff --git a/tools/hotplug/Linux/launch-xenstore.in >>>>>> b/tools/hotplug/Linux/launch-xenstore.in >>>>>> index e854ca1eb8..da4eeca7c5 100644 >>>>>> --- a/tools/hotplug/Linux/launch-xenstore.in >>>>>> +++ b/tools/hotplug/Linux/launch-xenstore.in >>>>>> @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . >>>>>> @CONFIG_DIR@/@CONFIG_LEAF >>>>>> [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 >>>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory >>>>>> $XENSTORE_DOMAIN_SIZE" >>>>>> [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || >>>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem >>>>>> $XENSTORE_MAX_DOMAIN_SIZE" >>>>>> + [ -z "$XENSTORED_TRACE" ] || >>>>>> XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" >>>>> >>>>> I am probably missing something, but any reason to not use >>>>> @XEN_LOG_DIR@/xenstored-trace.log as we do for xenstored? >>>> >>>> Yes. Stubdom has no access to XEN_LOG_DIR. >>> >>> Ok. This restriction is not that obvious... The documentation in >>> sysconfig.xencommons.in implies that it will be written in XEN_LOG_DIR: >>> >>> ## Type: string >>> ## Default: "" >>> # >>> # Additional commandline arguments to start xenstored, >>> # like "--trace-file @XEN_LOG_DIR@/xenstored-trace.log" >>> # See "@sbindir@/xenstored --help" for possible options. >>> # Only evaluated if XENSTORETYPE is "daemon". >>> XENSTORED_ARGS= >>> >>> Also, we are saying this is only supported when Xenstored is daemonized. So I >>> think there are some documentation update necessary in this patch. >> >> This is for the daemon. And the documentation example here is using an >> absolute filename, so this is very clear where the trace file will be written. > > Ah yes. Sorry I got confused. > >> >> For stubdom a related parameter "XENSTORE_DOMAIN_ARGS" is available. I can add >> a sentence to the explanation of that parameter describing setting of a >> possible trace file specification. > > Do you mean in the following comment: > > ## Type: string > ## Default: Not defined, tracing off > # > # Log xenstored messages > # Only evaluated if XENSTORETYPE is "daemon". > #XENSTORED_TRACE=[yes|on|1] > > I think here we need to remove the "Only...". Oh, I missed that one. It needs to be considered in the stubdom case, too. Juergen
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in index e854ca1eb8..da4eeca7c5 100644 --- a/tools/hotplug/Linux/launch-xenstore.in +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -98,6 +98,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF [ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8 XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE" [ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE" + [ -z "$XENSTORED_TRACE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS -T xenstored-trace.log" echo -n Starting $XENSTORE_DOMAIN_KERNEL... ${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS || exit 1 diff --git a/tools/xenstored/control.c b/tools/xenstored/control.c index b2f64d674f..dae23a5ac0 100644 --- a/tools/xenstored/control.c +++ b/tools/xenstored/control.c @@ -201,19 +201,6 @@ static int do_control_quota_s(const void *ctx, struct connection *conn, return EINVAL; } -#ifdef __MINIOS__ -static int do_control_memreport(const void *ctx, struct connection *conn, - const char **vec, int num) -{ - if (num) - return EINVAL; - - talloc_report_full(NULL, stdout); - - send_ack(conn, XS_CONTROL); - return 0; -} -#else static int do_control_logfile(const void *ctx, struct connection *conn, const char **vec, int num) { @@ -222,13 +209,26 @@ static int do_control_logfile(const void *ctx, struct connection *conn, close_log(); talloc_free(tracefile); - tracefile = talloc_strdup(NULL, vec[0]); + tracefile = absolute_filename(NULL, vec[0]); reopen_log(); send_ack(conn, XS_CONTROL); return 0; } +#ifdef __MINIOS__ +static int do_control_memreport(const void *ctx, struct connection *conn, + const char **vec, int num) +{ + if (num) + return EINVAL; + + talloc_report_full(NULL, stdout); + + send_ack(conn, XS_CONTROL); + return 0; +} +#else static int do_control_memreport(const void *ctx, struct connection *conn, const char **vec, int num) { @@ -309,10 +309,10 @@ static struct cmd_s cmds[] = { "[-c <cmdline>] [-F] [-t <timeout>] <file>\n" " Default timeout is 60 seconds.", 5 }, #endif + { "logfile", do_control_logfile, "<file>" }, #ifdef __MINIOS__ { "memreport", do_control_memreport, "" }, #else - { "logfile", do_control_logfile, "<file>" }, { "memreport", do_control_memreport, "[<file>]" }, #endif { "print", do_control_print, "<string>" }, diff --git a/tools/xenstored/minios.c b/tools/xenstored/minios.c index cfc2377857..9e78b96d07 100644 --- a/tools/xenstored/minios.c +++ b/tools/xenstored/minios.c @@ -92,6 +92,9 @@ static void mount_thread(void *p) free(xenbus_unwatch_path_token(XBT_NIL, P9_STATE_PATH, "9pfs")); p9_device = init_9pfront(0, XENSTORE_LIB_DIR); + + /* Start logging if selected. */ + reopen_log(); } void mount_9pfs(void)