Re: ssh nits
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
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
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
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
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
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
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
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}
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
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
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)
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)
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
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