vtysh: make hostname and domainname persistent #8847

anlancs · 2021-06-12T10:04:29Z

Two difficults to make persistent:

In integrated mode, watchfrr uses vtysh -w to save into configure file, but the new
vtysh can't get these names from current using vtysh.
When usesrs run vtysh, it will never reading /etf/frr/frr.conf, so
vtysh can't get the exact names.

To fixes this issue, need put these names saved into all daemons besides
current vtysh.

Two difficults to make persistent: 1) In integrated mode, watchfrr uses `vtysh -w` to save into configure file, but the new `vtysh` can't get these names from current using vtysh. 2) When usesrs run `vtysh`, it will never reading /etf/frr/frr.conf, so `vtysh` can't get the exact names. To fixes this issue, need put these names saved into all daemons besides current vtysh. Signed-off-by: anlancs <anlan_cs@tom.com>

polychaeta

Thanks for your contribution to FRR!

Click for style suggestions

To apply these suggestions:

curl -s https://gist.githubusercontent.com/polychaeta/efb5339cb02412eba398c098cb1a554f/raw/77dabf24fb32fd02d091572c5506ca2f4699696c/cr_8847_1623492274.diff | git apply

diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
index aac2ec166..59c1cbea3 100644
--- a/vtysh/vtysh.c
+++ b/vtysh/vtysh.c
@@ -476,12 +476,12 @@ int vtysh_client_run_special_commands()
 		/* suppress output to user */
 		vty->of_saved = vty->of;
 		vty->of = NULL;
-		vtysh_client_run_all(head_client, line, 1, vtysh_config_parse_name_line,
-				     NULL);
+		vtysh_client_run_all(head_client, line, 1,
+				     vtysh_config_parse_name_line, NULL);
 		vty->of = vty->of_saved;
-    }
+	}
 
-    return 1;
+	return 1;
 }
 
 /* Command execution over the vty interface. */
@@ -683,7 +683,8 @@ static int vtysh_execute_func(const char *line, int pager)
 			 */
 			if (!strcmp(cmd->string, "hostname WORD")
 			    || !strcmp(cmd->string, "doaminname WORD"))
-				(*cmd->func)(cmd, vty, 1, (struct cmd_token **)line);
+				(*cmd->func)(cmd, vty, 1,
+					     (struct cmd_token **)line);
 			else
 				(*cmd->func)(cmd, vty, 0, NULL);
 		}
@@ -3205,45 +3206,40 @@ DEFUNSH(VTYSH_ALL, no_vtysh_config_enable_password,
 }
 
 /* Hostname configuration */
-DEFUNSH(VTYSH_ALL, vtysh_config_hostname,
-       vtysh_hostname_cmd,
-       "hostname WORD",
-       "Set system's network name\n"
-       "This system's network name\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_hostname, vtysh_hostname_cmd, "hostname WORD",
+	"Set system's network name\n"
+	"This system's network name\n")
 {
 	/* Skip all checks, which are already checked in other daemons */
-        char * m = strrchr((char*)argv, ' ');
-        return cmd_hostname_set(m + 1);
+	char *m = strrchr((char *)argv, ' ');
+	return cmd_hostname_set(m + 1);
 }
 
-DEFUNSH(VTYSH_ALL, vtysh_config_no_hostname,
-       vtysh_no_hostname_cmd,
-       "no hostname [HOSTNAME]",
-       NO_STR
-       "Reset system's network name\n"
-       "Host name of this router\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_no_hostname, vtysh_no_hostname_cmd,
+	"no hostname [HOSTNAME]",
+	NO_STR
+	"Reset system's network name\n"
+	"Host name of this router\n")
 {
 	return cmd_hostname_set(NULL);
 }
 
 /* Domainname configuration */
-DEFUNSH(VTYSH_ALL, vtysh_config_domainname,
-      vtysh_domainname_cmd,
-      "domainname WORD",
-      "Set system's domain name\n"
-      "This system's domain name\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_domainname, vtysh_domainname_cmd,
+	"domainname WORD",
+	"Set system's domain name\n"
+	"This system's domain name\n")
 {
 	/* Skip all checks, which are already checked in other daemons */
-        char *m = strrchr((char*)argv, ' ');
-        return cmd_domainname_set(m + 1);
+	char *m = strrchr((char *)argv, ' ');
+	return cmd_domainname_set(m + 1);
 }
 
-DEFUNSH(VTYSH_ALL, vysh_config_no_domainname,
-      vtysh_no_domainname_cmd,
-      "no domainname [DOMAINNAME]",
-      NO_STR
-      "Reset system's domain name\n"
-      "domain name of this router\n")
+DEFUNSH(VTYSH_ALL, vysh_config_no_domainname, vtysh_no_domainname_cmd,
+	"no domainname [DOMAINNAME]",
+	NO_STR
+	"Reset system's domain name\n"
+	"domain name of this router\n")
 {
 	return cmd_domainname_set(NULL);
 }

If you are a new contributor to FRR, please see our contributing guidelines.

After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.

anlancs · 2021-06-12T10:06:54Z

fix #1497

LabN-CI · 2021-06-12T10:31:27Z

Outdated results 💚

Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/8847 `e7a8aeb`
Date	06/12/2021
Start	06:05:51
Finish	06:31:23
Run-Time	25:32
Total	1815
Pass	1815
Fail	0
Valgrind-Errors
Valgrind-Loss
Details	vncregress-2021-06-12-06:05:51.txt
Log	autoscript-2021-06-12-06:07:08.log.bz2
Memory	520 518 431

For details, please contact louberger

idryzhov

I didn't look deeply into the code, but IMO the correct way to make hostname and domainname persistent is to set them in the OS. Current "hostname" and "domainname" commands are needed to update the FRR state without restart when you changed hostname/domainname in the OS.

qlyoung

Based on your description I'm not sure I understand the problem being solved here.

Is it that you want the domainname and hostname commands persisted to the config file? If so, can you elaborate on why you want that?

qlyoung · 2021-06-29T14:55:49Z

vtysh/vtysh.c

+			 */
+			if (!strcmp(cmd->string, "hostname WORD")
+			    || !strcmp(cmd->string, "doaminname WORD"))
+				(*cmd->func)(cmd, vty, 1, (struct cmd_token **)line);


Sorry but this is a hard NACK. The last thing vtysh needs is more special casing.

Also, you misspelled domainname which tells me that whatever this does, it isn't tested.

Sorry for the misspelled word, i append a commit to fix it. It is caused by my git operation of combining many to one commit.
On the contrary, i do much test.

Yes, this hard code here is ugly. I have no idea of it, i will try other method. And can you shed light on it?

Since vtysh send commands to other daemons, then other daemons can call hostname_cmd.
vtysh need also run these commands (call hostname_cmd or domainname_cmd), here is for that.

Fix one wrong word: domainname Signed-off-by: anlan_cs <anlan_cs@tom.com>

polychaeta

Thanks for your contribution to FRR!

Click for style suggestions

To apply these suggestions:

curl -s https://gist.githubusercontent.com/polychaeta/a46cc62c52d2a1ecea8c4e7ae85366c7/raw/e91214d9d61ecc3d0a6b802c03b86ab3963c76be/cr_8847_1625232465.diff | git apply

diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c
index 7d4f3964f..a539be92b 100644
--- a/vtysh/vtysh.c
+++ b/vtysh/vtysh.c
@@ -476,12 +476,12 @@ int vtysh_client_run_special_commands()
 		/* suppress output to user */
 		vty->of_saved = vty->of;
 		vty->of = NULL;
-		vtysh_client_run_all(head_client, line, 1, vtysh_config_parse_name_line,
-				     NULL);
+		vtysh_client_run_all(head_client, line, 1,
+				     vtysh_config_parse_name_line, NULL);
 		vty->of = vty->of_saved;
-    }
+	}
 
-    return 1;
+	return 1;
 }
 
 /* Command execution over the vty interface. */
@@ -683,7 +683,8 @@ static int vtysh_execute_func(const char *line, int pager)
 			 */
 			if (!strcmp(cmd->string, "hostname WORD")
 			    || !strcmp(cmd->string, "domainname WORD"))
-				(*cmd->func)(cmd, vty, 1, (struct cmd_token **)line);
+				(*cmd->func)(cmd, vty, 1,
+					     (struct cmd_token **)line);
 			else
 				(*cmd->func)(cmd, vty, 0, NULL);
 		}
@@ -3205,45 +3206,40 @@ DEFUNSH(VTYSH_ALL, no_vtysh_config_enable_password,
 }
 
 /* Hostname configuration */
-DEFUNSH(VTYSH_ALL, vtysh_config_hostname,
-       vtysh_hostname_cmd,
-       "hostname WORD",
-       "Set system's network name\n"
-       "This system's network name\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_hostname, vtysh_hostname_cmd, "hostname WORD",
+	"Set system's network name\n"
+	"This system's network name\n")
 {
 	/* Skip all checks, which are already checked in other daemons */
-        char * m = strrchr((char*)argv, ' ');
-        return cmd_hostname_set(m + 1);
+	char *m = strrchr((char *)argv, ' ');
+	return cmd_hostname_set(m + 1);
 }
 
-DEFUNSH(VTYSH_ALL, vtysh_config_no_hostname,
-       vtysh_no_hostname_cmd,
-       "no hostname [HOSTNAME]",
-       NO_STR
-       "Reset system's network name\n"
-       "Host name of this router\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_no_hostname, vtysh_no_hostname_cmd,
+	"no hostname [HOSTNAME]",
+	NO_STR
+	"Reset system's network name\n"
+	"Host name of this router\n")
 {
 	return cmd_hostname_set(NULL);
 }
 
 /* Domainname configuration */
-DEFUNSH(VTYSH_ALL, vtysh_config_domainname,
-      vtysh_domainname_cmd,
-      "domainname WORD",
-      "Set system's domain name\n"
-      "This system's domain name\n")
+DEFUNSH(VTYSH_ALL, vtysh_config_domainname, vtysh_domainname_cmd,
+	"domainname WORD",
+	"Set system's domain name\n"
+	"This system's domain name\n")
 {
 	/* Skip all checks, which are already checked in other daemons */
-        char *m = strrchr((char*)argv, ' ');
-        return cmd_domainname_set(m + 1);
+	char *m = strrchr((char *)argv, ' ');
+	return cmd_domainname_set(m + 1);
 }
 
-DEFUNSH(VTYSH_ALL, vysh_config_no_domainname,
-      vtysh_no_domainname_cmd,
-      "no domainname [DOMAINNAME]",
-      NO_STR
-      "Reset system's domain name\n"
-      "domain name of this router\n")
+DEFUNSH(VTYSH_ALL, vysh_config_no_domainname, vtysh_no_domainname_cmd,
+	"no domainname [DOMAINNAME]",
+	NO_STR
+	"Reset system's domain name\n"
+	"domain name of this router\n")
 {
 	return cmd_domainname_set(NULL);
 }

If you are a new contributor to FRR, please see our contributing guidelines.

After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.

LabN-CI · 2021-07-02T14:07:12Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/8847 `24a4502`
Date	07/02/2021
Start	09:41:31
Finish	10:07:07
Run-Time	25:36
Total	1815
Pass	1815
Fail	0
Valgrind-Errors
Valgrind-Loss
Details	vncregress-2021-07-02-09:41:31.txt
Log	autoscript-2021-07-02-09:42:45.log.bz2
Memory	516 513 430

For details, please contact louberger

eqvinox · 2021-09-02T11:29:10Z

Sorry but this isn't going anywhere; the fact that hostname/domainname even exist in FRR is a historical artifact & they should just reflect the system values.

Adding autoclose label.

eqvinox · 2021-09-02T11:30:58Z

@polychaeta autoclose in 1 week

idryzhov · 2021-09-14T22:31:43Z

Poly didn't fulfill the request. Closing manually.

polychaeta added the vtysh label Jun 12, 2021

polychaeta reviewed Jun 12, 2021

View reviewed changes

anlancs mentioned this pull request Jun 12, 2021

doc: add a note of hostname/domainname for vtysh #8820

Merged

qlyoung self-requested a review June 15, 2021 15:54

idryzhov requested changes Jun 23, 2021

View reviewed changes

qlyoung requested changes Jun 29, 2021

View reviewed changes

vtysh: make hostname and domainname persistent

24a4502

Fix one wrong word: domainname Signed-off-by: anlan_cs <anlan_cs@tom.com>

polychaeta reviewed Jul 2, 2021

View reviewed changes

eqvinox added the autoclose label Sep 2, 2021

idryzhov closed this Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vtysh: make hostname and domainname persistent #8847

vtysh: make hostname and domainname persistent #8847

anlancs commented Jun 12, 2021

polychaeta left a comment

anlancs commented Jun 12, 2021

LabN-CI commented Jun 12, 2021 •

edited

Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

idryzhov left a comment

qlyoung left a comment

qlyoung Jun 29, 2021

anlancs Jul 2, 2021

anlancs Jul 2, 2021

polychaeta left a comment

LabN-CI commented Jul 2, 2021

eqvinox commented Sep 2, 2021

eqvinox commented Sep 2, 2021

idryzhov commented Sep 14, 2021

vtysh: make hostname and domainname persistent #8847

vtysh: make hostname and domainname persistent #8847

Conversation

anlancs commented Jun 12, 2021

polychaeta left a comment

Choose a reason for hiding this comment

anlancs commented Jun 12, 2021

LabN-CI commented Jun 12, 2021 • edited Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

idryzhov left a comment

Choose a reason for hiding this comment

qlyoung left a comment

Choose a reason for hiding this comment

qlyoung Jun 29, 2021

Choose a reason for hiding this comment

anlancs Jul 2, 2021

Choose a reason for hiding this comment

anlancs Jul 2, 2021

Choose a reason for hiding this comment

polychaeta left a comment

Choose a reason for hiding this comment

LabN-CI commented Jul 2, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

eqvinox commented Sep 2, 2021

eqvinox commented Sep 2, 2021

idryzhov commented Sep 14, 2021

LabN-CI commented Jun 12, 2021 •

edited

Loading