Skip to content

Commit

Permalink
lib, vtysh: Stop processing vtysh commands when FRR is shutting down.
Browse files Browse the repository at this point in the history 
Issue:
When rendering a large configuration (10K) in vtysh, if FRR restarts in the middle
of the configuration (e.g., systemctl restart frr), FRR crashes. The crashing daemon
 depends on the configuration present in frr.conf.

For example, if there are 10K static routes, the crash will occur in staticd.

Fix:
During FRR shutdown, vtysh should be notified to stop pushing the configuration.

Testing:
10k static routes
tests.l3.ospf.ospfv2.test_ospf_route_thrash.TestMain.run_test run_test 113 success
TESTS: 1 PASSED: 1 FAILURES: 0 ERRORS: 0 SKIPPED: 0

Ticket: #3700042

Signed-off-by: Donald Sharp <[email protected]>
Signed-off-by: Rajesh Varatharaj <[email protected]>
  • Loading branch information
@routingrocks @donaldsharp
routingrocks authored and donaldsharp committed Dec 18, 2024
1 parent ac2177e commit e0dc1ba
Show file tree
Hide file tree
Showing 9 changed files with 30 additions and 29 deletions.
2 changes: 1 addition & 1 deletion lib/command.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1741,7 +1741,7 @@ static int file_write_config(struct vty *vty)
vty_time_print(file_vty, 1);
vty_out(file_vty, "!\n");
vty_write_config(file_vty);
vty_close(file_vty);
vty_close(file_vty, false);

if (stat(config_file, &conf_stat) >= 0) {
if (unlink(config_file_sav) != 0)
Expand Down
2 changes: 1 addition & 1 deletion lib/northbound_cli.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ static int nb_write_config(struct nb_config *config, enum nb_cfg_format format,
if (config)
ret = nb_cli_show_config(file_vty, config, format, translator,
false);
vty_close(file_vty);
vty_close(file_vty, false);

return ret;
}
Expand Down
35 changes: 18 additions & 17 deletions lib/vty.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ void vty_mgmt_resume_response(struct vty *vty, int ret)
}

if (vty->status == VTY_CLOSE)
vty_close(vty);
vty_close(vty, false);
else if (vty->type != VTY_FILE)
vty_event(VTYSH_READ, vty);
else
Expand Down Expand Up @@ -1611,7 +1611,7 @@ static void vty_read(struct event *thread)

/* Check status. */
if (vty->status == VTY_CLOSE)
vty_close(vty);
vty_close(vty, false);
else {
vty_event(VTY_WRITE, vty);
vty_event(VTY_READ, vty);
Expand Down Expand Up @@ -1648,11 +1648,11 @@ static void vty_flush(struct event *thread)
vty->fd, vty->wfd);
buffer_reset(vty->lbuf);
buffer_reset(vty->obuf);
vty_close(vty);
vty_close(vty, false);
return;
case BUFFER_EMPTY:
if (vty->status == VTY_CLOSE)
vty_close(vty);
vty_close(vty, false);
else {
vty->status = VTY_NORMAL;
if (vty->lines == 0)
Expand Down Expand Up @@ -1757,7 +1757,7 @@ static struct vty *vty_create(int vty_sock, union sockunion *su)
if (host.password == NULL && host.password_encrypt == NULL) {
vty_out(vty, "Vty password is not set.\n");
vty->status = VTY_CLOSE;
vty_close(vty);
vty_close(vty, false);
return NULL;
}
}
Expand Down Expand Up @@ -1853,7 +1853,7 @@ void vty_stdio_close(void)
{
if (!stdio_vty)
return;
vty_close(stdio_vty);
vty_close(stdio_vty, false);
}

struct vty *vty_stdio(void (*atclose)(int isexit))
Expand Down Expand Up @@ -2208,7 +2208,7 @@ static int vtysh_flush(struct vty *vty)
__func__, vty->fd);
buffer_reset(vty->lbuf);
buffer_reset(vty->obuf);
vty_close(vty);
vty_close(vty, false);
return -1;
case BUFFER_EMPTY:
break;
Expand Down Expand Up @@ -2286,7 +2286,7 @@ bool mgmt_vty_read_configs(void)
vty->pending_allowed = false;

if (!count)
vty_close(vty);
vty_close(vty, false);
else
vty_read_file_finish(vty, NULL);

Expand Down Expand Up @@ -2334,7 +2334,7 @@ static void vtysh_read(struct event *thread)
}
buffer_reset(vty->lbuf);
buffer_reset(vty->obuf);
vty_close(vty);
vty_close(vty, false);
#ifdef VTYSH_DEBUG
printf("close vtysh\n");
#endif /* VTYSH_DEBUG */
Expand Down Expand Up @@ -2422,7 +2422,7 @@ static void vtysh_read(struct event *thread)
}

if (vty->status == VTY_CLOSE)
vty_close(vty);
vty_close(vty, false);
else
vty_event(VTYSH_READ, vty);
}
Expand Down Expand Up @@ -2473,7 +2473,7 @@ static void vty_error_delete(void *arg)
will be careful not to access the vty afterwards (since it has
now been freed). This is safest from top-level functions (called
directly by the thread dispatcher). */
void vty_close(struct vty *vty)
void vty_close(struct vty *vty, bool shutdown)
{
int i;
bool was_stdio = false;
Expand All @@ -2489,7 +2489,8 @@ void vty_close(struct vty *vty)
"vty closed, uncommitted config will be lost.");

/* Drop out of configure / transaction if needed. */
vty_config_exit(vty);
if (!shutdown)
vty_config_exit(vty);

if (mgmt_fe_client && vty->mgmt_session_id) {
debug_fe_client("closing vty session");
Expand Down Expand Up @@ -2574,7 +2575,7 @@ static void vty_timeout(struct event *thread)

/* Close connection. */
vty->status = VTY_CLOSE;
vty_close(vty);
vty_close(vty, false);
}

/* Read up configuration file from file_name. */
Expand Down Expand Up @@ -2678,7 +2679,7 @@ void vty_read_file_finish(struct vty *vty, struct nb_config *config)
__func__, nb_err_name(ret), errmsg);
}

vty_close(vty);
vty_close(vty, false);
}

static FILE *vty_use_backup_config(const char *fullpath)
Expand Down Expand Up @@ -3407,7 +3408,7 @@ void vty_reset(void)
buffer_reset(vty->lbuf);
buffer_reset(vty->obuf);
vty->status = VTY_CLOSE;
vty_close(vty);
vty_close(vty, false);
}

vty_timeout_val = VTY_TIMEOUT_DEFAULT;
Expand Down Expand Up @@ -3519,7 +3520,7 @@ static void vty_mgmt_session_notify(struct mgmt_fe_client *client,
vty->mgmt_session_id = 0;
/* We may come here by way of vty_close() and short-circuits */
if (vty->status != VTY_CLOSE)
vty_close(vty);
vty_close(vty, false);
}
}

Expand Down Expand Up @@ -4187,7 +4188,7 @@ void vty_terminate(void)
buffer_reset(vty->lbuf);
buffer_reset(vty->obuf);
vty->status = VTY_CLOSE;
vty_close(vty);
vty_close(vty, true);
}

vtys_fini(vtysh_sessions);
Expand Down
2 changes: 1 addition & 1 deletion lib/vty.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ extern void vty_read_file_finish(struct vty *vty, struct nb_config *config);
extern void vty_time_print(struct vty *, int);
extern void vty_serv_start(const char *, unsigned short, const char *);
extern void vty_serv_stop(void);
extern void vty_close(struct vty *);
extern void vty_close(struct vty *vty, bool shutdown);
extern char *vty_get_cwd(void);
extern void vty_update_xpath(const char *oldpath, const char *newpath);
extern int vty_config_enter(struct vty *vty, bool private_config,
Expand Down
2 changes: 1 addition & 1 deletion tests/bgpd/test_peer_attr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,7 @@ static void test_finish(struct test *test)

/* Cleanup allocated memory. */
if (test->vty) {
vty_close(test->vty);
vty_close(test->vty, false);
test->vty = NULL;
}
if (test->log)
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/cli/test_commands.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ int main(int argc, char **argv)
}
fprintf(stderr, "\nDone.\n");

vty_close(vty);
vty_close(vty, false);
prng_free(prng);
test_terminate();
return 0;
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/test_grpc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ static void static_startup(void)
static void static_shutdown(void)
{
hook_call(test_grpc_fini);
vty_close(vty);
vty_close(vty, true);
vrf_terminate();
vty_terminate();
cmd_terminate();
Expand Down
10 changes: 5 additions & 5 deletions vtysh/vtysh.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -801,28 +801,28 @@ int vtysh_mark_file(const char *filename)
fprintf(stderr, "line %d: Warning...: %s\n",
lineno, vty->buf);
fclose(confp);
vty_close(vty);
vty_close(vty, false);
XFREE(MTYPE_VTYSH_CMD, vty_buf_copy);
return ret;
case CMD_ERR_AMBIGUOUS:
fprintf(stderr, "line %d: %% Ambiguous command: %s\n",
lineno, vty->buf);
fclose(confp);
vty_close(vty);
vty_close(vty, false);
XFREE(MTYPE_VTYSH_CMD, vty_buf_copy);
return CMD_ERR_AMBIGUOUS;
case CMD_ERR_NO_MATCH:
fprintf(stderr, "line %d: %% Unknown command: %s\n",
lineno, vty->buf);
fclose(confp);
vty_close(vty);
vty_close(vty, false);
XFREE(MTYPE_VTYSH_CMD, vty_buf_copy);
return CMD_ERR_NO_MATCH;
case CMD_ERR_INCOMPLETE:
fprintf(stderr, "line %d: %% Command incomplete: %s\n",
lineno, vty->buf);
fclose(confp);
vty_close(vty);
vty_close(vty, false);
XFREE(MTYPE_VTYSH_CMD, vty_buf_copy);
return CMD_ERR_INCOMPLETE;
case CMD_SUCCESS:
Expand All @@ -848,7 +848,7 @@ int vtysh_mark_file(const char *filename)
}
/* This is the end */
vty_out(vty, "\nend\n");
vty_close(vty);
vty_close(vty, false);
XFREE(MTYPE_VTYSH_CMD, vty_buf_copy);

if (confp != stdin)
Expand Down
2 changes: 1 addition & 1 deletion vtysh/vtysh_config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ static int vtysh_read_file(FILE *confp, bool dry_run)
vtysh_execute_no_pager("end");
vtysh_execute_no_pager("disable");

vty_close(vty);
vty_close(vty, false);

return (ret);
}
Expand Down

0 comments on commit e0dc1ba

Please sign in to comment.