[ovs-dev] [RFC 2/3] coverage: Add command for reading counter value

[Jakub Sitnicki]

On Mon, 21 May 2018 16:22:07 -0700
Han Zhou <zhouhan at gmail.com> wrote:

> On Fri, May 18, 2018 at 9:55 AM, Jakub Sitnicki <jkbs at redhat.com> wrote:
> >
> > Facilitate checking coverage counters from scripts and tests by
> > providing new "coverage/read-count" command that returns the value of a
> > given counter.
> >
> > Same could be achieved by scraping the output of "coverage/show" command
> > but the difficulties there are that output is in human readable format
> > and zero-value counters are not included.
> >
> > Signed-off-by: Jakub Sitnicki <jkbs at redhat.com>
> > ---
> >  lib/coverage.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/lib/coverage.c b/lib/coverage.c
> > index 6cef82614..4aa283be8 100644
> > --- a/lib/coverage.c
> > +++ b/lib/coverage.c
> > @@ -44,6 +44,8 @@ static unsigned int idx_count = 0;
> >  static void coverage_read(struct svec *);
> >  static unsigned int coverage_array_sum(const unsigned int *arr,
> >                                         const unsigned int len);
> > +static bool coverage_get_count(const char *counter_name,
> > +                               unsigned long long int *count);
> >
> >  /* Registers a coverage counter with the coverage core */
> >  void
> > @@ -72,11 +74,32 @@ coverage_unixctl_show(struct unixctl_conn *conn, int  
> argc OVS_UNUSED,
> >      svec_destroy(&lines);
> >  }
> >
> > +static void
> > +coverage_unixctl_read_count(struct unixctl_conn *conn, int argc  
> OVS_UNUSED,
> > +                            const char *argv[], void *aux OVS_UNUSED)
> > +{
> > +    unsigned long long count;
> > +    char *reply;
> > +    bool ok;
> > +
> > +    ok = coverage_get_count(argv[1], &count);
> > +    if (!ok) {
> > +        unixctl_command_reply_error(conn, "No such counter");
> > +        return;
> > +    }
> > +
> > +    reply = xasprintf("%llu\n", count);
> > +    unixctl_command_reply(conn, reply);
> > +    free(reply);
> > +}
> > +
> >  void
> >  coverage_init(void)
> >  {
> >      unixctl_command_register("coverage/show", "", 0, 0,
> >                               coverage_unixctl_show, NULL);
> > +    unixctl_command_register("coverage/read-count", "COUNTER", 1, 1,
> > +                             coverage_unixctl_read_count, NULL);
> >  }
> >
> >  /* Sorts coverage counters in descending order by total, within equal
> > @@ -372,3 +395,21 @@ coverage_array_sum(const unsigned int *arr, const  
> unsigned int len)
> >      ovs_mutex_unlock(&coverage_mutex);
> >      return sum;
> >  }
> > +
> > +static bool
> > +coverage_get_count(const char *counter_name, unsigned long long int  
> *count)
> > +{
> > +    for (size_t i = 0; i < n_coverage_counters; i++) {
> > +        struct coverage_counter *counter = coverage_counters[i];
> > +
> > +        if (!strcmp(counter->name, counter_name)) {
> > +            ovs_mutex_lock(&coverage_mutex);  
> 
> This lock seems unnecessary, since it is protecting a single counter read.
> 
> > +            *count = counter->total;
> > +            ovs_mutex_unlock(&coverage_mutex);
> > +
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > --
> > 2.14.3  
> 
> Better to add some documentation, at least in lib/coverage-unixctl.man for
> this new command.
> 
> (Note: not for this patch, but I just noticed that documentation is missing
> in ovn-controller/ovn-northd for coverage. ovsdb-server and ovs-vswitchd
> shares same documentation by including .so lib/coverage-unixctl.man,
> however, it doesn't apply to the xml based documentation for ovn daemons.
> This should be addressed separately)

Ack, I will make sure to add docs in v1.

Thanks for taking a look at the RFC.

-Jakub

> 
> Thanks,
> Han