Thanks Donald. I will send the updated patch. -Gautam
From: Donald Sharp <[email protected]<mailto:[email protected]>> Date: Monday, September 28, 2015 at 2:50 PM To: Kumar Gautam <[email protected]<mailto:[email protected]>> Cc: "[email protected]<mailto:[email protected]>" <[email protected]<mailto:[email protected]>>, Sergii Vystoropskyi <[email protected]<mailto:[email protected]>> Subject: Re: [quagga-dev 13188] [PATCH] Fix vtysh hang, when client's config size is close to vtysh's buffer size I would prefer that code restructuring be in their own series of patches separate from actual bug fixes, if they are unrelated to the actual bug. This provides the ability to look at each individual change with a clear description of what it is trying to do. I stopped reviewing the meat of the bug when I couldn't understand why the Makefile.am and code restructuring I pointed out was done given the description as that they didn't jive. I am not saying that the changes are good or bad, but as structured I would rather spend my limited time looking at code with clear/concise descriptions of what is being fixed and why. If what the code does and what the description says don't jive to me, I am going to stop reviewing and let the person know of the issue. donald On Mon, Sep 28, 2015 at 5:29 PM, Kumar, Gautam <[email protected]<mailto:[email protected]>> wrote: Thanks Donald for taking look. The reason I did rework on Makefile.am is to create vtysh(libvtysh_a_sources) as a library which can be used for unit test. We already have this for bgp (libbgp_a_sources). This has helped to create unit test for vtysh, however I can change if you think this will have any side effect on build system. Same reasoning applies to the move of 'struct vtysh_client' from vtysh.c to vtysh.h as well as the exposure of vtysh_client_execute. Here is the more description. Initially we had two functions vtysh_client_execute and vtysh_client_config, both of these function had same logic one was executing those config other was just passing it to vtysh_config_parser function. However the vtysh_client_execute has the right logic to handle the buffer while vtysh_client_config doesn’t. Hence I avoided the code duplication by combining these two functions which also fixs the buffer handling issue in vtysh_client_config. Since these functions were static initially It was difficult to write unit test for them. Hence the reason for exposure. I look forward for your comment. Regards, Gautam From: "[email protected]<mailto:[email protected]>" <[email protected]<mailto:[email protected]>> Date: Monday, September 28, 2015 at 6:48 AM To: Kumar Gautam <[email protected]<mailto:[email protected]>> Cc: "[email protected]<mailto:[email protected]>" <[email protected]<mailto:[email protected]>>, Sergii Vystoropskyi <[email protected]<mailto:[email protected]>> Subject: Re: [quagga-dev 13188] [PATCH] Fix vtysh hang, when client's config size is close to vtysh's buffer size I don't understand the rework of the Makefile.am, nor do I understand the move of 'struct vtysh_client' from vtysh.c to vtysh.h. Why also the exposure of vtysh_client_execute? Am I missreading the code? There is no-one calling vtysh_client_execute outside of vtysh.c. thanks! donald On Sun, Sep 27, 2015 at 5:49 AM, Gautam Kumar <[email protected]<mailto:[email protected]>> wrote: --- vtysh/Makefile.am | 6 +- vtysh/vtysh.c | 207 +++++++++++++++++++++++++---------------------------- vtysh/vtysh.h | 14 ++++ vtysh/vtysh_main.c | 4 +- 4 files changed, 115 insertions(+), 116 deletions(-) diff --git a/vtysh/Makefile.am b/vtysh/Makefile.am index d347735..9d5f8d3 100644 --- a/vtysh/Makefile.am +++ b/vtysh/Makefile.am @@ -8,12 +8,14 @@ LIBS = @LIBS@ @CURSES@ @LIBPAM@ AM_CFLAGS = $(WERROR) bin_PROGRAMS = vtysh +noinst_LIBRARIES = libvtysh.a -vtysh_SOURCES = vtysh_main.c vtysh.c vtysh_user.c vtysh_config.c +libvtysh_a_SOURCES = vtysh.c vtysh_user.c vtysh_config.c +vtysh_SOURCES = vtysh_main.c nodist_vtysh_SOURCES = vtysh_cmd.c CLEANFILES = vtysh_cmd.c noinst_HEADERS = vtysh.h vtysh_user.h -vtysh_LDADD = ../lib/libzebra.la<http://libzebra.la> @LIBCAP@ @LIBREADLINE@ +vtysh_LDADD = libvtysh.a ../lib/libzebra.la<http://libzebra.la> @LIBCAP@ @LIBREADLINE@ examplesdir = $(exampledir) dist_examples_DATA = vtysh.conf.sample diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c index 04ac550..0e33dea 100644 --- a/vtysh/vtysh.c +++ b/vtysh/vtysh.c @@ -44,13 +44,7 @@ struct vty *vty; char *vtysh_pager_name = NULL; /* VTY shell client structure. */ -struct vtysh_client -{ - int fd; - const char *name; - int flag; - const char *path; -} vtysh_client[] = +struct vtysh_client vtysh_client[] = { { .fd = -1, .name = "zebra", .flag = VTYSH_ZEBRA, .path = ZEBRA_VTYSH_PATH}, { .fd = -1, .name = "ripd", .flag = VTYSH_RIPD, .path = RIP_VTYSH_PATH}, @@ -62,6 +56,11 @@ struct vtysh_client { .fd = -1, .name = "pimd", .flag = VTYSH_PIMD, .path = PIM_VTYSH_PATH}, }; +/* Flag for indicate executing child command. */ +int execute_flag = 0; + +/* To store the vtysh buffer size */ +int vtysh_buf_sz; /* We need direct access to ripd to implement vtysh_exit_ripd_only. */ static struct vtysh_client *ripd_client = NULL; @@ -87,9 +86,9 @@ vclient_close (struct vtysh_client *vclient) /* Following filled with debug code to trace a problematic condition * under load - it SHOULD handle it. */ -#define ERR_WHERE_STRING "vtysh(): vtysh_client_config(): " -static int -vtysh_client_config (struct vtysh_client *vclient, char *line) +#define ERR_WHERE_STRING "vtysh(): vtysh_client_execute(): " +int +vtysh_client_execute (struct vtysh_client *vclient, char *line, FILE *fp) { int ret; char *buf; @@ -100,6 +99,7 @@ vtysh_client_config (struct vtysh_client *vclient, char *line) int nbytes; int i; int readln; + int numnulls = 0; if (vclient->fd < 0) return CMD_SUCCESS; @@ -112,7 +112,7 @@ vtysh_client_config (struct vtysh_client *vclient, char *line) } /* Allow enough room for buffer to read more than a few pages from socket. */ - bufsz = 5 * getpagesize() + 1; + bufsz = vtysh_buf_sz; buf = XMALLOC(MTYPE_TMP, bufsz); memset(buf, 0, bufsz); pbuf = buf; @@ -145,112 +145,90 @@ vtysh_client_config (struct vtysh_client *vclient, char *line) XFREE(MTYPE_TMP, buf); return CMD_SUCCESS; } - - pbuf[nbytes] = '\0'; - - if (nbytes >= 4) - { - i = nbytes - 4; - if (pbuf[i] == '\0' && pbuf[i + 1] == '\0' && pbuf[i + 2] == '\0') - { - ret = pbuf[i + 3]; - break; - } - } - pbuf += nbytes; - - /* See if a line exists in buffer, if so parse and consume it, and - * reset read position. */ - if ((eoln = strrchr(buf, '\n')) == NULL) - continue; - - if (eoln >= ((buf + bufsz) - 1)) - { - fprintf (stderr, ERR_WHERE_STRING \ - "warning - eoln beyond buffer end.\n"); - } - vtysh_config_parse(buf); - - eoln++; - left = (size_t)(buf + bufsz - eoln); - memmove(buf, eoln, left); - buf[bufsz-1] = '\0'; - pbuf = buf + strlen(buf); + else + { + /* If we have already seen 3 nulls, then current byte is ret code */ + if ((numnulls == 3) && (nbytes == 1)) + { + ret = pbuf[0]; + break; + } + pbuf[nbytes] = '\0'; + /* If the config needs to be written in file or stdout */ + if (fp) + { + fputs(pbuf, fp); + fflush (fp); + } + + /* At max look last four bytes */ + if (nbytes >= 4) + { + i = nbytes - 4; + numnulls = 0; + } + else + i = 0; + + /* Count the numnulls */ + while (i < nbytes && numnulls <3) + { + if (pbuf[i++] == '\0') + numnulls++; + else + numnulls = 0; + } + /* We might have seen 3 consecutive nulls so store the ret code before updating pbuf*/ + ret = pbuf[nbytes-1]; + pbuf += nbytes; + + + /* See if a line exists in buffer, if so parse and consume it, and + * reset read position. If 3 nulls has been encountered consume the buffer before + * next read. + */ + if (((eoln = strrchr(buf, '\n')) == NULL) && (numnulls<3)) + continue; + + if (eoln >= ((buf + bufsz) - 1)) + { + fprintf (stderr, ERR_WHERE_STRING \ + "warning - eoln beyond buffer end.\n"); + } + + /* If the config needs parsing, consume it */ + if(!fp) + vtysh_config_parse(buf); + + eoln++; + left = (size_t)(buf + bufsz - eoln); + /* + * This check is required since when a config line split between two consecutive reads, + * then buf will have first half of config line and current read will bring rest of the + * line. So in this case eoln will be 1 here, hence calculation of left will be wrong. + * In this case we don't need to do memmove, because we have already seen 3 nulls. + */ + if(left < bufsz) + memmove(buf, eoln, left); + + buf[bufsz-1] = '\0'; + pbuf = buf + strlen(buf); + /* got 3 or more trailing NULs? */ + if ((numnulls >=3) && (i < nbytes)) + { + break; + } + } } /* Parse anything left in the buffer. */ - - vtysh_config_parse (buf); + if(!fp) + vtysh_config_parse (buf); XFREE(MTYPE_TMP, buf); return ret; } -static int -vtysh_client_execute (struct vtysh_client *vclient, const char *line, FILE *fp) -{ - int ret; - char buf[1001]; - int nbytes; - int i; - int numnulls = 0; - - if (vclient->fd < 0) - return CMD_SUCCESS; - - ret = write (vclient->fd, line, strlen (line) + 1); - if (ret <= 0) - { - vclient_close (vclient); - return CMD_SUCCESS; - } - - while (1) - { - nbytes = read (vclient->fd, buf, sizeof(buf)-1); - - if (nbytes <= 0 && errno != EINTR) - { - vclient_close (vclient); - return CMD_SUCCESS; - } - - if (nbytes > 0) - { - if ((numnulls == 3) && (nbytes == 1)) - return buf[0]; - - buf[nbytes] = '\0'; - fputs (buf, fp); - fflush (fp); - - /* check for trailling \0\0\0<ret code>, - * even if split across reads - * (see lib/vty.c::vtysh_read) - */ - if (nbytes >= 4) - { - i = nbytes-4; - numnulls = 0; - } - else - i = 0; - - while (i < nbytes && numnulls < 3) - { - if (buf[i++] == '\0') - numnulls++; - else - numnulls = 0; - } - - /* got 3 or more trailing NULs? */ - if ((numnulls >= 3) && (i < nbytes)) - return (buf[nbytes-1]); - } - } -} - static void vtysh_exit_ripd_only (void) { @@ -1739,7 +1717,7 @@ DEFUN (vtysh_write_terminal, vty_out (vty, "!%s", VTY_NEWLINE); for (i = 0; i < array_size(vtysh_client); i++) - vtysh_client_config (&vtysh_client[i], line); + vtysh_client_execute (&vtysh_client[i], line, NULL); /* Integrate vtysh specific configuration. */ vtysh_config_write (); @@ -1812,7 +1790,7 @@ write_config_integrated(void) } for (i = 0; i < array_size(vtysh_client); i++) - vtysh_client_config (&vtysh_client[i], line); + vtysh_client_execute (&vtysh_client[i], line, NULL); vtysh_config_dump (fp); @@ -2485,3 +2463,10 @@ vtysh_init_vty (void) install_element (CONFIG_NODE, &no_vtysh_enable_password_cmd); } + +void +vty_buf_sz_init(void) +{ + /* Allow enough room for buffer to read more than a few pages from socket. */ + vtysh_buf_sz = 5 * getpagesize() + 1;; +} diff --git a/vtysh/vtysh.h b/vtysh/vtysh.h index 1681a71..b712ca5 100644 --- a/vtysh/vtysh.h +++ b/vtysh/vtysh.h @@ -38,6 +38,15 @@ /* vtysh local configuration file. */ #define VTYSH_DEFAULT_CONFIG "vtysh.conf" +/* VTY shell client structure. */ +struct vtysh_client +{ + int fd; + const char *name; + int flag; + const char *path; +}; + void vtysh_init_vty (void); void vtysh_init_cmd (void); extern int vtysh_connect_all (const char *optional_daemon_name); @@ -46,6 +55,7 @@ void vtysh_user_init (void); int vtysh_execute (const char *); int vtysh_execute_no_pager (const char *); +int vtysh_client_execute (struct vtysh_client *vclient, const char *line, FILE *fp); char *vtysh_prompt (void); @@ -63,9 +73,13 @@ void vtysh_config_init (void); void vtysh_pager_init (void); +void vty_buf_sz_init(void); + /* Child process execution flag. */ extern int execute_flag; extern struct vty *vty; +extern int vtysh_buf_sz; + #endif /* VTYSH_H */ diff --git a/vtysh/vtysh_main.c b/vtysh/vtysh_main.c index aa7d021..df790c6 100644 --- a/vtysh/vtysh_main.c +++ b/vtysh/vtysh_main.c @@ -44,9 +44,6 @@ char *progname; char config_default[] = SYSCONFDIR VTYSH_DEFAULT_CONFIG; char history_file[MAXPATHLEN]; -/* Flag for indicate executing child command. */ -int execute_flag = 0; - /* For sigsetjmp() & siglongjmp(). */ static sigjmp_buf jmpbuf; @@ -289,6 +286,7 @@ main (int argc, char **argv, char **env) vtysh_signal_init (); /* Make vty structure and register commands. */ + vty_buf_sz_init (); vtysh_init_vty (); vtysh_init_cmd (); vtysh_user_init (); -- 2.5.3 _______________________________________________ Quagga-dev mailing list [email protected]<mailto:[email protected]> https://lists.quagga.net/mailman/listinfo/quagga-dev
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
