[ovs-dev] [PATCH v3] ovs-vsctl: add parent process name and ID

William Tu u9012063 at gmail.com
Thu Jan 21 00:14:05 UTC 2016


Thank you!
William

On Wed, Jan 20, 2016 at 3:54 PM, Ben Pfaff <blp at 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 <u9012063 at 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) {
>



More information about the dev mailing list