Messages in this thread |  | | Date | Tue, 16 Jun 2020 13:34:43 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH v2 13/24] dyndbg: combine flags & mask into a struct, use that |
| |
On Mon 2020-06-15 23:47:26, jim.cromie@gmail.com wrote: > On Mon, Jun 15, 2020 at 9:14 AM Petr Mladek <pmladek@suse.com> wrote: > > > > On Sat 2020-06-13 09:57:27, Jim Cromie wrote: > > > combine flags & mask into a struct, and replace those 2 parameters in > > > 3 functions: ddebug_change, ddebug_parse_flags, ddebug_read_flags, > > > altering the derefs in them accordingly. > > > > > > This simplifies the 3 function sigs, preparing for more changes. > > > We dont yet need mask from ddebug_read_flags, but will soon. > > > --- > > > lib/dynamic_debug.c | 46 +++++++++++++++++++++++---------------------- > > > 1 file changed, 24 insertions(+), 22 deletions(-) > > > > > > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > > > index 93c627e9c094..8dc073a6e8a4 100644 > > > --- a/lib/dynamic_debug.c > > > +++ b/lib/dynamic_debug.c > > > +struct flagsettings { > > > + unsigned int flags; > > > + unsigned int mask; > > > +}; > > > > static int ddebug_change(const struct ddebug_query *query, > > > - unsigned int pflags, unsigned int mask) > > > + struct flagsettings *mods) > > > > > -static int ddebug_read_flags(const char *str, unsigned int *flags) > > > +static int ddebug_read_flags(const char *str, struct flagsettings *f) > > > > > -static int ddebug_parse_flags(const char *str, unsigned int *flagsp, > > > - unsigned int *maskp) > > > +static int ddebug_parse_flags(const char *str, struct flagsettings *mods) > > > > What "mods" stands for, please? > > > modifying_flags, or modifiers. > the original flags & mask bundled together > ie the pfmlt in > echo +pfmlt > control
Honestly, storing flags and mask is a hack that makes the code tricky like hell.
IMHO, it would be much easier to define something like:
struct flags_operation { unsinged int flags; enum flags_operation_type op; }
Where the opration would be:
enum flags_operation_type { DD_FLAGS_SET, /* for '=' */ DD_FLAGS_ADD, /* for '+' */ DD_FLAGS_DEL, /* for '-' */ DD_FLAGS_FILTER_MATCH, DD_FLAGS_FILTER_NON_MATCH, };
Then you could define
struct flags_operation fop_change; struct flags_operation fop_filter;
Then you could do in ddebug_change():
if (fop_filter) { switch(fop_filter->op) { case DD_FLAGS_FILTER_MATCH: if ((dp->flags & fop_filter->flags) != fop_filter->flags) continue; break; case: DD_FLAGS_FILTER_NON_MATCH: if ((dp->flags & fop_filter->flags) continue; break; default: // warn error; } }
switch (fop_change->op) { case DD_FLAGS_SET: dp->flags = fop_change->flags; break; case DD_FLAGS_ADD: dp->flags |= fop_change->flags; break; case DD_FLAGS_DEL: dp->flgas &= ^(fop_change->flgas); break; default: // error; }
It might be more lines. But the bit operations will become straightforward. and the code self-explaining,
> does the above help ? > I could go with modifying_flags > keep in mind the name has to suit all + - = operations > > I'll review all the new names. I recall you didnt like filterflags either, > so I wasnt sufficently clear there either. > Im mulling a better explanation.
The above would make the code manageable. Another question is the user interface.
I still wonder if it is worth it. What is the motivation for this fitlering? Is it requested by users? Or is it just a prerequisite for the user-specific filters?
We need to be really careful. User interface is hard to change or remove later.
Best Regards, Petr
|  |