Hi Donald, Sorry about that. I’ve sent the patch from wrong branch.
-Gautam From: Donald Sharp <[email protected]<mailto:[email protected]>> Date: Tuesday, September 29, 2015 at 12:44 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 13206] [PATCH] Fix vtysh hang, when client's config size is close to vtysh's buffer size Gautam - How does this even compile what you have sent? You renamed vtysh_client_config to vtysh_client_execute but didn't modify the calls of vtysh_client_config to use it. In addition you have introduced compiler warnings that cause the build to fail with -werror turned on: vtysh.c: In function ‘vtysh_exit_ripd_only’: vtysh.c:236:5: error: passing argument 2 of ‘vtysh_client_execute’ discards ‘const’ qualifier from pointer target type [-Werror] vtysh.c:92:1: note: expected ‘char *’ but argument is of type ‘const char *’ vtysh.c: In function ‘vtysh_execute_func’: vtysh.c:357:10: error: passing argument 2 of ‘vtysh_client_execute’ discards ‘const’ qualifier from pointer target type [-Werror] vtysh.c:92:1: note: expected ‘char *’ but argument is of type ‘const char *’ vtysh.c:398:10: error: passing argument 2 of ‘vtysh_client_execute’ discards ‘const’ qualifier from pointer target type [-Werror] vtysh.c:92:1: note: expected ‘char *’ but argument is of type ‘const char *’ vtysh.c: In function ‘vtysh_write_terminal’: vtysh.c:1720:5: error: implicit declaration of function ‘vtysh_client_config’ [-Werror=implicit-function-declaration] vtysh.c: In function ‘vtysh_write_terminal_daemon’: vtysh.c:1766:3: error: passing argument 2 of ‘vtysh_client_execute’ discards ‘const’ qualifier from pointer target type [-Werror] vtysh.c:92:1: note: expected ‘char *’ but argument is of type ‘const char *’ cc1: all warnings being treated as errors make: *** [vtysh.o] Error 1 You Added a new FILE *fp to vtysh_client_execute but all invocations of vtysh_client_config set it to NULL? Why? Look inline for a few more comments. On Tue, Sep 29, 2015 at 2:27 PM, Gautam Kumar <[email protected]<mailto:[email protected]>> wrote: --- @@ -100,6 +100,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; @@ -145,112 +146,89 @@ 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 The only exit case of the if () block above is return. There is no need for this else, please remove it. donald + { + /* 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_config (&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_config (&vtysh_client[i], line, NULL); vtysh_config_dump (fp); -- 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
