mbox series

[bpf-next,0/8] bpf: Follow up to RCU enforcement in the verifier.

Message ID 20230404045029.82870-1-alexei.starovoitov@gmail.com (mailing list archive)
Headers show
Series bpf: Follow up to RCU enforcement in the verifier. | expand

Message

Alexei Starovoitov April 4, 2023, 4:50 a.m. UTC
From: Alexei Starovoitov <ast@kernel.org>

The patch set is addressing a fallout from
commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
It was too aggressive with PTR_UNTRUSTED marks.
Patches 1-6 are cleanup and adding verifier smartness to address real
use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
The partial revert is done in patch 7 anyway.

Alexei Starovoitov (8):
  bpf: Invoke btf_struct_access() callback only for writes.
  bpf: Remove unused arguments from btf_struct_access().
  bpf: Refactor btf_nested_type_is_trusted().
  bpf: Teach verifier that certain helpers accept NULL pointer.
  bpf: Refactor NULL-ness check in check_reg_type().
  bpf: Allowlist few fields similar to __rcu tag.
  bpf: Undo strict enforcement for walking untagged fields.
  selftests/bpf: Add tracing tests for walking skb and req.

 include/linux/bpf.h                           | 10 +-
 include/linux/filter.h                        |  3 +-
 kernel/bpf/bpf_cgrp_storage.c                 |  4 +-
 kernel/bpf/bpf_inode_storage.c                |  4 +-
 kernel/bpf/bpf_task_storage.c                 |  8 +-
 kernel/bpf/btf.c                              | 44 ++++-----
 kernel/bpf/verifier.c                         | 91 ++++++++++++++-----
 net/bpf/bpf_dummy_struct_ops.c                | 14 ++-
 net/core/bpf_sk_storage.c                     |  4 +-
 net/core/filter.c                             | 21 ++---
 net/ipv4/bpf_tcp_ca.c                         |  6 +-
 net/netfilter/nf_conntrack_bpf.c              |  3 +-
 .../bpf/progs/test_sk_storage_tracing.c       | 16 ++++
 13 files changed, 131 insertions(+), 97 deletions(-)

Comments

David Vernet April 4, 2023, 2:51 p.m. UTC | #1
On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The patch set is addressing a fallout from
> commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> It was too aggressive with PTR_UNTRUSTED marks.
> Patches 1-6 are cleanup and adding verifier smartness to address real
> use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> The partial revert is done in patch 7 anyway.
> 
> Alexei Starovoitov (8):
>   bpf: Invoke btf_struct_access() callback only for writes.
>   bpf: Remove unused arguments from btf_struct_access().
>   bpf: Refactor btf_nested_type_is_trusted().
>   bpf: Teach verifier that certain helpers accept NULL pointer.
>   bpf: Refactor NULL-ness check in check_reg_type().
>   bpf: Allowlist few fields similar to __rcu tag.
>   bpf: Undo strict enforcement for walking untagged fields.
>   selftests/bpf: Add tracing tests for walking skb and req.

For whole series:

Acked-by: David Vernet <void@manifault.com>

I left one comment on 4/8 in [0], but it's not a blocker and everything
else LGTM.

[0]: https://lore.kernel.org/all/20230404144652.GA3896@maniforge/
Andrii Nakryiko April 5, 2023, 12:02 a.m. UTC | #2
On Tue, Apr 4, 2023 at 7:51 AM David Vernet <void@manifault.com> wrote:
>
> On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > The patch set is addressing a fallout from
> > commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> > It was too aggressive with PTR_UNTRUSTED marks.
> > Patches 1-6 are cleanup and adding verifier smartness to address real
> > use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> > The partial revert is done in patch 7 anyway.
> >
> > Alexei Starovoitov (8):
> >   bpf: Invoke btf_struct_access() callback only for writes.
> >   bpf: Remove unused arguments from btf_struct_access().
> >   bpf: Refactor btf_nested_type_is_trusted().
> >   bpf: Teach verifier that certain helpers accept NULL pointer.
> >   bpf: Refactor NULL-ness check in check_reg_type().
> >   bpf: Allowlist few fields similar to __rcu tag.
> >   bpf: Undo strict enforcement for walking untagged fields.
> >   selftests/bpf: Add tracing tests for walking skb and req.
>
> For whole series:
>
> Acked-by: David Vernet <void@manifault.com>

Added David's acks manually (we really need to teach pw-apply to do
this automatically...) and applied. I've added a single sentence to
patch #1 with why (I think) btf_struct_access() callback
simplification was done, I didn't want to hold the patch set just due
to that, as the rest looked good. But please do consider renaming the
callback to more write-access implying name as a follow up, as current
situation with the same name but different semantics is confusing.

Applied to bpf-next, thanks.

>
> I left one comment on 4/8 in [0], but it's not a blocker and everything
> else LGTM.
>
> [0]: https://lore.kernel.org/all/20230404144652.GA3896@maniforge/
patchwork-bot+netdevbpf@kernel.org April 5, 2023, 12:10 a.m. UTC | #3
Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Mon,  3 Apr 2023 21:50:21 -0700 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The patch set is addressing a fallout from
> commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> It was too aggressive with PTR_UNTRUSTED marks.
> Patches 1-6 are cleanup and adding verifier smartness to address real
> use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> The partial revert is done in patch 7 anyway.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/8] bpf: Invoke btf_struct_access() callback only for writes.
    https://git.kernel.org/bpf/bpf-next/c/7d64c5132844
  - [bpf-next,2/8] bpf: Remove unused arguments from btf_struct_access().
    https://git.kernel.org/bpf/bpf-next/c/b7e852a9ec96
  - [bpf-next,3/8] bpf: Refactor btf_nested_type_is_trusted().
    https://git.kernel.org/bpf/bpf-next/c/63260df13965
  - [bpf-next,4/8] bpf: Teach verifier that certain helpers accept NULL pointer.
    https://git.kernel.org/bpf/bpf-next/c/91571a515d1b
  - [bpf-next,5/8] bpf: Refactor NULL-ness check in check_reg_type().
    https://git.kernel.org/bpf/bpf-next/c/add68b843f33
  - [bpf-next,6/8] bpf: Allowlist few fields similar to __rcu tag.
    https://git.kernel.org/bpf/bpf-next/c/30ee9821f943
  - [bpf-next,7/8] bpf: Undo strict enforcement for walking untagged fields.
    https://git.kernel.org/bpf/bpf-next/c/afeebf9f57a4
  - [bpf-next,8/8] selftests/bpf: Add tracing tests for walking skb and req.
    https://git.kernel.org/bpf/bpf-next/c/69f41a787761

You are awesome, thank you!
Alexei Starovoitov April 5, 2023, 12:16 a.m. UTC | #4
On Tue, Apr 4, 2023 at 5:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 4, 2023 at 7:51 AM David Vernet <void@manifault.com> wrote:
> >
> > On Mon, Apr 03, 2023 at 09:50:21PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > The patch set is addressing a fallout from
> > > commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
> > > It was too aggressive with PTR_UNTRUSTED marks.
> > > Patches 1-6 are cleanup and adding verifier smartness to address real
> > > use cases in bpf programs that broke with too aggressive PTR_UNTRUSTED.
> > > The partial revert is done in patch 7 anyway.
> > >
> > > Alexei Starovoitov (8):
> > >   bpf: Invoke btf_struct_access() callback only for writes.
> > >   bpf: Remove unused arguments from btf_struct_access().
> > >   bpf: Refactor btf_nested_type_is_trusted().
> > >   bpf: Teach verifier that certain helpers accept NULL pointer.
> > >   bpf: Refactor NULL-ness check in check_reg_type().
> > >   bpf: Allowlist few fields similar to __rcu tag.
> > >   bpf: Undo strict enforcement for walking untagged fields.
> > >   selftests/bpf: Add tracing tests for walking skb and req.
> >
> > For whole series:
> >
> > Acked-by: David Vernet <void@manifault.com>
>
> Added David's acks manually (we really need to teach pw-apply to do
> this automatically...) and applied.

+1
I was hoping that patchwork will add this feature eventually,
but it seems faster to hack the pw-apply script instead.

> I've added a single sentence to
> patch #1 with why (I think) btf_struct_access() callback
> simplification was done, I didn't want to hold the patch set just due
> to that, as the rest looked good. But please do consider renaming the
> callback to more write-access implying name as a follow up, as current
> situation with the same name but different semantics is confusing.
>
> Applied to bpf-next, thanks.

Thanks.
Renaming either the btf_struct_access() function or (*btf_struct_access) field
was on the todo list as a potential workaround,
since this name caused a weird issue with clang in LTO build.
For some reason two global symbols were generated.
Yonghong is investigating.

fwiw btf_struct_write_access sounds fine as a new name.
Jakub Kicinski April 5, 2023, 1:51 a.m. UTC | #5
On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
> > Added David's acks manually (we really need to teach pw-apply to do
> > this automatically...) and applied.  
> 
> +1
> I was hoping that patchwork will add this feature eventually,
> but it seems faster to hack the pw-apply script instead.

pw-apply can kind of do it. It exports an env variable called ADD_TAGS
if it spots any tags in reply to the cover letter.

You need to add a snippet like this to your .git/hooks/applypatch-msg:

  while IFS= read -r tag; do
    echo -e Adding tag: '\e[35m'$tag'\e[0m'
      git interpret-trailers --in-place \
          --if-exists=addIfDifferent \
          --trailer "$tag" \
          "$1"
  done <<< "$ADD_TAGS"

to transfer those tags onto the commits.

Looking at the code you may also need to use -M to get ADD_TAGS
exported. I'm guessing I put this code under -M so that the extra curl
requests don't slow down the script for everyone. But we can probably
"graduate" that into the main body if you find it useful and hate -M :)
Andrii Nakryiko April 5, 2023, 5:22 p.m. UTC | #6
On Tue, Apr 4, 2023 at 6:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
> > > Added David's acks manually (we really need to teach pw-apply to do
> > > this automatically...) and applied.
> >
> > +1
> > I was hoping that patchwork will add this feature eventually,
> > but it seems faster to hack the pw-apply script instead.
>
> pw-apply can kind of do it. It exports an env variable called ADD_TAGS
> if it spots any tags in reply to the cover letter.
>
> You need to add a snippet like this to your .git/hooks/applypatch-msg:
>
>   while IFS= read -r tag; do
>     echo -e Adding tag: '\e[35m'$tag'\e[0m'
>       git interpret-trailers --in-place \
>           --if-exists=addIfDifferent \
>           --trailer "$tag" \
>           "$1"
>   done <<< "$ADD_TAGS"
>
> to transfer those tags onto the commits.
>
> Looking at the code you may also need to use -M to get ADD_TAGS
> exported. I'm guessing I put this code under -M so that the extra curl
> requests don't slow down the script for everyone. But we can probably
> "graduate" that into the main body if you find it useful and hate -M :)

So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
everything locally. I'd expect that at this time the script would
detect any Acked-by replies on *cover letter patch*, and apply them
across all patches in the series. Such that we (humans) can look at
them, fix them, add them, etc. Doing something like this in git hook
seems unnecessary?

So I think the only thing that's missing is the code that would fetch
all replies on the cover letter "patch" (e.g., like on [0]) and just
apply it across everything. We must be doing something like this for
acks on individual patches, so I imagine we are not far off to make
this work, but I haven't looked at pw-apply carefully enough to know
for sure.

  [0] https://patchwork.kernel.org/project/netdevbpf/cover/20230404045029.82870-1-alexei.starovoitov@gmail.com/
Jakub Kicinski April 5, 2023, 6:19 p.m. UTC | #7
On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> everything locally.

I think you can throw -M after -c $url? It can only help... :)

> I'd expect that at this time the script would
> detect any Acked-by replies on *cover letter patch*, and apply them
> across all patches in the series. Such that we (humans) can look at
> them, fix them, add them, etc. Doing something like this in git hook
> seems unnecessary?

Maybe mb2q can do it, IDK. I don't use the mb2q thing.
I don't think git has a way of doing git am and insert these tags if
they don't exist, in a single command.

> So I think the only thing that's missing is the code that would fetch
> all replies on the cover letter "patch" (e.g., like on [0]) and just
> apply it across everything. We must be doing something like this for
> acks on individual patches, so I imagine we are not far off to make
> this work, but I haven't looked at pw-apply carefully enough to know
> for sure.

The individual patches are handled by patchwork.

Don't get me wrong, I'm not disagreeing with you. Just trying to help
and point out existing workarounds..
Daniel Borkmann April 5, 2023, 7:24 p.m. UTC | #8
On 4/5/23 7:22 PM, Andrii Nakryiko wrote:
> On Tue, Apr 4, 2023 at 6:51 PM Jakub Kicinski <kuba@kernel.org> wrote:
>> On Tue, 4 Apr 2023 17:16:27 -0700 Alexei Starovoitov wrote:
>>>> Added David's acks manually (we really need to teach pw-apply to do
>>>> this automatically...) and applied.
>>>
>>> +1
>>> I was hoping that patchwork will add this feature eventually,
>>> but it seems faster to hack the pw-apply script instead.
>>
>> pw-apply can kind of do it. It exports an env variable called ADD_TAGS
>> if it spots any tags in reply to the cover letter.
>>
>> You need to add a snippet like this to your .git/hooks/applypatch-msg:
>>
>>    while IFS= read -r tag; do
>>      echo -e Adding tag: '\e[35m'$tag'\e[0m'
>>        git interpret-trailers --in-place \
>>            --if-exists=addIfDifferent \
>>            --trailer "$tag" \
>>            "$1"
>>    done <<< "$ADD_TAGS"
>>
>> to transfer those tags onto the commits.
>>
>> Looking at the code you may also need to use -M to get ADD_TAGS
>> exported. I'm guessing I put this code under -M so that the extra curl
>> requests don't slow down the script for everyone. But we can probably
>> "graduate" that into the main body if you find it useful and hate -M :)
> 
> So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> everything locally. I'd expect that at this time the script would
> detect any Acked-by replies on *cover letter patch*, and apply them
> across all patches in the series. Such that we (humans) can look at
> them, fix them, add them, etc. Doing something like this in git hook
> seems unnecessary?
> 
> So I think the only thing that's missing is the code that would fetch
> all replies on the cover letter "patch" (e.g., like on [0]) and just
> apply it across everything. We must be doing something like this for
> acks on individual patches, so I imagine we are not far off to make
> this work, but I haven't looked at pw-apply carefully enough to know
> for sure.

I always use pw-apply based on the mbox, e.g. ...

   pw-apply -b https://patchwork.kernel.org/[...]/mbox/ -m <branch-name> -- -a "Foo Bar <foo@bar.com>"

... still manual, but it will propagate the -a (Acked-by) / -r (Reviewed-by) /
-t (Tested-by) to the individual patches.

Thanks,
Daniel
Andrii Nakryiko April 5, 2023, 8:11 p.m. UTC | #9
On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > everything locally.
>
> I think you can throw -M after -c $url? It can only help... :)
>
> > I'd expect that at this time the script would
> > detect any Acked-by replies on *cover letter patch*, and apply them
> > across all patches in the series. Such that we (humans) can look at
> > them, fix them, add them, etc. Doing something like this in git hook
> > seems unnecessary?
>
> Maybe mb2q can do it, IDK. I don't use the mb2q thing.
> I don't think git has a way of doing git am and insert these tags if
> they don't exist, in a single command.
>
> > So I think the only thing that's missing is the code that would fetch
> > all replies on the cover letter "patch" (e.g., like on [0]) and just
> > apply it across everything. We must be doing something like this for
> > acks on individual patches, so I imagine we are not far off to make
> > this work, but I haven't looked at pw-apply carefully enough to know
> > for sure.
>
> The individual patches are handled by patchwork.
>

Ah, I see, so yeah, it would be harder to do in pw-apply then.

> Don't get me wrong, I'm not disagreeing with you. Just trying to help
> and point out existing workarounds..

Oh, absolutely, I think we are all on the same page here. Daniel
mentioned -a in another reply, I didn't know about that and will use
that for the time being, still will save a bunch of time next time.

When I feel inspired to deal with a bit of bash, curl, and stuff, I'll
see if we can extend `pw-apply -c` to do this automatically. Thanks
for the discussion!
Alexei Starovoitov April 6, 2023, 5:13 a.m. UTC | #10
On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > everything locally.
>
> I think you can throw -M after -c $url? It can only help... :)

Yeah. If only...
I'm exclusively using -c.
-M only works with -s, but I couldn't make -s -M work either.
Do you pass the series as a number?
but then series_json=$(curl -s $srv/series/$1/) line
doesn't look right, since it's missing "/mbox/" ?
User error on my side, I guess.
My bash skills were too weak to make -c and -M work,
but .git/hooks tip is great!
Thank you.
Jakub Kicinski April 6, 2023, 3:42 p.m. UTC | #11
On Wed, 5 Apr 2023 22:13:26 -0700 Alexei Starovoitov wrote:
> On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:  
> > > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > > everything locally.  
> >
> > I think you can throw -M after -c $url? It can only help... :)  
> 
> Yeah. If only...
> I'm exclusively using -c.
> -M only works with -s, but I couldn't make -s -M work either.
> Do you pass the series as a number?

Yes, it copy just the numerical ID into the terminal.

> but then series_json=$(curl -s $srv/series/$1/) line
> doesn't look right, since it's missing "/mbox/" ?

That's loading JSON from the patchwork's REST API.

> User error on my side, I guess.
> My bash skills were too weak to make -c and -M work,
> but .git/hooks tip is great!
> Thank you.

FWIW I think the below may work for using -c instead of -s.
But it is mixing "Daniel paths" and "Jakub paths" in the script.
The output is still a bit different than when using -s.

diff --git a/pw-apply b/pw-apply
index 5fc37a24b027..c9cec94a4a8c 100755
--- a/pw-apply
+++ b/pw-apply
@@ -81,17 +81,15 @@ accept_series()
 }
 
 cover_from_url()
 {
   curl -s $1 | gunzip -f -c > tmp.i
-  series_num=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
+  series=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
   cover_url=`grep "href=\"/project/netdevbpf/cover" tmp.i|cut -d\" -f2`
   if [ ! -z "$cover_url" ]; then
-    curl -s https://patchwork.kernel.org${cover_url}mbox/ | gunzip -f -c > cover.i
     merge="1"
   fi
-  curl -s https://patchwork.kernel.org/series/$series_num/mbox/ | gunzip -f -c > mbox.i
 }
 
 edits=""
 am_flags=""
 branch="mbox"
@@ -118,18 +116,20 @@ while true; do
     -h | --help ) usage; break ;;
     -- ) shift; break ;;
     * )  break ;;
   esac
 done
+# Load the info from cover first, it may will populate $series and $merge
+[ ! -z "$cover" ]  && cover_from_url $cover
+
 [ ! -z "$auto_branch" ] && [ -z "$series" ] && usage
 [ ! -z "$mbox" ]   && [ ! -z "$series" ] && usage
 [   -z "$mbox" ]   && [   -z "$series" ] && [ -z "$cover" ] && usage
 [ ! -z "$accept" ] && [ ! -z "$mbox" ]   && usage
 [ ! -z "$series" ] && mbox_from_series $series
 [ ! -z "$mbox" ]   && mbox_from_url $mbox
 [ ! -z "$accept" ] && accept_series $series
-[ ! -z "$cover" ]  && cover_from_url $cover
 
 target=$(git branch --show-current)
 
 body=
 author=XYZ
Alexei Starovoitov April 7, 2023, 1:17 a.m. UTC | #12
On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Apr 2023 22:13:26 -0700 Alexei Starovoitov wrote:
> > On Wed, Apr 5, 2023 at 11:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Wed, 5 Apr 2023 10:22:16 -0700 Andrii Nakryiko wrote:
> > > > So I'm exclusively using `pw-apply -c <patchworks-url>` to apply
> > > > everything locally.
> > >
> > > I think you can throw -M after -c $url? It can only help... :)
> >
> > Yeah. If only...
> > I'm exclusively using -c.
> > -M only works with -s, but I couldn't make -s -M work either.
> > Do you pass the series as a number?
>
> Yes, it copy just the numerical ID into the terminal.
>
> > but then series_json=$(curl -s $srv/series/$1/) line
> > doesn't look right, since it's missing "/mbox/" ?
>
> That's loading JSON from the patchwork's REST API.

This line still doesn't work for me.
curl -s https://patchwork.kernel.org/series/736654/
returns:
The page URL requested (<code>/series/736654/</code>) does not exist.

while
curl -s https://patchwork.kernel.org/series/736654/mbox/
returns proper mbox format.

> > User error on my side, I guess.
> > My bash skills were too weak to make -c and -M work,
> > but .git/hooks tip is great!
> > Thank you.
>
> FWIW I think the below may work for using -c instead of -s.
> But it is mixing "Daniel paths" and "Jakub paths" in the script.
> The output is still a bit different than when using -s.
>
> diff --git a/pw-apply b/pw-apply
> index 5fc37a24b027..c9cec94a4a8c 100755
> --- a/pw-apply
> +++ b/pw-apply
> @@ -81,17 +81,15 @@ accept_series()
>  }
>
>  cover_from_url()
>  {
>    curl -s $1 | gunzip -f -c > tmp.i
> -  series_num=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
> +  series=`grep "href=\"/series" tmp.i|cut -d/ -f3|head -1`
>    cover_url=`grep "href=\"/project/netdevbpf/cover" tmp.i|cut -d\" -f2`
>    if [ ! -z "$cover_url" ]; then
> -    curl -s https://patchwork.kernel.org${cover_url}mbox/ | gunzip -f -c > cover.i
>      merge="1"
>    fi
> -  curl -s https://patchwork.kernel.org/series/$series_num/mbox/ | gunzip -f -c > mbox.i
>  }
>
>  edits=""
>  am_flags=""
>  branch="mbox"
> @@ -118,18 +116,20 @@ while true; do
>      -h | --help ) usage; break ;;
>      -- ) shift; break ;;
>      * )  break ;;
>    esac
>  done
> +# Load the info from cover first, it may will populate $series and $merge
> +[ ! -z "$cover" ]  && cover_from_url $cover
> +
>  [ ! -z "$auto_branch" ] && [ -z "$series" ] && usage
>  [ ! -z "$mbox" ]   && [ ! -z "$series" ] && usage
>  [   -z "$mbox" ]   && [   -z "$series" ] && [ -z "$cover" ] && usage
>  [ ! -z "$accept" ] && [ ! -z "$mbox" ]   && usage
>  [ ! -z "$series" ] && mbox_from_series $series
>  [ ! -z "$mbox" ]   && mbox_from_url $mbox
>  [ ! -z "$accept" ] && accept_series $series
> -[ ! -z "$cover" ]  && cover_from_url $cover

This part completely makes sense! Would be great to converge
on common usage, but json fetch doesn't work for me for some reason.
While mbox via original -c works fine.
Jakub Kicinski April 7, 2023, 1:23 a.m. UTC | #13
On Thu, 6 Apr 2023 18:17:57 -0700 Alexei Starovoitov wrote:
> On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > Yeah. If only...
> > > I'm exclusively using -c.
> > > -M only works with -s, but I couldn't make -s -M work either.
> > > Do you pass the series as a number?  
> >
> > Yes, it copy just the numerical ID into the terminal.
> >  
> > > but then series_json=$(curl -s $srv/series/$1/) line
> > > doesn't look right, since it's missing "/mbox/" ?  
> >
> > That's loading JSON from the patchwork's REST API.  
> 
> This line still doesn't work for me.
> curl -s https://patchwork.kernel.org/series/736654/
> returns:
> The page URL requested (<code>/series/736654/</code>) does not exist.
> 
> while
> curl -s https://patchwork.kernel.org/series/736654/mbox/
> returns proper mbox format.

Check if your git config is right:

$ git config --get pw.server
https://patchwork.kernel.org/api/1.1/

that's where $srv comes from
Alexei Starovoitov April 7, 2023, 1:32 a.m. UTC | #14
On Thu, Apr 6, 2023 at 6:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 6 Apr 2023 18:17:57 -0700 Alexei Starovoitov wrote:
> > On Thu, Apr 6, 2023 at 8:42 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > Yeah. If only...
> > > > I'm exclusively using -c.
> > > > -M only works with -s, but I couldn't make -s -M work either.
> > > > Do you pass the series as a number?
> > >
> > > Yes, it copy just the numerical ID into the terminal.
> > >
> > > > but then series_json=$(curl -s $srv/series/$1/) line
> > > > doesn't look right, since it's missing "/mbox/" ?
> > >
> > > That's loading JSON from the patchwork's REST API.
> >
> > This line still doesn't work for me.
> > curl -s https://patchwork.kernel.org/series/736654/
> > returns:
> > The page URL requested (<code>/series/736654/</code>) does not exist.
> >
> > while
> > curl -s https://patchwork.kernel.org/series/736654/mbox/
> > returns proper mbox format.
>
> Check if your git config is right:
>
> $ git config --get pw.server
> https://patchwork.kernel.org/api/1.1/
>
> that's where $srv comes from

Ahh. All works now!
I like the new output.
I'll play with it more.
Should -M be a default? Any downside?
Jakub Kicinski April 7, 2023, 1:57 a.m. UTC | #15
On Thu, 6 Apr 2023 18:32:33 -0700 Alexei Starovoitov wrote:
> > Check if your git config is right:
> >
> > $ git config --get pw.server
> > https://patchwork.kernel.org/api/1.1/
> >
> > that's where $srv comes from  
> 
> Ahh. All works now!
> I like the new output.
> I'll play with it more.
> Should -M be a default? Any downside?

There should be no difference, AFAICT. I'm happy with making it 
the default.

There's a minor difference in the merge-message formatting between
-c and -s we could possibly remove if we make -M the default. 
Daniel uses the subject of the series as a fake branch name on the
 
  Merge branch '$branch_name'

line, while I convert the subject to a format which can be a real
branch name in git (no spaces, special chars etc) and put the subject
as the first line of the merge text.