[PATCH v2 0/5] tools/xenstored: remove some command line options

[PATCH v2 0/5] tools/xenstored: remove some command line options

[Juergen Gross]

Remove some command line options which have no real use case.

Changes in V2:
- moved removal of "-N" into last patch of the series, as this is the
  only option which seems to have a use case (OTOH using it has some
  downsides as well).

Juergen Gross (5):
  tools/xenstored: remove "-D" command line parameter
  tools/xenstored: remove "-V" command line option
  tools/xenstored: remove the "-P" command line option
  tools/xenstored: remove the "-R" command line option
  tools/xenstored: remove "-N" command line option

 tools/xenstored/core.c | 81 +++++++-----------------------------------
 1 file changed, 12 insertions(+), 69 deletions(-)

-- 
2.35.3

[PATCH v2 1/5] tools/xenstored: remove "-D" command line parameter

[Juergen Gross]

Remove the "-D" command parameter, which is disabling initialization of
the mandatory domain data handling.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstored/core.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 311764eb0c..3465b7ecf1 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -2661,7 +2661,6 @@ static void usage(void)
 "\n"
 "where options may include:\n"
 "\n"
-"  -D, --no-domain-init    to state that xenstored should not initialise dom0,\n"
 "  -F, --pid-file <file>   giving a file for the daemon's pid to be written,\n"
 "  -H, --help              to output this message,\n"
 "  -N, --no-fork           to request that the daemon does not fork,\n"
@@ -2708,7 +2707,6 @@ static void usage(void)
 
 
 static struct option options[] = {
-	{ "no-domain-init", 0, NULL, 'D' },
 	{ "entry-nb", 1, NULL, 'E' },
 	{ "pid-file", 1, NULL, 'F' },
 	{ "event", 1, NULL, 'e' },
@@ -2841,7 +2839,6 @@ int main(int argc, char *argv[])
 	int sock_pollfd_idx = -1;
 	bool dofork = true;
 	bool outputpid = false;
-	bool no_domain_init = false;
 	bool live_update = false;
 	const char *pidfile = NULL;
 	int timeout;
@@ -2850,12 +2847,9 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "DE:F:H::KNPS:t:A:M:Q:q:T:RVW:w:U",
+				  "E:F:H::KNPS:t:A:M:Q:q:T:RVW:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
-		case 'D':
-			no_domain_init = true;
-			break;
 		case 'E':
 			hard_quotas[ACC_NODES].val = get_optval_uint(optarg);
 			break;
@@ -2964,7 +2958,7 @@ int main(int argc, char *argv[])
 	init_pipe(reopen_log_pipe);
 
 	/* Listen to hypervisor. */
-	if (!no_domain_init && !live_update) {
+	if (!live_update) {
 		domain_init(-1);
 		dom0_init();
 	}
-- 
2.35.3

[PATCH v2 2/5] tools/xenstored: remove "-V" command line option

[Juergen Gross]

The "-V" (verbose) command line option is nearly completely redundant
with "io" tracing. Just the time of the printed data is a little bit
different, while the tracing is more informative.

Remove the verbose option.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstored/core.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 3465b7ecf1..194e24238b 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -76,7 +76,6 @@ static int sock = -1;
 int orig_argc;
 char **orig_argv;
 
-static bool verbose = false;
 LIST_HEAD(connections);
 int tracefd = -1;
 static bool recovery = true;
@@ -329,11 +328,6 @@ static bool write_messages(struct connection *conn)
 		return true;
 
 	if (out->inhdr) {
-		if (verbose)
-			xprintf("Writing msg %s (%.*s) out to %p\n",
-				sockmsg_string(out->hdr.msg.type),
-				out->hdr.msg.len,
-				out->buffer, conn);
 		ret = conn->funcs->write(conn, out->hdr.raw + out->used,
 					 sizeof(out->hdr) - out->used);
 		if (ret < 0)
@@ -2134,11 +2128,6 @@ static bool process_delayed_message(struct delayed_request *req)
 
 static void consider_message(struct connection *conn)
 {
-	if (verbose)
-		xprintf("Got message %s len %i from %p\n",
-			sockmsg_string(conn->in->hdr.msg.type),
-			conn->in->hdr.msg.len, conn);
-
 	conn->is_stalled = false;
 	/*
 	 * Currently, Live-Update is not supported if there is active
@@ -2701,8 +2690,7 @@ static void usage(void)
 "  -R, --no-recovery       to request that no recovery should be attempted when\n"
 "                          the store is corrupted (debug only),\n"
 "  -K, --keep-orphans      don't delete nodes owned by a domain when the\n"
-"                          domain is deleted (this is a security risk!)\n"
-"  -V, --verbose           to request verbose execution.\n");
+"                          domain is deleted (this is a security risk!)\n");
 }
 
 
@@ -2726,7 +2714,6 @@ static struct option options[] = {
 	{ "timeout", 1, NULL, 'w' },
 	{ "no-recovery", 0, NULL, 'R' },
 	{ "keep-orphans", 0, NULL, 'K' },
-	{ "verbose", 0, NULL, 'V' },
 	{ "watch-nb", 1, NULL, 'W' },
 #ifndef NO_LIVE_UPDATE
 	{ "live-update", 0, NULL, 'U' },
@@ -2847,7 +2834,7 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "E:F:H::KNPS:t:A:M:Q:q:T:RVW:w:U",
+				  "E:F:H::KNPS:t:A:M:Q:q:T:RW:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
 		case 'E':
@@ -2884,9 +2871,6 @@ int main(int argc, char *argv[])
 		case 'K':
 			keep_orphans = true;
 			break;
-		case 'V':
-			verbose = true;
-			break;
 		case 'W':
 			hard_quotas[ACC_WATCH].val = get_optval_uint(optarg);
 			break;
-- 
2.35.3

[PATCH v2 3/5] tools/xenstored: remove the "-P" command line option

[Juergen Gross]

The "-P" command line option just results in printing the PID of the
xenstored daemon to stdout before stdout is being closed. The same
information can be retrieved from the PID file via the "-F" option.

Remove the redundant "-P" option.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstored/core.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 194e24238b..3ce50b313b 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -2653,7 +2653,6 @@ static void usage(void)
 "  -F, --pid-file <file>   giving a file for the daemon's pid to be written,\n"
 "  -H, --help              to output this message,\n"
 "  -N, --no-fork           to request that the daemon does not fork,\n"
-"  -P, --output-pid        to request that the pid of the daemon is output,\n"
 "  -T, --trace-file <file> giving the file for logging, and\n"
 "      --trace-control=+<switch> activate a specific <switch>\n"
 "      --trace-control=-<switch> deactivate a specific <switch>\n"
@@ -2702,7 +2701,6 @@ static struct option options[] = {
 	{ "help", 0, NULL, 'H' },
 	{ "no-fork", 0, NULL, 'N' },
 	{ "priv-domid", 1, NULL, 'p' },
-	{ "output-pid", 0, NULL, 'P' },
 	{ "entry-size", 1, NULL, 'S' },
 	{ "trace-file", 1, NULL, 'T' },
 	{ "trace-control", 1, NULL, 1 },
@@ -2825,7 +2823,6 @@ int main(int argc, char *argv[])
 	int opt;
 	int sock_pollfd_idx = -1;
 	bool dofork = true;
-	bool outputpid = false;
 	bool live_update = false;
 	const char *pidfile = NULL;
 	int timeout;
@@ -2834,7 +2831,7 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "E:F:H::KNPS:t:A:M:Q:q:T:RW:w:U",
+				  "E:F:H::KNS:t:A:M:Q:q:T:RW:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
 		case 'E':
@@ -2849,9 +2846,6 @@ int main(int argc, char *argv[])
 		case 'N':
 			dofork = false;
 			break;
-		case 'P':
-			outputpid = true;
-			break;
 		case 'R':
 			recovery = false;
 			break;
@@ -2947,11 +2941,6 @@ int main(int argc, char *argv[])
 		dom0_init();
 	}
 
-	if (outputpid) {
-		printf("%ld\n", (long)getpid());
-		fflush(stdout);
-	}
-
 	/* redirect to /dev/null now we're ready to accept connections */
 	if (dofork && !live_update)
 		finish_daemonize();
-- 
2.35.3

[PATCH v2 4/5] tools/xenstored: remove the "-R" command line option

[Juergen Gross]

The "-R" (no recovery) command line option enables to omit fixing the
node store in case of detected inconsistencies.

This might have been of interest in the past, when the node data base
was kept in a file, but now the usability of this option is zero.

Remove the "-R" option.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstored/core.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 3ce50b313b..43be89c4fc 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -78,7 +78,6 @@ char **orig_argv;
 
 LIST_HEAD(connections);
 int tracefd = -1;
-static bool recovery = true;
 bool keep_orphans = false;
 static int reopen_log_pipe[2];
 static int reopen_log_pipe0_pollfd_idx = -1;
@@ -2443,9 +2442,6 @@ int remember_string(struct hashtable *hash, const char *str)
  * have a corresponding child node (and if so, delete them).  Each valid child
  * is then recursively checked.
  *
- * No deleting is performed if the recovery flag is cleared (i.e. -R was
- * passed on the command line).
- *
  * As we go, we record each node in the given reachable hashtable.  These
  * entries will be used later in clean_store.
  */
@@ -2462,8 +2458,7 @@ static int check_store_step(const void *ctx, struct connection *conn,
 
 	if (hashtable_search(data->reachable, (void *)node->name)) {
 		log("check_store: '%s' is duplicated!", node->name);
-		return recovery ? WALK_TREE_RM_CHILDENTRY
-				: WALK_TREE_SKIP_CHILDREN;
+		return WALK_TREE_RM_CHILDENTRY;
 	}
 
 	if (remember_string(data->reachable, node->name))
@@ -2479,7 +2474,7 @@ static int check_store_enoent(const void *ctx, struct connection *conn,
 {
 	log("check_store: node '%s' not found", name);
 
-	return recovery ? WALK_TREE_RM_CHILDENTRY : WALK_TREE_OK;
+	return WALK_TREE_RM_CHILDENTRY;
 }
 
 
@@ -2504,8 +2499,7 @@ static int clean_store_(const void *key, void *val, void *private)
 	}
 	if (!hashtable_search(reachable, name)) {
 		log("clean_store: '%s' is orphaned!", name);
-		if (recovery)
-			db_delete(NULL, name, NULL);
+		db_delete(NULL, name, NULL);
 	}
 
 	talloc_free(name);
@@ -2686,8 +2680,6 @@ static void usage(void)
 "  -w, --timeout <what>=<seconds>   set the timeout in seconds for <what>,\n"
 "                          allowed timeout candidates are:\n"
 "                          watch-event: time a watch-event is kept pending\n"
-"  -R, --no-recovery       to request that no recovery should be attempted when\n"
-"                          the store is corrupted (debug only),\n"
 "  -K, --keep-orphans      don't delete nodes owned by a domain when the\n"
 "                          domain is deleted (this is a security risk!)\n");
 }
@@ -2710,7 +2702,6 @@ static struct option options[] = {
 	{ "quota", 1, NULL, 'Q' },
 	{ "quota-soft", 1, NULL, 'q' },
 	{ "timeout", 1, NULL, 'w' },
-	{ "no-recovery", 0, NULL, 'R' },
 	{ "keep-orphans", 0, NULL, 'K' },
 	{ "watch-nb", 1, NULL, 'W' },
 #ifndef NO_LIVE_UPDATE
@@ -2831,7 +2822,7 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "E:F:H::KNS:t:A:M:Q:q:T:RW:w:U",
+				  "E:F:H::KNS:t:A:M:Q:q:T:W:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
 		case 'E':
@@ -2846,9 +2837,6 @@ int main(int argc, char *argv[])
 		case 'N':
 			dofork = false;
 			break;
-		case 'R':
-			recovery = false;
-			break;
 		case 'S':
 			hard_quotas[ACC_NODESZ].val = get_optval_uint(optarg);
 			break;
-- 
2.35.3

[PATCH v2 5/5] tools/xenstored: remove "-N" command line option

[Juergen Gross]

The "-N" (do not daemonize) command line option is of questionable use:
its sole purpose seems to be to aid debugging of xenstored by making it
easier to start xenstored under gdb, or to see any debug messages
easily.

Debug messages can as well be sent to syslog(), while gdb can be
attached to the daemon easily. The only not covered case is an error
while initializing xenstored, but this could be handled e.g. by saving
a core dump, which can be analyzed later.

The call of talloc_enable_leak_report_full() done only with "-N"
specified is no longer needed, as the same can be achieved via
"xenstore-control memreport".

Signed-off-by: Juergen Gross <jgross@suse.com>
---
Slightly RFC, as this is making debugging a little bit harder in
specific cases. OTOH I didn't use this option since years, in spite of
having done a _lot_ of xenstore hacking.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstored/core.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index 43be89c4fc..f5766452fe 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -2646,7 +2646,6 @@ static void usage(void)
 "\n"
 "  -F, --pid-file <file>   giving a file for the daemon's pid to be written,\n"
 "  -H, --help              to output this message,\n"
-"  -N, --no-fork           to request that the daemon does not fork,\n"
 "  -T, --trace-file <file> giving the file for logging, and\n"
 "      --trace-control=+<switch> activate a specific <switch>\n"
 "      --trace-control=-<switch> deactivate a specific <switch>\n"
@@ -2691,7 +2690,6 @@ static struct option options[] = {
 	{ "event", 1, NULL, 'e' },
 	{ "master-domid", 1, NULL, 'm' },
 	{ "help", 0, NULL, 'H' },
-	{ "no-fork", 0, NULL, 'N' },
 	{ "priv-domid", 1, NULL, 'p' },
 	{ "entry-size", 1, NULL, 'S' },
 	{ "trace-file", 1, NULL, 'T' },
@@ -2813,7 +2811,6 @@ int main(int argc, char *argv[])
 {
 	int opt;
 	int sock_pollfd_idx = -1;
-	bool dofork = true;
 	bool live_update = false;
 	const char *pidfile = NULL;
 	int timeout;
@@ -2822,7 +2819,7 @@ int main(int argc, char *argv[])
 	orig_argv = argv;
 
 	while ((opt = getopt_long(argc, argv,
-				  "E:F:H::KNS:t:A:M:Q:q:T:W:w:U",
+				  "E:F:H::KS:t:A:M:Q:q:T:W:w:U",
 				  options, NULL)) != -1) {
 		switch (opt) {
 		case 'E':
@@ -2834,9 +2831,6 @@ int main(int argc, char *argv[])
 		case 'H':
 			usage();
 			return 0;
-		case 'N':
-			dofork = false;
-			break;
 		case 'S':
 			hard_quotas[ACC_NODESZ].val = get_optval_uint(optarg);
 			break;
@@ -2899,18 +2893,13 @@ int main(int argc, char *argv[])
 	/* Errors ignored here, will be reported when we open files */
 	mkdir(xenstore_daemon_rundir(), 0755);
 
-	if (dofork) {
-		openlog("xenstored", 0, LOG_DAEMON);
-		if (!live_update)
-			daemonize();
-	}
+	openlog("xenstored", 0, LOG_DAEMON);
+	if (!live_update)
+		daemonize();
+
 	if (pidfile)
 		write_pidfile(pidfile);
 
-	/* Talloc leak reports go to stderr, which is closed if we fork. */
-	if (!dofork)
-		talloc_enable_leak_report_full();
-
 	/* Don't kill us with SIGPIPE. */
 	signal(SIGPIPE, SIG_IGN);
 
@@ -2930,11 +2919,10 @@ int main(int argc, char *argv[])
 	}
 
 	/* redirect to /dev/null now we're ready to accept connections */
-	if (dofork && !live_update)
+	if (!live_update)
 		finish_daemonize();
 #ifndef __MINIOS__
-	if (dofork)
-		xprintf = trace;
+	xprintf = trace;
 #endif
 
 	signal(SIGHUP, trigger_reopen_log);
-- 
2.35.3

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 842 bytes --]

On 21.11.23 12:40, Juergen Gross wrote:
> Remove some command line options which have no real use case.
> 
> Changes in V2:
> - moved removal of "-N" into last patch of the series, as this is the
>    only option which seems to have a use case (OTOH using it has some
>    downsides as well).
> 
> Juergen Gross (5):
>    tools/xenstored: remove "-D" command line parameter
>    tools/xenstored: remove "-V" command line option
>    tools/xenstored: remove the "-P" command line option
>    tools/xenstored: remove the "-R" command line option
>    tools/xenstored: remove "-N" command line option
> 
>   tools/xenstored/core.c | 81 +++++++-----------------------------------
>   1 file changed, 12 insertions(+), 69 deletions(-)
> 

I think at least patches 1-4 can go in as they all have the required Acks.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Jan Beulich]

On 07.12.2023 09:26, Juergen Gross wrote:
> On 21.11.23 12:40, Juergen Gross wrote:
>> Remove some command line options which have no real use case.
>>
>> Changes in V2:
>> - moved removal of "-N" into last patch of the series, as this is the
>>    only option which seems to have a use case (OTOH using it has some
>>    downsides as well).
>>
>> Juergen Gross (5):
>>    tools/xenstored: remove "-D" command line parameter
>>    tools/xenstored: remove "-V" command line option
>>    tools/xenstored: remove the "-P" command line option
>>    tools/xenstored: remove the "-R" command line option
>>    tools/xenstored: remove "-N" command line option
>>
>>   tools/xenstored/core.c | 81 +++++++-----------------------------------
>>   1 file changed, 12 insertions(+), 69 deletions(-)
>>
> 
> I think at least patches 1-4 can go in as they all have the required Acks.

I'll try to remember to include them in the next swipe. I was kind of
assuming Julien would be taking care of them.

Jan

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Julien Grall]

Hi Jan,

On 07/12/2023 09:30, Jan Beulich wrote:
> On 07.12.2023 09:26, Juergen Gross wrote:
>> On 21.11.23 12:40, Juergen Gross wrote:
>>> Remove some command line options which have no real use case.
>>>
>>> Changes in V2:
>>> - moved removal of "-N" into last patch of the series, as this is the
>>>     only option which seems to have a use case (OTOH using it has some
>>>     downsides as well).
>>>
>>> Juergen Gross (5):
>>>     tools/xenstored: remove "-D" command line parameter
>>>     tools/xenstored: remove "-V" command line option
>>>     tools/xenstored: remove the "-P" command line option
>>>     tools/xenstored: remove the "-R" command line option
>>>     tools/xenstored: remove "-N" command line option
>>>
>>>    tools/xenstored/core.c | 81 +++++++-----------------------------------
>>>    1 file changed, 12 insertions(+), 69 deletions(-)
>>>
>>
>> I think at least patches 1-4 can go in as they all have the required Acks.
> 
> I'll try to remember to include them in the next swipe. I was kind of
> assuming Julien would be taking care of them.

Sorry this fell through the cracks. I can do it if you haven't yet done it.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Jan Beulich]

On 07.12.2023 13:14, Julien Grall wrote:
> Hi Jan,
> 
> On 07/12/2023 09:30, Jan Beulich wrote:
>> On 07.12.2023 09:26, Juergen Gross wrote:
>>> On 21.11.23 12:40, Juergen Gross wrote:
>>>> Remove some command line options which have no real use case.
>>>>
>>>> Changes in V2:
>>>> - moved removal of "-N" into last patch of the series, as this is the
>>>>     only option which seems to have a use case (OTOH using it has some
>>>>     downsides as well).
>>>>
>>>> Juergen Gross (5):
>>>>     tools/xenstored: remove "-D" command line parameter
>>>>     tools/xenstored: remove "-V" command line option
>>>>     tools/xenstored: remove the "-P" command line option
>>>>     tools/xenstored: remove the "-R" command line option
>>>>     tools/xenstored: remove "-N" command line option
>>>>
>>>>    tools/xenstored/core.c | 81 +++++++-----------------------------------
>>>>    1 file changed, 12 insertions(+), 69 deletions(-)
>>>>
>>>
>>> I think at least patches 1-4 can go in as they all have the required Acks.
>>
>> I'll try to remember to include them in the next swipe. I was kind of
>> assuming Julien would be taking care of them.
> 
> Sorry this fell through the cracks. I can do it if you haven't yet done it.

I haven't yet, no.

Jan

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Julien Grall]

On 07/12/2023 12:39, Jan Beulich wrote:
> On 07.12.2023 13:14, Julien Grall wrote:
>> Hi Jan,
>>
>> On 07/12/2023 09:30, Jan Beulich wrote:
>>> On 07.12.2023 09:26, Juergen Gross wrote:
>>>> On 21.11.23 12:40, Juergen Gross wrote:
>>>>> Remove some command line options which have no real use case.
>>>>>
>>>>> Changes in V2:
>>>>> - moved removal of "-N" into last patch of the series, as this is the
>>>>>      only option which seems to have a use case (OTOH using it has some
>>>>>      downsides as well).
>>>>>
>>>>> Juergen Gross (5):
>>>>>      tools/xenstored: remove "-D" command line parameter
>>>>>      tools/xenstored: remove "-V" command line option
>>>>>      tools/xenstored: remove the "-P" command line option
>>>>>      tools/xenstored: remove the "-R" command line option
>>>>>      tools/xenstored: remove "-N" command line option
>>>>>
>>>>>     tools/xenstored/core.c | 81 +++++++-----------------------------------
>>>>>     1 file changed, 12 insertions(+), 69 deletions(-)
>>>>>
>>>>
>>>> I think at least patches 1-4 can go in as they all have the required Acks.
>>>
>>> I'll try to remember to include them in the next swipe. I was kind of
>>> assuming Julien would be taking care of them.
>>
>> Sorry this fell through the cracks. I can do it if you haven't yet done it.
> 
> I haven't yet, no.

Ok. I will take care of patches #1-#4 then.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Julien Grall]

On 07/12/2023 08:26, Juergen Gross wrote:
> On 21.11.23 12:40, Juergen Gross wrote:
>> Remove some command line options which have no real use case.
>>
>> Changes in V2:
>> - moved removal of "-N" into last patch of the series, as this is the
>>    only option which seems to have a use case (OTOH using it has some
>>    downsides as well).
>>
>> Juergen Gross (5):
>>    tools/xenstored: remove "-D" command line parameter
>>    tools/xenstored: remove "-V" command line option
>>    tools/xenstored: remove the "-P" command line option
>>    tools/xenstored: remove the "-R" command line option
>>    tools/xenstored: remove "-N" command line option
>>
>>   tools/xenstored/core.c | 81 +++++++-----------------------------------
>>   1 file changed, 12 insertions(+), 69 deletions(-)
>>
> 
> I think at least patches 1-4 can go in as they all have the required Acks.

Thanks for the reminder. I have committed patches #1-#4.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Anthony PERARD]

On Tue, Nov 21, 2023 at 12:40:43PM +0100, Juergen Gross wrote:
> Remove some command line options which have no real use case.
> 
> Changes in V2:
> - moved removal of "-N" into last patch of the series, as this is the
>   only option which seems to have a use case (OTOH using it has some
>   downsides as well).
> 
> Juergen Gross (5):
>   tools/xenstored: remove "-D" command line parameter
>   tools/xenstored: remove "-V" command line option
>   tools/xenstored: remove the "-P" command line option
>   tools/xenstored: remove the "-R" command line option
>   tools/xenstored: remove "-N" command line option

Should we have en entry in the changelog about all these removals? Who
knows if they are used by someone or not...

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 0/5] tools/xenstored: remove some command line options

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 892 bytes --]

On 08.12.23 16:33, Anthony PERARD wrote:
> On Tue, Nov 21, 2023 at 12:40:43PM +0100, Juergen Gross wrote:
>> Remove some command line options which have no real use case.
>>
>> Changes in V2:
>> - moved removal of "-N" into last patch of the series, as this is the
>>    only option which seems to have a use case (OTOH using it has some
>>    downsides as well).
>>
>> Juergen Gross (5):
>>    tools/xenstored: remove "-D" command line parameter
>>    tools/xenstored: remove "-V" command line option
>>    tools/xenstored: remove the "-P" command line option
>>    tools/xenstored: remove the "-R" command line option
>>    tools/xenstored: remove "-N" command line option
> 
> Should we have en entry in the changelog about all these removals? Who
> knows if they are used by someone or not...

I'll send a patch in this regard when patch 5 has gone in.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 5/5] tools/xenstored: remove "-N" command line option

[Andrew Cooper]

On 21/11/2023 11:40 am, Juergen Gross wrote:
> The "-N" (do not daemonize) command line option is of questionable use:
> its sole purpose seems to be to aid debugging of xenstored by making it
> easier to start xenstored under gdb, or to see any debug messages
> easily.
>
> Debug messages can as well be sent to syslog(), while gdb can be
> attached to the daemon easily. The only not covered case is an error
> while initializing xenstored, but this could be handled e.g. by saving
> a core dump, which can be analyzed later.
>
> The call of talloc_enable_leak_report_full() done only with "-N"
> specified is no longer needed, as the same can be achieved via
> "xenstore-control memreport".
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Systemd wants daemons to not fork, because systemd can start them up in
a better-prescribed environment than deamonise() can make.

It was a lazy port to system which has caused us not to be using -N in
the first place.

So no - I think this option specifically wants to stay, and the systemd
integration improved.

~Andrew

Re: [PATCH v2 5/5] tools/xenstored: remove "-N" command line option

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1787 bytes --]

On 08.12.23 16:46, Andrew Cooper wrote:
> On 21/11/2023 11:40 am, Juergen Gross wrote:
>> The "-N" (do not daemonize) command line option is of questionable use:
>> its sole purpose seems to be to aid debugging of xenstored by making it
>> easier to start xenstored under gdb, or to see any debug messages
>> easily.
>>
>> Debug messages can as well be sent to syslog(), while gdb can be
>> attached to the daemon easily. The only not covered case is an error
>> while initializing xenstored, but this could be handled e.g. by saving
>> a core dump, which can be analyzed later.
>>
>> The call of talloc_enable_leak_report_full() done only with "-N"
>> specified is no longer needed, as the same can be achieved via
>> "xenstore-control memreport".
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Systemd wants daemons to not fork, because systemd can start them up in
> a better-prescribed environment than deamonise() can make.
> 
> It was a lazy port to system which has caused us not to be using -N in
> the first place.
> 
> So no - I think this option specifically wants to stay, and the systemd
> integration improved.

The problem with this approach is that we have some functionality in the
launch-xenstore script relying on [o]xenstored coming back after having
forked off the daemon: we are setting the oom-score, which can be done
only when knowing the process id.

So we need a solution for this problem before we can really encourage
users to use the -N option.

Please note that setting the oom-score from within xenstored was rejected
back when I posted a patch in this regard, especially as it is specific to
Linux. Additionally this would mean we need to add this functionality to
xenstored AND oxenstored.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2 5/5] tools/xenstored: remove "-N" command line option

[Anthony PERARD]

On Fri, Dec 08, 2023 at 04:58:52PM +0100, Juergen Gross wrote:
> On 08.12.23 16:46, Andrew Cooper wrote:
> > On 21/11/2023 11:40 am, Juergen Gross wrote:
> > > The "-N" (do not daemonize) command line option is of questionable use:
> > > its sole purpose seems to be to aid debugging of xenstored by making it
> > > easier to start xenstored under gdb, or to see any debug messages
> > > easily.
> > > 
> > > Debug messages can as well be sent to syslog(), while gdb can be
> > > attached to the daemon easily. The only not covered case is an error
> > > while initializing xenstored, but this could be handled e.g. by saving
> > > a core dump, which can be analyzed later.
> > > 
> > > The call of talloc_enable_leak_report_full() done only with "-N"
> > > specified is no longer needed, as the same can be achieved via
> > > "xenstore-control memreport".
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > Systemd wants daemons to not fork, because systemd can start them up in
> > a better-prescribed environment than deamonise() can make.
> > 
> > It was a lazy port to system which has caused us not to be using -N in
> > the first place.
> > 
> > So no - I think this option specifically wants to stay, and the systemd
> > integration improved.
> 
> The problem with this approach is that we have some functionality in the
> launch-xenstore script relying on [o]xenstored coming back after having
> forked off the daemon: we are setting the oom-score, which can be done

It's perfectly reasonable to bane the use of "-N" when using
`./launch-xenstore` to start xenstored. It doesn't mean that the option
needs to be removed from xenstored.

> only when knowing the process id.
> 
> So we need a solution for this problem before we can really encourage
> users to use the -N option.
> 
> Please note that setting the oom-score from within xenstored was rejected
> back when I posted a patch in this regard, especially as it is specific to
> Linux. Additionally this would mean we need to add this functionality to
> xenstored AND oxenstored.

There's still a world where -N can be used, and oom-score can be set,
I'm pretty sure that can be done with an hand-crafted "systemd.service"
file. One probably need to have something like "OOMPolicy=" or
"OOMScoreAdjust=" in their service file, and start the `xenstored`
daemon of their choice directly, even with -N as systemd can detect when
the daemon is ready because we use sd_notify() (at least in cxenstored).


So, I think we should keep -N.

Cheers,

-- 
Anthony PERARD

Re: [PATCH v2 5/5] tools/xenstored: remove "-N" command line option

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2700 bytes --]

On 08.12.23 17:16, Anthony PERARD wrote:
> On Fri, Dec 08, 2023 at 04:58:52PM +0100, Juergen Gross wrote:
>> On 08.12.23 16:46, Andrew Cooper wrote:
>>> On 21/11/2023 11:40 am, Juergen Gross wrote:
>>>> The "-N" (do not daemonize) command line option is of questionable use:
>>>> its sole purpose seems to be to aid debugging of xenstored by making it
>>>> easier to start xenstored under gdb, or to see any debug messages
>>>> easily.
>>>>
>>>> Debug messages can as well be sent to syslog(), while gdb can be
>>>> attached to the daemon easily. The only not covered case is an error
>>>> while initializing xenstored, but this could be handled e.g. by saving
>>>> a core dump, which can be analyzed later.
>>>>
>>>> The call of talloc_enable_leak_report_full() done only with "-N"
>>>> specified is no longer needed, as the same can be achieved via
>>>> "xenstore-control memreport".
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Systemd wants daemons to not fork, because systemd can start them up in
>>> a better-prescribed environment than deamonise() can make.
>>>
>>> It was a lazy port to system which has caused us not to be using -N in
>>> the first place.
>>>
>>> So no - I think this option specifically wants to stay, and the systemd
>>> integration improved.
>>
>> The problem with this approach is that we have some functionality in the
>> launch-xenstore script relying on [o]xenstored coming back after having
>> forked off the daemon: we are setting the oom-score, which can be done
> 
> It's perfectly reasonable to bane the use of "-N" when using
> `./launch-xenstore` to start xenstored. It doesn't mean that the option
> needs to be removed from xenstored.
> 
>> only when knowing the process id.
>>
>> So we need a solution for this problem before we can really encourage
>> users to use the -N option.
>>
>> Please note that setting the oom-score from within xenstored was rejected
>> back when I posted a patch in this regard, especially as it is specific to
>> Linux. Additionally this would mean we need to add this functionality to
>> xenstored AND oxenstored.
> 
> There's still a world where -N can be used, and oom-score can be set,
> I'm pretty sure that can be done with an hand-crafted "systemd.service"
> file. One probably need to have something like "OOMPolicy=" or
> "OOMScoreAdjust=" in their service file, and start the `xenstored`
> daemon of their choice directly, even with -N as systemd can detect when
> the daemon is ready because we use sd_notify() (at least in cxenstored).
> 
> 
> So, I think we should keep -N.

Okay, fair enough.

Lets drop the patch then.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]