Re: [PATCH 11/12] kernel-shark: In model, handle the case when all bins are empty

[Slavomir Kaslev <slavomir.kaslev@gmail.com>]

On Thu, Mar 14, 2019 at 5:11 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> ksmodel_set_bin_counts() should not crash in the case when all
> bins of the model, except the Upper Overflow bin, are empty.
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/libkshark-model.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
> index af3440b..29676c7 100644
> --- a/kernel-shark/src/libkshark-model.c
> +++ b/kernel-shark/src/libkshark-model.c
> @@ -290,7 +290,7 @@ static void ksmodel_set_next_bin_edge(struct kshark_trace_histo *histo,
>  static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
>  {
>         int i = 0, prev_not_empty;
> -       ssize_t count_tmp;
> +       ssize_t count_tmp = 0;
>
>         histo->tot_count = 0;
>         memset(&histo->bin_count[0], 0,
> @@ -303,7 +303,7 @@ static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
>                 prev_not_empty = LOB(histo);
>         } else {
>                 /* Loop till the first non-empty bin. */
> -               while (histo->map[i] < 0) {
> +               while (histo->map[i] < 0 && i < histo->n_bins) {

I would suggest switching the order of those checks

    while (i < histo->n_bins && histo->map[i] < 0) {

It's safe as is since hist->map has histo->n_bins+2 entries but
checking bounds before dereferencing is more idiomatic.

-- Slavi

>                         ++i;
>                 }
>
> @@ -316,7 +316,8 @@ static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
>          */
>         for (; i < histo->n_bins; ++i) {
>                 if (histo->map[i] != KS_EMPTY_BIN) {
> -                       /* The current bin is not empty, take its data row and
> +                       /*
> +                        * The current bin is not empty, take its data row and
>                          * subtract it from the data row of the previous not
>                          * empty bin, which will give us the number of data
>                          * rows in the "prev_not_empty" bin.
> @@ -358,7 +359,7 @@ static void ksmodel_set_bin_counts(struct kshark_trace_histo *histo)
>          * We will do a sanity check. The number of data rows in the last not
>          * empty bin must be greater than zero.
>          */
> -       assert(count_tmp > 0);
> +       assert(count_tmp >= 0);
>         histo->tot_count += histo->bin_count[prev_not_empty] = count_tmp;
>  }
>
> --
> 2.19.1
>


-- 
Slavomir Kaslev