A concerning commit which breaks compatibility
Hello OpenBSD devs. It has come to my attention that a mysterious commit , unlogged by CVS, has appeared. This commit changes language, breaking compatibility on header and source files. Thankfully, it was logged by the Github mirror. The commit's author is the Github username "djmdjm", and the one who okayed it was "markus@". Please, I ask of you and specially of Theo to look at this strange commit, and decide what to do about it. Its link is https://github.com/openbsd/src/commit/5bde2954c180034a27b079acaff46073dc75139b cc @misc @tech
Re: pf: route-to least-states
Hello Yasuoka,
> - interface is not selected properly if selected table entry specifies
> an interface.
to be honest I don't quite understand what's going on here.
can you share some details of configuration/scenario, which
triggers the bug your diff is fixing?
the part of your change, which I'm not able to figure out is
this single line:
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (1);
> + /* revert the kif which was set by pfr_pool_get() */
> + rpool->kif = kif;
> break;
> }
your fix changes behavior, which is there since least-state
option has been introduced. I believe it does not matter
for case when route-to specifies single interface such as:
route-to 192.168.1.10@em0 least-states
I'm not sure what will happen in situation, when there are more interfaces
specified in combination with sticky-address:
route-to {192.168.1.10@em0, 192.168.1.20@em1} last-states sticky-address
the resulting code does not look quite right with your diff applied:
602 } while (pf_match_addr(1, , rmask, >counter,
af) &&
603 (states > 0));
604
605 if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
606 return (1);
607 /* revert the kif which was set by pfr_pool_get() */
608 rpool->kif = kif;
609 break;
610 }
611
612 if (rpool->opts & PF_POOL_STICKYADDR) {
613 if (sns[type] != NULL) {
614 pf_remove_src_node(sns[type]);
615 sns[type] = NULL;
616 }
617 if (pf_insert_src_node([type], r, type, af, saddr,
naddr,
618 rpool->kif))
619 return (1);
620 }
at line 608 new code reverts kif set by pfr_pool_get(). At line 617
(executed when sticky-address is set) the original code passes kif chosen be
pfr_pool_get(). You diff changes that behavior. So my question is simple:
is that intentional change?
thanks and
regards
sashan
Re: A concerning commit which breaks compatibility
Go away troll. - todd
Re: A concerning commit which breaks compatibility
On Thu, Jul 23, 2020 at 09:54:56PM +, goldeneagle96 wrote: > Hello OpenBSD devs. It has come to my attention that a mysterious commit > , unlogged by CVS, has appeared. This commit changes language, breaking > compatibility on header and source files. > Thankfully, it was logged by the Github mirror. > The commit's author is the Github username "djmdjm", and the one who > okayed it was "markus@". > Please, I ask of you and specially of Theo to look at this strange > commit, and decide what to do about it. > Its link is > https://github.com/openbsd/src/commit/5bde2954c180034a27b079acaff46073dc75139b > cc @misc @tech There is nothing "mysterious" about this commit, and the GitHub repo is a read-only mirror, nobody commits to it. https://marc.info/?l=openbsd-cvs=159399360827650=2 > .. breaking compatibility on header and source files. OpenSSH installs no libraries or headers, and has no public API. -Bryan.
Re: mailwrapper: hostsat and purgestat symlinks
Since there are mailers which want these features, I think removing this out of base is silly. mailwrapper exists to abstract it, and why shouldn't it remain as-is? Todd C. Miller wrote: > On Thu, 23 Jul 2020 21:41:53 +0200, Klemens Nanni wrote: > > > Ping. > > > > Feedback? Objections? OK? > > Not OK as-is. > > This will break hoststat/purgestat for anyone who uses the sendmail > port. See mail/sendmail/files/mailer.conf.sendmail in ports for > how this is used. > > However, since these are really sendmail-specific we could consider > adding moving the links to the sendmail package and install them > in /usr/local/bin. > > - todd >
Re: top: add / as alias for g (grep)
On Thu, 23 Jul 2020 22:04:24 +0200, Klemens Nanni wrote: > I've somehow hit the slash way to often for searching a particular > command, would anyone object if I added it as a command character? No objection from me. - todd
Re: mailwrapper: hostsat and purgestat symlinks
On Thu, 23 Jul 2020 21:41:53 +0200, Klemens Nanni wrote: > Ping. > > Feedback? Objections? OK? Not OK as-is. This will break hoststat/purgestat for anyone who uses the sendmail port. See mail/sendmail/files/mailer.conf.sendmail in ports for how this is used. However, since these are really sendmail-specific we could consider adding moving the links to the sendmail package and install them in /usr/local/bin. - todd
Re: mailwrapper: hostsat and purgestat symlinks
On Thu, Jul 23, 2020 at 10:25:01PM +0100, Jason McIntyre wrote: > yes, i supplied feedback to this diff on the day you mailed it. my reply > was: > > sendmail. they add compatibility for sendmail-compatible mailers. some > folks are used to having them around. > > i guess gilles or guenther or millert would be good folks to ask about > whether its worth removing them. > > some aspects of the mailer system are a bit weird because of the > dominance of sendmail. > > and you never replied. > > did you ask any of our mailer guys? Pardon, I simply wrote this mail to tech@ as means to reach out to mail folks rather than addressing some folks directly (and perhaps missing anyone).
Re: top: add / as alias for g (grep)
On Thu, Jul 23, 2020 at 10:04:24PM +0200, Klemens Nanni wrote: > I've somehow hit the slash way to often for searching a particular > command, would anyone object if I added it as a command character? > > While here, what's up with the weird markup in top.1 for `n|# count'? > I've simplified that before adopting it, `mandoc -Tlint ./top.1' is > happy with it. > we used to use \*(Ba instead of "|". i think it made mark up simpler in some cases. and i think ingo wants us to reduce it (and he'll probably chip in) so i guess that particular change is fine. jmc
Re: mailwrapper: hostsat and purgestat symlinks
On Thu, Jul 23, 2020 at 09:41:53PM +0200, Klemens Nanni wrote: > On Mon, Jun 22, 2020 at 04:09:49AM +0200, Klemens Nanni wrote: > > Doing "*stat " in my shell I came across those two entries > > under /usr/bin/ which are undocumented: > > > > $ man -k any~'^(host|purge)stat$' > > man: nothing appropriate > > > > /etc/mailer.conf has no entries for them but mailer.conf(5)' EXAMPLES > > section demonstrates using them with the mail/sendmail port. > > > > CVS log has it that we've been installing those two symlinks two > > mailwrapper since its import from NetBSD in 1999 and mentioning the > > examples since 2001, but I couldn't find a specific commit that > > eventually removed their supporting code. > > > > All other symlinks to mailwrapper(8) (sendmail, newaliases, mailq and > > mailmap) have their own manual pages and respective argv specific code > > under /usr/src/usr.sbin/. > > > > Assuming that purgestat and hoststat usage/code went away with removing > > sendmail from base at latest, can their symlinks be considered leftovers > > and removed? > Ping. > > Feedback? Objections? OK? > yes, i supplied feedback to this diff on the day you mailed it. my reply was: sendmail. they add compatibility for sendmail-compatible mailers. some folks are used to having them around. i guess gilles or guenther or millert would be good folks to ask about whether its worth removing them. some aspects of the mailer system are a bit weird because of the dominance of sendmail. and you never replied. did you ask any of our mailer guys? jmc
top: add / as alias for g (grep)
I've somehow hit the slash way to often for searching a particular
command, would anyone object if I added it as a command character?
While here, what's up with the weird markup in top.1 for `n|# count'?
I've simplified that before adopting it, `mandoc -Tlint ./top.1' is
happy with it.
Feedback? OK?
Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.62
diff -u -p -r1.62 display.c
--- display.c 27 Jun 2020 01:09:57 - 1.62
+++ display.c 23 Jul 2020 20:00:05 -
@@ -810,7 +810,7 @@ show_help(void)
"C- toggle the display of command line arguments\n"
"d count - show `count' displays, then exit\n"
"e- list errors generated by last \"kill\" or
\"renice\" command\n"
- "g string - filter on command name (g+ selects all commands)\n"
+ "g|/ string - filter on command name (g+ or /+ selects all
commands)\n"
"h | ?- help; show this text\n"
"H- toggle the display of threads\n"
"I | i- toggle the display of idle processes\n"
Index: top.1
===
RCS file: /cvs/src/usr.bin/top/top.1,v
retrieving revision 1.74
diff -u -p -r1.74 top.1
--- top.1 23 Jun 2020 21:39:19 - 1.74
+++ top.1 23 Jul 2020 20:00:54 -
@@ -308,12 +308,14 @@ Display a list of system errors (if any)
or
.Li renice
command.
-.It g Ar string
+.It g|/ Ar string
Display only processes that contain
.Ar string
in their command name.
If displaying of arguments is enabled, the arguments are searched too.
.Sq g+
+or
+.Sq /+
shows all processes.
.It H
Toggle the display of process threads.
@@ -330,7 +332,7 @@ by default) to process
.Ar pid .
This acts similarly to the command
.Xr kill 1 .
-.It n\*(Ba# Ar count
+.It n|# Ar count
Show
.Ar count
processes.
Index: top.c
===
RCS file: /cvs/src/usr.bin/top/top.c,v
retrieving revision 1.103
diff -u -p -r1.103 top.c
--- top.c 25 Jun 2020 20:38:41 - 1.103
+++ top.c 23 Jul 2020 19:50:49 -
@@ -131,6 +131,7 @@ struct statics statics;
#define CMD_up 25
#define CMD_pagedown 26
#define CMD_pageup 27
+#define CMD_grep2 28
static void
usage(void)
@@ -631,7 +632,7 @@ rundisplay(void)
char ch, *iptr;
int change, i;
struct pollfd pfd[1];
- static char command_chars[] = "\f qh?en#sdkriIuSopCHg+P109)(";
+ static char command_chars[] = "\f qh?en#sdkriIuSopCHg+P109)(/";
/*
* assume valid command unless told
@@ -937,6 +938,7 @@ rundisplay(void)
break;
case CMD_grep:
+ case CMD_grep2:
new_message(MT_standout,
"Grep command name: ");
if (readline(tempbuf, sizeof(tempbuf)) > 0) {
Re: mailwrapper: hostsat and purgestat symlinks
On Mon, Jun 22, 2020 at 04:09:49AM +0200, Klemens Nanni wrote:
> Doing "*stat " in my shell I came across those two entries
> under /usr/bin/ which are undocumented:
>
> $ man -k any~'^(host|purge)stat$'
> man: nothing appropriate
>
> /etc/mailer.conf has no entries for them but mailer.conf(5)' EXAMPLES
> section demonstrates using them with the mail/sendmail port.
>
> CVS log has it that we've been installing those two symlinks two
> mailwrapper since its import from NetBSD in 1999 and mentioning the
> examples since 2001, but I couldn't find a specific commit that
> eventually removed their supporting code.
>
> All other symlinks to mailwrapper(8) (sendmail, newaliases, mailq and
> mailmap) have their own manual pages and respective argv specific code
> under /usr/src/usr.sbin/.
>
> Assuming that purgestat and hoststat usage/code went away with removing
> sendmail from base at latest, can their symlinks be considered leftovers
> and removed?
Ping.
Feedback? Objections? OK?
Index: distrib/sets/lists/base/mi
===
RCS file: /cvs/src/distrib/sets/lists/base/mi,v
retrieving revision 1.991
diff -u -p -r1.991 mi
--- distrib/sets/lists/base/mi 30 Jun 2020 01:42:18 - 1.991
+++ distrib/sets/lists/base/mi 30 Jun 2020 14:30:03 -
@@ -411,7 +411,6 @@
./usr/bin/help
./usr/bin/hexdump
./usr/bin/host
-./usr/bin/hoststat
./usr/bin/htpasswd
./usr/bin/id
./usr/bin/ident
@@ -510,7 +509,6 @@
./usr/bin/printenv
./usr/bin/printf
./usr/bin/prove
-./usr/bin/purgestat
./usr/bin/quota
./usr/bin/radioctl
./usr/bin/rcs
Index: usr.sbin/mailwrapper/Makefile
===
RCS file: /cvs/src/usr.sbin/mailwrapper/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- usr.sbin/mailwrapper/Makefile 11 Sep 2016 07:06:30 - 1.6
+++ usr.sbin/mailwrapper/Makefile 22 Jun 2020 00:46:59 -
@@ -11,11 +11,8 @@ afterinstall:
ln -fs /usr/sbin/mailwrapper ${DESTDIR}/usr/bin/newaliases
ln -fs /usr/sbin/mailwrapper ${DESTDIR}/usr/bin/mailq
ln -fs /usr/sbin/mailwrapper ${DESTDIR}/usr/sbin/makemap
- ln -fs /usr/sbin/mailwrapper ${DESTDIR}/usr/bin/hoststat
- ln -fs /usr/sbin/mailwrapper ${DESTDIR}/usr/bin/purgestat
chown -h ${BINOWN}:${BINGRP} ${DESTDIR}/usr/sbin/sendmail \
${DESTDIR}/usr/bin/newaliases ${DESTDIR}/usr/bin/mailq \
- ${DESTDIR}/usr/sbin/makemap ${DESTDIR}/usr/bin/hoststat \
- ${DESTDIR}/usr/bin/purgestat
+ ${DESTDIR}/usr/sbin/makemap
.include
Re: Add ability to set control values with video(1)
Hi Laurie, On Thu, 23 Jul 2020 21:07:26 +0100 Laurence Tratt wrote: > On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote: > > Hello Marcus, > > > We read the current value of the white balance temperature auto > > control (since this gets set correctly during the reset), and just > > reset it again on the next cam start up after the video stream has > > been started? > > Aha, very clever -- I hadn't thought of doing that! > > I've tried your version and it works correctly in all the scenarios I > can think of. I think this is now ready to go in! Ok, cool - I try to collect an OK therefore :-) > > Laurie > Thanks, Marcus
Re: Add ability to set control values with video(1)
On Thu, Jul 23, 2020 at 09:56:39PM +0200, Marcus Glocker wrote: Hello Marcus, > We read the current value of the white balance temperature auto control > (since this gets set correctly during the reset), and just reset it again > on the next cam start up after the video stream has been started? Aha, very clever -- I hadn't thought of doing that! I've tried your version and it works correctly in all the scenarios I can think of. I think this is now ready to go in! Laurie
Re: Add ability to set control values with video(1)
On Wed, 22 Jul 2020 21:52:27 +0100
Laurence Tratt wrote:
> On Wed, Jul 22, 2020 at 10:23:19PM +0200, Marcus Glocker wrote:
>
> Hello Marcus,
>
> > I've tested this here as well in the meantime by leaving
> > mmap_init() on its original location (doesn't get involved for '-c
> > reset') with three different cams:
> >
> > * Logitech C930e: I see the same problem like you do with your C920
> > finally.
> > * Logitech QuickCam Pro 9000: reset works fine.
> > * SunplusIT Inc Integrated Camera: reset works fine.
>
> Hmph, that's slightly annoying of Logitech :/
>
> > This seems to be a problem only with some cams when turning the
> > auto white balance temperature back on while the stream is off, the
> > setting doesn't get recognized by the cam afterwards.
> >
> > I'm basically OK with your last diff, except the mmap_init()
> > location change: I don't like to turn the cam stream on only for
> > setting this parameter because some cams can't handle the obvious.
> >
> > I tried out some things with the C930e to get the auto white balance
> > temperature back on without having the stream on, but no luck so
> > far.
> >
> > I would aim to get your diff in without the mmap_init() change.
> > Maybe we'll find a solution/workaround for this partial problem
> > later?
>
> If we can find a change that allows this, I'd be happy! When I briefly
> tested things on Windows, the Logitech app there activated the stream
> before changing settings, so it's quite possible that they never
> tested changing some settings with the stream off. v4l2-ctl might
> have some clues in it, but their model is subtly different than ours
> and forces the user to understand a lot more about their camera (e.g.
> they force the user to turn off auto white balance before they allow
> them to set manual white balance, a differentiation which IMHO only
> makes sense if you've read the UVC spec).
>
> That makes me think that if/when uvideo is extended to deal with
> terminal control requests (e.g. zoom, exposure, focus) we'll find
> that other settings with "auto" options have similar problems.
> Honestly, if the price of controlling exposure and focus is having to
> turn the camera stream on, I think I will consider that an acceptable
> trade-off, given how useful those settings are :)
How's that?
We read the current value of the white balance temperature auto control
(since this gets set correctly during the reset), and just reset it
again on the next cam start up after the video stream has been started?
By that we avoid the video stream on/off dance when using '-c'.
I've quickly tested this here and it seems to work for me ...
Index: video.1
===
RCS file: /cvs/xenocara/app/video/video.1,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 video.1
--- video.1 17 Jul 2020 07:51:23 - 1.15
+++ video.1 23 Jul 2020 19:44:21 -
@@ -27,6 +27,7 @@
.Bk -words
.Op Fl \
.Op Fl a Ar adaptor
+.Op Fl c Ar reset | control=value
.Op Fl e Ar encoding
.Op Fl f Ar file
.Op Fl i Ar input
@@ -81,6 +82,15 @@ Index of
adaptor to use.
The default is 0, the first adaptor reported by
.Xr X 7 .
+.It Fl c Ar reset | control=value
+Set control value (e.g. brightness) and exit. The special name
+.Ql reset
+resets all values to their default. The available controls can be found
+with
+.Fl q
+and the default values are displayed during a reset with
+.Fl c Ar reset
+.Fl v .
.It Fl e Ar encoding
Lowercase FOURCC name of video encoding to use.
Valid arguments are
Index: video.c
===
RCS file: /cvs/xenocara/app/video/video.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 video.c
--- video.c 17 Jul 2020 07:51:23 - 1.31
+++ video.c 23 Jul 2020 19:44:22 -
@@ -192,7 +192,10 @@ struct video {
#define M_IN_FILE 0x4
#define M_OUT_FILE 0x8
#define M_QUERY0x10
+#define M_RESET0x20
+#define M_SET_CTRL 0x40
int mode;
+ char*set_ctrl_str;
int verbose;
};
@@ -212,10 +215,12 @@ int dev_get_ctrls(struct video *);
void dev_dump_info(struct video *);
void dev_dump_query(struct video *);
int dev_init(struct video *);
-void dev_set_ctrl(struct video *, int, int);
-void dev_set_ctrl_auto_white_balance(struct video *, int);
+void dev_set_ctrl_abs(struct video *vid, int, int);
+void dev_set_ctrl_rel(struct video *, int, int);
+void dev_set_ctrl_auto_white_balance(struct video *, int, int);
void dev_reset_ctrls(struct video *);
+int parse_ctrl(struct video *, int *, int *);
int parse_size(struct video *);
int choose_size(struct video *);
int choose_enc(struct video *);
@@ -241,9 +246,9 @@ void
usage(void)
{
fprintf(stderr, "usage: %s [-gqRv] "
- "[-a adaptor] [-e encoding] [-f file] [-i input] [-O output]\n"
- " %*s [-o output] [-r rate] [-s
Re: pf: route-to {random,srchash} in an anchor
Hello Yasuok,
On Thu, Jul 23, 2020 at 08:01:18PM +0900, YASUOKA Masahiko wrote:
> Hi,
>
> Last month, I fixed the problem "route-to least-state" in an anchor
> didn't work.
>
> https://marc.info/?t=15911745782=1=2
>
> I noticed the same problem happens on "random" and "srchash" as well.
>
> ok?
change looks good. I have just one nit-pick comment. I leave decision
whether it's worth to adjust your diff or commit as-is up to you.
see in-line further below.
OK sashan@
>
> Use the table on root always if current table is not active.
>
> Index: sys/net/pf_lb.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 pf_lb.c
> --- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
> +++ sys/net/pf_lb.c 23 Jul 2020 10:45:48 -
> @@ -345,6 +345,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
> struct pf_addr faddr;
> struct pf_addr *raddr = >addr.v.a.addr;
> struct pf_addr *rmask = >addr.v.a.mask;
> + struct pfr_ktable *kt;
> u_int64_tstates;
> u_int16_tweight;
> u_int64_tload;
> @@ -396,18 +397,17 @@ pf_map_addr(sa_family_t af, struct pf_ru
> pf_poolmask(naddr, raddr, rmask, saddr, af);
> break;
> case PF_POOL_RANDOM:
> - if (rpool->addr.type == PF_ADDR_TABLE) {
> - cnt = rpool->addr.p.tbl->pfrkt_cnt;
> - if (cnt == 0)
> - rpool->tblidx = 0;
> + if (rpool->addr.type == PF_ADDR_TABLE ||
> + rpool->addr.type == PF_ADDR_DYNIFTL) {
> + if (rpool->addr.type == PF_ADDR_TABLE)
> + kt = rpool->addr.p.tbl;
> else
> - rpool->tblidx = (int)arc4random_uniform(cnt);
> - memset(>counter, 0, sizeof(rpool->counter));
> - if (pfr_pool_get(rpool, , , af))
> + kt = rpool->addr.p.dyn->pfid_kt;
> + kt = pfr_ktable_select_active(kt);
> + if (!kt)
I would prefer to use 'kt == NULL', so it becomes
clear we deal with pointer.
> @@ -453,18 +453,18 @@ pf_map_addr(sa_family_t af, struct pf_ru
> case PF_POOL_SRCHASH:
> hashidx =
> pf_hash(saddr, (struct pf_addr *), >key, af);
> - if (rpool->addr.type == PF_ADDR_TABLE) {
> - cnt = rpool->addr.p.tbl->pfrkt_cnt;
> - if (cnt == 0)
> - rpool->tblidx = 0;
> +
> + if (rpool->addr.type == PF_ADDR_TABLE ||
> + rpool->addr.type == PF_ADDR_DYNIFTL) {
> + if (rpool->addr.type == PF_ADDR_TABLE)
> + kt = rpool->addr.p.tbl;
> else
> - rpool->tblidx = (int)(hashidx % cnt);
> - memset(>counter, 0, sizeof(rpool->counter));
> - if (pfr_pool_get(rpool, , , af))
> + kt = rpool->addr.p.dyn->pfid_kt;
> + kt = pfr_ktable_select_active(kt);
> + if (!kt)
same here.
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pf_table.c,v
> retrieving revision 1.133
> diff -u -p -r1.133 pf_table.c
> --- sys/net/pf_table.c24 Jun 2020 22:03:43 - 1.133
> +++ sys/net/pf_table.c23 Jul 2020 10:45:48 -
> @@ -2108,9 +2108,8 @@ pfr_kentry_byaddr(struct pfr_ktable *kt,
> struct sockaddr_in6 tmp6;
> #endif /* INET6 */
>
> - if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE) && kt->pfrkt_root != NULL)
> - kt = kt->pfrkt_root;
> - if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
> + kt = pfr_ktable_select_active(kt);
> + if (!kt)
^^^
and here too.
> @@ -2308,9 +2306,8 @@ pfr_pool_get(struct pf_pool *rpool, stru
> kt = rpool->addr.p.dyn->pfid_kt;
> else
> return (-1);
> - if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE) && kt->pfrkt_root != NULL)
> - kt = kt->pfrkt_root;
> - if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
> + kt = pfr_ktable_select_active(kt);
> + if (!kt)
here as well
I like the introduction of pfr_ktable_select_active(), it reads far better
than current code in tree.
thanks and
regards
sashan
Re: join(1) remove redundant memory copy
Hi Martijn,
this is a nice simplification, OK schwarze@.
A few nits:
* The MAXIMUM() macro is now unused, so i prefer that you delete
the definition.
* The second getline(3) argument should be size_t, not u_long,
so change that in the struct declaration (it's not used anywhere
else). Not sure whether your type mismatch might cause trouble
on any of our platforms, or if the two types are identical
everywhere. While there, make the comment more intelligible.
* From one comment, delete "and copy line" which is no longer true.
I'm appending the version of your patch that i tested.
Todd C. Miller wrote on Wed, Jul 22, 2020 at 01:19:47PM -0600:
> On Wed, 22 Jul 2020 20:29:06 +0200, Martijn van Duren wrote:
>> /* Remove trailing newline, if it exists, and copy line. */
>> -if (line[len - 1] == '\n')
>> +if (lp->line[len - 1] == '\n')
>> len--;
>> lp->line[len] = '\0';
> You could replace "len--" with "lp->line[len - 1] = '\0'" here (or
> "lp->line[--len] = '\0'" if you prefer but len is otherwise unused).
> Then there would be no need to NUL terminate here since getline(3)
> always NUL terminates.
Indeed. I think this form is clearest, shortest, and safest:
/* Remove the trailing newline, if any. */
if (lp->line[len - 1] == '\n')
p->line[--len] = '\0';
Yours,
Ingo
Index: join.c
===
RCS file: /cvs/src/usr.bin/join/join.c,v
retrieving revision 1.32
diff -u -p -r1.32 join.c
--- join.c 14 Nov 2018 15:16:09 - 1.32
+++ join.c 23 Jul 2020 14:47:06 -
@@ -43,8 +43,6 @@
#include
#include
-#define MAXIMUM(a, b) (((a) > (b)) ? (a) : (b))
-
/*
* There's a structure per input file which encapsulates the state of the
* file. We repeatedly read lines from each file until we've read in all
@@ -53,7 +51,7 @@
*/
typedef struct {
char *line; /* line */
- u_long linealloc; /* line allocated count */
+ size_t linealloc; /* bytes allocated for line */
char **fields; /* line field(s) */
u_long fieldcnt;/* line field(s) count */
u_long fieldalloc; /* line field(s) allocated count */
@@ -271,9 +269,8 @@ slurp(INPUT *F)
{
LINE *lp, *lastlp, tmp;
ssize_t len;
- size_t linesize;
u_long cnt;
- char *bp, *fieldp, *line;
+ char *bp, *fieldp;
/*
* Read all of the lines from an input file that have the same
@@ -281,8 +278,6 @@ slurp(INPUT *F)
*/
F->setcnt = 0;
- line = NULL;
- linesize = 0;
for (lastlp = NULL; ; ++F->setcnt) {
/*
* If we're out of space to hold line structures, allocate
@@ -320,23 +315,12 @@ slurp(INPUT *F)
F->pushbool = 0;
continue;
}
- if ((len = getline(, , F->fp)) == -1)
+ if ((len = getline(&(lp->line), &(lp->linealloc), F->fp)) == -1)
break;
- /* Remove trailing newline, if it exists, and copy line. */
- if (line[len - 1] == '\n')
- len--;
- if (lp->linealloc <= len + 1) {
- char *p;
- u_long newsize = lp->linealloc +
- MAXIMUM(100, len + 1 - lp->linealloc);
- if ((p = realloc(lp->line, newsize)) == NULL)
- err(1, NULL);
- lp->line = p;
- lp->linealloc = newsize;
- }
- memcpy(lp->line, line, len);
- lp->line[len] = '\0';
+ /* Remove the trailing newline, if any. */
+ if (lp->line[len - 1] == '\n')
+ lp->line[--len] = '\0';
/* Split the line into fields, allocate space as necessary. */
lp->fieldcnt = 0;
@@ -363,7 +347,6 @@ slurp(INPUT *F)
break;
}
}
- free(line);
}
char *
Re: pf: route-to least-states
> On 23. Jul 2020, at 13:23, YASUOKA Masahiko wrote:
>
> The diff fixes 2 problems of "least-states":
>
> - states whose address is selected by sticky-address is not counted
> for the number of states.
> - interface is not selected properly if selected table entry specifies
> an interface.
>
> ok?
Good catch, diff looks good to me.
ok jung@
> Increase state counter for least-states when the address is selected
> by sticky-address. Also fix the problem that the interface which is
> specified by the selected table entry is not used properly.
>
> Index: sys/net/pf_lb.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 pf_lb.c
> --- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
> +++ sys/net/pf_lb.c 23 Jul 2020 11:06:05 -
> @@ -97,6 +97,8 @@ u_int64_tpf_hash(struct pf_addr *, st
> intpf_get_sport(struct pf_pdesc *, struct pf_rule *,
> struct pf_addr *, u_int16_t *, u_int16_t,
> u_int16_t, struct pf_src_node **);
> +int pf_map_addr_states_increase(sa_family_t,
> + struct pf_pool *, struct pf_addr *);
> intpf_get_transaddr_af(struct pf_rule *,
> struct pf_pdesc *, struct pf_src_node **);
> intpf_map_addr_sticky(sa_family_t, struct pf_rule *,
> @@ -319,6 +321,12 @@ pf_map_addr_sticky(sa_family_t af, struc
> sns[type] = NULL;
> return (-1);
> }
> +
> + if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (-1);
> + }
> +
> if (!PF_AZERO(cached, af))
> pf_addrcpy(naddr, cached, af);
> if (pf_status.debug >= LOG_DEBUG) {
> @@ -345,6 +353,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
> struct pf_addr faddr;
> struct pf_addr *raddr = >addr.v.a.addr;
> struct pf_addr *rmask = >addr.v.a.mask;
> + struct pfi_kif *kif;
> u_int64_tstates;
> u_int16_tweight;
> u_int64_tload;
> @@ -539,6 +548,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
>
> states = rpool->states;
> weight = rpool->weight;
> + kif = rpool->kif;
>
> if ((rpool->addr.type == PF_ADDR_TABLE &&
> rpool->addr.p.tbl->pfrkt_refcntcost > 0) ||
> @@ -581,6 +591,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
> if (cload < load) {
> states = rpool->states;
> weight = rpool->weight;
> + kif = rpool->kif;
> load = cload;
>
> pf_addrcpy(naddr, >counter, af);
> @@ -591,29 +602,10 @@ pf_map_addr(sa_family_t af, struct pf_ru
> } while (pf_match_addr(1, , rmask, >counter, af) &&
> (states > 0));
>
> - if (rpool->addr.type == PF_ADDR_TABLE) {
> - if (pfr_states_increase(rpool->addr.p.tbl,
> - naddr, af) == -1) {
> - if (pf_status.debug >= LOG_DEBUG) {
> - log(LOG_DEBUG,"pf: pf_map_addr: "
> - "selected address ");
> - pf_print_host(naddr, 0, af);
> - addlog(". Failed to increase count!\n");
> - }
> - return (1);
> - }
> - } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
> - if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
> - naddr, af) == -1) {
> - if (pf_status.debug >= LOG_DEBUG) {
> - log(LOG_DEBUG, "pf: pf_map_addr: "
> - "selected address ");
> - pf_print_host(naddr, 0, af);
> - addlog(". Failed to increase count!\n");
> - }
> - return (1);
> - }
> - }
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (1);
> + /* revert the kif which was set by pfr_pool_get() */
> + rpool->kif = kif;
> break;
> }
>
> @@ -642,6 +634,38 @@ pf_map_addr(sa_family_t af, struct pf_ru
> addlog("\n");
> }
>
> + return (0);
> +}
> +
> +int
> +pf_map_addr_states_increase(sa_family_t af, struct pf_pool *rpool,
> +struct pf_addr *naddr)
> +{
>
pf: route-to least-states
Hi,
The diff fixes 2 problems of "least-states":
- states whose address is selected by sticky-address is not counted
for the number of states.
- interface is not selected properly if selected table entry specifies
an interface.
ok?
Increase state counter for least-states when the address is selected
by sticky-address. Also fix the problem that the interface which is
specified by the selected table entry is not used properly.
Index: sys/net/pf_lb.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
retrieving revision 1.64
diff -u -p -r1.64 pf_lb.c
--- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
+++ sys/net/pf_lb.c 23 Jul 2020 11:06:05 -
@@ -97,6 +97,8 @@ u_int64_t pf_hash(struct pf_addr *, st
int pf_get_sport(struct pf_pdesc *, struct pf_rule *,
struct pf_addr *, u_int16_t *, u_int16_t,
u_int16_t, struct pf_src_node **);
+int pf_map_addr_states_increase(sa_family_t,
+ struct pf_pool *, struct pf_addr *);
int pf_get_transaddr_af(struct pf_rule *,
struct pf_pdesc *, struct pf_src_node **);
int pf_map_addr_sticky(sa_family_t, struct pf_rule *,
@@ -319,6 +321,12 @@ pf_map_addr_sticky(sa_family_t af, struc
sns[type] = NULL;
return (-1);
}
+
+ if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
+ if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ return (-1);
+ }
+
if (!PF_AZERO(cached, af))
pf_addrcpy(naddr, cached, af);
if (pf_status.debug >= LOG_DEBUG) {
@@ -345,6 +353,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
struct pf_addr faddr;
struct pf_addr *raddr = >addr.v.a.addr;
struct pf_addr *rmask = >addr.v.a.mask;
+ struct pfi_kif *kif;
u_int64_tstates;
u_int16_tweight;
u_int64_tload;
@@ -539,6 +548,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
states = rpool->states;
weight = rpool->weight;
+ kif = rpool->kif;
if ((rpool->addr.type == PF_ADDR_TABLE &&
rpool->addr.p.tbl->pfrkt_refcntcost > 0) ||
@@ -581,6 +591,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
if (cload < load) {
states = rpool->states;
weight = rpool->weight;
+ kif = rpool->kif;
load = cload;
pf_addrcpy(naddr, >counter, af);
@@ -591,29 +602,10 @@ pf_map_addr(sa_family_t af, struct pf_ru
} while (pf_match_addr(1, , rmask, >counter, af) &&
(states > 0));
- if (rpool->addr.type == PF_ADDR_TABLE) {
- if (pfr_states_increase(rpool->addr.p.tbl,
- naddr, af) == -1) {
- if (pf_status.debug >= LOG_DEBUG) {
- log(LOG_DEBUG,"pf: pf_map_addr: "
- "selected address ");
- pf_print_host(naddr, 0, af);
- addlog(". Failed to increase count!\n");
- }
- return (1);
- }
- } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
- if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
- naddr, af) == -1) {
- if (pf_status.debug >= LOG_DEBUG) {
- log(LOG_DEBUG, "pf: pf_map_addr: "
- "selected address ");
- pf_print_host(naddr, 0, af);
- addlog(". Failed to increase count!\n");
- }
- return (1);
- }
- }
+ if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ return (1);
+ /* revert the kif which was set by pfr_pool_get() */
+ rpool->kif = kif;
break;
}
@@ -642,6 +634,38 @@ pf_map_addr(sa_family_t af, struct pf_ru
addlog("\n");
}
+ return (0);
+}
+
+int
+pf_map_addr_states_increase(sa_family_t af, struct pf_pool *rpool,
+struct pf_addr *naddr)
+{
+ if (rpool->addr.type == PF_ADDR_TABLE) {
+ if (pfr_states_increase(rpool->addr.p.tbl,
+ naddr, af) == -1) {
+
pf: route-to {random,srchash} in an anchor
Hi,
Last month, I fixed the problem "route-to least-state" in an anchor
didn't work.
https://marc.info/?t=15911745782=1=2
I noticed the same problem happens on "random" and "srchash" as well.
ok?
Use the table on root always if current table is not active.
Index: sys/net/pf_lb.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
retrieving revision 1.64
diff -u -p -r1.64 pf_lb.c
--- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
+++ sys/net/pf_lb.c 23 Jul 2020 10:45:48 -
@@ -345,6 +345,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
struct pf_addr faddr;
struct pf_addr *raddr = >addr.v.a.addr;
struct pf_addr *rmask = >addr.v.a.mask;
+ struct pfr_ktable *kt;
u_int64_tstates;
u_int16_tweight;
u_int64_tload;
@@ -396,18 +397,17 @@ pf_map_addr(sa_family_t af, struct pf_ru
pf_poolmask(naddr, raddr, rmask, saddr, af);
break;
case PF_POOL_RANDOM:
- if (rpool->addr.type == PF_ADDR_TABLE) {
- cnt = rpool->addr.p.tbl->pfrkt_cnt;
- if (cnt == 0)
- rpool->tblidx = 0;
+ if (rpool->addr.type == PF_ADDR_TABLE ||
+ rpool->addr.type == PF_ADDR_DYNIFTL) {
+ if (rpool->addr.type == PF_ADDR_TABLE)
+ kt = rpool->addr.p.tbl;
else
- rpool->tblidx = (int)arc4random_uniform(cnt);
- memset(>counter, 0, sizeof(rpool->counter));
- if (pfr_pool_get(rpool, , , af))
+ kt = rpool->addr.p.dyn->pfid_kt;
+ kt = pfr_ktable_select_active(kt);
+ if (!kt)
return (1);
- pf_addrcpy(naddr, >counter, af);
- } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
- cnt = rpool->addr.p.dyn->pfid_kt->pfrkt_cnt;
+
+ cnt = kt->pfrkt_cnt;
if (cnt == 0)
rpool->tblidx = 0;
else
@@ -453,18 +453,18 @@ pf_map_addr(sa_family_t af, struct pf_ru
case PF_POOL_SRCHASH:
hashidx =
pf_hash(saddr, (struct pf_addr *), >key, af);
- if (rpool->addr.type == PF_ADDR_TABLE) {
- cnt = rpool->addr.p.tbl->pfrkt_cnt;
- if (cnt == 0)
- rpool->tblidx = 0;
+
+ if (rpool->addr.type == PF_ADDR_TABLE ||
+ rpool->addr.type == PF_ADDR_DYNIFTL) {
+ if (rpool->addr.type == PF_ADDR_TABLE)
+ kt = rpool->addr.p.tbl;
else
- rpool->tblidx = (int)(hashidx % cnt);
- memset(>counter, 0, sizeof(rpool->counter));
- if (pfr_pool_get(rpool, , , af))
+ kt = rpool->addr.p.dyn->pfid_kt;
+ kt = pfr_ktable_select_active(kt);
+ if (!kt)
return (1);
- pf_addrcpy(naddr, >counter, af);
- } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
- cnt = rpool->addr.p.dyn->pfid_kt->pfrkt_cnt;
+
+ cnt = kt->pfrkt_cnt;
if (cnt == 0)
rpool->tblidx = 0;
else
Index: sys/net/pf_table.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pf_table.c,v
retrieving revision 1.133
diff -u -p -r1.133 pf_table.c
--- sys/net/pf_table.c 24 Jun 2020 22:03:43 - 1.133
+++ sys/net/pf_table.c 23 Jul 2020 10:45:48 -
@@ -2108,9 +2108,8 @@ pfr_kentry_byaddr(struct pfr_ktable *kt,
struct sockaddr_in6 tmp6;
#endif /* INET6 */
- if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE) && kt->pfrkt_root != NULL)
- kt = kt->pfrkt_root;
- if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
+ kt = pfr_ktable_select_active(kt);
+ if (!kt)
return (0);
switch (af) {
@@ -2153,9 +2152,8 @@ pfr_update_stats(struct pfr_ktable *kt,
int dir_idx = (pd->dir == PF_OUT);
int op_idx;
- if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE) && kt->pfrkt_root != NULL)
- kt = kt->pfrkt_root;
- if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
+ kt = pfr_ktable_select_active(kt);
+ if (!kt)
return;
switch (af) {
@@ -2308,9 +2306,8 @@ pfr_pool_get(struct pf_pool *rpool, stru
