Hi David, Please see inline,
On Mon, Mar 14, 2016 at 6:59 AM, David Lamparter < [email protected]> wrote: > On Sun, Mar 13, 2016 at 06:29:22PM -0700, Avneesh Sachdev wrote: > > On Sun, Mar 13, 2016 at 1:04 PM, Donald Sharp < > [email protected]> > > wrote: > > > Can you expand a bit on why adding the ability to call random functions > > > from the cli is better than just adding a 'test XXX' function as > needed? > > > > The functionality is equivalent. The benefits of the 'invoke' approach > are: > > > > - It is slightly easier to add a test -- just write a function enclosed > > inside an ifdef DEV_BUILD. The cli plumbing (help text, installation of > the > > command node etc.) is not necessary. > > True. > > > - In a similar vein, when writing tests you can restart a protocol > daemon > > and run new tests without having to update vtysh. > > You can do that as-is, if you invoke the function through the telnet > interface. > > > - Test functions can be located in any file, not just in the files that > > are parsed for cli commands. > > Same here, if you go through telnet it doesn't matter which file a > function is in. > > (Yes, I think using telnet is an acceptable tradeoff for debugging. We > also have code to open a VTY on stdio, but there's glue missing to have > that as commandline argument on all daemons.) > It looks like we agree that: - test functions should be easier to write. - test code should not be part of a production build (whether via conditional compilation or separate binaries). Given the above, why not let the user use the familiar vtysh interface to invoke tests? > - Non-development builds are smaller because test code is compiled out. > > You still need to wrap the test code in #ifdef DEV_BUILD - the test > functions won't automatically disappear without that. > > > - If (when?) quagga switches to a better cli, we won't have to plumb > each > > test function for the new cli -- just the one command (invoke). > > You're assuming the CLI will be replaced by another CLI ;) - can you > attend tomorrow's monthly Quagga meeting? I'm giving a presentation > afterwards on the API work we've been doing. > Sure. > > - The 'invoke' handler also prints out some useful meta information, > such > > as CPU time taken. > > > > - AFAICT, 'test' commands are present in shipped code by default. This > > does not seem like a good idea for unit tests. It is certainly possible > to > > omit the commands from a build, but this requires additional touches to > the > > code, and an understanding of how the cli works. > > This, again, is an argument for the DEV_BUILD macro, not the "invoke" > function. > > My opinion on this is: > - the DEV_BUILD macro is a bad idea. Test code should always be > compiled, or it will fall into bit-rot. We have that problem with the > code in tests/ already, people don't update it when changing some > structure and it breaks. Bit rot should not be a problem if a CI system runs the tests. There is a different argument for separating out the test code, i.e. > not having it in the installed binaries. The programs in tests/ have > that automatically since they're independent programs. For extra code > bits in the daemons, the nicest way would be to have them in separate > files which for example get linked into a second binary (like > "zebra/testzebra"). This doesn't work for anything "static" in source > files, unfortunately... not sure what the best way there would be. > A CI system can do multiple out-of-source builds: one for production and another for testing. Thereafter, the same automation that is used to run quagga today can be used create various scenarios and invoke test functions. This requires no changes to the makefiles/build system or the quagga automation. Cheers, Avneesh > - I agree that adding test functions should be easier, but I would > strongly prefer doing this another way. Some macro magic to make it > easier to add a test DEFUN, maybe? > > Cheers, > > > -David > > > In summary, I think that 'invoke' makes it slightly easier to write a > test. > > Possibly enough to encourage people to write more tests :-). > > > > Additionally how do you plan to allow the end user to pass in useful data > > > to functions if the function you want to call takes a complex data > > > structure? > > > > > > > The expectation is that most functions will be unit tests that require no > > inputs or few inputs. > > > > Since a function runs in the regular context of a daemon, most setup can > be > > performed via config commands (for example, a static route). > > > > If a test works on a fixed set of inputs, these can be compiled in > > (conditionally, just like the test) as static C structures. > > > > If user-supplied inputs are really necessary for some reason, they would > be > > passed in through cli arguments. Complex inputs can be saved to a file, > > with the file name passed to the test via an argument. Presumably one > would > > do something similar with a 'test xxx' command. > > > > Thanks, > > Avneesh > > > > I'm pretty dubious about adding a cause a crash button even for developer > > > builds, especially since adding a test XXX function is an easy task. > > > > > > donald > > > > > > On Fri, Mar 11, 2016 at 3:21 PM, Avneesh Sachdev <[email protected]> > > > wrote: > > > > > >> This functionality is only compiled into a developer build, It allows > > >> test functions to be invoked by name from the vtysh as follows. > > >> > > >> Changes: > > >> > > >> * vtysh/vtysh.c > > >> > > >> Handle the 'invoke <component> function <function-name> [args]' > > >> command. > > >> Forward it the client with the given name. > > >> > > >> * lib/vty_invoke.c > > >> > > >> Code that handles an 'invoke function' command inside a vtysh > > >> client process (e;g., a routing protocol daemon). It invokes the > > >> named function, and prints out some stats about the execution > > >> time. > > >> > > >> * lib/thread.h > > >> > > >> Extern timeval_elapsed(). > > >> > > >> * lib/vty.[ch] > > >> > > >> - vty_init(): > > >> > > >> Install element for 'invoke' command handler. This sets up the > > >> handler that runs inside of a vtysh client. > > >> > > >> * lib/Makefile.am > > >> > > >> Also compile vty_invoke.c. > > >> > > >> * configure.ac > > >> > > >> Include '-rdynamic -ldl' in LDFLAGS for a development build. These > > >> options allow dlsym() to work for functions inside a quagga > > >> program. > > >> > > >> Signed-off-by: Avneesh Sachdev <[email protected]> > > >> --- > > >> configure.ac | 4 ++ > > >> lib/Makefile.am | 2 +- > > >> lib/thread.h | 3 ++ > > >> lib/vty.c | 4 ++ > > >> lib/vty_invoke.c | 120 > > >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > >> lib/vty_invoke.h | 30 ++++++++++++++ > > >> vtysh/vtysh.c | 78 ++++++++++++++++++++++++++++++++++++ > > >> 7 files changed, 240 insertions(+), 1 deletion(-) > > >> create mode 100644 lib/vty_invoke.c > > >> create mode 100644 lib/vty_invoke.h > > >> > > >> diff --git a/configure.ac b/configure.ac > > >> index ae2e527..60a9118 100755 > > >> --- a/configure.ac > > >> +++ b/configure.ac > > >> @@ -331,6 +331,10 @@ fi > > >> > > >> if test "x${enable_dev_build}" = "xyes"; then > > >> AC_DEFINE(DEV_BUILD,,Build for development) > > >> + > > >> + # Link with -ldl so that the developer build can use dlsym and > > >> + # company. > > >> + LDFLAGS="${LDFLAGS} -rdynamic -ldl" > > >> fi > > >> AM_CONDITIONAL([DEV_BUILD], [test "x$enable_dev_build" = "xyes"]) > > >> > > >> diff --git a/lib/Makefile.am b/lib/Makefile.am > > >> index ac51fc6..17dc575 100644 > > >> --- a/lib/Makefile.am > > >> +++ b/lib/Makefile.am > > >> @@ -13,7 +13,7 @@ libzebra_la_SOURCES = \ > > >> sockunion.c prefix.c thread.c if.c memory.c buffer.c table.c > > >> hash.c \ > > >> filter.c routemap.c distribute.c stream.c str.c log.c plist.c > \ > > >> zclient.c sockopt.c smux.c agentx.c snmp.c md5.c if_rmap.c > > >> keychain.c privs.c \ > > >> - sigevent.c pqueue.c jhash.c memtypes.c workqueue.c vrf.c > > >> + sigevent.c pqueue.c jhash.c memtypes.c workqueue.c vrf.c > > >> vty_invoke.c > > >> > > >> BUILT_SOURCES = memtypes.h route_types.h gitversion.h > > >> > > >> diff --git a/lib/thread.h b/lib/thread.h > > >> index 5bc756c..215ac5d 100644 > > >> --- a/lib/thread.h > > >> +++ b/lib/thread.h > > >> @@ -229,6 +229,9 @@ extern time_t quagga_time (time_t *); > > >> extern unsigned long thread_consumed_time(RUSAGE_T *after, RUSAGE_T > > >> *before, > > >> unsigned long > > >> *cpu_time_elapsed); > > >> > > >> +/* Returns elapsed time in microseconds */ > > >> +extern unsigned long timeval_elapsed (struct timeval a, struct > timeval > > >> b); > > >> + > > >> /* Global variable containing a recent result from gettimeofday. > This > > >> can > > >> be used instead of calling gettimeofday if a recent value is > > >> sufficient. > > >> This is guaranteed to be refreshed before a thread is called. */ > > >> diff --git a/lib/vty.c b/lib/vty.c > > >> index 8befcb0..657ed97 100644 > > >> --- a/lib/vty.c > > >> +++ b/lib/vty.c > > >> @@ -40,6 +40,8 @@ > > >> #include <arpa/telnet.h> > > >> #include <termios.h> > > >> > > >> +#include "vty_invoke.h" > > >> + > > >> /* Vty events */ > > >> enum event > > >> { > > >> @@ -3073,6 +3075,8 @@ vty_init (struct thread_master *master_thread) > > >> install_element (VTY_NODE, &vty_ipv6_access_class_cmd); > > >> install_element (VTY_NODE, &no_vty_ipv6_access_class_cmd); > > >> #endif /* HAVE_IPV6 */ > > >> + > > >> + vty_invoke_init (); > > >> } > > >> > > >> void > > >> diff --git a/lib/vty_invoke.c b/lib/vty_invoke.c > > >> new file mode 100644 > > >> index 0000000..3c43568 > > >> --- /dev/null > > >> +++ b/lib/vty_invoke.c > > >> @@ -0,0 +1,120 @@ > > >> +/* > > >> + * vty_invoke.c > > >> + * > > >> + * @copyright Copyright (C) 2016 Sproute Networks, Inc. > > >> + * > > >> + * @author Avneesh Sachdev <[email protected]> > > >> + * > > >> + * This file is part of GNU Zebra. > > >> + * > > >> + * GNU Zebra is free software; you can redistribute it and/or modify > it > > >> + * under the terms of the GNU General Public License as published by > the > > >> + * Free Software Foundation; either version 2, or (at your option) > any > > >> + * later version. > > >> + * > > >> + * GNU Zebra is distributed in the hope that it will be useful, but > > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > >> + * General Public License for more details. > > >> + * > > >> + * You should have received a copy of the GNU General Public License > > >> + * along with GNU Zebra; see the file COPYING. If not, write to the > Free > > >> + * Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA > > >> + * 02111-1307, USA. > > >> + */ > > >> + > > >> +/* > > >> + * Support for invoking functions by name from a vty shell. > > >> + * > > >> + * This code is only compiled into a developer build, and allows > > >> + * functions that follow certain signatures to be invoked by name > from > > >> + * the vtysh. > > >> + */ > > >> + > > >> +#include <zebra.h> > > >> + > > >> +#include <dlfcn.h> > > >> + > > >> +#include "vector.h" > > >> +#include "vty.h" > > >> +#include "command.h" > > >> + > > >> +#include "vty_invoke.h" > > >> + > > >> +typedef int (*vty_invoke_func_t) (int argc, const char **argv); > > >> + > > >> +#ifdef DEV_BUILD > > >> + > > >> +DEFUN (invoke_function, > > >> + invoke_function_cmd, > > >> + "invoke function NAME [ARG1] [ARG2] [ARG3] [ARG4] [ARG5] > [ARG6]", > > >> + "Invoke\n" > > >> + "Invoke a function\n" > > >> + "Name of function to invoke\n" > > >> + "First argument\n" > > >> + "Second argument\n" > > >> + "Third argument\n" > > >> + "Fourth argument\n" > > >> + "Fifth argument\n" > > >> + "Sixth argument\n") > > >> +{ > > >> + int i; > > >> + const char *func_name; > > >> + vty_invoke_func_t func; > > >> + RUSAGE_T before, after; > > >> + ulong elapsed; > > >> + > > >> + assert (argc >= 1); > > >> + > > >> + func_name = argv[0]; > > >> + > > >> + func = dlsym (NULL, func_name); > > >> + if (!func) > > >> + { > > >> + vty_out (vty, "Can't find function %s", func_name); > > >> + return CMD_WARNING; > > >> + } > > >> + > > >> + vty_out (vty, "Invoking %s(", func_name); > > >> + > > >> + for (i = 1; i < argc; i++) > > >> + { > > >> + vty_out (vty, "%s%s", i == 1 ? "" : ", ", argv[i]); > > >> + } > > >> + vty_out (vty, ")\n"); > > >> + > > >> + GETRUSAGE (&before); > > >> + i = func (argc - 1, argv + 1); > > >> + GETRUSAGE (&after); > > >> + > > >> + vty_out (vty, "Return value: %d\n\n", i); > > >> + > > >> + elapsed = timeval_elapsed (after.real, before.real); > > >> + vty_out (vty, "%-20s %9lu ms\n", "Real time:", elapsed / 1000); > > >> + > > >> +#ifdef HAVE_RUSAGE > > >> + { > > >> + elapsed = timeval_elapsed (after.cpu.ru_utime, > before.cpu.ru_utime); > > >> + > > >> + vty_out (vty, "%-20s %9lu ms\n", "User time:", elapsed / 1000); > > >> + > > >> + elapsed = timeval_elapsed (after.cpu.ru_stime, > before.cpu.ru_stime); > > >> + vty_out (vty, "%-20s %9lu ms\n", "System time:", elapsed / 1000); > > >> + } > > >> +#endif > > >> + > > >> + return CMD_SUCCESS; > > >> +} > > >> + > > >> +#endif /* DEV_BUILD */ > > >> + > > >> +/* > > >> + * vty_invoke_init > > >> + */ > > >> +void > > >> +vty_invoke_init (void) > > >> +{ > > >> +#ifdef DEV_BUILD > > >> + install_element (ENABLE_NODE, &invoke_function_cmd); > > >> +#endif > > >> +} > > >> diff --git a/lib/vty_invoke.h b/lib/vty_invoke.h > > >> new file mode 100644 > > >> index 0000000..a2364a2 > > >> --- /dev/null > > >> +++ b/lib/vty_invoke.h > > >> @@ -0,0 +1,30 @@ > > >> +/* > > >> + * vty_invoke.h > > >> + * > > >> + * @copyright Copyright (C) 2016 Sproute Networks, Inc. > > >> + * > > >> + * @author Avneesh Sachdev <[email protected]> > > >> + * > > >> + * This file is part of GNU Zebra. > > >> + * > > >> + * GNU Zebra is free software; you can redistribute it and/or modify > it > > >> + * under the terms of the GNU General Public License as published by > the > > >> + * Free Software Foundation; either version 2, or (at your option) > any > > >> + * later version. > > >> + * > > >> + * GNU Zebra is distributed in the hope that it will be useful, but > > >> + * WITHOUT ANY WARRANTY; without even the implied warranty of > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > >> + * General Public License for more details. > > >> + * > > >> + * You should have received a copy of the GNU General Public License > > >> + * along with GNU Zebra; see the file COPYING. If not, write to the > Free > > >> + * Software Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA > > >> + * 02111-1307, USA. > > >> + */ > > >> +#ifndef _VTY_INVOKE_H > > >> +#define _VTY_INVOKE_H > > >> + > > >> +extern void vty_invoke_init (void); > > >> + > > >> +#endif /* _VTY_INVOKE_H */ > > >> diff --git a/vtysh/vtysh.c b/vtysh/vtysh.c > > >> index b55c671..b0f96d9 100644 > > >> --- a/vtysh/vtysh.c > > >> +++ b/vtysh/vtysh.c > > >> @@ -2149,6 +2149,80 @@ DEFUN (vtysh_start_zsh, > > >> return CMD_SUCCESS; > > >> } > > >> > > >> +#ifdef DEV_BUILD > > >> + > > >> +/* > > >> + * 'invoke' command > > >> + * > > >> + * Command that allows user to invoke functions from the cli shell in > > >> + * development build. > > >> + */ > > >> +DEFUN (invoke_function, > > >> + invoke_function_cmd, > > >> + "invoke COMPONENT function NAME [ARG1] [ARG2] [ARG3] [ARG4] > > >> [ARG5] [ARG6]", > > >> + "Invoke\n" > > >> + "Component name in which to invoke function\n" > > >> + "Invoke a function\n" > > >> + "Name of function to invoke\n" > > >> + "First argument\n" > > >> + "Second argument\n" > > >> + "Third argument\n" > > >> + "Fourth argument\n" > > >> + "Fifth argument\n" > > >> + "Sixth argument\n") > > >> +{ > > >> + unsigned int u; > > >> + char line[1000]; > > >> + char *cur, *end; > > >> + int ret, i; > > >> + const char *component_name; > > >> + struct vtysh_client *client; > > >> + > > >> + cur = line; > > >> + end = cur + sizeof (line); > > >> + > > >> + cur += snprintf (cur, end - cur, "invoke function "); > > >> + > > >> + /* > > >> + * Build command string, skipping over the component name. > > >> + */ > > >> + for (i = 1; i < argc; i++) > > >> + { > > >> + ret = snprintf (cur, end - cur, "%s ", argv[i]); > > >> + if (ret < 0 || ret == (end - cur)) > > >> + { > > >> + vty_out (vty, "Command string is too long"); > > >> + return CMD_WARNING; > > >> + } > > >> + cur += ret; > > >> + } > > >> + > > >> + component_name = argv[0]; > > >> + > > >> + for (u = 0; u < array_size (vtysh_client); u++) > > >> + { > > >> + client = &vtysh_client[u]; > > >> + if (strcmp (client->name, component_name)) > > >> + { > > >> + continue; > > >> + } > > >> + if (client->fd < 0) > > >> + { > > >> + fprintf (stdout, "Not connected to component %s\n", > > >> component_name); > > >> + return CMD_WARNING; > > >> + } > > >> + > > >> + ret = vtysh_client_execute (client, line, stdout); > > >> + fprintf (stdout, "\n"); > > >> + return ret; > > >> + } > > >> + > > >> + fprintf (stdout, "Could not find component %s\n", component_name); > > >> + return CMD_WARNING; > > >> +} > > >> + > > >> +#endif /* DEV_BUILD */ > > >> + > > >> static void > > >> vtysh_install_default (enum node_type node) > > >> { > > >> @@ -2530,4 +2604,8 @@ vtysh_init_vty (void) > > >> install_element (CONFIG_NODE, &vtysh_enable_password_text_cmd); > > >> install_element (CONFIG_NODE, &no_vtysh_enable_password_cmd); > > >> > > >> +#ifdef DEV_BUILD > > >> + install_element (ENABLE_NODE, &invoke_function_cmd); > > >> +#endif > > >> + > > >> } > > >> -- > > >> 1.9.1 > > >> > > >> > > >> _______________________________________________ > > >> Quagga-dev mailing list > > >> [email protected] > > >> https://lists.quagga.net/mailman/listinfo/quagga-dev > > >> > > > > > > > > > _______________________________________________ > > Quagga-dev mailing list > > [email protected] > > https://lists.quagga.net/mailman/listinfo/quagga-dev > >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
