Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vtysh: make hostname and domainname persistent #8847

Conversation

anlancs
Copy link
Contributor

@anlancs anlancs commented Jun 12, 2021

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

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>
Copy link

@polychaeta polychaeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

anlancs commented Jun 12, 2021

fix #1497

@LabN-CI
Copy link
Collaborator

LabN-CI commented Jun 12, 2021

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

@qlyoung qlyoung self-requested a review June 15, 2021 15:54
Copy link
Contributor

@idryzhov idryzhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@qlyoung qlyoung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link

@polychaeta polychaeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

LabN-CI commented Jul 2, 2021

💚 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
Copy link
Contributor

eqvinox commented Sep 2, 2021

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
Copy link
Contributor

eqvinox commented Sep 2, 2021

@polychaeta autoclose in 1 week

@idryzhov
Copy link
Contributor

Poly didn't fulfill the request. Closing manually.

@idryzhov idryzhov closed this Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants