Thank you!
William
On Wed, Jan 20, 2016 at 3:54 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Tue, Jan 12, 2016 at 02:24:53PM -0800, William Tu wrote:
> > This patch forces appending "parent_process_name(PID)" when invoking
> > ovs-vsctl, in order to assist debugging. The patch is for Linux only.
> > For example:
> > User adds br0 by "ovs-vsctl add-br0", the log shows:
> > "ovs-vsctl (invoked by base(1528)): ovs-vsctl add-br br0"
> >
> > Signed-off-by: William Tu <u9012...@gmail.com>
>
> Thanks for the new version.
>
> I applied the following incremental to it, for mostly stylistic changes:
>
> * Extra blank line.
>
> * More ifdefs than needed.
>
> * Unused variable 'total_size'.
>
> * VLOG_ERR indicates an error that can't be recovered from, so downgrade
> to VLOG_WARN.
>
> * VLOG argument doesn't need trailing \n
>
> * Error log message should include what the error was.
>
> * fread() is overkill for reading a character, that's what getc() is
> for.
>
> * We customarily use for (;;) for an infinite loop.
>
> * In testing I found "(invoked by /bin/bash (pid 20216))" easier to read
> and clearer than "(invoked by /bin/bash(20216))".
>
> * ds_steal_cstr() doesn't need to be followed by destroying the string
> (though it does no harm).
>
> I also changed the log message first line to start with a capital letter
> and end with a period, as is my preference.
>
> With those changes, I applied this to master.
>
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 801113a..c5a28e0 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira,
> Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -2468,53 +2468,43 @@ run_prerequisites(struct ctl_command *commands,
> size_t n_commands,
> }
> }
>
> -#ifdef __linux__
> -
> -static char*
> +static char *
> vsctl_parent_process_info(void)
> {
> +#ifdef __linux__
> pid_t parent_pid;
> char *procfile;
> - char *msg_ppid;
> - char *msg;
> struct ds s;
> FILE *f;
> - size_t total_size = 0;
>
> parent_pid = getppid();
> procfile = xasprintf("/proc/%d/cmdline", parent_pid);
>
> f = fopen(procfile, "r");
> if (!f) {
> - VLOG_ERR("can not open file: %s\n", procfile);
> + VLOG_WARN("%s: open failed (%s)", procfile, ovs_strerror(errno));
> free(procfile);
> return NULL;
> }
> + free(procfile);
>
> ds_init(&s);
> -
> - while (1) {
> - size_t size;
> - char c;
> - size = fread(&c, 1, 1, f);
> - if (c == '\0' || size < 1) {
> + for (;;) {
> + int c = getc(f);
> + if (!c || c == EOF) {
> break;
> }
> ds_put_char(&s, c);
> - total_size += size;
> }
> fclose(f);
>
> - msg_ppid = xasprintf("(%d)", parent_pid);
> - ds_put_cstr(&s, msg_ppid);
> + ds_put_format(&s, " (pid %d)", parent_pid);
>
> - msg = ds_steal_cstr(&s);
> - free(procfile);
> - free(msg_ppid);
> - ds_destroy(&s);
> - return msg;
> -}
> + return ds_steal_cstr(&s);
> +#else
> + return NULL;
> #endif
> +}
>
> static void
> do_vsctl(const char *args, struct ctl_command *commands, size_t
> n_commands,
> @@ -2536,19 +2526,14 @@ do_vsctl(const char *args, struct ctl_command
> *commands, size_t n_commands,
> ovsdb_idl_txn_set_dry_run(txn);
> }
>
> -#ifdef __linux__
> ppid_info = vsctl_parent_process_info();
> if (ppid_info) {
> ovsdb_idl_txn_add_comment(txn, "ovs-vsctl (invoked by %s): %s",
> ppid_info, args);
> free(ppid_info);
> - }
> - else {
> -#endif
> + } else {
> ovsdb_idl_txn_add_comment(txn, "ovs-vsctl: %s", args);
> -#ifdef __linux__
> }
> -#endif
>
> ovs = ovsrec_open_vswitch_first(idl);
> if (!ovs) {
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
