Mailing List Archive: [PATCH] vtysh: make warnings about node installs a non-default compile-time option

[PATCH] vtysh: make warnings about node installs a non-default compile-time option

Oct 29, 2016, 4:17 PM

Post #1 of 5 (992 views)

commit 92193665 warns about duplicate node installs. This should only be
enabled for dev builds beacuse it causes unwanted noise on production
builds. I've enclosed the relevant commands in #ifdef DEV_BUILD, which may
or may not be appropriate. If there's a more appropriate compile-time
option available, that could be used instead.

---
lib/command.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/lib/command.c b/lib/command.c
index ab46fc4..14da6f7 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -636,6 +636,7 @@ install_element (enum node_type ntype, struct cmd_element *cmd)
exit (1);
}

+#ifdef DEV_BUILD
if (hash_lookup (cnode->cmd_hash, cmd) != NULL)
{
fprintf (stderr,
@@ -645,6 +646,7 @@ install_element (enum node_type ntype, struct cmd_element *cmd)
}

assert (hash_get (cnode->cmd_hash, cmd, hash_alloc_intern));
+#endif

vector_set (cnode->cmd_vector, cmd);
if (cmd->tokens == NULL)
--
2.10.1

_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: [PATCH] vtysh: make warnings about node installs a non-default compile-time option [ In reply to ]

jafar at atcorp

Dec 6, 2016, 4:52 PM

Post #2 of 5 (941 views)

Permalink

I tested this patch with the latest sources and got a weird situation
where vtysh shell doesn't exit. "end", "exit", "quit" failed to make it
do so. I moved the #ifdef DEV_BUILD inside the if statement to cover
only the fprintf statement and that fixed it. The reason I think is that
the ifdef prevents an early bailout (return) that takes place in the
normal case. HEre is the code looks:

if (hash_lookup (cnode->cmd_hash, cmd) != NULL)
{
fprintf (stderr,
"Multiple command installs to node %d of command:\n%s\n",
ntype, cmd->string);
return;
}

assert (hash_get (cnode->cmd_hash, cmd, hash_alloc_intern));

The ifdef should only be around the fprintf statement. I have to NACK
this patch.

Regards,
Jafar

On 10/29/2016 6:17 PM, Nick Hilliard wrote:
> commit 92193665 warns about duplicate node installs. This should only be
> enabled for dev builds beacuse it causes unwanted noise on production
> builds. I've enclosed the relevant commands in #ifdef DEV_BUILD, which may
> or may not be appropriate. If there's a more appropriate compile-time
> option available, that could be used instead.
>
> ---
> lib/command.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/command.c b/lib/command.c
> index ab46fc4..14da6f7 100644
> --- a/lib/command.c
> +++ b/lib/command.c
> @@ -636,6 +636,7 @@ install_element (enum node_type ntype, struct cmd_element *cmd)
> exit (1);
> }
>
> +#ifdef DEV_BUILD
> if (hash_lookup (cnode->cmd_hash, cmd) != NULL)
> {
> fprintf (stderr,
> @@ -645,6 +646,7 @@ install_element (enum node_type ntype, struct cmd_element *cmd)
> }
>
> assert (hash_get (cnode->cmd_hash, cmd, hash_alloc_intern));
> +#endif
>
> vector_set (cnode->cmd_vector, cmd);
> if (cmd->tokens == NULL)

_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: [PATCH] vtysh: make warnings about node installs a non-default compile-time option [ In reply to ]

nick at foobar

Dec 7, 2016, 5:27 AM

Post #3 of 5 (940 views)

Permalink

Jafar Al-Gharaibeh wrote:
> I tested this patch with the latest sources and got a weird situation
> where vtysh shell doesn't exit. "end", "exit", "quit" failed to make it
> do so. I moved the #ifdef DEV_BUILD inside the if statement to cover
> only the fprintf statement and that fixed it. The reason I think is that
> the ifdef prevents an early bailout (return) that takes place in the
> normal case. HEre is the code looks:

good catch - I think you're right.

Nick

_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: [PATCH] vtysh: make warnings about node installs a non-default compile-time option [ In reply to ]

nick at foobar

Dec 27, 2016, 2:51 PM

Post #4 of 5 (905 views)

Permalink

On Tue, Dec 06, 2016 at 06:52:14PM -0600, Jafar Al-Gharaibeh wrote:
> I tested this patch with the latest sources and got a weird situation
> where vtysh shell doesn't exit. "end", "exit", "quit" failed to make it
> do so. I moved the #ifdef DEV_BUILD inside the if statement to cover
> only the fprintf statement and that fixed it. The reason I think is that
> the ifdef prevents an early bailout (return) that takes place in the
> normal case. HEre is the code looks:

fix for patchwork:

diff --git a/lib/command.c b/lib/command.c
index ab46fc4a..bff86e58 100644
--- a/lib/command.c
+++ b/lib/command.c
@@ -638,9 +638,11 @@ install_element (enum node_type ntype, struct cmd_element *cmd)

if (hash_lookup (cnode->cmd_hash, cmd) != NULL)
{
+#ifdef DEV_BUILD
fprintf (stderr,
"Multiple command installs to node %d of command:\n%s\n",
ntype, cmd->string);
+#endif
return;
}

_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

Re: [PATCH] vtysh: make warnings about node installs a non-default compile-time option [ In reply to ]

paul at jakma

Jan 4, 2017, 2:25 PM

Post #5 of 5 (895 views)

Permalink

On Tue, 27 Dec 2016, Nick Hilliard wrote:

> fix for patchwork:

Other possibility (so long as this bit of debug is kept) would be to
check for some property of vtysh (e.g. argv[0]).

regards,
--
Paul Jakma | paul@jakma.org | @pjakma | Key ID: 0xD86BF79464A2FF6A
Fortune:
Detroit is Cleveland without the glitter.

_______________________________________________
Quagga-dev mailing list
Quagga-dev@lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev