Re: regress/bgpd: allow specifying daemon binary
Wow, suddenly this is about awk? I am going to ask you to do something: Please run the entire regress test on a system. Why not run it twice, without rebooting. Report back, ok? The complexity you wish to embrace so much results in a variety of costs you are about to be introduced to. How we got to this situation *IS DIRECTLY RELATED TO THE COMPLEXITY* Every little bit of abstraction that one person wants, stands in the way of someone else making the regression tests more robust and self-repairing after running a test. In essence, it makes people stop fixing regression tests. Fill the regression tests full of your desires, and other people walk away. Steffen Nurpmeso wrote: > Theo de Raadt wrote in <87985.1537307...@cvs.openbsd.org>: > |I honestly think this is a foolishly complicated. > > Maybe for OpenBSD only software. But i think it is worth the > hazzle whenever affordable for certain types of software. > > |Just install the program, then run regress. Install an older version > |without the broken changes if it doesn't work. > | > |I tire of these interactions between environment variables, > |base build methods, fork+exec paths in privsep programs, and now > |getting tied into regress tests. > > Ok. Yes that i understand. > > |In a word, YUCK. > > But, this is a makefile. And also one which is most often run in > the magic environment of BSD make system. It could even fail to > run as necessary by using prechecks, or restart itself via "env > -i" otherwise. (You know all this of course.) > > |I think this isn't "convenience". Rather it comes off as artifically > |complicated, trying to solve a problem which doesn't need to be exist > |at all. Perhaps even perceiving there to be a problem which needs > |solving via such abstration is the true problem. > > This is why i step in: i have found it valuable more than once. > For example i (this is me you know) was able to interest the > busybox maintain for an awk bug, he even downloaded reproducers; > like so (i trim this, from a bugzilla page, issue 10596): > > You can use the mdocmx.1 file from the same URL [1] mdocmx.sh is > at, it is the smallest (3010 bytes) i have that can be processed > by the script: > > ?0[steffen@essex]$ AWK=nawk ./mdocmx.sh(stdin)= e949a0a362d409f79a171583ff5354a678032dcc > ?0[steffen@essex]$ AWK=mawk ./mdocmx.sh(stdin)= e949a0a362d409f79a171583ff5354a678032dcc > ?0[steffen@essex]$ AWK=gawk ./mdocmx.sh(stdin)= e949a0a362d409f79a171583ff5354a678032dcc > ?0[steffen@essex]$ AWK='/bin/busybox awk' ./mdocmx.sh openssl sha1 > (stdin)= ee1f17a703d6fa8260854e69f129eabe12db7271 > > It would have been easy for this one, only a single place had to > become edited. But like this i could continue development and > perform very easy checks without any further maintenance cost, > with only a little hurdle at the development start. > > --steffen > | > |Der Kragenbaer,The moon bear, > |der holt sich munter he cheerfully and one by one > |einen nach dem anderen runter wa.ks himself off > |(By Robert Gernhardt) (sine curve emphasis, then egress) >
Re: regress/bgpd: allow specifying daemon binary
Theo de Raadt wrote in <87985.1537307...@cvs.openbsd.org>: |I honestly think this is a foolishly complicated. Maybe for OpenBSD only software. But i think it is worth the hazzle whenever affordable for certain types of software. |Just install the program, then run regress. Install an older version |without the broken changes if it doesn't work. | |I tire of these interactions between environment variables, |base build methods, fork+exec paths in privsep programs, and now |getting tied into regress tests. Ok. Yes that i understand. |In a word, YUCK. But, this is a makefile. And also one which is most often run in the magic environment of BSD make system. It could even fail to run as necessary by using prechecks, or restart itself via "env -i" otherwise. (You know all this of course.) |I think this isn't "convenience". Rather it comes off as artifically |complicated, trying to solve a problem which doesn't need to be exist |at all. Perhaps even perceiving there to be a problem which needs |solving via such abstration is the true problem. This is why i step in: i have found it valuable more than once. For example i (this is me you know) was able to interest the busybox maintain for an awk bug, he even downloaded reproducers; like so (i trim this, from a bugzilla page, issue 10596): You can use the mdocmx.1 file from the same URL [1] mdocmx.sh is at, it is the smallest (3010 bytes) i have that can be processed by the script: ?0[steffen@essex]$ AWK=nawk ./mdocmx.sh
Re: regress/bgpd: allow specifying daemon binary
Klemens Nanni wrote: > On Tue, Sep 18, 2018 at 03:44:27PM -0600, Theo de Raadt wrote: > > I honestly think this is a foolishly complicated. > > > > Just install the program, then run regress. Install an older version > > without the broken changes if it doesn't work. > > > > I tire of these interactions between environment variables, > > base build methods, fork+exec paths in privsep programs, and now > > getting tied into regress tests. > > > > In a word, YUCK. > > > > I think this isn't "convenience". Rather it comes off as artifically > > complicated, trying to solve a problem which doesn't need to be exist > > at all. Perhaps even perceiving there to be a problem which needs > > solving via such abstration is the true problem. > For me it clearly is convenient and actually less overhead. So piles of chicken scratches -- just for you? > Why elevating to root and writing to /usr for each test instead of just > pointing it at my local build? A regress test's purpose is to verify that the new program passes known tests. You may as well install it, because you endeavor to make a correct program, don't you? I find it strange you are so terrified of evelating to root to install the program which later on gets run as root. But basically, I believe there must be resistance against the continual desire to abstract the regression tests further and further, to the point where fewer people understand them and fewer people run them. The desire to introduce complexity is a disease. So basically you can see I disagree with the approach being taken here for numerous reasons.
Re: regress/bgpd: allow specifying daemon binary
On Tue, Sep 18, 2018 at 03:44:27PM -0600, Theo de Raadt wrote: > I honestly think this is a foolishly complicated. > > Just install the program, then run regress. Install an older version > without the broken changes if it doesn't work. > > I tire of these interactions between environment variables, > base build methods, fork+exec paths in privsep programs, and now > getting tied into regress tests. > > In a word, YUCK. > > I think this isn't "convenience". Rather it comes off as artifically > complicated, trying to solve a problem which doesn't need to be exist > at all. Perhaps even perceiving there to be a problem which needs > solving via such abstration is the true problem. For me it clearly is convenient and actually less overhead. Why elevating to root and writing to /usr for each test instead of just pointing it at my local build?
bgpd: sync host*() changes from pfctl
This simplifies host() and merges host_v{4,6}() into host_ip() as
recently done for pfctl and ntpd.
config regress still passes but I don't have a real BGP setup to tinker
with so proper testing is highly appreciated.
Feedback? OK?
Index: config.c
===
RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
retrieving revision 1.73
diff -u -p -r1.73 config.c
--- config.c9 Sep 2018 11:00:51 - 1.73
+++ config.c18 Sep 2018 21:22:22 -
@@ -37,8 +37,7 @@
#include "log.h"
u_int32_t get_bgpid(void);
-inthost_v4(const char *, struct bgpd_addr *, u_int8_t *);
-inthost_v6(const char *, struct bgpd_addr *);
+inthost_ip(const char *, struct bgpd_addr *, u_int8_t *);
void free_networks(struct network_head *);
void free_rdomains(struct rdomain_head *);
@@ -317,80 +316,58 @@ get_bgpid(void)
int
host(const char *s, struct bgpd_addr *h, u_int8_t *len)
{
- int done = 0;
- int mask;
+ int mask = 128;
char*p, *ps;
const char *errstr;
- if ((p = strrchr(s, '/')) != NULL) {
- mask = strtonum(p + 1, 0, 128, &errstr);
+ if ((ps = strdup(s)) == NULL)
+ fatal("%s: strdup", __func__);
+
+ if ((p = strrchr(ps, '/')) != NULL) {
+ mask = strtonum(p+1, 0, 128, &errstr);
if (errstr) {
- log_warnx("prefixlen is %s: %s", errstr, p + 1);
+ log_warnx("prefixlen is %s: %s", errstr, p+1);
return (0);
}
- if ((ps = malloc(strlen(s) - strlen(p) + 1)) == NULL)
- fatal("host: malloc");
- strlcpy(ps, s, strlen(s) - strlen(p) + 1);
- } else {
- if ((ps = strdup(s)) == NULL)
- fatal("host: strdup");
- mask = 128;
- }
-
- bzero(h, sizeof(struct bgpd_addr));
-
- /* IPv4 address? */
- if (!done)
- done = host_v4(s, h, len);
-
- /* IPv6 address? */
- if (!done) {
- done = host_v6(ps, h);
- *len = mask;
+ p[0] = '\0';
}
- free(ps);
-
- return (done);
-}
+ bzero(h, sizeof(*h));
-int
-host_v4(const char *s, struct bgpd_addr *h, u_int8_t *len)
-{
- struct in_addr ina = { 0 };
- int bits = 32;
-
- if (strrchr(s, '/') != NULL) {
- if ((bits = inet_net_pton(AF_INET, s, &ina, sizeof(ina))) == -1)
- return (0);
- } else {
- if (inet_pton(AF_INET, s, &ina) != 1)
- return (0);
+ if (host_ip(ps, h, len) == 0) {
+ free(ps);
+ return (0);
}
- h->aid = AID_INET;
- h->v4.s_addr = ina.s_addr;
- *len = bits;
+ if (p != NULL)
+ *len = mask;
+ free(ps);
return (1);
}
int
-host_v6(const char *s, struct bgpd_addr *h)
+host_ip(const char *s, struct bgpd_addr *h, u_int8_t *len)
{
struct addrinfo hints, *res;
+ int bits;
bzero(&hints, sizeof(hints));
- hints.ai_family = AF_INET6;
+ hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_DGRAM; /*dummy*/
hints.ai_flags = AI_NUMERICHOST;
- if (getaddrinfo(s, "0", &hints, &res) == 0) {
+ if (getaddrinfo(s, NULL, &hints, &res) == 0) {
+ *len = res->ai_family == AF_INET6 ? 128 : 32;
sa2addr(res->ai_addr, h);
freeaddrinfo(res);
- return (1);
+ } else {/* ie. for 10/8 parsing */
+ if ((bits = inet_net_pton(AF_INET, s, &h->v4, sizeof(h->v4)))
== -1)
+ return (0);
+ *len = bits;
+ h->aid = AID_INET;
}
- return (0);
+ return (1);
}
void
===
Stats: --- 50 lines 1196 chars
Stats: +++ 27 lines 785 chars
Stats: -23 lines
Stats: -411 chars
Re: regress/bgpd: allow specifying daemon binary
I honestly think this is a foolishly complicated.
Just install the program, then run regress. Install an older version
without the broken changes if it doesn't work.
I tire of these interactions between environment variables,
base build methods, fork+exec paths in privsep programs, and now
getting tied into regress tests.
In a word, YUCK.
I think this isn't "convenience". Rather it comes off as artifically
complicated, trying to solve a problem which doesn't need to be exist
at all. Perhaps even perceiving there to be a problem which needs
solving via such abstration is the true problem.
Klemens Nanni wrote:
> Same as in pfctl or route so I can easily test my changes with
>
> $ make BGPD=/usr/obj/usr.sbin/bgpd/bgpd config
>
> OK?
>
> Index: config/Makefile
> ===
> RCS file: /cvs/src/regress/usr.sbin/bgpd/config/Makefile,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile
> --- config/Makefile 10 Sep 2018 14:20:25 - 1.5
> +++ config/Makefile 18 Sep 2018 20:53:24 -
> @@ -1,5 +1,7 @@
> # $OpenBSD: Makefile,v 1.5 2018/09/10 14:20:25 benno Exp $
>
> +BGPD ?= /usr/sbin/bgpd
> +
> BGPDTESTS=1 2 3 4 5 6 7 8
>
> REGRESS_TARGETS = config
> @@ -9,12 +11,12 @@ BGPD_TARGETS+=bgpd${n}
> BGPD_UPDATES+=bgpd${n}-update
>
> bgpd${n}:
> - bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> + ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> sed 's/router-id .*/router-id 127.0.0.1/' | \
> diff -u ${.CURDIR}/bgpd.conf.${n}.ok /dev/stdin
>
> bgpd${n}-update:
> - bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> + ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> sed 's/router-id .*/router-id 127.0.0.1/' > \
> ${.CURDIR}/bgpd.conf.${n}.ok
> .endfor
> @@ -24,11 +26,11 @@ bgpd-update: ${BGPD_UPDATES}
>
> # check that the example configuration file we ship is ok
> bgpd-example:
> - bgpd -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
> + ${BGPD} -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
>
> # check that the output of bgpd -nvv is parseable
> bgpd-printconf:
> - bgpd -nvf ${.CURDIR}/bgpd.conf.printconf | \
> - bgpd -nf /dev/stdin
> + ${BGPD} -nvf ${.CURDIR}/bgpd.conf.printconf | \
> + ${BGPD} -nf /dev/stdin
>
> .include
>
Re: regress/bgpd: allow specifying daemon binary
On Tue, Sep 18, 2018 at 11:29:19PM +0200, Klemens Nanni wrote:
> Same as in pfctl or route so I can easily test my changes with
>
> $ make BGPD=/usr/obj/usr.sbin/bgpd/bgpd config
>
> OK?
OK bluhm@
> Index: config/Makefile
> ===
> RCS file: /cvs/src/regress/usr.sbin/bgpd/config/Makefile,v
> retrieving revision 1.5
> diff -u -p -r1.5 Makefile
> --- config/Makefile 10 Sep 2018 14:20:25 - 1.5
> +++ config/Makefile 18 Sep 2018 20:53:24 -
> @@ -1,5 +1,7 @@
> # $OpenBSD: Makefile,v 1.5 2018/09/10 14:20:25 benno Exp $
>
> +BGPD ?= /usr/sbin/bgpd
> +
> BGPDTESTS=1 2 3 4 5 6 7 8
>
> REGRESS_TARGETS = config
> @@ -9,12 +11,12 @@ BGPD_TARGETS+=bgpd${n}
> BGPD_UPDATES+=bgpd${n}-update
>
> bgpd${n}:
> - bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> + ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> sed 's/router-id .*/router-id 127.0.0.1/' | \
> diff -u ${.CURDIR}/bgpd.conf.${n}.ok /dev/stdin
>
> bgpd${n}-update:
> - bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> + ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
> sed 's/router-id .*/router-id 127.0.0.1/' > \
> ${.CURDIR}/bgpd.conf.${n}.ok
> .endfor
> @@ -24,11 +26,11 @@ bgpd-update: ${BGPD_UPDATES}
>
> # check that the example configuration file we ship is ok
> bgpd-example:
> - bgpd -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
> + ${BGPD} -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
>
> # check that the output of bgpd -nvv is parseable
> bgpd-printconf:
> - bgpd -nvf ${.CURDIR}/bgpd.conf.printconf | \
> - bgpd -nf /dev/stdin
> + ${BGPD} -nvf ${.CURDIR}/bgpd.conf.printconf | \
> + ${BGPD} -nf /dev/stdin
>
> .include
regress/bgpd: allow specifying daemon binary
Same as in pfctl or route so I can easily test my changes with
$ make BGPD=/usr/obj/usr.sbin/bgpd/bgpd config
OK?
Index: config/Makefile
===
RCS file: /cvs/src/regress/usr.sbin/bgpd/config/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- config/Makefile 10 Sep 2018 14:20:25 - 1.5
+++ config/Makefile 18 Sep 2018 20:53:24 -
@@ -1,5 +1,7 @@
# $OpenBSD: Makefile,v 1.5 2018/09/10 14:20:25 benno Exp $
+BGPD ?= /usr/sbin/bgpd
+
BGPDTESTS=1 2 3 4 5 6 7 8
REGRESS_TARGETS = config
@@ -9,12 +11,12 @@ BGPD_TARGETS+=bgpd${n}
BGPD_UPDATES+=bgpd${n}-update
bgpd${n}:
- bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
+ ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
sed 's/router-id .*/router-id 127.0.0.1/' | \
diff -u ${.CURDIR}/bgpd.conf.${n}.ok /dev/stdin
bgpd${n}-update:
- bgpd -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
+ ${BGPD} -nv -f /dev/stdin < ${.CURDIR}/bgpd.conf.${n}.in | \
sed 's/router-id .*/router-id 127.0.0.1/' > \
${.CURDIR}/bgpd.conf.${n}.ok
.endfor
@@ -24,11 +26,11 @@ bgpd-update: ${BGPD_UPDATES}
# check that the example configuration file we ship is ok
bgpd-example:
- bgpd -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
+ ${BGPD} -nf ${.CURDIR}/../../../../etc/examples/bgpd.conf
# check that the output of bgpd -nvv is parseable
bgpd-printconf:
- bgpd -nvf ${.CURDIR}/bgpd.conf.printconf | \
- bgpd -nf /dev/stdin
+ ${BGPD} -nvf ${.CURDIR}/bgpd.conf.printconf | \
+ ${BGPD} -nf /dev/stdin
.include
Re: timecounter memory barriers
> Date: Tue, 18 Sep 2018 17:59:21 +0200
> From: Alexander Bluhm
>
> Hi,
>
> In timecounter code, generation number and timehands are volatile,
> but there are no memory barriers. In general such code is wrong
> for SMP as it tells the compiler to care about ordering but ignores
> cache reordering.
>
> NetBSD puts membar_producer() at places where I would put them.
> But in binuptime() they have a comment that barriers would be too
> expensive and give an argument for code that we don't have.
>
> * Memory barriers are also too expensive to use for such a
> * performance critical function. The good news is that we do not
> * need memory barriers for this type of exclusion, as the thread
> * updating timecounter_removals will issue a broadcast cross call
> * before inspecting our l_tcgen value (this elides memory ordering
> * issues).
>
> FreeBSD has put atomic operations in binuptime() and tc_windup(),
> but I don't understand exctly what they do.
>
> gen = atomic_load_acq_int(&th->th_generation);
> atomic_thread_fence_acq();
> atomic_thread_fence_rel();
> atomic_store_rel_int(&th->th_generation, ogen);
Those are atomic operations with acquire and release semantics.
They're pretty much a combination of an atomic instruction with a
memory barrier. Combining these has some benefits on arcitectures,
such as x86, where atomic operations have builtin memory ordering
effects. They confuse the hell out of me though.
> So I think with our membar functions the code should look like this.
> On amd64 they are just compiler barriers anyway.
>
> ok?
I think this is correct.
ok kettenis@
> Index: kern/kern_tc.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_tc.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 kern_tc.c
> --- kern/kern_tc.c28 May 2018 18:05:42 - 1.33
> +++ kern/kern_tc.c18 Sep 2018 15:39:54 -
> @@ -22,6 +22,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -141,8 +142,10 @@ binuptime(struct bintime *bt)
> do {
> th = timehands;
> gen = th->th_generation;
> + membar_consumer();
> *bt = th->th_offset;
> bintime_addx(bt, th->th_scale * tc_delta(th));
> + membar_consumer();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> @@ -199,7 +202,9 @@ getnanouptime(struct timespec *tsp)
> do {
> th = timehands;
> gen = th->th_generation;
> + membar_consumer();
> bintime2timespec(&th->th_offset, tsp);
> + membar_consumer();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> @@ -212,7 +217,9 @@ getmicrouptime(struct timeval *tvp)
> do {
> th = timehands;
> gen = th->th_generation;
> + membar_consumer();
> bintime2timeval(&th->th_offset, tvp);
> + membar_consumer();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> @@ -225,7 +232,9 @@ getnanotime(struct timespec *tsp)
> do {
> th = timehands;
> gen = th->th_generation;
> + membar_consumer();
> *tsp = th->th_nanotime;
> + membar_consumer();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> @@ -238,7 +247,9 @@ getmicrotime(struct timeval *tvp)
> do {
> th = timehands;
> gen = th->th_generation;
> + membar_consumer();
> *tvp = th->th_microtime;
> + membar_consumer();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> @@ -390,6 +401,7 @@ tc_windup(void)
> th = tho->th_next;
> ogen = th->th_generation;
> th->th_generation = 0;
> + membar_producer();
> memcpy(th, tho, offsetof(struct timehands, th_generation));
>
> /*
> @@ -481,11 +493,13 @@ tc_windup(void)
>*/
> if (++ogen == 0)
> ogen = 1;
> + membar_producer();
> th->th_generation = ogen;
>
> /* Go live with the new struct timehands. */
> time_second = th->th_microtime.tv_sec;
> time_uptime = th->th_offset.sec;
> + membar_producer();
> timehands = th;
> }
>
>
>
Re: csh: simplify strsave()
On Tue, 18 Sep 2018 11:51:26 -0600, "Todd C. Miller" wrote: > To use strchr(3) here we either need mask the char value with 0xff > or figure out why asyntax() is calling any() with a char value of > -32768. The -32768 comes from a Char with the QUOTE bit set. This gets sign-extended when cast to an int for any(). Masking c with TRIM to gets rid of the funny value but doesn't fix the underlying issue. - todd
Re: csh: simplify strsave()
On Sat, 15 Sep 2018 12:42:22 +0200, Martijn van Duren wrote: > While here, should we also remove any in favour of strchr? Only > difference seems to be the return type (bool vs pointer). This turned out to be a problem. There are callers of any() that pass in a value that is not in the range [0, 255]. This causes unexpected behavior with the i386 assembler version of strchr(3). To use strchr(3) here we either need mask the char value with 0xff or figure out why asyntax() is calling any() with a char value of -32768. - todd
Re: ix0/1/2/3 at pci8 dev 0 function 0 "Intel X553 SFP+" rev 0x11: msi, address 00:30:18:xxxxxxx
On 18.9.2018. 18:18, sven falempin wrote: > On Tue, Sep 18, 2018 at 4:39 AM Hrvoje Popovski wrote: > >> On 17.9.2018. 22:32, sven falempin wrote: >>> Dear Tech reader, >>> >>> I am recently working on Intel(R) Atom(TM) CPU C3758 intel devices. >>> SFP Intel card are not working in 6.3/current openBSD base >>> >>> I did patch intel driver reading netbsd, freebsd and intel code of ixgbe >>> driver. >>> >>> I am now transferring data between two openBSD at ~1.50 Gb/s >>> for more than 48 hours ( been looping all week end ) >>> >>> First, i d like to find other user with ix card to check for regression ! >>> Secondly, can i get some feedback on how to test 10Gb /s transfer >>> - i usually download ramfs file through nginx or use iperf . >>> Third, how can i get a patch accepted into base : ie, how do i clean this >>> work ? >> >> Hi, >> >> user here. Thank you for doing this. I think that your primary concern >> at this point should be stability of this driver. >> While transferring data over ix interfaces you could try shutting down >> interfaces and bring them up. maybe something like this in loop: >> >> ifconfig ix0 down && ifconfig ix0 up && ifconfig ifconfig ix1 down && >> ifconfig ix1 up && ifconfig ix0 down && ifconfig ix1 down && sh netstart >> >> > that is working okay, and unplugging too > > >> >> if you have some sx or lx sfp+ modules insert/remove them in ix >> interfaces and stuff link that. >> > > I only have one type of sfp+ with me > > >> >> regarding performance testing if you have other box with 10G interfaces >> you could directly connect these boxes and generate lots of traffic over >> your driver and doing all that crazy stuff.. >> >> > i only have openbsd denverton hardware and it s kinda hard to sustain 10Gb > of traffic > > > I had a request to extract the github patch file directly inside the email, > i m thinking the 17 K lines would not make sense inside the mail. > > Is there someone with ixgbe hardware ( especially with a fan ? ) i think that x540-t2 does have fan. i will test your diff with x520 and x540.
Re: ix0/1/2/3 at pci8 dev 0 function 0 "Intel X553 SFP+" rev 0x11: msi, address 00:30:18:xxxxxxx
On Tue, Sep 18, 2018 at 4:39 AM Hrvoje Popovski wrote: > On 17.9.2018. 22:32, sven falempin wrote: > > Dear Tech reader, > > > > I am recently working on Intel(R) Atom(TM) CPU C3758 intel devices. > > SFP Intel card are not working in 6.3/current openBSD base > > > > I did patch intel driver reading netbsd, freebsd and intel code of ixgbe > > driver. > > > > I am now transferring data between two openBSD at ~1.50 Gb/s > > for more than 48 hours ( been looping all week end ) > > > > First, i d like to find other user with ix card to check for regression ! > > Secondly, can i get some feedback on how to test 10Gb /s transfer > > - i usually download ramfs file through nginx or use iperf . > > Third, how can i get a patch accepted into base : ie, how do i clean this > > work ? > > Hi, > > user here. Thank you for doing this. I think that your primary concern > at this point should be stability of this driver. > While transferring data over ix interfaces you could try shutting down > interfaces and bring them up. maybe something like this in loop: > > ifconfig ix0 down && ifconfig ix0 up && ifconfig ifconfig ix1 down && > ifconfig ix1 up && ifconfig ix0 down && ifconfig ix1 down && sh netstart > > that is working okay, and unplugging too > > if you have some sx or lx sfp+ modules insert/remove them in ix > interfaces and stuff link that. > I only have one type of sfp+ with me > > regarding performance testing if you have other box with 10G interfaces > you could directly connect these boxes and generate lots of traffic over > your driver and doing all that crazy stuff.. > > i only have openbsd denverton hardware and it s kinda hard to sustain 10Gb of traffic I had a request to extract the github patch file directly inside the email, i m thinking the 17 K lines would not make sense inside the mail. Is there someone with ixgbe hardware ( especially with a fan ? ) Best. -- -- - Knowing is not enough; we must apply. Willing is not enough; we must do
Re: urndis0: urndis_decap invalid buffer len 1 < minimum header 44
On Sat, Aug 18, 2018 at 11:05:04PM +0300, Artturi Alm wrote:
> On Thu, Jan 11, 2018 at 12:41:29AM +0200, Artturi Alm wrote:
> > On Wed, Sep 13, 2017 at 05:51:27AM +0300, Artturi Alm wrote:
> > > Hi,
> > >
> > > even after having recently updated the phone to a newer version of
> > > android,
> > > i'm still spammed by urndis w/msg on subject.
> > >
> > > doesn't really matter to me what you do to silence it, but something like
> > > below does work for me, and thanks in advacne:)
> > > -Artturi
> > >
> >
> > ping?
> > i was told i don't reason my diffs, so here's sorry attempt:
> > $ dmesg | wc -l
> > 1040
> > $ dmesg | grep urndis_decap | wc -l
> > 1039
> >
> > either of the diffs below would work for me.
> > -Artturi
> >
> >
> > ... this ...
> >
> > diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
> > index 5d148da4ab5..7dc12573c0d 100644
> > --- a/sys/dev/usb/if_urndis.c
> > +++ b/sys/dev/usb/if_urndis.c
> > @@ -834,11 +834,11 @@ urndis_decap(struct urndis_softc *sc, struct
> > urndis_chain *c, u_int32_t len)
> > len));
> >
> > if (len < sizeof(*msg)) {
> > - printf("%s: urndis_decap invalid buffer len %u < "
> > + DPRINTF(("%s: urndis_decap invalid buffer len %u < "
> > "minimum header %zu\n",
> > DEVNAME(sc),
> > len,
> > - sizeof(*msg));
> > + sizeof(*msg)));
> > return;
> > }
> >
> >
> >
> > ... or this ...
> >
> > diff --git a/sys/dev/usb/if_urndis.c b/sys/dev/usb/if_urndis.c
> > index 5d148da4ab5..4b2c6e89ec9 100644
> > --- a/sys/dev/usb/if_urndis.c
> > +++ b/sys/dev/usb/if_urndis.c
> > @@ -834,6 +834,8 @@ urndis_decap(struct urndis_softc *sc, struct
> > urndis_chain *c, u_int32_t len)
> > len));
> >
> > if (len < sizeof(*msg)) {
> > + if (len == 1) /* workaround for spamming androids */
> > + return;
> > printf("%s: urndis_decap invalid buffer len %u < "
> > "minimum header %zu\n",
> > DEVNAME(sc),
>
> Hi,
>
> this should fix the urndis_decap() spam more properly by allowing operation
> as is needed by the linux(android) gadget/function code for usb rndis too,
> which is explained there to be
> "zlp framing on tx for strict CDC-Ether conformance",
> and our cdce(4) does have somewhat similar if (total_len <= 1) goto done;.
>
> also, i think the _decap spam is as bad as it is because of those returns,
> dropping valid packet met before "error", leading to retries likely being
> the same; triggering the spam loop..
>
pong? the spam and dropping of a valid packet is not correct.
this is far from making the driver bug-free/following the spec,
but definately worth fixing as the only user-visible annoyance.
i don't think i can explain why any better than is done here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/usb-short-packets
above is specifically about/for rndis, even if the url doesn't hint so,
and there is a note it'll take two minutes to read..
> -Artturi
>
>
> diff --git sys/dev/usb/if_urndis.c sys/dev/usb/if_urndis.c
> index 5d148da4ab5..136e3e22af8 100644
> --- sys/dev/usb/if_urndis.c
> +++ sys/dev/usb/if_urndis.c
> @@ -820,13 +820,15 @@ urndis_decap(struct urndis_softc *sc, struct
> urndis_chain *c, u_int32_t len)
> struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> struct rndis_packet_msg *msg;
> struct ifnet*ifp;
> + int got;
> int s;
> int offset;
>
> ifp = GET_IFP(sc);
> + got = 0;
> offset = 0;
>
> - while (len > 0) {
> + while (len > 1) {
> msg = (struct rndis_packet_msg *)((char*)c->sc_buf + offset);
> m = c->sc_mbuf;
>
> @@ -839,7 +841,7 @@ urndis_decap(struct urndis_softc *sc, struct urndis_chain
> *c, u_int32_t len)
> DEVNAME(sc),
> len,
> sizeof(*msg));
> - return;
> + break;
> }
>
> DPRINTF(("%s: urndis_decap len %u data(off:%u len:%u) "
> @@ -859,14 +861,14 @@ urndis_decap(struct urndis_softc *sc, struct
> urndis_chain *c, u_int32_t len)
> DEVNAME(sc),
> letoh32(msg->rm_type),
> REMOTE_NDIS_PACKET_MSG);
> - return;
> + break;
> }
> if (letoh32(msg->rm_len) < sizeof(*msg)) {
> printf("%s: urndis_decap invalid msg len %u < %zu\n",
> DEVNAME(sc),
> letoh32(msg->rm_len),
> sizeof(*msg));
> -
bgpd: more refactoring for ROA sets
Since the first bit of ROA sets is in here some refactoring of the code.
Split up as_set into a set_table and an as_set. The first is what does
the lookup and will now also be used in roa-set tries. The as_set is glue
to add the name and dirty flag. Add an accessor to get the set data so
that the imsg sending and printing can be moved into the right places.
This is done mainly because roa-sets need similar but slightly different
versions and this is the best way fixing this.
This diff is agains /usr/src since it includes bgpctl and regress changes
as well.
As usual looking for OKs :)
--
:wq Claudio
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.216
diff -u -p -r1.216 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c14 Sep 2018 10:22:55 - 1.216
+++ usr.sbin/bgpctl/bgpctl.c18 Sep 2018 15:55:45 -
@@ -2656,8 +2656,8 @@ msg_type(u_int8_t type)
return (msgtypenames[type]);
}
-void *
+int
as_set_match(const struct as_set *a, u_int32_t asnum)
{
- return (NULL);
+ return (0);
}
Index: usr.sbin/bgpd/bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.198
diff -u -p -r1.198 bgpd.c
--- usr.sbin/bgpd/bgpd.c9 Sep 2018 11:00:51 - 1.198
+++ usr.sbin/bgpd/bgpd.c18 Sep 2018 15:55:45 -
@@ -436,6 +436,7 @@ reconfigure(char *conffile, struct bgpd_
struct listen_addr *la;
struct rde_rib *rr;
struct rdomain *rd;
+ struct as_set *aset;
struct prefixset*ps;
struct prefixset_item *psi;
@@ -521,10 +522,36 @@ reconfigure(char *conffile, struct bgpd_
}
/* as-sets for filters in the RDE */
- if (as_sets_send(ibuf_rde, conf->as_sets) == -1)
- return (-1);
- as_sets_free(conf->as_sets);
- conf->as_sets = NULL;
+ while ((aset = SIMPLEQ_FIRST(conf->as_sets)) != NULL) {
+ struct ibuf *wbuf;
+ u_int32_t *as;
+ size_t i, l, n;
+
+ SIMPLEQ_REMOVE_HEAD(conf->as_sets, entry);
+
+ as = set_get(aset->set, &n);
+ if ((wbuf = imsg_create(ibuf_rde, IMSG_RECONF_AS_SET, 0, 0,
+ sizeof(n) + sizeof(aset->name))) == NULL)
+ return -1;
+ if (imsg_add(wbuf, &n, sizeof(n)) == -1 ||
+ imsg_add(wbuf, aset->name, sizeof(aset->name)) == -1)
+ return -1;
+ imsg_close(ibuf_rde, wbuf);
+
+ for (i = 0; i < n; i += l) {
+ l = (n - i > 1024 ? 1024 : n - i);
+ if (imsg_compose(ibuf_rde, IMSG_RECONF_AS_SET_ITEMS,
+ 0, 0, -1, as + i, l) == -1)
+ return -1;
+ }
+
+ if (imsg_compose(ibuf_rde, IMSG_RECONF_AS_SET_DONE, 0, 0, -1,
+ NULL, 0) == -1)
+ return -1;
+
+ set_free(aset->set);
+ free(aset);
+ }
/* filters for the RDE */
while ((r = TAILQ_FIRST(conf->filters)) != NULL) {
Index: usr.sbin/bgpd/bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.341
diff -u -p -r1.341 bgpd.h
--- usr.sbin/bgpd/bgpd.h18 Sep 2018 15:14:07 - 1.341
+++ usr.sbin/bgpd/bgpd.h18 Sep 2018 15:55:45 -
@@ -215,6 +215,7 @@ SIMPLEQ_HEAD(prefixset_head, prefixset);
struct rde_prefixset_head;
struct rde_prefixset;
+struct set_table;
struct as_set;
SIMPLEQ_HEAD(as_set_head, as_set);
@@ -970,6 +971,13 @@ struct prefixset {
SIMPLEQ_ENTRY(prefixset) entry;
};
+struct as_set {
+ char name[SET_NAME_LEN];
+ SIMPLEQ_ENTRY(as_set)entry;
+ struct set_table*set;
+ int dirty;
+};
+
struct rdomain {
SIMPLEQ_ENTRY(rdomain) entry;
chardescr[PEER_DESCR_LEN];
@@ -1169,20 +1177,21 @@ void filterset_move(struct filter_set_
const char *filterset_name(enum action_types);
/* rde_sets.c */
-voidas_sets_insert(struct as_set_head *, struct as_set *);
struct as_set *as_sets_lookup(struct as_set_head *, const char *);
+struct as_set *as_sets_new(struct as_set_head *, const char *, size_t,
+ size_t);
voidas_sets_free(struct as_set_head *);
-voidas_sets_print(struct as_set_head *);
-int as_sets_send(struct imsgbuf *, struct as_set_head *);
voidas_sets_mark_dirty(struct as_set_head *, struct as_set_head *);
+int as_set_match(const struct as_set *, u_int32_t);
-struct as_s
timecounter memory barriers
Hi,
In timecounter code, generation number and timehands are volatile,
but there are no memory barriers. In general such code is wrong
for SMP as it tells the compiler to care about ordering but ignores
cache reordering.
NetBSD puts membar_producer() at places where I would put them.
But in binuptime() they have a comment that barriers would be too
expensive and give an argument for code that we don't have.
* Memory barriers are also too expensive to use for such a
* performance critical function. The good news is that we do not
* need memory barriers for this type of exclusion, as the thread
* updating timecounter_removals will issue a broadcast cross call
* before inspecting our l_tcgen value (this elides memory ordering
* issues).
FreeBSD has put atomic operations in binuptime() and tc_windup(),
but I don't understand exctly what they do.
gen = atomic_load_acq_int(&th->th_generation);
atomic_thread_fence_acq();
atomic_thread_fence_rel();
atomic_store_rel_int(&th->th_generation, ogen);
So I think with our membar functions the code should look like this.
On amd64 they are just compiler barriers anyway.
ok?
bluhm
Index: kern/kern_tc.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_tc.c,v
retrieving revision 1.33
diff -u -p -r1.33 kern_tc.c
--- kern/kern_tc.c 28 May 2018 18:05:42 - 1.33
+++ kern/kern_tc.c 18 Sep 2018 15:39:54 -
@@ -22,6 +22,7 @@
*/
#include
+#include
#include
#include
#include
@@ -141,8 +142,10 @@ binuptime(struct bintime *bt)
do {
th = timehands;
gen = th->th_generation;
+ membar_consumer();
*bt = th->th_offset;
bintime_addx(bt, th->th_scale * tc_delta(th));
+ membar_consumer();
} while (gen == 0 || gen != th->th_generation);
}
@@ -199,7 +202,9 @@ getnanouptime(struct timespec *tsp)
do {
th = timehands;
gen = th->th_generation;
+ membar_consumer();
bintime2timespec(&th->th_offset, tsp);
+ membar_consumer();
} while (gen == 0 || gen != th->th_generation);
}
@@ -212,7 +217,9 @@ getmicrouptime(struct timeval *tvp)
do {
th = timehands;
gen = th->th_generation;
+ membar_consumer();
bintime2timeval(&th->th_offset, tvp);
+ membar_consumer();
} while (gen == 0 || gen != th->th_generation);
}
@@ -225,7 +232,9 @@ getnanotime(struct timespec *tsp)
do {
th = timehands;
gen = th->th_generation;
+ membar_consumer();
*tsp = th->th_nanotime;
+ membar_consumer();
} while (gen == 0 || gen != th->th_generation);
}
@@ -238,7 +247,9 @@ getmicrotime(struct timeval *tvp)
do {
th = timehands;
gen = th->th_generation;
+ membar_consumer();
*tvp = th->th_microtime;
+ membar_consumer();
} while (gen == 0 || gen != th->th_generation);
}
@@ -390,6 +401,7 @@ tc_windup(void)
th = tho->th_next;
ogen = th->th_generation;
th->th_generation = 0;
+ membar_producer();
memcpy(th, tho, offsetof(struct timehands, th_generation));
/*
@@ -481,11 +493,13 @@ tc_windup(void)
*/
if (++ogen == 0)
ogen = 1;
+ membar_producer();
th->th_generation = ogen;
/* Go live with the new struct timehands. */
time_second = th->th_microtime.tv_sec;
time_uptime = th->th_offset.sec;
+ membar_producer();
timehands = th;
}
Re: [patch] Use the same backlog parameter for listen() function in netcat.c
I don't see the point of making such a change.
Nan Xiao wrote:
> Hi tech@,
>
> Since netcat can only process one connection at one time, maybe
> UNIX-domain socket can use the same backlog parameter for listen()
> function as network socket, thanks!
>
> diff --git netcat.c netcat.c
> index 341e7e5..b6fd199 100644
> --- netcat.c
> +++ netcat.c
> @@ -894,7 +894,7 @@ unix_listen(char *path)
> if ((s = unix_bind(path, 0)) < 0)
> return -1;
>
> - if (listen(s, 5) < 0) {
> + if (listen(s, 1) < 0) {
> close(s);
> return -1;
> }
> --
> Best Regards
> Nan Xiao
>
[patch] Fix "Address already in use" issue when using netcat with UNIX-domain socket
Hi tech@,
Assume I use netcat with UNIX-domain socket, and there is no
temp_socket. Launch the server:
# ./nc -U -l temp_socket
It works normally. But after netcat exits, launch it again:
# nc -U -l temp_socket
nc: Address already in use
The only method seems to delete temp_socket.
I am not sure this behavior is as expected, and come out following patch
may fix this issue, thanks!
diff --git usr.bin/nc/netcat.c usr.bin/nc/netcat.c
index 341e7e50485..3b2150a01dc 100644
--- usr.bin/nc/netcat.c
+++ usr.bin/nc/netcat.c
@@ -749,6 +749,9 @@ unix_bind(char *path, int flags)
return -1;
}
+ if (lflag)
+ unlink(path);
+
if (bind(s, (struct sockaddr *)&s_un, sizeof(s_un)) < 0) {
save_errno = errno;
close(s);
--
Best Regards
Nan Xiao
PF: skip on VS pass in
Hi list, OpenBSD 6.3 -stable: I was playing with local network - tunning various things on pf, and i observe this odd(maybe not?) performance when using different approach: (trunk0 is local network, ext_1 is external, NAT is performed) when i used: set skip on trunk0 … … pass out on $ext_1 from 192.168.50.0/24 to any nat-to ($ext_1) pf was performing at around 600Mbps but when i used: pass out on $ext_1 from 192.168.50.0/24 to any nat-to ($ext_1) pass in on trunk0 performance dropped by 50% to 300Mbps other rules on pf do not matter - i ruled out their influence on performance, but i am curious if anyone observed this and have some insights on that, or am I doing something terribly wrong? (btw, yes i know what skip-on means, just i am surprised by diff in performance by using that 2 options…) _ Zbyszek Żółkiewski
Re: mandoc: typo fix (?) in mandoc.1
Hi Ingo, Thanks for the quick feedback. Sorry for the wrong assumption. Regarding the styling documentation, I believe it should stay in, but maybe also add a small note describing it's hardcoded? That might however make the over-documentation situation even worse. Greetings, Sascha On 18.09.2018 08:13, Ingo Schwarze wrote: Hi Sascha, Sascha Paunovic wrote on Tue, Sep 18, 2018 at 06:39:51AM +0200: while reading mandoc(1), I noticed that under the PostScript output section, it said the line-height was 1.4m, while it is 1.4em. I doubt the line-height is approximately as tall as a 5th grader, so let's clarify this: No, "1.4m" is a roff scaling width, see the roff(7) manual: https://man.openbsd.org/roff.7#Scaling_Widths So the patch is not OK. I wonder whether this is over-documented, though. Font family, point size, line height, and margins are hard-coded and cannot be changed by the user, so i wonder why they are even mentioned. Well, maybe it occasionally helps to understand why the output looks as it does - though people who care about such details should probably use a real typesetter like groff to generate PostScript and not rely on the mandoc -Tps output... Yours, Ingo diff --git usr.bin/mandoc/mandoc.1 usr.bin/mandoc/mandoc.1 index aa19c49d6..e180145ee 100644 --- usr.bin/mandoc/mandoc.1 +++ usr.bin/mandoc/mandoc.1 @@ -459,7 +459,7 @@ Level-2 pages may be generated by Output pages default to letter sized and are rendered in the Times font family, 11-point. Margins are calculated as 1/9 the page length and width. -Line-height is 1.4m. +Line-height is 1.4em. .Pp Special characters are rendered as in .Sx ASCII Output .
Re: uninitialized variable in if_mue.c
On Tue, Sep 18, 2018 at 08:40:24AM +0100, Ricardo Mestre wrote:
>
> Ouch, of course! Still not enough caffeine in the system!
>
> Unfortunately I don't have such a card to test it, but this is the way it's
> done
> everywhere else, the writes are always done unconditionally and we just need
> to
> ensure that the hash table is initialized to 0 so I prefer to keep the
> consistency.
Thanks for fixing it. ok kevlo@
> Index: if_mue.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_mue.c,v
> retrieving revision 1.4
> diff -u -p -u -r1.4 if_mue.c
> --- if_mue.c 15 Aug 2018 07:13:51 - 1.4
> +++ if_mue.c 18 Sep 2018 07:27:27 -
> @@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc)
> rxfilt = mue_csr_read(sc, reg);
> rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH |
> MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST);
> + memset(hashtbl, 0, sizeof(hashtbl));
> ifp->if_flags &= ~IFF_ALLMULTI;
>
> /* Always accept broadcast frames. */
> @@ -1028,9 +1029,6 @@ mue_iff(struct mue_softc *sc)
> rxfilt |= MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST;
> } else {
> rxfilt |= MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH;
> -
> - /* Clear hash table. */
> - memset(hashtbl, 0, sizeof(hashtbl));
>
> /* Now program new ones. */
> ETHER_FIRST_MULTI(step, ac, enm);
>
> On 09:06 Tue 18 Sep , Claudio Jeker wrote:
> > On Tue, Sep 18, 2018 at 07:55:43AM +0100, Ricardo Mestre wrote:
> > > Hi,
> > >
> > > In the case that a mue(4) device is put in promiscuous mode then hashtbl
> > > will
> > > be used uninitialized a little bit down the road so set it 0 like it's
> > > done in
> > > a lot of other devices. Coverity ID 1473316.
> > >
> > > OK?
> >
> > Please also remove the later memset(). There is no need to do it twice.
> > It would also be an option to not issue mue_dataport_write(MUE_DP_SEL_VHF)
> > if the hash is not used (e.g. moving that up into the else block.
> > Which ever option taken it needs to be tested on real hardware.
> >
> > --
> > :wq Claudio
>
>
Re: ix0/1/2/3 at pci8 dev 0 function 0 "Intel X553 SFP+" rev 0x11: msi, address 00:30:18:xxxxxxx
On 17.9.2018. 22:32, sven falempin wrote: > Dear Tech reader, > > I am recently working on Intel(R) Atom(TM) CPU C3758 intel devices. > SFP Intel card are not working in 6.3/current openBSD base > > I did patch intel driver reading netbsd, freebsd and intel code of ixgbe > driver. > > I am now transferring data between two openBSD at ~1.50 Gb/s > for more than 48 hours ( been looping all week end ) > > First, i d like to find other user with ix card to check for regression ! > Secondly, can i get some feedback on how to test 10Gb /s transfer > - i usually download ramfs file through nginx or use iperf . > Third, how can i get a patch accepted into base : ie, how do i clean this > work ? Hi, user here. Thank you for doing this. I think that your primary concern at this point should be stability of this driver. While transferring data over ix interfaces you could try shutting down interfaces and bring them up. maybe something like this in loop: ifconfig ix0 down && ifconfig ix0 up && ifconfig ifconfig ix1 down && ifconfig ix1 up && ifconfig ix0 down && ifconfig ix1 down && sh netstart if you have some sx or lx sfp+ modules insert/remove them in ix interfaces and stuff link that. regarding performance testing if you have other box with 10G interfaces you could directly connect these boxes and generate lots of traffic over your driver and doing all that crazy stuff..
Re: uninitialized variable in if_mue.c
> Date: Tue, 18 Sep 2018 07:55:43 +0100 > From: Ricardo Mestre > > Hi, > > In the case that a mue(4) device is put in promiscuous mode then hashtbl will > be used uninitialized a little bit down the road so set it 0 like it's done in > a lot of other devices. Coverity ID 1473316. > > OK? > > Index: if_mue.c > === > RCS file: /cvs/src/sys/dev/usb/if_mue.c,v > retrieving revision 1.4 > diff -u -p -u -r1.4 if_mue.c > --- if_mue.c 15 Aug 2018 07:13:51 - 1.4 > +++ if_mue.c 18 Sep 2018 06:47:54 - > @@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc) > rxfilt = mue_csr_read(sc, reg); > rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH | > MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST); > + memset(hashtbl, 0x00, sizeof(hashtbl)); > ifp->if_flags &= ~IFF_ALLMULTI; > > /* Always accept broadcast frames. */ > > Thare already is a memset call to clear the hash table in the else block. I think that should simply be moved up.
Re: uninitialized variable in if_mue.c
Ouch, of course! Still not enough caffeine in the system!
Unfortunately I don't have such a card to test it, but this is the way it's done
everywhere else, the writes are always done unconditionally and we just need to
ensure that the hash table is initialized to 0 so I prefer to keep the
consistency.
Index: if_mue.c
===
RCS file: /cvs/src/sys/dev/usb/if_mue.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 if_mue.c
--- if_mue.c15 Aug 2018 07:13:51 - 1.4
+++ if_mue.c18 Sep 2018 07:27:27 -
@@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc)
rxfilt = mue_csr_read(sc, reg);
rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH |
MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST);
+ memset(hashtbl, 0, sizeof(hashtbl));
ifp->if_flags &= ~IFF_ALLMULTI;
/* Always accept broadcast frames. */
@@ -1028,9 +1029,6 @@ mue_iff(struct mue_softc *sc)
rxfilt |= MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST;
} else {
rxfilt |= MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH;
-
- /* Clear hash table. */
- memset(hashtbl, 0, sizeof(hashtbl));
/* Now program new ones. */
ETHER_FIRST_MULTI(step, ac, enm);
On 09:06 Tue 18 Sep , Claudio Jeker wrote:
> On Tue, Sep 18, 2018 at 07:55:43AM +0100, Ricardo Mestre wrote:
> > Hi,
> >
> > In the case that a mue(4) device is put in promiscuous mode then hashtbl
> > will
> > be used uninitialized a little bit down the road so set it 0 like it's done
> > in
> > a lot of other devices. Coverity ID 1473316.
> >
> > OK?
>
> Please also remove the later memset(). There is no need to do it twice.
> It would also be an option to not issue mue_dataport_write(MUE_DP_SEL_VHF)
> if the hash is not used (e.g. moving that up into the else block.
> Which ever option taken it needs to be tested on real hardware.
>
> --
> :wq Claudio
Re: memory leaks in bwfm
The code hands off the reponsibility of ctl and ctl->buf to bs_txctl
which will free both buffers if there is an error enqueueing the
command. Only if bs_txctl succeeds in enqueueing and there is a
response timeout we can free it. Thus, not ok. If this pattern
is not understandable then we can work on that, but the diff as is
will add double frees on error.
On Tue, Sep 18, 2018 at 03:52:45PM +1000, Jonathan Gray wrote:
> Index: bwfm.c
> ===
> RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 bwfm.c
> --- bwfm.c25 Jul 2018 20:37:11 - 1.54
> +++ bwfm.c18 Sep 2018 05:21:30 -
> @@ -1297,6 +1297,7 @@ bwfm_proto_bcdc_query_dcmd(struct bwfm_s
>
> if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, &size)) {
> DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
> return ret;
> }
>
> @@ -1337,6 +1338,7 @@ bwfm_proto_bcdc_set_dcmd(struct bwfm_sof
>
> if (bwfm_proto_bcdc_txctl(sc, reqid, (char *)dcmd, &size)) {
> DPRINTF(("%s: txctl failed\n", DEVNAME(sc)));
> + free(dcmd, M_TEMP, size);
> return ret;
> }
>
> @@ -1361,6 +1363,7 @@ bwfm_proto_bcdc_txctl(struct bwfm_softc
>
> if (sc->sc_bus_ops->bs_txctl(sc, ctl)) {
> DPRINTF(("%s: tx failed\n", DEVNAME(sc)));
> + free(ctl, M_TEMP, sizeof(*ctl));
> return 1;
> }
>
>
Re: memory leak in amdisplay_attach()
On Tue, Sep 18, 2018 at 08:34:55AM +0200, Claudio Jeker wrote:
> On Tue, Sep 18, 2018 at 03:49:15PM +1000, Jonathan Gray wrote:
> > Index: amdisplay.c
> > ===
> > RCS file: /cvs/src/sys/arch/armv7/omap/amdisplay.c,v
> > retrieving revision 1.7
> > diff -u -p -r1.7 amdisplay.c
> > --- amdisplay.c 25 Oct 2017 14:34:22 - 1.7
> > +++ amdisplay.c 18 Sep 2018 05:12:41 -
> > @@ -272,6 +272,7 @@ amdisplay_attach(struct device *parent,
> >
> > if (rasops_init(&sc->sc_ro, 200, 200)) {
> > printf("%s: no rasops\n", DEVNAME(sc));
> > + free(edid_buf, M_DEVBUF, EDID_LENGTH);
> > amdisplay_detach(self, 0);
> > return;
> > }
> >
>
> I think it is better to free the edid_buf further up in that function
> since it is unused after calling edid_parse(edid_buf, &sc->sc_edid) on
> line 215. So currently there is still a leak at the end of the function.
sounds good to me, ok
>
> --
> :wq Claudio
>
> Index: arch/armv7/omap/amdisplay.c
> ===
> RCS file: /cvs/src/sys/arch/armv7/omap/amdisplay.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 amdisplay.c
> --- arch/armv7/omap/amdisplay.c 25 Oct 2017 14:34:22 - 1.7
> +++ arch/armv7/omap/amdisplay.c 18 Sep 2018 06:32:43 -
> @@ -219,6 +219,8 @@ amdisplay_attach(struct device *parent,
> return;
> }
>
> + free(edid_buf, M_DEVBUF, EDID_LENGTH);
> +
> #ifdef LCD_DEBUG
> edid_print(&sc->sc_edid);
> #endif
> @@ -246,7 +248,6 @@ amdisplay_attach(struct device *parent,
> /* configure DMA framebuffer */
> if (amdisplay_setup_dma(sc)) {
> printf("%s: couldn't allocate DMA framebuffer\n", DEVNAME(sc));
> - free(edid_buf, M_DEVBUF, EDID_LENGTH);
> amdisplay_detach(self, 0);
> return;
> }
>
Re: uninitialized variable in if_mue.c
On Tue, Sep 18, 2018 at 07:55:43AM +0100, Ricardo Mestre wrote: > Hi, > > In the case that a mue(4) device is put in promiscuous mode then hashtbl will > be used uninitialized a little bit down the road so set it 0 like it's done in > a lot of other devices. Coverity ID 1473316. > > OK? Please also remove the later memset(). There is no need to do it twice. It would also be an option to not issue mue_dataport_write(MUE_DP_SEL_VHF) if the hash is not used (e.g. moving that up into the else block. Which ever option taken it needs to be tested on real hardware. > Index: if_mue.c > === > RCS file: /cvs/src/sys/dev/usb/if_mue.c,v > retrieving revision 1.4 > diff -u -p -u -r1.4 if_mue.c > --- if_mue.c 15 Aug 2018 07:13:51 - 1.4 > +++ if_mue.c 18 Sep 2018 06:47:54 - > @@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc) > rxfilt = mue_csr_read(sc, reg); > rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH | > MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST); > + memset(hashtbl, 0x00, sizeof(hashtbl)); > ifp->if_flags &= ~IFF_ALLMULTI; > > /* Always accept broadcast frames. */ > -- :wq Claudio
