cli: accept long path

[vincenzopalazzo]

This allows to accept safely long paths as options and does not truncate them as #5576 described

Fixes #5576
Alternative to #5703

Suggested-by: @rustyrussell rusty@rustcorp.com.au
Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com

[SimonVrouwe]

CLN accept long path as cli parameter also if ccan has a limit to it, but also specific machine has a limit on the length
of the path,

I don't understand above sentence

so this explains why Rusty hard coded
the upper limit of the length into ccan.

I don't think think the limit is related to length of socket paths, instead it seems the 80 chars limit is just the default width of ancient terminals.

lightning/ccan/ccan/opt/opt.h

Lines 432 to 433 in efd7a58

	/* Maximum length of arg to show in opt_usage */
	#define OPT_SHOW_LEN 80

Pull request for the ccan library don't belong in this repo, but here ?

Maybe change to ccan is not needed, as this seems purely an opt->show issue, which can be fixed (I think) by changing opt_show_charp callback into something that uses a larger buffer, like:

diff --git a/common/configdir.c b/common/configdir.c
index c03abe18c..96f4209cc 100644
--- a/common/configdir.c
+++ b/common/configdir.c
@@ -290,6 +290,17 @@ void parse_config_files(const char *config_filename,
        parse_state = CMDLINE;
 }
 
+static void opt_show_longpath(char *buf, char *const *p)
+{
+       if (*p){
+               size_t len = strlen(*p);
+               buf[0] = '"';
+               strncpy(buf+1, *p, len);
+               buf[1+len] = '"';
+               buf[2+len] = '\0';
+       }
+}
+
 void initial_config_opts(const tal_t *ctx,
                         int argc, char *argv[],
                         char **config_filename,
@@ -330,7 +341,7 @@ void initial_config_opts(const tal_t *ctx,
        /* If they set --conf it can still set --lightning-dir */
        if (!*config_filename) {
                opt_register_early_arg("--lightning-dir=<dir>",
-                                      opt_restricted_forceconf_only, opt_show_charp,
+                                      opt_restricted_forceconf_only, opt_show_longpath,
                                       config_basedir,
                                       "Set base directory: network-specific subdirectory is under here");
        } else {
@@ -392,7 +403,7 @@ void initial_config_opts(const tal_t *ctx,
 
        /* This is never in a default config file (since we used the defaults to find it!). */
        opt_register_early_arg("--lightning-dir=<dir>",
-                              opt_restricted_forceconf_only, opt_show_charp,
+                              opt_restricted_forceconf_only, opt_show_longpath,
                               config_basedir,
                               "Set base directory: network-specific subdirectory is under here");
        opt_register_early_arg("--network",
diff --git a/lightningd/options.c b/lightningd/options.c
index 968f0ae82..844ebf340 100644
--- a/lightningd/options.c
+++ b/lightningd/options.c
@@ -1522,7 +1522,7 @@ static void add_config(struct lightningd *ld,
 {
        char *name0 = tal_strndup(tmpctx, name, len);
        char *answer = NULL;
-       char buf[OPT_SHOW_LEN + sizeof("...")];
+       char buf[256];
 
 #if DEVELOPER
        if (strstarts(name0, "dev-")) {
@@ -1590,7 +1590,6 @@ static void add_config(struct lightningd *ld,
                        /* Ignore hidden options (deprecated) */
                } else if (opt->show) {
                        opt->show(buf, opt->u.carg);
-                       strcpy(buf + OPT_SHOW_LEN - 1, "...");
 
                        if (streq(buf, "true") || streq(buf, "false")
                            || (!streq(buf, "") && strspn(buf, "0123456789.") == strlen(buf))) {

The buf length 256 is more of a guess, but it makes below test pass:
TEST_DIR=/tmp/aaaaaaaaaaaaaaaaaa VALGRIND=0 pytest tests/ -k 'test_relative_config_dir'

A general tip: c-lightning's config/options system is pretty complex and hard to read (for example it uses callback signatures to recognizes options in add_config), which I could only understand after connecting gdb debugger.

Good luck!

[vincenzopalazzo]

I don't understand the above sentence

Can be summarized in: There is no reason to accept string with length > max length supported

I don't think think the limit is related to length of socket paths, instead it seems the 80 chars limit is just the default width of ancient terminals.

From this comment #5576 (comment) I can see that we can fall in the case of the path too long and this is OS related

Pull request for the ccan library don't belong in this repo, but here ?

PR already opened! the commit will be removed when I fixed the tests with a workable solution

Maybe change to ccan is not needed, as this seems purely an opt->show issue, which can be fixed (I think) by changing opt_show_charp callback into something that uses a larger buffer, like:

indeed, let me play with it a little bit

[SimonVrouwe]

Some ideas:

• char buf[OPT_SHOW_LEN + sizeof("...")]; can be turned into char *buf; (to make it arbitrary length).
• inside opt_show_longpath(char *buf, char *const *p) (which needs a better name) just buf = tal_strdup(tmpctx, *p) ?
• maybe adding (in opt_show_longpath) and subsequent removal of the double quotes " can be also be cleaned up.
  

  lightning/lightningd/options.c

  Lines 1603 to 1607 in 3a39c63

  	/* opt_show_charp surrounds with "", strip them */
  	if (strstarts(buf, "\"")) {
  	char *end = strrchr(buf, '"');
  	memmove(end, end + 1, strlen(end));
  	answer = buf + 1;

[rustyrussell]

Yes, I prefer to implement it like this:

diff --git a/lightningd/options.c b/lightningd/options.c
index f4267ecd8..67a8b9f1e 100644
--- a/lightningd/options.c
+++ b/lightningd/options.c
@@ -1644,6 +1644,9 @@ static void add_config(struct lightningd *ld,
 	} else if (opt->type & OPT_HASARG) {
 		if (opt->desc == opt_hidden) {
 			/* Ignore hidden options (deprecated) */
+		} else if (opt->show == (void *)opt_show_charp) {
+			/* Don't truncate! */
+			answer = tal_strdup(tmpctx, *(char **)opt->u.carg);
 		} else if (opt->show) {
 			opt->show(buf, opt->u.carg);
 			strcpy(buf + OPT_SHOW_LEN - 1, "...");
@@ -1655,14 +1658,7 @@ static void add_config(struct lightningd *ld,
 				json_add_primitive(response, name0, buf);
 				return;
 			}
-
-			/* opt_show_charp surrounds with "", strip them */
-			if (strstarts(buf, "\"")) {
-				char *end = strrchr(buf, '"');
-				memmove(end, end + 1, strlen(end));
-				answer = buf + 1;
-			} else
-				answer = buf;
+			answer = buf;
 		} else if (opt->cb_arg == (void *)opt_set_talstr
 			   || opt->cb_arg == (void *)opt_set_charp
 			   || is_restricted_print_if_nonnull(opt->cb_arg)) {

[vincenzopalazzo]

Ok, I just rebased and apply the Rusty Patch! thanks!

[rustyrussell]

Ack ae138b2