Re: ssh nits

2023-03-08 Thread Darren Tucker
On Thu, 9 Mar 2023 at 06:50, joshua stein  wrote:
> On Thu, 09 Mar 2023 at 06:41:50 +1100, Darren Tucker wrote:
> > This seems to be one too many parens?   ie
> > if (negate = (attrib[0] == '!'))
>
> clang warns if there's not the extra set of parens in case it's an
> accidental = instead of ==.

ok dtucker for this one.

"no objection" to the sshpty.c one.



--
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.



Re: [PATCH] White space found in param.h

2023-03-08 Thread Theo de Raadt
Right.

Developers will perform these tasks when they encounter it.  Please do
not submit diffs of this kind.


>Thanks. I dislike trailing whitespace since it screws up code navigation,
>and I tend to remove it in code I'm working on since it distracts me.
>
>However, to be honest, sending diffs removing it is a waste of time.
>Someone has to check the diff, apply it, compile it, write a message and
>commit it. These are a few steps too many for a measly tab.
>
>



Re: ssh nits

2023-03-08 Thread Damien Miller



On Thu, 9 Mar 2023, Darren Tucker wrote:

> On Thu, 9 Mar 2023 at 02:09, joshua stein  wrote:
> > cppcheck found these, are they worth fixing?
> >
> > In the non-fail case, done is set to NULL and then free()d.
> > free(NULL) is legal but maybe worth removing?
> 
> ssh uses this pattern a lot, and I agree with millert that it's not
> worth changing.
> 
> char *thing = NULL;
> [lots of stuff that might set thing]
> if (maybe()) {
> free(thing);
> thing = something;
> }
> [more stuff]
> free(thing)
> 
> We actually went through and changed all the "if (thing) free(thing)"
> instances to drop the "if" some time ago.
> 
> > grp == NULL fatal()s, so remove the ternary operations that will
> > never be the conditionals they aspire to be.
> 
> That's true in OpenBSD, but not in -portable where the check is a
> debug only.  That said, there's already a diff in this code block
> that'll stop future syncs from applying cleanly so I don't mind either
> way.
> 
> > +   if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
> > +   (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||
> 
> I'll wait for djm to comment on this one.
> 
> > +   if ((negate = (attrib[0] == '!')))
> 
> This seems to be one too many parens?   ie
> if (negate = (attrib[0] == '!'))
> 
> > -   if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
> > -   (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
> > +   if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
> > +   (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)
> 
> also djm.

ok djm for these



Re: [PATCH] White space found in param.h

2023-03-08 Thread Theo Buehler
Thanks. I dislike trailing whitespace since it screws up code navigation,
and I tend to remove it in code I'm working on since it distracts me.

However, to be honest, sending diffs removing it is a waste of time.
Someone has to check the diff, apply it, compile it, write a message and
commit it. These are a few steps too many for a measly tab.



installer: remove obsolete libLLVM.so.[0-6].0

2023-03-08 Thread Christian Weisgerber
The installer deletes obsolete libLLVM.so versions during an upgrade.
However, a number of libLLVM.so versions have come and gone and the
installer hasn't been synced.  We're now at .7.0, so delete all the
earlier ones.

ok?

---
remove obsolete libLLVM.so.[0-6].0 during upgrade

diff 4e440c72920f8bfbe8fe8a752bd5eafff5c4c644 
8593f3cca4c3f1bbac0f69172c991145b5bead7f
commit - 4e440c72920f8bfbe8fe8a752bd5eafff5c4c644
commit + 8593f3cca4c3f1bbac0f69172c991145b5bead7f
blob - 954feb267763c40f0761fc1144ea5a6f4f47574f
blob + 3a276cf34810970279dbc514330cfddcba00968b
--- distrib/miniroot/install.sub
+++ distrib/miniroot/install.sub
@@ -1761,7 +1761,7 @@ install_files() {
if [[ $MODE == upgrade ]]; then
if isin base$VERSION.tgz $_get_sets; then
rm -f /mnt/usr/share/relink/usr/lib/*
-   rm -rf /mnt/usr/lib/libLLVM.so.[012].0
+   rm -rf /mnt/usr/lib/libLLVM.so.[0-6].0
rm -rf /mnt/usr/libdata/perl5
fi
if isin comp$VERSION.tgz $_get_sets; then

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de



Re: ssh nits

2023-03-08 Thread joshua stein
On Thu, 09 Mar 2023 at 06:41:50 +1100, Darren Tucker wrote:
> > +   if ((negate = (attrib[0] == '!')))
> 
> This seems to be one too many parens?   ie
> if (negate = (attrib[0] == '!'))

clang warns if there's not the extra set of parens in case it's an 
accidental = instead of ==.

/usr/src/usr.bin/ssh/ssh/../readconf.c:605:14: warning: using the result of an 
assignment as a condition without parentheses [-Wparentheses]
if (negate = (attrib[0] == '!'))



Re: ssh nits

2023-03-08 Thread Darren Tucker
On Thu, 9 Mar 2023 at 02:09, joshua stein  wrote:
> cppcheck found these, are they worth fixing?
>
> In the non-fail case, done is set to NULL and then free()d.
> free(NULL) is legal but maybe worth removing?

ssh uses this pattern a lot, and I agree with millert that it's not
worth changing.

char *thing = NULL;
[lots of stuff that might set thing]
if (maybe()) {
free(thing);
thing = something;
}
[more stuff]
free(thing)

We actually went through and changed all the "if (thing) free(thing)"
instances to drop the "if" some time ago.

> grp == NULL fatal()s, so remove the ternary operations that will
> never be the conditionals they aspire to be.

That's true in OpenBSD, but not in -portable where the check is a
debug only.  That said, there's already a diff in this code block
that'll stop future syncs from applying cleanly so I don't mind either
way.

> +   if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
> +   (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||

I'll wait for djm to comment on this one.

> +   if ((negate = (attrib[0] == '!')))

This seems to be one too many parens?   ie
if (negate = (attrib[0] == '!'))

> -   if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
> -   (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
> +   if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
> +   (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)

also djm.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.



Re: ssh nits

2023-03-08 Thread Todd C . Miller
On Wed, 08 Mar 2023 09:02:08 -0600, joshua stein wrote:

> In the non-fail case, done is set to NULL and then free()d.  
> free(NULL) is legal but maybe worth removing?

Please leave this as-is.  I don't think it is worth appeasing
cppcheck in this case.

> diff --git usr.bin/ssh/scp.c usr.bin/ssh/scp.c
> index f0f09bba623..acb7bd8a8a1 100644
> --- usr.bin/ssh/scp.c
> +++ usr.bin/ssh/scp.c
> @@ -935,19 +935,21 @@ brace_expand(const char *pattern, char ***patternsp, si
> ze_t *npatternsp)
>   *npatternsp = ndone;
>   done = NULL;
>   ndone = 0;
>   ret = 0;
>   fail:
>   for (i = 0; i < nactive; i++)
>   free(active[i]);
>   free(active);
> - for (i = 0; i < ndone; i++)
> - free(done[i]);
> - free(done);
> + if (done) {
> + for (i = 0; i < ndone; i++)
> + free(done[i]);
> + free(done);
> + }
>   return ret;
>  }
>  
>  static struct sftp_conn *
>  do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
> int *reminp, int *remoutp, int *pidp)
>  {
>   if (sftp_direct == NULL) {
>
>
> grp == NULL fatal()s, so remove the ternary operations that will 
> never be the conditionals they aspire to be.

OK

> diff --git usr.bin/ssh/sshpty.c usr.bin/ssh/sshpty.c
> index faf8960cfb5..690263a8cf3 100644
> --- usr.bin/ssh/sshpty.c
> +++ usr.bin/ssh/sshpty.c
> @@ -143,8 +143,8 @@ pty_setowner(struct passwd *pw, const char *tty)
>   grp = getgrnam("tty");
>   if (grp == NULL)
>   fatal("no tty group");
> - gid = (grp != NULL) ? grp->gr_gid : pw->pw_gid;
> - mode = (grp != NULL) ? 0620 : 0600;
> + gid = grp->gr_gid;
> + mode = 0620;
>  
>   /*
>* Change owner and mode of the tty as required.
>
>
> These parentheses checking the result of an assignment were 
> confusing, so move them.

OK.  This will result in encode_dest_constraint() and
parse_dest_constraint() returning an SSH_ERR_* instead of 1 on error
but that seems like an improvement.  It won't affect the caller's
logic as far as I can tell.  I would wait for an OK from djm@ though.

> diff --git usr.bin/ssh/authfd.c usr.bin/ssh/authfd.c
> index 4b81b385637..05011f8c5c9 100644
> --- usr.bin/ssh/authfd.c
> +++ usr.bin/ssh/authfd.c
> @@ -489,8 +489,8 @@ encode_dest_constraint(struct sshbuf *m, const struct des
> t_constraint *dc)
>  
>   if ((b = sshbuf_new()) == NULL)
>   return SSH_ERR_ALLOC_FAIL;
> - if ((r = encode_dest_constraint_hop(b, &dc->from) != 0) ||
> - (r = encode_dest_constraint_hop(b, &dc->to) != 0) ||
> + if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
> + (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||
>   (r = sshbuf_put_string(b, NULL, 0)) != 0) /* reserved */
>   goto out;
>   if ((r = sshbuf_put_stringb(m, b)) != 0)
> diff --git usr.bin/ssh/readconf.c usr.bin/ssh/readconf.c
> index e9d3a756896..81456c9b6d3 100644
> --- usr.bin/ssh/readconf.c
> +++ usr.bin/ssh/readconf.c
> @@ -602,7 +602,7 @@ match_cfg_line(Options *options, char **condition, struct
>  passwd *pw,
>   }
>   arg = criteria = NULL;
>   this_result = 1;
> - if ((negate = attrib[0] == '!'))
> + if ((negate = (attrib[0] == '!')))
>   attrib++;
>   /* Criterion "all" has no argument and must appear alone */
>   if (strcasecmp(attrib, "all") == 0) {
> diff --git usr.bin/ssh/ssh-agent.c usr.bin/ssh/ssh-agent.c
> index 0e4d7f675ab..de1cdb049a2 100644
> --- usr.bin/ssh/ssh-agent.c
> +++ usr.bin/ssh/ssh-agent.c
> @@ -1010,8 +1010,8 @@ parse_dest_constraint(struct sshbuf *m, struct dest_con
> straint *dc)
>   error_fr(r, "parse");
>   goto out;
>   }
> - if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
> - (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
> + if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
> + (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)
>   goto out; /* already logged */
>   if (elen != 0) {
>   error_f("unsupported extensions (len %zu)", elen);



Re: mg: handle prefix argument in shell-command{,-on-region}

2023-03-08 Thread Omar Polo
friendly 12 week ping :)

On 2022/12/15 09:19:27 +0100, Omar Polo  wrote:
> > > > On 2022/10/13 12:25:00 +0200, Omar Polo  wrote:
> > > > > shell-command (M-!) and shell-command-on-region (M-|) works by
> > > > > displaying the output of the command in a new buffer, but in emacs
> > > > > using a prefix argument (C-u) allows to operate on the current buffer.
> > > > > 
> > > > > diff belows adds that for mg.  I can finally C-u M-! got diff RET when
> > > > > composing mails :)
> > > > > 
> > > > > A possible drawback is that now the *Shell Command Output* buffer
> > > > > gains an undo history.  linsert is also possibly slower than addline
> > > > > but on the plus side we're no more limited to BUFSIZ long lines.
> > > > > 
> > > > > ok/comments/improvements?
> > > > 
> > > > Here's a slightly tweaked version that adds a missing parens around a
> > > > return value and uses ssize_t for some vars in preadin.  it also changes
> > > > the size read(2) from BUFSIZ-1 to BUFSIZ since we no longer need to NUL
> > > > terminate it.
> > > > 
> > > > This has been more useful than I originally expected.  I wanted it to
> > > > include diffs and the like more easily, now i'm using it also for all
> > > > sorts of stuff that mg doesn't do out-of-the-box (like using C-u M-|
> > > > sort RET instead of M-x sort-lines.)
> > > > 
> > > > If it were for me, M-| and M-! would operate by default on the buffer
> > > > and with C-u on a scratch one, but this is what emacs does and i'm
> > > > probably several decades too late :)

diffstat /home/op/w/mg
 M  region.c  |  50+  59-

1 file changed, 50 insertions(+), 59 deletions(-)

diff /home/op/w/mg
commit - 4e440c72920f8bfbe8fe8a752bd5eafff5c4c644
path + /home/op/w/mg
blob - 21c5174f52d21103b9cd15942620eb746b5069b2
file + region.c
--- region.c
+++ region.c
@@ -26,14 +26,13 @@ static char leftover[BUFSIZ];
 
 #define TIMEOUT 1
 
-static char leftover[BUFSIZ];
-
 static int getregion(struct region *);
 static int iomux(int, char * const, int, struct buffer *);
 static int preadin(int, struct buffer *);
 static voidpwriteout(int, char **, int *);
 static int setsize(struct region *, RSIZE);
-static int shellcmdoutput(char * const[], char * const, int);
+static int shellcmdoutput(char * const[], char * const, int,
+   struct buffer *);
 
 /*
  * Kill the region.  Ask "getregion" to figure out the bounds of the region.
@@ -419,14 +418,11 @@ piperegion(int f, int n)
 piperegion(int f, int n)
 {
struct region region;
+   struct buffer *bp = NULL;
int len;
char *cmd, cmdbuf[NFILEN], *text;
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
 
-   /* C-u M-| is not supported yet */
-   if (n > 1)
-   return (ABORT);
-
if (curwp->w_markp == NULL) {
dobeep();
ewprintf("The mark is not set now, so there is no region");
@@ -452,7 +448,13 @@ piperegion(int f, int n)
 
region_get_data(®ion, text, len);
 
-   return shellcmdoutput(argv, text, len);
+   if (n > 1) {
+   bp = curbp;
+   killregion(FFRAND, 1);
+   kdelete();
+   }
+
+   return (shellcmdoutput(argv, text, len, bp));
 }
 
 /*
@@ -462,12 +464,12 @@ shellcommand(int f, int n)
 int
 shellcommand(int f, int n)
 {
-
+   struct buffer *bp = NULL;
char *cmd, cmdbuf[NFILEN];
char *argv[] = {"sh", "-c", (char *) NULL, (char *) NULL};
 
if (n > 1)
-   return (ABORT);
+   bp = curbp;
 
if ((cmd = eread("Shell command: ", cmdbuf, sizeof(cmdbuf),
EFNEW | EFCR)) == NULL || (cmd[0] == '\0'))
@@ -475,36 +477,43 @@ shellcommand(int f, int n)
 
argv[2] = cmd;
 
-   return shellcmdoutput(argv, NULL, 0);
+   return (shellcmdoutput(argv, NULL, 0, bp));
 }
 
-
 int
-shellcmdoutput(char* const argv[], char* const text, int len)
+shellcmdoutput(char* const argv[], char* const text, int len,
+struct buffer *bp)
 {
-
-   struct buffer *bp;
+   struct mgwin *wp;
char*shellp;
-   int  ret;
+   int  ret, special = 0;
 
-   bp = bfind("*Shell Command Output*", TRUE);
-   bp->b_flag |= BFREADONLY;
-   if (bclear(bp) != TRUE) {
-   free(text);
-   return (FALSE);
+   if (bp == NULL) {
+   special = 1;
+   bp = bfind("*Shell Command Output*", TRUE);
+   bp->b_flag &= ~BFREADONLY;  /* disable read-only */
+   wp = popbuf(bp, WNONE);
+   if (wp == NULL || bclear(bp) != TRUE) {
+   free(text);
+   return (FALSE);
+   }
+   curbp = bp;
+   curwp = wp;
}
 
shellp = getenv("SHELL");
 
ret = pipeio(shellp, argv, text, len, bp);
-
if (ret == TRUE) {
eerase();
-   if (lforw(bp->b_headp) == bp->b_headp)
+ 

[PATCH] White space found in param.h

2023-03-08 Thread Purple Rain
Index: param.h
===
RCS file: /cvs/src/sys/sys/param.h,v
retrieving revision 1.140
diff -u -p -r1.140 param.h
--- param.h 4 Mar 2023 14:49:37 -   1.140
+++ param.h 8 Mar 2023 00:17:10 -
@@ -216,7 +216,7 @@
  */
 #define_FSHIFT 11  /* bits to right of fixed binary point 
*/
 #ifdef _KERNEL
-#defineFSHIFT  _FSHIFT 
+#defineFSHIFT  _FSHIFT
 #endif
 #define FSCALE (1<<_FSHIFT)
 



ssh nits

2023-03-08 Thread joshua stein
cppcheck found these, are they worth fixing?


In the non-fail case, done is set to NULL and then free()d.  
free(NULL) is legal but maybe worth removing?

diff --git usr.bin/ssh/scp.c usr.bin/ssh/scp.c
index f0f09bba623..acb7bd8a8a1 100644
--- usr.bin/ssh/scp.c
+++ usr.bin/ssh/scp.c
@@ -935,19 +935,21 @@ brace_expand(const char *pattern, char ***patternsp, 
size_t *npatternsp)
*npatternsp = ndone;
done = NULL;
ndone = 0;
ret = 0;
  fail:
for (i = 0; i < nactive; i++)
free(active[i]);
free(active);
-   for (i = 0; i < ndone; i++)
-   free(done[i]);
-   free(done);
+   if (done) {
+   for (i = 0; i < ndone; i++)
+   free(done[i]);
+   free(done);
+   }
return ret;
 }
 
 static struct sftp_conn *
 do_sftp_connect(char *host, char *user, int port, char *sftp_direct,
int *reminp, int *remoutp, int *pidp)
 {
if (sftp_direct == NULL) {


grp == NULL fatal()s, so remove the ternary operations that will 
never be the conditionals they aspire to be.

diff --git usr.bin/ssh/sshpty.c usr.bin/ssh/sshpty.c
index faf8960cfb5..690263a8cf3 100644
--- usr.bin/ssh/sshpty.c
+++ usr.bin/ssh/sshpty.c
@@ -143,8 +143,8 @@ pty_setowner(struct passwd *pw, const char *tty)
grp = getgrnam("tty");
if (grp == NULL)
fatal("no tty group");
-   gid = (grp != NULL) ? grp->gr_gid : pw->pw_gid;
-   mode = (grp != NULL) ? 0620 : 0600;
+   gid = grp->gr_gid;
+   mode = 0620;
 
/*
 * Change owner and mode of the tty as required.


These parentheses checking the result of an assignment were 
confusing, so move them.

diff --git usr.bin/ssh/authfd.c usr.bin/ssh/authfd.c
index 4b81b385637..05011f8c5c9 100644
--- usr.bin/ssh/authfd.c
+++ usr.bin/ssh/authfd.c
@@ -489,8 +489,8 @@ encode_dest_constraint(struct sshbuf *m, const struct 
dest_constraint *dc)
 
if ((b = sshbuf_new()) == NULL)
return SSH_ERR_ALLOC_FAIL;
-   if ((r = encode_dest_constraint_hop(b, &dc->from) != 0) ||
-   (r = encode_dest_constraint_hop(b, &dc->to) != 0) ||
+   if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
+   (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||
(r = sshbuf_put_string(b, NULL, 0)) != 0) /* reserved */
goto out;
if ((r = sshbuf_put_stringb(m, b)) != 0)
diff --git usr.bin/ssh/readconf.c usr.bin/ssh/readconf.c
index e9d3a756896..81456c9b6d3 100644
--- usr.bin/ssh/readconf.c
+++ usr.bin/ssh/readconf.c
@@ -602,7 +602,7 @@ match_cfg_line(Options *options, char **condition, struct 
passwd *pw,
}
arg = criteria = NULL;
this_result = 1;
-   if ((negate = attrib[0] == '!'))
+   if ((negate = (attrib[0] == '!')))
attrib++;
/* Criterion "all" has no argument and must appear alone */
if (strcasecmp(attrib, "all") == 0) {
diff --git usr.bin/ssh/ssh-agent.c usr.bin/ssh/ssh-agent.c
index 0e4d7f675ab..de1cdb049a2 100644
--- usr.bin/ssh/ssh-agent.c
+++ usr.bin/ssh/ssh-agent.c
@@ -1010,8 +1010,8 @@ parse_dest_constraint(struct sshbuf *m, struct 
dest_constraint *dc)
error_fr(r, "parse");
goto out;
}
-   if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
-   (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
+   if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
+   (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)
goto out; /* already logged */
if (elen != 0) {
error_f("unsupported extensions (len %zu)", elen);



Re: installer: handle WEP failure (bwfm)

2023-03-08 Thread Klemens Nanni
06.03.2023 14:13, Jonathan Gray пишет:
> Parts of bwfm already mention wep.  Is it just a matter of needing to
> set IEEE80211_C_WEP or is more needed?

stsp quote from the initial 'Re: ifconfig: return non-zero on failed "nwkey"'
https://marc.info/?l=openbsd-tech&m=163587479013162&w=2 :

> But regardless, this error should never happen.
> It looks like bwfm(4) does support WEP but the C_WEP capability is missing
> from ic_caps so net80211 believes WEP was not supported by this driver.
> 
> I don't think we have any supported wifi device that does not support WEP.
> Even an(4) supports WEP!

Noone ever stood up and gave WEP on bwfm a try.

Until someone fixes it or rips out WEP completely, we should remove the
false claim from the manual.

I've intentionally left the recommendation against WEP and WPA1 untouched
to it informative and the diff simple.

OK?

Index: bwfm.4
===
RCS file: /cvs/src/share/man/man4/bwfm.4,v
retrieving revision 1.18
diff -u -p -r1.18 bwfm.4
--- bwfm.4  23 Apr 2022 18:41:13 -  1.18
+++ bwfm.4  8 Mar 2023 12:17:21 -
@@ -78,7 +78,6 @@ for other cards.
 The
 .Nm
 driver can be configured to use
-Wired Equivalent Privacy (WEP) or
 Wi-Fi Protected Access (WPA1 and WPA2).
 WPA2 is the current encryption standard for wireless networks.
 It is strongly recommended that neither WEP nor WPA1



Re: installer: handle WEP failure (bwfm)

2023-03-08 Thread Klemens Nanni
06.03.2023 13:52, Mark Kettenis пишет:
> Maybe we should drop WEP support in the installer completely at some
> point.  Your diff makes that easier, so no objection from me.

To push the alternatives and/or because there would be a significant
size reduction in install media?

I'm not familiar with the wifi stack.



Re: bgpd, improve RFC9234 support

2023-03-08 Thread Theo Buehler
On Tue, Feb 28, 2023 at 01:11:14PM +0100, Claudio Jeker wrote:
> When I implemented RFC9234 support I was a bit to conservative and only
> enabled the loop detection if the capability was enabled. This is
> how every other capability works but RFC9234 is special.
> On top of this with the addition of ASPA support the way BGP roles are
> handled changed and the code turned up a bit strange.
> Considering all of this and rereading RFC9234 I came up with this diff:
> 
> - add role output to bgpctl, also adjust the capability output.
>   Note, this changes the JSON output of neighbors a bit.
> - adjust the config parser to enable the RFC9234 role capability when
>   there is a role set. iBGP and sessions with no role will not announce
>   the role capability.
> - adjust the role capability announcement to be only on sessions that
>   use either AFI IPv4 or IPv6 and SAFI 1 (AID_INET, AID_INET6). If a
>   session has neither of the two then do not announce the role (even if
>   set)
> - if there is an OPEN notification indicating that the role capability
>   is bad only disable the capability if it is not enforced. If enforced
>   do nothing and let the session fail (again and again).
> - Adjust capability negotiation, store remote_role on the peer since
>   the neighbors role is no longer needed.
> - inject the OTC attribute on ingress only for AID_INET and AID_INET6.
>   For other AIDs clear the F_ATTR_OTC_LOOP flag since OTC loop detection
>   must be skipped for other address families.
> - Adjust the role logic in the RDE and use the peer->role (local role of
>   the system) for all checks. Also remove the check if the role capability
>   was present (removal of peer_has_open_policy()).
>   This change switches all role checks from raw capability codes to
>   enum role and at the same time flips the logic since the view of the
>   checks is changed from remote system to local system.
>   The roles change like this: rs <-> rs-client, customer <-> provider,
>   peer <-> peer
> - In prefix_eligible() check also if the F_ATTR_OTC_LOOP flag is set.
>   The RFC requires that prefixes must be considered ineligible (and not
>   treat as withdraw)
> - When generating an UPDATE include the OTC attribute unless the AID is
>   neither AID_INET or AID_INET6 or the roles do not require the addtion.
>   No longer depend on peer_has_open_policy() there.
> 
> Please test and check that my logic is actually correct and following the
> RFC.

This all looks good to me. I could not spot anything wrong with it.

ok tb