On Mon 2020-06-15 23:47:26, jim.cro...@gmail.com wrote: > On Mon, Jun 15, 2020 at 9:14 AM Petr Mladek <pmla...@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