Re: Unbreak X:Y user/group spec in pf.conf
16 января 2020 г. 9:14:43 GMT+03:00, Theo de Raadt пишет:
>I'll bite, using text from your regress.
>
>> +pass out proto tcp all user 1234:12345 flags S/SA
>> +pass out proto tcp all user 0:12345 flags S/SA
>> +pass out proto tcp all group 1234:12345 flags S/SA
>> +pass out proto tcp all group 0:12345 flags S/SA
>
>What does 1234:12345 mean. It must be uid 1234 _and_ gid 12345?
The manpage for "user"and "group" clauses say they have similar syntax with
"port". I was surprised when tried to use "X:Y" notation in "user"clause,
though.
>I don't quite understand, what is the purpose to this precise check?
>How often does this kind of precise request happen?
>
>i would expect the normal pattern is to pass "for this uid" or "for
>this gid", but I'm surprised to see "for this uid AND this gid, both
>must match", and I doubt you are representing "either this uid or this
>gid".
>
>Can you explain the use case?
I have a server with shell accounts for students. Their uids are in, say, 2000
to 5999 range. I want to allow all of them to connect e.g. to Github or
anoncvs, but not to whole Internet.
I can use "1999><6000" as alternative, of course, but then manual must be
tweaked, IMO.
>Vadim Zhukov wrote:
>
>> I've just found that pfctl doesn't like 'X:Y' syntax for user and
>group
>> clauses, despite of the words in manpage. The problem is caused by
>> parser eating the colon character in STRING version of "uid" and
>"gid"
>> rules. The solution is similar to the way ports parsing is done. Now
>we
>> have "uidrange" and "gidrange" rules, similar to "portrange". I
>didn't
>> try to unify those two (or even three) to avoid increasing
>parentheses
>> madness, but if somebody would come with better diff/idea, I'm open.
>:)
>>
>> This diff also adds a regression test, for the sake of completeness.
>> Existing regression tests pass on amd64.
>>
>> OK?
>>
>> --
>> WBR,
>> Vadim Zhukov
>>
>>
>> Index: sbin/pfctl/parse.y
>> ===
>> RCS file: /cvs/src/sbin/pfctl/parse.y,v
>> retrieving revision 1.699
>> diff -u -p -r1.699 parse.y
>> --- sbin/pfctl/parse.y 17 Oct 2019 21:54:28 - 1.699
>> +++ sbin/pfctl/parse.y 16 Jan 2020 00:44:14 -
>> @@ -468,6 +468,7 @@ typedef struct {
>> #define PPORT_RANGE 1
>> #define PPORT_STAR 2
>> int parseport(char *, struct range *r, int);
>> +int parse_ugid(char *, struct range *r, int);
>>
>> #define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \
>> (!((addr).iflags & PFI_AFLAG_NOALIAS) || \
>> @@ -496,7 +497,7 @@ int parseport(char *, struct range *r, i
>> %tokenNUMBER
>> %token PORTBINARY
>> %type interface if_list if_item_not if_item
>> -%type number icmptype icmp6type uid gid
>> +%type number icmptype icmp6type
>> %type tos not yesno optnodf
>> %typeprobability
>> %type optweight
>> @@ -504,7 +505,7 @@ int parseport(char *, struct range *r, i
>> %type sourcetrack flush unaryop statelock
>> %type action
>> %type flags flag blockspec prio
>> -%type portplain portstar portrange
>> +%type portplain portstar portrange uidrange
>> gidrange
>> %typehashkey
>> %type proto proto_list proto_item
>> %type protoval
>> @@ -2957,69 +2958,67 @@ uid_list : uid_item optnl{ $$ =
>> $1; }
>> }
>> ;
>>
>> -uid_item: uid {
>> +uid_item: uidrange {
>> $$ = calloc(1, sizeof(struct node_uid));
>> if ($$ == NULL)
>> err(1, "uid_item: calloc");
>> -$$->uid[0] = $1;
>> -$$->uid[1] = $1;
>> -$$->op = PF_OP_EQ;
>> +$$->uid[0] = $1.a;
>> +$$->uid[1] = $1.b;
>> +if ($1.t)
>> +$$->op = PF_OP_RRG;
>> +else
>> +$$->op = PF_OP_EQ;
>> $$->next = NULL;
>> $$->tail = $$;
>> }
>> -| unaryop uid {
>> -if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
>> +| unaryop uidrange {
>> +if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
>> yyerror("user unknown requires operator = or "
>> "!=");
>> YYERROR;
>> }
>> +if ($2.t) {
>> +yyerror("':' cannot be used with an other
Re: Unbreak X:Y user/group spec in pf.conf
I'll bite, using text from your regress.
> +pass out proto tcp all user 1234:12345 flags S/SA
> +pass out proto tcp all user 0:12345 flags S/SA
> +pass out proto tcp all group 1234:12345 flags S/SA
> +pass out proto tcp all group 0:12345 flags S/SA
What does 1234:12345 mean. It must be uid 1234 _and_ gid 12345?
I don't quite understand, what is the purpose to this precise check?
How often does this kind of precise request happen?
i would expect the normal pattern is to pass "for this uid" or "for
this gid", but I'm surprised to see "for this uid AND this gid, both
must match", and I doubt you are representing "either this uid or this
gid".
Can you explain the use case?
Vadim Zhukov wrote:
> I've just found that pfctl doesn't like 'X:Y' syntax for user and group
> clauses, despite of the words in manpage. The problem is caused by
> parser eating the colon character in STRING version of "uid" and "gid"
> rules. The solution is similar to the way ports parsing is done. Now we
> have "uidrange" and "gidrange" rules, similar to "portrange". I didn't
> try to unify those two (or even three) to avoid increasing parentheses
> madness, but if somebody would come with better diff/idea, I'm open. :)
>
> This diff also adds a regression test, for the sake of completeness.
> Existing regression tests pass on amd64.
>
> OK?
>
> --
> WBR,
> Vadim Zhukov
>
>
> Index: sbin/pfctl/parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.699
> diff -u -p -r1.699 parse.y
> --- sbin/pfctl/parse.y17 Oct 2019 21:54:28 - 1.699
> +++ sbin/pfctl/parse.y16 Jan 2020 00:44:14 -
> @@ -468,6 +468,7 @@ typedef struct {
> #define PPORT_RANGE 1
> #define PPORT_STAR 2
> int parseport(char *, struct range *r, int);
> +int parse_ugid(char *, struct range *r, int);
>
> #define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \
> (!((addr).iflags & PFI_AFLAG_NOALIAS) || \
> @@ -496,7 +497,7 @@ int parseport(char *, struct range *r, i
> %token NUMBER
> %token PORTBINARY
> %type interface if_list if_item_not if_item
> -%type number icmptype icmp6type uid gid
> +%type number icmptype icmp6type
> %type tos not yesno optnodf
> %type probability
> %type optweight
> @@ -504,7 +505,7 @@ int parseport(char *, struct range *r, i
> %type sourcetrack flush unaryop statelock
> %type action
> %type flags flag blockspec prio
> -%type portplain portstar portrange
> +%type portplain portstar portrange uidrange
> gidrange
> %type hashkey
> %type proto proto_list proto_item
> %type protoval
> @@ -2957,69 +2958,67 @@ uid_list : uid_item optnl{ $$ =
> $1; }
> }
> ;
>
> -uid_item : uid {
> +uid_item : uidrange {
> $$ = calloc(1, sizeof(struct node_uid));
> if ($$ == NULL)
> err(1, "uid_item: calloc");
> - $$->uid[0] = $1;
> - $$->uid[1] = $1;
> - $$->op = PF_OP_EQ;
> + $$->uid[0] = $1.a;
> + $$->uid[1] = $1.b;
> + if ($1.t)
> + $$->op = PF_OP_RRG;
> + else
> + $$->op = PF_OP_EQ;
> $$->next = NULL;
> $$->tail = $$;
> }
> - | unaryop uid {
> - if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
> + | unaryop uidrange {
> + if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
> yyerror("user unknown requires operator = or "
> "!=");
> YYERROR;
> }
> + if ($2.t) {
> + yyerror("':' cannot be used with an other "
> + "user operator");
> + YYERROR;
> + }
> $$ = calloc(1, sizeof(struct node_uid));
> if ($$ == NULL)
> err(1, "uid_item: calloc");
> - $$->uid[0] = $2;
> - $$->uid[1] = $2;
> + $$->uid[0] = $2.a;
> + $$->uid[1] = $2.b;
> $$->op = $1;
> $$->next = NULL;
>
Unbreak X:Y user/group spec in pf.conf
Hi all.
I've just found that pfctl doesn't like 'X:Y' syntax for user and group
clauses, despite of the words in manpage. The problem is caused by
parser eating the colon character in STRING version of "uid" and "gid"
rules. The solution is similar to the way ports parsing is done. Now we
have "uidrange" and "gidrange" rules, similar to "portrange". I didn't
try to unify those two (or even three) to avoid increasing parentheses
madness, but if somebody would come with better diff/idea, I'm open. :)
This diff also adds a regression test, for the sake of completeness.
Existing regression tests pass on amd64.
OK?
--
WBR,
Vadim Zhukov
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.699
diff -u -p -r1.699 parse.y
--- sbin/pfctl/parse.y 17 Oct 2019 21:54:28 - 1.699
+++ sbin/pfctl/parse.y 16 Jan 2020 00:44:14 -
@@ -468,6 +468,7 @@ typedef struct {
#define PPORT_RANGE1
#define PPORT_STAR 2
intparseport(char *, struct range *r, int);
+intparse_ugid(char *, struct range *r, int);
#define DYNIF_MULTIADDR(addr) ((addr).type == PF_ADDR_DYNIFTL && \
(!((addr).iflags & PFI_AFLAG_NOALIAS) || \
@@ -496,7 +497,7 @@ int parseport(char *, struct range *r, i
%token NUMBER
%tokenPORTBINARY
%type interface if_list if_item_not if_item
-%typenumber icmptype icmp6type uid gid
+%typenumber icmptype icmp6type
%typetos not yesno optnodf
%type probability
%typeoptweight
@@ -504,7 +505,7 @@ int parseport(char *, struct range *r, i
%type sourcetrack flush unaryop statelock
%type action
%type flags flag blockspec prio
-%type portplain portstar portrange
+%type portplain portstar portrange uidrange gidrange
%type hashkey
%type proto proto_list proto_item
%typeprotoval
@@ -2957,69 +2958,67 @@ uid_list: uid_item optnl{ $$ =
$1; }
}
;
-uid_item : uid {
+uid_item : uidrange {
$$ = calloc(1, sizeof(struct node_uid));
if ($$ == NULL)
err(1, "uid_item: calloc");
- $$->uid[0] = $1;
- $$->uid[1] = $1;
- $$->op = PF_OP_EQ;
+ $$->uid[0] = $1.a;
+ $$->uid[1] = $1.b;
+ if ($1.t)
+ $$->op = PF_OP_RRG;
+ else
+ $$->op = PF_OP_EQ;
$$->next = NULL;
$$->tail = $$;
}
- | unaryop uid {
- if ($2 == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
+ | unaryop uidrange {
+ if ($2.a == -1 && $1 != PF_OP_EQ && $1 != PF_OP_NE) {
yyerror("user unknown requires operator = or "
"!=");
YYERROR;
}
+ if ($2.t) {
+ yyerror("':' cannot be used with an other "
+ "user operator");
+ YYERROR;
+ }
$$ = calloc(1, sizeof(struct node_uid));
if ($$ == NULL)
err(1, "uid_item: calloc");
- $$->uid[0] = $2;
- $$->uid[1] = $2;
+ $$->uid[0] = $2.a;
+ $$->uid[1] = $2.b;
$$->op = $1;
$$->next = NULL;
$$->tail = $$;
}
- | uid PORTBINARY uid{
- if ($1 == -1 || $3 == -1) {
+ | uidrange PORTBINARY uidrange {
+ if ($1.a == -1 || $3.a == -1) {
yyerror("user unknown requires operator = or "
"!=");
YYERROR;
}
+ if ($1.t || $3.t) {
+ yyerror("':' cannot be used with an other "
+ "user operator");
+ YYERROR;
+ }
$$ = calloc(1, sizeof(struct node_uid));
if ($$ == NULL)
err(1, "uid_item: calloc");
- $$->uid[0] = $1;
- $$->uid[1] =
Re: pfctl: Fail on missing anchor
Hello, On Thu, Jan 16, 2020 at 12:15:39AM +0100, Klemens Nanni wrote: > There is no reason to continue on anchor specific paths if the given > anchor does not exist, so fix the last occurences better error messages: > > # pfctl -a regress\* -Fa > Anchor 'regress' not found. > pfctl: pfctl_get_anchors failed to retrieve list of anchors, can't > continue > # ./obj/pfctl -a regress\* -Fa > pfctl: Anchor does not exist > > Feedback? OK? > looks OK to me. thanks and regards sashan
pfctl: Fail on missing anchor
There is no reason to continue on anchor specific paths if the given
anchor does not exist, so fix the last occurences better error messages:
# pfctl -a regress\* -Fa
Anchor 'regress' not found.
pfctl: pfctl_get_anchors failed to retrieve list of anchors, can't
continue
# ./obj/pfctl -a regress\* -Fa
pfctl: Anchor does not exist
Feedback? OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.381
diff -u -p -r1.381 pfctl.c
--- pfctl.c 15 Jan 2020 22:38:31 - 1.381
+++ pfctl.c 15 Jan 2020 22:50:04 -
@@ -967,13 +967,8 @@ pfctl_show_rules(int dev, char *path, in
memset(, 0, sizeof(prs));
memcpy(prs.path, npath, sizeof(prs.path));
- if (ioctl(dev, DIOCGETRULESETS, ) == -1) {
- if (errno == EINVAL)
- fprintf(stderr, "Anchor '%s' "
- "not found.\n", anchorname);
- else
- err(1, "DIOCGETRULESETS");
- }
+ if (ioctl(dev, DIOCGETRULESETS, ) == -1)
+ errx(1, "%s", pf_strerror(errno));
mnr = prs.nr;
for (nr = 0; nr < mnr; ++nr) {
@@ -2206,13 +2201,8 @@ pfctl_walk_anchors(int dev, int opts, co
memset(, 0, sizeof(pr));
strlcpy(pr.path, anchor, sizeof(pr.path));
- if (ioctl(dev, DIOCGETRULESETS, ) == -1) {
- if (errno == EINVAL)
- fprintf(stderr, "Anchor '%s' not found.\n", anchor);
- else
- err(1, "DIOCGETRULESETS");
- return (-1);
- }
+ if (ioctl(dev, DIOCGETRULESETS, ) == -1)
+ errx(1, "%s", pf_strerror(errno));
mnr = pr.nr;
for (nr = 0; nr < mnr; ++nr) {
char sub[PATH_MAX];
Re: pfctl: unify error message for nonexisting anchors
On Wed, Jan 15, 2020 at 11:12:46PM +0100, Alexandr Nedvedicky wrote: > rename pfr_strerror() to pf_strerror() and move it from pfctl_radix.c > to pfctl.c as it becomes more generic now. Yes, makes sense; "pfr" is not the appropiate namespace here.
Re: pfctl: unify error message for nonexisting anchors
Hello, > There are other occasions as well but those probably need additional > tweaks, so here's the first round. > > Feedback? OK? I like the idea. Just have one 'bike shedding' suggestion: rename pfr_strerror() to pf_strerror() and move it from pfctl_radix.c to pfctl.c as it becomes more generic now. You have my OK. thanks and regards sashan
avoid uvideo lockup when nothing is transferred
Hi all.
This fixes an issue I'm seeing with a uvideo(4), that doesn't like
commands we're sending to it. The camera simply sends nothing,
and since we're sleeping forever (xfer timeout is 0, which is
USBD_NO_TIMEOUT), we never get out from 'while (bulk_running)' loop,
visible in the scond chunk of the diff. This not only makes video(1)
and other V4L2 users lockup, but also results in process freeze upon
detach: it calls uvideo_vs_close(), which in turn calls
usbd_ref_wait(), which doesn't return because we still have something
in queue.
Also, in case of error we keep bulk_running set. This seems just
a leftover, as well as the first chunk: if we failed to create kthread,
there won't be anything running under bulk_running==1 for sure.
I must thank kettenis@ for help during diagnosis and mpi@ for a patch
for related issue.
OK?
--
WBR,
Vadim Zhukov
Index: uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.205
diff -u -p -r1.205 uvideo.c
--- uvideo.c14 Oct 2019 09:20:48 - 1.205
+++ uvideo.c15 Jan 2020 21:54:43 -
@@ -2026,6 +2027,7 @@ uvideo_vs_start_bulk(struct uvideo_softc
error = kthread_create(uvideo_vs_start_bulk_thread, sc, NULL,
DEVNAME(sc));
if (error) {
+ sc->sc_vs_cur->bulk_running = 0;
printf("%s: can't create kernel thread!", DEVNAME(sc));
return (error);
}
@@ -2044,6 +2046,12 @@ uvideo_vs_start_bulk_thread(void *arg)
while (sc->sc_vs_cur->bulk_running) {
size = UGETDW(sc->sc_desc_probe.dwMaxPayloadTransferSize);
+ /*
+* We can't wait infinitely, since otherwise we'll
+* block forever if camera stops (or don't even starts)
+* sending frames. Use '2*' multiplier to compensate
+* possible lags.
+*/
usbd_setup_xfer(
sc->sc_vs_cur->bxfer.xfer,
sc->sc_vs_cur->pipeh,
@@ -2051,10 +2059,11 @@ uvideo_vs_start_bulk_thread(void *arg)
sc->sc_vs_cur->bxfer.buf,
size,
USBD_NO_COPY | USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS,
- 0,
+ 2 * 1000 / (sc->sc_frame_rate || 1),
NULL);
error = usbd_transfer(sc->sc_vs_cur->bxfer.xfer);
if (error != USBD_NORMAL_COMPLETION) {
+ sc->sc_vs_cur->bulk_running = 0;
DPRINTF(1, "%s: error in bulk xfer: %s!\n",
DEVNAME(sc), usbd_errstr(error));
break;
Re: iked(8): get rid of IPv6 flow and -6 flag?
On Wed, Jan 15, 2020 at 07:41:46PM +, Stuart Henderson wrote:
> On 2020/01/14 21:48, Stuart Henderson wrote:
> > > while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
> > > switch (c) {
> > > case '6':
> > > - opts |= IKED_OPT_NOIPV6BLOCKING;
> > > + log_warnx("the -6 option is deprecated and will be "
> > > + "removed in the future.");
> >
> > "deprecated" implies that it still works but you shouldn't use it any more.
> >
> > Perhaps "ignored" or "no longer supported" instead?
> >
>
> Now that this is committed anyway - can I do this or something similar?
>
> deprecate -> "discouragement of use of some terminology, feature,
> design, or practice, typically because it has been superseded or is no
> longer considered efficient or safe, without completely removing it or
> prohibiting its use"
>
> Index: iked.c
> ===
> RCS file: /cvs/src/sbin/iked/iked.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 iked.c
> --- iked.c14 Jan 2020 22:28:29 - 1.39
> +++ iked.c15 Jan 2020 19:39:37 -
> @@ -76,7 +76,7 @@ main(int argc, char *argv[])
> while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
> switch (c) {
> case '6':
> - log_warnx("the -6 option is deprecated and will be "
> + log_warnx("the -6 option is ignored and will be "
> "removed in the future.");
> break;
> case 'd':
>
I totally missed that part of your previous mail, sorry.
Reading the definition it seems you are right that "ignored" is the better
word here.
ok tobhe@
Re: iked(8): get rid of IPv6 flow and -6 flag?
I strongly agree that we should avoid use of the word 'deprecated'
towards the public. People interpret what it means differently,
so try to be EXACT.
'deprecated' is our choice to make the change, but 'ignored' is
the result of that decision upon the people.
Stuart Henderson wrote:
> On 2020/01/14 21:48, Stuart Henderson wrote:
> > > while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
> > > switch (c) {
> > > case '6':
> > > - opts |= IKED_OPT_NOIPV6BLOCKING;
> > > + log_warnx("the -6 option is deprecated and will be "
> > > + "removed in the future.");
> >
> > "deprecated" implies that it still works but you shouldn't use it any more.
> >
> > Perhaps "ignored" or "no longer supported" instead?
> >
>
> Now that this is committed anyway - can I do this or something similar?
>
> deprecate -> "discouragement of use of some terminology, feature,
> design, or practice, typically because it has been superseded or is no
> longer considered efficient or safe, without completely removing it or
> prohibiting its use"
>
> Index: iked.c
> ===
> RCS file: /cvs/src/sbin/iked/iked.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 iked.c
> --- iked.c14 Jan 2020 22:28:29 - 1.39
> +++ iked.c15 Jan 2020 19:39:37 -
> @@ -76,7 +76,7 @@ main(int argc, char *argv[])
> while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
> switch (c) {
> case '6':
> - log_warnx("the -6 option is deprecated and will be "
> + log_warnx("the -6 option is ignored and will be "
> "removed in the future.");
> break;
> case 'd':
>
Re: iked(8): get rid of IPv6 flow and -6 flag?
On 2020/01/14 21:48, Stuart Henderson wrote:
> > while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
> > switch (c) {
> > case '6':
> > - opts |= IKED_OPT_NOIPV6BLOCKING;
> > + log_warnx("the -6 option is deprecated and will be "
> > + "removed in the future.");
>
> "deprecated" implies that it still works but you shouldn't use it any more.
>
> Perhaps "ignored" or "no longer supported" instead?
>
Now that this is committed anyway - can I do this or something similar?
deprecate -> "discouragement of use of some terminology, feature,
design, or practice, typically because it has been superseded or is no
longer considered efficient or safe, without completely removing it or
prohibiting its use"
Index: iked.c
===
RCS file: /cvs/src/sbin/iked/iked.c,v
retrieving revision 1.39
diff -u -p -r1.39 iked.c
--- iked.c 14 Jan 2020 22:28:29 - 1.39
+++ iked.c 15 Jan 2020 19:39:37 -
@@ -76,7 +76,7 @@ main(int argc, char *argv[])
while ((c = getopt(argc, argv, "6dD:nf:vSTt")) != -1) {
switch (c) {
case '6':
- log_warnx("the -6 option is deprecated and will be "
+ log_warnx("the -6 option is ignored and will be "
"removed in the future.");
break;
case 'd':
pfctl: unify error message for nonexisting anchors
According to pf(4) all of DIOCGETRULE, DIOCGETRULES and DIOCGETRULESET
return EINVAL if the specified anchor does not exist; I double checked
pf_ioctl.c to verify.
This diff makes pfctl consistently use pfr_strerror() which now handles
EINVAL such that
# pfctl -a nope -sr
pfctl: DIOCGETRULES: Invalid argument
# ./obj/pfctl -a nope -sr
pfctl: Anchor does not exist
There are other occasions as well but those probably need additional
tweaks, so here's the first round.
Feedback? OK?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.379
diff -u -p -r1.379 pfctl.c
--- pfctl.c 15 Jan 2020 13:42:39 - 1.379
+++ pfctl.c 15 Jan 2020 19:20:57 -
@@ -863,7 +863,7 @@ pfctl_show_rules(int dev, char *path, in
if (opts & PF_OPT_SHOWALL) {
pr.rule.action = PF_PASS;
if (ioctl(dev, DIOCGETRULES, ) == -1) {
- warn("DIOCGETRULES");
+ warnx("%s", pfr_strerror(errno));
ret = -1;
goto error;
}
@@ -878,7 +878,7 @@ pfctl_show_rules(int dev, char *path, in
pr.rule.action = PF_PASS;
if (ioctl(dev, DIOCGETRULES, ) == -1) {
- warn("DIOCGETRULES");
+ warnx("%s", pfr_strerror(errno));
ret = -1;
goto error;
}
@@ -979,7 +979,7 @@ pfctl_show_rules(int dev, char *path, in
for (nr = 0; nr < mnr; ++nr) {
prs.nr = nr;
if (ioctl(dev, DIOCGETRULESET, ) == -1)
- err(1, "DIOCGETRULESET");
+ errx(1, "%s", pfr_strerror(errno));
INDENT(depth, !(opts & PF_OPT_VERBOSE));
printf("anchor \"%s\" all {\n", prs.name);
pfctl_show_rules(dev, npath, opts,
@@ -2219,7 +2219,7 @@ pfctl_walk_anchors(int dev, int opts, co
pr.nr = nr;
if (ioctl(dev, DIOCGETRULESET, ) == -1)
- err(1, "DIOCGETRULESET");
+ errx(1, "%s", pfr_strerror(errno));
if (!strcmp(pr.name, PF_RESERVED_ANCHOR))
continue;
sub[0] = '\0';
Index: pfctl_optimize.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_optimize.c,v
retrieving revision 1.43
diff -u -p -r1.43 pfctl_optimize.c
--- pfctl_optimize.c12 Dec 2019 21:00:51 - 1.43
+++ pfctl_optimize.c15 Jan 2020 19:20:54 -
@@ -873,7 +873,7 @@ load_feedback_profile(struct pfctl *pf,
memset(, 0, sizeof(pr));
pr.rule.action = PF_PASS;
if (ioctl(pf->dev, DIOCGETRULES, ) == -1) {
- warn("DIOCGETRULES");
+ warnx("%s", pfr_strerror(errno));
return (1);
}
mnr = pr.nr;
@@ -887,7 +887,7 @@ load_feedback_profile(struct pfctl *pf,
}
pr.nr = nr;
if (ioctl(pf->dev, DIOCGETRULE, ) == -1) {
- warn("DIOCGETRULES");
+ warnx("%s", pfr_strerror(errno));
free(por);
return (1);
}
Index: pfctl_radix.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.36
diff -u -p -r1.36 pfctl_radix.c
--- pfctl_radix.c 15 Jan 2020 16:15:08 - 1.36
+++ pfctl_radix.c 15 Jan 2020 19:20:54 -
@@ -567,6 +567,7 @@ pfr_strerror(int errnum)
switch (errnum) {
case ESRCH:
return "Table does not exist";
+ case EINVAL:
case ENOENT:
return "Anchor does not exist";
default:
Re: lpt(4): timeout_add(9) -> timeout_add_msec(9), tsleep(9) -> tsleep_nsec(9)
On Wed, Jan 15, 2020 at 11:08:04AM +0100, Martin Pieuchot wrote:
> On 14/01/20(Tue) 18:37, Scott Cheloha wrote:
> > Ticks to milliseconds.
> > [...]
> > @@ -303,16 +304,17 @@ lptpushbytes(struct lpt_softc *sc)
> > while (NOT_READY()) {
> > if (++spin < sc->sc_spinmax)
> > continue;
> > - tic = 0;
> > + msecs = 10;
> > /* adapt busy-wait algorithm */
> > sc->sc_spinmax++;
> > while (NOT_READY_ERR()) {
> > /* exponential backoff */
> > - tic = tic + tic + 1;
> > - if (tic > TIMEOUT)
> > - tic = TIMEOUT;
> > - error = tsleep((caddr_t)sc,
> > - LPTPRI | PCATCH, "lptpsh", tic);
> > + msecs = msecs + msecs + 1;
>
> Shouldn't the backoff algorithm be:
>
> msecs = 0;
> while (/* not ready */) {
> msecs = msecs + msecs + 10;
> ...
> }
Yep, that'd be closer to the original.
Index: ic/lpt.c
===
RCS file: /cvs/src/sys/dev/ic/lpt.c,v
retrieving revision 1.14
diff -u -p -r1.14 lpt.c
--- ic/lpt.c11 May 2015 02:01:01 - 1.14
+++ ic/lpt.c15 Jan 2020 16:07:26 -
@@ -71,8 +71,8 @@
#include "lpt.h"
-#defineTIMEOUT hz*16 /* wait up to 16 seconds for a ready */
-#defineSTEPhz/4
+#defineTIMEOUT 16000 /* wait up to 16 seconds for a ready */
+#defineSTEP250 /* 1/4 seconds */
#defineLPTPRI (PZERO+8)
#defineLPT_BSIZE 1024
@@ -199,7 +199,8 @@ lptopen(dev_t dev, int flag, int mode, s
}
/* wait 1/4 second, give up if we get a signal */
- error = tsleep((caddr_t)sc, LPTPRI | PCATCH, "lptopen", STEP);
+ error = tsleep_nsec(sc, LPTPRI | PCATCH, "lptopen",
+ MSEC_TO_NSEC(STEP));
if (sc->sc_state == 0)
return (EIO);
if (error != EWOULDBLOCK) {
@@ -256,7 +257,7 @@ lptwakeup(void *arg)
splx(s);
if (sc->sc_state != 0)
- timeout_add(>sc_wakeup_tmo, STEP);
+ timeout_add_msec(>sc_wakeup_tmo, STEP);
}
/*
@@ -293,7 +294,7 @@ lptpushbytes(struct lpt_softc *sc)
int error;
if (sc->sc_flags & LPT_NOINTR) {
- int spin, tic;
+ int msecs, spin;
u_int8_t control = sc->sc_control;
while (sc->sc_count > 0) {
@@ -303,16 +304,17 @@ lptpushbytes(struct lpt_softc *sc)
while (NOT_READY()) {
if (++spin < sc->sc_spinmax)
continue;
- tic = 0;
+ msecs = 0;
/* adapt busy-wait algorithm */
sc->sc_spinmax++;
while (NOT_READY_ERR()) {
/* exponential backoff */
- tic = tic + tic + 1;
- if (tic > TIMEOUT)
- tic = TIMEOUT;
- error = tsleep((caddr_t)sc,
- LPTPRI | PCATCH, "lptpsh", tic);
+ msecs = msecs + msecs + 10;
+ if (msecs > TIMEOUT)
+ msecs = TIMEOUT;
+ error = tsleep_nsec(sc,
+ LPTPRI | PCATCH, "lptpsh",
+ MSEC_TO_NSEC(msecs));
if (sc->sc_state == 0)
error = EIO;
if (error != EWOULDBLOCK)
@@ -345,8 +347,8 @@ lptpushbytes(struct lpt_softc *sc)
}
if (sc->sc_state == 0)
return (EIO);
- error = tsleep((caddr_t)sc, LPTPRI | PCATCH,
- "lptwrite2", 0);
+ error = tsleep_nsec(sc, LPTPRI | PCATCH,
+ "lptwrite2", INFSLP);
if (sc->sc_state == 0)
error = EIO;
if (error)
Re: pfctl: Refine error message
On Wed, Jan 15, 2020 at 04:44:55PM +0100, Klemens Nanni wrote: > While code in pf/pfctl confusingly uses either anchor or ruleset > depending on the context, pfctl(8) (both manual and user interface) > should be consistent. There should be no difference between anchor and > ruleset for users, implying there is one only makes for confusion: > > # pfctl -a regress -Fa > pfctl: Anchor or Ruleset does not exist. > > The synopsis itself is `-a anchor' and with the other diff this error > message format should be well in line with how we use err(3) across the > tree. > > # ./obj/pfctl -a regress -Fa > pfctl: Anchor does not exist > > OK? makes sense. OK sashan
Re: carp: send only IPv4 carp packets on dual stack interface
On 2020/01/15 12:04, Janne Johansson wrote: > Den ons 15 jan. 2020 kl 11:57 skrev Christopher Zimmermann < > chr...@openbsd.org>: > > > So I propose to send IPv6 advertisements only when IPv4 is not possible. > > > > Why? > > > > - Noise can be reduced by using unicast advertisements. > >This is only possible for IPv4 by ``ifconfig carppeer``. > >I don't like flooding the whole network with carp advertisements when > >I may also unicast them. > > > > This sentence seems to assume people would only ever use two members in a > carp group, whereas having more than that is not impossible. > In those cases full meshed unicast would cause more packets than > multicasting one packet every second (+advskew). > > -- > May the most significant bit of your life be positive. I just filter the MACs on my switches..
pfctl: Refine error message
While code in pf/pfctl confusingly uses either anchor or ruleset depending on the context, pfctl(8) (both manual and user interface) should be consistent. There should be no difference between anchor and ruleset for users, implying there is one only makes for confusion: # pfctl -a regress -Fa pfctl: Anchor or Ruleset does not exist. The synopsis itself is `-a anchor' and with the other diff this error message format should be well in line with how we use err(3) across the tree. # ./obj/pfctl -a regress -Fa pfctl: Anchor does not exist OK? Index: pfctl_radix.c === RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v retrieving revision 1.35 diff -u -p -r1.35 pfctl_radix.c --- pfctl_radix.c 28 Jun 2019 13:32:45 - 1.35 +++ pfctl_radix.c 15 Jan 2020 15:44:42 - @@ -568,7 +568,7 @@ pfr_strerror(int errnum) case ESRCH: return "Table does not exist"; case ENOENT: - return "Anchor or Ruleset does not exist"; + return "Anchor does not exist"; default: return strerror(errnum); }
ftp(1): setjmp, volatile and errno fixes
Some of those tweaks were mentioned first in
https://marc.info/?l=openbsd-tech=157672366409171=2
* Fix setjmp vs volatile usage:
- "buf" shouldn't be modified after setjmp, else we break the setjmp
contract:
--8<--
All accessible objects have values as of the time the longjmp() routine
was called, except that the values of objects of automatic storage
invocation duration that do not have the volatile type and have been
changed between the setjmp() invocation and longjmp() call are
indeterminate.
-->8--
Looks like we're being lucky here.
- a bunch of local variables are marked as volatile even though they're
not touched between setjmp and longjmp. This leads to inefficient
compiled code.
* Sync read/write loop with file_get():
- no need for temp variable "i", "wlen" is enough and appropriately typed
- make the loop condition/exit more similar (simpler)
- there's no point in checking for write(2) returning 0
- properly save and restore errno in case of fread(3) failure
size(1) says the resulting executables shrink on amd64.
Reviews and oks welcome.
Index: fetch.c
===
--- fetch.c.orig
+++ fetch.c
@@ -77,7 +77,7 @@ static char hextochar(const char *);
static char*urldecode(const char *);
static char*recode_credentials(const char *_userinfo);
static char*ftp_readline(FILE *, size_t *);
-static voidftp_close(FILE **, struct tls **, volatile int *);
+static voidftp_close(FILE **, struct tls **, int *);
static const char *sockerror(struct tls *);
#ifdef SMALL
#defineftp_printf(fp, ...) fprintf(fp, __VA_ARGS__)
@@ -311,13 +311,13 @@ url_get(const char *origline, const char
char pbuf[NI_MAXSERV], hbuf[NI_MAXHOST], *cp, *portnum, *path, ststr[4];
char *hosttail, *cause = "unknown", *newline, *host, *port, *buf = NULL;
char *epath, *redirurl, *loctail, *h, *p, gerror[200];
- int error, i, isftpurl = 0, isredirect = 0, rval = -1;
+ int error, isftpurl = 0, isredirect = 0, rval = -1;
int isunavail = 0, retryafter = -1;
struct addrinfo hints, *res0, *res;
- const char * volatile savefile;
- char * volatile proxyurl = NULL;
+ const char *savefile;
+ char *proxyurl = NULL;
char *credentials = NULL;
- volatile int fd = -1, out = -1;
+ int fd = -1, out = -1;
volatile sig_t oldintr, oldinti;
FILE *fin = NULL;
off_t hashbytes;
@@ -1013,6 +1013,10 @@ noslash:
#endif
}
+ free(buf);
+ if ((buf = malloc(buflen)) == NULL)
+ errx(1, "Can't allocate memory for transfer buffer");
+
/* Trap signals */
oldintr = NULL;
oldinti = NULL;
@@ -1029,11 +1033,7 @@ noslash:
hashbytes = mark;
progressmeter(-1, path);
- free(buf);
-
/* Finally, suck down the file. */
- if ((buf = malloc(buflen)) == NULL)
- errx(1, "Can't allocate memory for transfer buffer");
oldinti = signal(SIGINFO, psummary);
if (chunked) {
error = save_chunked(fin, tls, out, buf, buflen);
@@ -1041,18 +1041,14 @@ noslash:
if (error == -1)
goto cleanup_url_get;
} else {
- i = 0;
- len = 1;
- while (len > 0) {
- len = fread(buf, 1, buflen, fin);
+ while ((len = fread(buf, 1, buflen, fin)) > 0) {
bytes += len;
- for (cp = buf, wlen = len; wlen > 0; wlen -= i, cp +=
i) {
- if ((i = write(out, cp, wlen)) == -1) {
+ for (cp = buf; len > 0; len -= wlen, cp += wlen) {
+ if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
signal(SIGINFO, oldinti);
goto cleanup_url_get;
- } else if (i == 0)
- break;
+ }
}
if (hash && !progress) {
while (bytes >= hashbytes) {
@@ -1062,6 +1058,7 @@ noslash:
(void)fflush(ttyout);
}
}
+ save_errno = errno;
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -1069,7 +1066,8 @@ noslash:
(void)putc('\n', ttyout);
(void)fflush(ttyout);
}
- if (len != 0) {
+ if (len == 0 && ferror(fin)) {
+ errno = save_errno;
warnx("Reading from socket: %s", sockerror(tls));
goto
Re: pfctl: Merge radix_perror() into simpler warnx()/errx() usage
On Wed, Jan 15, 2020 at 04:26:20PM +0100, Klemens Nanni wrote: > Less nesting for clearer code. > > OK? > > The macro backslashes got adjusted but diff with `-w' for ease of review. > OK sashan
pfctl: Merge radix_perror() into simpler warnx()/errx() usage
Less nesting for clearer code.
OK?
The macro backslashes got adjusted but diff with `-w' for ease of review.
Index: pfctl_table.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_table.c,v
retrieving revision 1.82
diff -u -p -w -r1.82 pfctl_table.c
--- pfctl_table.c 15 Jan 2020 11:52:50 - 1.82
+++ pfctl_table.c 15 Jan 2020 15:23:16 -
@@ -59,7 +59,6 @@ static void print_tstats(struct pfr_tsta
static int load_addr(struct pfr_buffer *, int, char *[], char *, int, int);
static voidprint_addrx(struct pfr_addr *, struct pfr_addr *, int);
static voidprint_astats(struct pfr_astats *, int);
-static voidradix_perror(void);
static voidxprintf(int, const char *, ...);
static voidprint_iface(struct pfi_kif *, int);
@@ -78,7 +77,7 @@ static const char *istats_text[2][2][2]
(opts & PF_OPT_DUMMYACTION)) && \
(fct)) {\
if ((opts & PF_OPT_RECURSE) == 0)\
- radix_perror(); \
+ warnx("%s", pfr_strerror(errno)); \
goto _error;\
} \
} while (0)
@@ -91,7 +90,7 @@ static const char *istats_text[2][2][2]
(opts & PF_OPT_DUMMYACTION)) && \
(pfr_add_tables(, 1, , flags)) &&\
(errno != EPERM)) { \
- radix_perror(); \
+ warnx("%s", pfr_strerror(errno)); \
goto _error;\
} \
if (nadd) { \
@@ -509,13 +508,6 @@ print_astats(struct pfr_astats *as, int
(unsigned long long)as->pfras_bytes[dir][op]);
}
-void
-radix_perror(void)
-{
- extern char *__progname;
- fprintf(stderr, "%s: %s.\n", __progname, pfr_strerror(errno));
-}
-
int
pfctl_define_table(char *name, int flags, int addrs, const char *anchor,
struct pfr_buffer *ab, u_int32_t ticket)
@@ -598,10 +590,8 @@ pfctl_show_ifaces(const char *filter, in
for (;;) {
pfr_buf_grow(, b.pfrb_size);
b.pfrb_size = b.pfrb_msize;
- if (pfi_get_ifaces(filter, b.pfrb_caddr, _size)) {
- radix_perror();
- exit(1);
- }
+ if (pfi_get_ifaces(filter, b.pfrb_caddr, _size))
+ errx(1, "%s", pfr_strerror(errno));
if (b.pfrb_size <= b.pfrb_msize)
break;
i++;
Re: MAKE: some older keywords
More specifically, I'm running with this patch
The specific choice of keywords to deprecate is up for grabs, the
infrastructure for actually being able to error out on these keywords is
probably something I should commit anyhow.
diff --git a/gnode.h b/gnode.h
index 283fead..04e5462 100644
--- a/gnode.h
+++ b/gnode.h
@@ -77,6 +77,8 @@
#define SPECIAL_NOTHING6U /* this is used for things we
* recognize for compatibility but
* don't do anything with... */
+#define SPECIAL_DEPRECATED 7U /* this is an old keyword and it will
+* trigger a fatal error. */
#define SPECIAL_INVISIBLE 8U
#define SPECIAL_JOIN 9U
#define SPECIAL_MADE 11U
diff --git a/parse.c b/parse.c
index dfc2abc..555d2cd 100644
--- a/parse.c
+++ b/parse.c
@@ -184,13 +184,13 @@ static struct {
unsigned int special;
unsigned int special_op;
} specials[] = {
-{ P(NODE_EXEC),SPECIAL_EXEC, OP_EXEC },
+{ P(NODE_EXEC),SPECIAL_DEPRECATED, OP_EXEC },
{ P(NODE_IGNORE), SPECIAL_IGNORE, OP_IGNORE },
-{ P(NODE_INCLUDES),SPECIAL_NOTHING,0 },
-{ P(NODE_INVISIBLE), SPECIAL_INVISIBLE, OP_INVISIBLE },
-{ P(NODE_JOIN),SPECIAL_JOIN, OP_JOIN },
-{ P(NODE_LIBS),SPECIAL_NOTHING,0 },
-{ P(NODE_MADE),SPECIAL_MADE, OP_MADE },
+{ P(NODE_INCLUDES),SPECIAL_DEPRECATED, 0 },
+{ P(NODE_INVISIBLE), SPECIAL_DEPRECATED, OP_INVISIBLE },
+{ P(NODE_JOIN),SPECIAL_DEPRECATED, OP_JOIN },
+{ P(NODE_LIBS),SPECIAL_DEPRECATED, 0 },
+{ P(NODE_MADE),SPECIAL_DEPRECATED, OP_MADE },
{ P(NODE_MAIN),SPECIAL_MAIN, 0 },
{ P(NODE_MAKE),SPECIAL_MAKE, OP_MAKE },
{ P(NODE_MAKEFLAGS), SPECIAL_MFLAGS, 0 },
@@ -198,7 +198,7 @@ static struct {
{ P(NODE_NOTMAIN), SPECIAL_NOTMAIN,OP_NOTMAIN },
{ P(NODE_NOTPARALLEL), SPECIAL_NOTPARALLEL,0 },
{ P(NODE_NO_PARALLEL), SPECIAL_NOTPARALLEL,0 },
-{ P(NODE_NULL),SPECIAL_NOTHING,0 },
+{ P(NODE_NULL),SPECIAL_DEPRECATED, 0 },
{ P(NODE_OPTIONAL),SPECIAL_OPTIONAL, OP_OPTIONAL },
{ P(NODE_ORDER), SPECIAL_ORDER, 0 },
{ P(NODE_PARALLEL),SPECIAL_PARALLEL, 0 },
@@ -418,6 +418,11 @@ ParseDoSrc(
const char *esrc)
{
GNode *gn = Targ_FindNodei(src, esrc, TARG_CREATE);
+ if (gn->special == SPECIAL_DEPRECATED) {
+ Parse_Error(PARSE_FATAL, "Deprecated keyword found %s\n",
+ gn->name);
+ return;
+ }
if (gn->special_op) {
Array_ForEach(targets, ParseDoSpecial, gn->special_op);
return;
@@ -702,6 +707,13 @@ handle_special_targets(Lst paths)
for (i = 0; i < gtargets.n; i++) {
type = gtargets.a[i]->special;
+ if (type == SPECIAL_DEPRECATED) {
+ Parse_Error(PARSE_FATAL,
+ "Deprecated keyword found %s\n",
+ gtargets.a[i]->name);
+ specType = SPECIAL_ERROR;
+ return 0;
+ }
if (type == SPECIAL_PATH) {
seen_path++;
Lst_AtEnd(paths, find_suffix_path(gtargets.a[i]));
Re: Towards splitting SCHED_LOCK()
On 14/01/20(Tue) 17:30, Martin Pieuchot wrote:
> On 14/01/20(Tue) 10:34, Jonathan Gray wrote:
> > On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote:
> > > I'm facing a lock ordering issue while profiling the scheduler which
> > > cannot be solved without using a different lock for the global sleepqueue
> > > and the runqueues.
> > >
> > > The SCHED_LOCK() currently protects both data structures as well as the
> > > related fields in 'struct proc'. One of this fields is `p_wchan' which
> > > is obviously related to the global sleepqueue.
> > >
> > > The cleaning diff below introduces a new function, awake(), that unify
> > > the multiple uses of the following idiom:
> > >
> > > if (p->p_stat == SSLEEP)
> > > setrunnable(p);
> > > else
> > > unsleep(p);
> > >
> > > This is generally done after checking if the thread `p' is on the
> > > sleepqueue.
> >
> > Why not name it wakeup_proc() instead or something like endsleep()?
> >
> > awake() does not describe the action that is being done and
> > if (awake()) reads like it could be
> > if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
>
> Sure, updated diff that also keep the SCHED_LOCK() in endtsleep() as
> pointed out by visa@. The reason is that sleep_finish_timeout() is
> executed under the SCHED_LOCK() and we want to ensure the P_TIMEOUT
> flag is set and check under this lock to avoid races.
Any strong preference for the name or should I move forward with
wakeup_proc()?
> Index: dev/pci/drm/drm_linux.c
> ===
> RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 drm_linux.c
> --- dev/pci/drm/drm_linux.c 5 Jan 2020 13:46:02 - 1.55
> +++ dev/pci/drm/drm_linux.c 14 Jan 2020 16:22:38 -
> @@ -115,20 +115,8 @@ schedule_timeout(long timeout)
> int
> wake_up_process(struct proc *p)
> {
> - int s, r = 0;
> -
> - SCHED_LOCK(s);
> atomic_cas_ptr(_proc, p, NULL);
> - if (p->p_wchan) {
> - if (p->p_stat == SSLEEP) {
> - setrunnable(p);
> - r = 1;
> - } else
> - unsleep(p);
> - }
> - SCHED_UNLOCK(s);
> -
> - return r;
> + return wakeup_proc(p, NULL);
> }
>
> void
> Index: kern/kern_sig.c
> ===
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.241
> diff -u -p -r1.241 kern_sig.c
> --- kern/kern_sig.c 14 Jan 2020 08:52:18 - 1.241
> +++ kern/kern_sig.c 14 Jan 2020 16:20:42 -
> @@ -1109,7 +1109,7 @@ ptsignal(struct proc *p, int signum, enu
>* runnable and can look at the signal. But don't make
>* the process runnable, leave it stopped.
>*/
> - if (p->p_wchan && p->p_flag & P_SINTR)
> + if (p->p_flag & P_SINTR)
> unsleep(p);
> goto out;
>
> Index: kern/kern_synch.c
> ===
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.157
> diff -u -p -r1.157 kern_synch.c
> --- kern/kern_synch.c 14 Jan 2020 08:52:18 - 1.157
> +++ kern/kern_synch.c 14 Jan 2020 16:23:02 -
> @@ -469,8 +469,7 @@ sleep_setup_signal(struct sleep_state *s
>*/
> atomic_setbits_int(>p_flag, P_SINTR);
> if (p->p_p->ps_single != NULL || (sls->sls_sig = CURSIG(p)) != 0) {
> - if (p->p_wchan)
> - unsleep(p);
> + unsleep(p);
> p->p_stat = SONPROC;
> sls->sls_do_sleep = 0;
> } else if (p->p_wchan == 0) {
> @@ -505,6 +504,25 @@ sleep_finish_signal(struct sleep_state *
> return (error);
> }
>
> +int
> +wakeup_proc(struct proc *p, const volatile void *chan)
> +{
> + int s, awakened = 0;
> +
> + SCHED_LOCK(s);
> + if (p->p_wchan != NULL &&
> +((chan == NULL) || (p->p_wchan == chan))) {
> + awakened = 1;
> + if (p->p_stat == SSLEEP)
> + setrunnable(p);
> + else
> + unsleep(p);
> + }
> + SCHED_UNLOCK(s);
> +
> + return awakened;
> +}
> +
> /*
> * Implement timeout for tsleep.
> * If process hasn't been awakened (wchan non-zero),
> @@ -518,13 +536,8 @@ endtsleep(void *arg)
> int s;
>
> SCHED_LOCK(s);
> - if (p->p_wchan) {
> - if (p->p_stat == SSLEEP)
> - setrunnable(p);
> - else
> - unsleep(p);
> + if (wakeup_proc(p, NULL))
> atomic_setbits_int(>p_flag, P_TIMEOUT);
> - }
> SCHED_UNLOCK(s);
> }
>
> @@ -536,7 +549,7 @@ unsleep(struct proc *p)
> {
> SCHED_ASSERT_LOCKED();
>
> - if (p->p_wchan) {
> + if (p->p_wchan != NULL) {
> TAILQ_REMOVE([LOOKUP(p->p_wchan)], p, p_runq);
>
Re: carp: send only IPv4 carp packets on dual stack interface
On January 15, 2020 12:04:56 PM GMT+01:00, Janne Johansson wrote: >Den ons 15 jan. 2020 kl 11:57 skrev Christopher Zimmermann < >chr...@openbsd.org>: > >> So I propose to send IPv6 advertisements only when IPv4 is not >possible. >> >> Why? >> >> - Noise can be reduced by using unicast advertisements. >>This is only possible for IPv4 by ``ifconfig carppeer``. >>I don't like flooding the whole network with carp advertisements >when >>I may also unicast them. >> > >This sentence seems to assume people would only ever use two members in >a >carp group Well, even though it may have seemed like it, I did not assume this. My concern is more about sending out through two network stacks, one of which (IPv6) may fail and produce unwanted effects in carp's demote logic. Is there something to gain by sending out though both stacks? http://gmerlin.de OpenPGP: http://gmerlin.de/christopher.pub CB07 DA40 B0B6 571D 35E2 0DEF 87E2 92A7 13E5 DEE1
Re: carp: send only IPv4 carp packets on dual stack interface
Den ons 15 jan. 2020 kl 11:57 skrev Christopher Zimmermann < chr...@openbsd.org>: > So I propose to send IPv6 advertisements only when IPv4 is not possible. > > Why? > > - Noise can be reduced by using unicast advertisements. >This is only possible for IPv4 by ``ifconfig carppeer``. >I don't like flooding the whole network with carp advertisements when >I may also unicast them. > This sentence seems to assume people would only ever use two members in a carp group, whereas having more than that is not impossible. In those cases full meshed unicast would cause more packets than multicasting one packet every second (+advskew). -- May the most significant bit of your life be positive.
carp: send only IPv4 carp packets on dual stack interface
Hi,
as far as I can see a dual stack carp interface does not care whether it
receives advertisements addressed to IPv4 or IPv6. Any one will do.
So I propose to send IPv6 advertisements only when IPv4 is not possible.
Why?
- Noise can be reduced by using unicast advertisements.
This is only possible for IPv4 by ``ifconfig carppeer``.
I don't like flooding the whole network with carp advertisements when
I may also unicast them.
- breaking IPv6 connectivity (for example by running iked without -6)
will start a preempt-war, because failing ip6_output will cause the
demote counter to be increased. That's what hit me.
I would suggest a change like this:
Index: ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.342
diff -u -p -r1.342 ip_carp.c
--- ip_carp.c 8 Nov 2019 07:51:41 - 1.342
+++ ip_carp.c 15 Jan 2020 10:45:56 -
@@ -1175,7 +1175,7 @@ carp_send_ad(struct carp_vhost_entry *vh
}
}
#ifdef INET6
- if (sc->sc_naddrs6) {
+ else if (sc->sc_naddrs6) {
struct ip6_hdr *ip6;
MGETHDR(m, M_DONTWAIT, MT_HEADER);
one could also use a slightly smaller hammer and only avoid sending IPv6
if the user requested an IPv4 unicast address:
- if (sc->sc_naddrs6) {
+ if (sc->sc_naddrs6 &&
+ (! sc->sc_naddrs ||
+ sc->sc_peer.s_addr == INADDR_CARP_GROUP) ) {
Christopher
--
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
CB07 DA40 B0B6 571D 35E2 0DEF 87E2 92A7 13E5 DEE1
Re: lpt(4): timeout_add(9) -> timeout_add_msec(9), tsleep(9) -> tsleep_nsec(9)
On 14/01/20(Tue) 18:37, Scott Cheloha wrote:
> Ticks to milliseconds.
> [...]
> @@ -303,16 +304,17 @@ lptpushbytes(struct lpt_softc *sc)
> while (NOT_READY()) {
> if (++spin < sc->sc_spinmax)
> continue;
> - tic = 0;
> + msecs = 10;
> /* adapt busy-wait algorithm */
> sc->sc_spinmax++;
> while (NOT_READY_ERR()) {
> /* exponential backoff */
> - tic = tic + tic + 1;
> - if (tic > TIMEOUT)
> - tic = TIMEOUT;
> - error = tsleep((caddr_t)sc,
> - LPTPRI | PCATCH, "lptpsh", tic);
> + msecs = msecs + msecs + 1;
Shouldn't the backoff algorithm be:
msecs = 0;
while (/* not ready */) {
msecs = msecs + msecs + 10;
...
}
