Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200613155738.2249399-1-jim.cromie@gmail.com>
 <20200613155738.2249399-8-jim.cromie@gmail.com> <20200615132028.GE31238@alley>
In-Reply-To: <20200615132028.GE31238@alley>
From:   jim.cromie@gmail.com
Date:   Mon, 15 Jun 2020 15:53:45 -0600
Message-ID: <CAJfuBxxdoZ1T9nLi-X2g=zk8poLX-CtRAA=0A-i1KPq2_RhX9Q@mail.gmail.com>
Subject: Re: [PATCH v2 07/24] dyndbg: fix a BUG_ON in ddebug_describe_flags
To:     Petr Mladek <pmladek@suse.com>
Cc:     Jason Baron <jbaron@akamai.com>,
        LKML <linux-kernel@vger.kernel.org>, akpm@linuxfoundation.org,
        Greg KH <gregkh@linuxfoundation.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 15, 2020 at 7:20 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Sat 2020-06-13 09:57:21, Jim Cromie wrote:

> In all patches is missing:
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

right, I missed the -s invoking format-patch, v3 will have them


>
> > ---
> >  lib/dynamic_debug.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 9b2445507988..aaace13d7536 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -87,22 +87,22 @@ static struct { unsigned flag:8; char opt_char; } opt_array[] = {
> >       { _DPRINTK_FLAGS_NONE, '_' },
> >  };
> >
> > +struct flagsbuf { char buf[ARRAY_SIZE(opt_array)+1]; };
>
> This looks too complicated. What about?
>
>         typedef char flags_buf[ARRAY_SIZE(opt_array) + 1];
>     used as
>         flags_buf fb;
>

I used the struct to give type safety.
old code passed a pointer to a string, and hoped it was big enough.
then used a BUG_ON to insist.
passing (the address of the) struct means the contained string is
known big enough.
and the addy is also that of the string itself (member offset 0), no overhead.

>
>         #define FLAGS_BUF_SIZE (ARRAY_SIZE(opt_array) + 1)
>     used as
>         char flags_buf[FLAGS_BUF_SIZE];
>

I never needed that constant, cuz the string is filled once,
in the function just below the struct def, using the same expression (sans +1)

I would/will update the 1-line comment on ddebug_describe_flags
and add another on the struct itself, once I figure out how to say
all of this succinctly and clearly enough.
Im open to suggestions.

thanks
jimc

>
> Best Regards,
> Petr