-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
vtysh: make hostname and domainname persistent #8847
Conversation
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>
There was a problem hiding this 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.
fix #1497 |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
There was a problem hiding this 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.
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
There was a problem hiding this 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.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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. |
@polychaeta autoclose in 1 week |
Poly didn't fulfill the request. Closing manually. |
Two difficults to make persistent:
In integrated mode, watchfrr uses
vtysh -w
to save into configure file, but the newvtysh
can't get these names from current using vtysh.When usesrs run
vtysh
, it will never reading /etf/frr/frr.conf, sovtysh
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