All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, nelio.laranjeiro@6wind.com
Subject: Re: [PATCH] scripts: add check for net driver names without net prefix
Date: Wed, 06 Jul 2016 11:46:23 +0200	[thread overview]
Message-ID: <22935387.s4eUy1AmSX@xps13> (raw)
In-Reply-To: <20160706092524.GC17404@bricha3-MOBL3>

2016-07-06 10:25, Bruce Richardson:
> On Wed, Jul 06, 2016 at 11:10:48AM +0200, Thomas Monjalon wrote:
> > 2016-07-05 15:40, Bruce Richardson:
> > > +# check headline label for net/ prefix if needed
> > > +bad=$(echo "$headlines" | grep -E --color=always \
> > > +	-e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \
> > > +	| sed 's,^,\t,')
> > > +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n"
> > 
> > This check is definitely a good idea.
> > We need the same thing for crypto.
> > Few nits:
> > - -E is not needed for this regex
> > - "/base:" cases are not handled because of the colon at the end
> > of the regex.
> 
> Good point. Easy enough to fix, I suspect.
> 
> > - patches touching several drivers are not handled. Examples:
> >   "mlx:" -> "net/mlx:"
> >   "drivers/net:" -> "net:"
> > 
> Yes, however, those are in the minority of cases, and we can always add
> special case extra regex's for those - hardcoded like the other regex's in the
> file. I did the pulling directory list so we don't have to update the checks
> every time a new driver was added.

Another issue: I suspect you will have false positives with librte_ring and
librte_vhost.

> > By the way, I am looking at an idea, that Nelio sent me, based on
> > modified files. It can complete this check.
> 
> Great. I'll look forward to seeing that. If it works out well, by all means
> drop this patch. If it's not coming any time soon though, I can do a V2 of
> this patch, but I won't bother right now in the expectation that a better
> solution is coming.

My concern is that the file-based solution will handle only the patches where
every changes are in drivers/ (not config/ or anything else).
I'm thinking how to merge both solutions.

I'm also concerned by the time passed developing these checks ;)

  reply	other threads:[~2016-07-06  9:46 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-05 14:40 [PATCH] scripts: add check for net driver names without net prefix Bruce Richardson
2016-07-06  9:10 ` Thomas Monjalon
2016-07-06  9:25   ` Bruce Richardson
2016-07-06  9:46     ` Thomas Monjalon [this message]
2016-07-06 10:28       ` Bruce Richardson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=22935387.s4eUy1AmSX@xps13 \
    --to=thomas.monjalon@6wind.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=nelio.laranjeiro@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.