Re: 7.3: Squid 6.3 with multiple workers - net.unix.dgram.sendspace too low
On Thu, Oct 19, 2023 at 03:09:08PM +0200, Silamael Darkomen wrote: > Hi, > > Today I upgraded to the brand new Squid version 6.3 from ports and noticed, > that Squid no longer starts properly if configured with multiple worker > processes. > > After some debugging the limit from net.unix.dgram.sendspace came up as > cause. The 2k default is way to low. > In ktrace I saw sendmessage calls with messages slightly over 4k. > > Increasing this limit to 16k as net.unix.dgram.recvspace fixes the problem > and Squid can start. > > Perhaps this historically low limit should be adjusted accordingly? > All for all other Unix sockets, sendspace and recvspace share the same > limits, just dgram sockets are out of line. > > PS: Tested this with 7.3 but 7.4 seems to have the same limitations. > It is a SOCK_DGRAM socket, it is supposed to be limited. >From the man-page: A SOCK_DGRAM socket supports datagrams (connectionless, unreliable messages of a fixed (typically small) maximum length). The program should set the socket buffer size via setsockopt() using SO_SNDBUF. It seems squid just YOLOs this and hopes for the best. So the best way to fix this is in squid itself. -- :wq Claudio
Re: 7.3: Squid 6.3 with multiple workers not starting - net.unix.dgram.sendspace too low
On 19 Oct 2023 16:36, Silamael Darkomen wrote: Hi, I just upgraded to Squid 6.3 under 7.3 and noticed that it no longer starts if configured to use multiple worker processes. After some debugging I found that net.unix.dgram.sendspace with its 2k limit is the reason. Squid uses Unix sockets for IPC and sends messages larger than those 2k (saw slightly more than 4k in ktrace). As this limit seems pretty historical and all other Unix sockets use larger limits and have sendspace same as recvspace I would suggest to increase net.unix.dgram.sendspace to 16k as net.unix.dgram.recvspace. PS: Tested on 7.3 but 7.4 seems to use the same old limits. Greetings, Matthias Sorry for the second mail.
7.3: Squid 6.3 with multiple workers - net.unix.dgram.sendspace too low
Hi, Today I upgraded to the brand new Squid version 6.3 from ports and noticed, that Squid no longer starts properly if configured with multiple worker processes. After some debugging the limit from net.unix.dgram.sendspace came up as cause. The 2k default is way to low. In ktrace I saw sendmessage calls with messages slightly over 4k. Increasing this limit to 16k as net.unix.dgram.recvspace fixes the problem and Squid can start. Perhaps this historically low limit should be adjusted accordingly? All for all other Unix sockets, sendspace and recvspace share the same limits, just dgram sockets are out of line. PS: Tested this with 7.3 but 7.4 seems to have the same limitations. Greetings, Matthias
7.3: Squid 6.3 with multiple workers not starting - net.unix.dgram.sendspace too low
Hi, I just upgraded to Squid 6.3 under 7.3 and noticed that it no longer starts if configured to use multiple worker processes. After some debugging I found that net.unix.dgram.sendspace with its 2k limit is the reason. Squid uses Unix sockets for IPC and sends messages larger than those 2k (saw slightly more than 4k in ktrace). As this limit seems pretty historical and all other Unix sockets use larger limits and have sendspace same as recvspace I would suggest to increase net.unix.dgram.sendspace to 16k as net.unix.dgram.recvspace. PS: Tested on 7.3 but 7.4 seems to use the same old limits. Greetings, Matthias
Re: snmpd: remove filter-pf-addresses support
On Thu, Oct 19, 2023 at 04:13:41PM +0200, Martijn van Duren wrote:
> OpenBSD 7.4 is here. upgrade72.html already mentions it's deprecation.
>
> OK?
ok
>
> martijn@
>
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
> retrieving revision 1.78
> diff -u -p -r1.78 parse.y
> --- parse.y 6 Oct 2022 14:41:08 - 1.78
> +++ parse.y 19 Oct 2023 14:12:55 -
> @@ -140,7 +140,7 @@ typedef struct {
> %token SYSTEM CONTACT DESCR LOCATION NAME OBJECTID SERVICES RTFILTER
> %token READONLY READWRITE OCTETSTRING INTEGER COMMUNITY TRAP RECEIVER
> %token SECLEVEL NONE AUTH ENC USER AUTHKEY ENCKEY ERROR
> -%token HANDLE DEFAULT SRCADDR TCP UDP PFADDRFILTER BLOCKLIST PORT
> +%token HANDLE DEFAULT SRCADDR TCP UDP BLOCKLIST PORT
> %token STRING
> %token NUMBER
> %type usmuser community optcommunity
> @@ -336,26 +336,6 @@ main : LISTEN ON listen_udptcp
> else
> conf->sc_rtfilter = 0;
> }
> - /* XXX Remove after 7.4 */
> - | PFADDRFILTER yesno{
> - struct ber_oid *blocklist;
> -
> - log_warnx("filter-pf-addresses is deprecated. "
> - "Please use blocklist pfTblAddrTable instead.");
> - if ($2) {
> - blocklist = recallocarray(conf->sc_blocklist,
> - conf->sc_nblocklist,
> - conf->sc_nblocklist + 1,
> - sizeof(*blocklist));
> - if (blocklist == NULL) {
> - yyerror("malloc");
> - YYERROR;
> - }
> - conf->sc_blocklist = blocklist;
> - smi_string2oid("pfTblAddrTable",
> - &(blocklist[conf->sc_nblocklist++]));
> - }
> - }
> | seclevel {
> conf->sc_min_seclevel = $1;
> }
> @@ -1195,7 +1175,6 @@ lookup(char *s)
> { "enc",ENC },
> { "enckey", ENCKEY },
> { "engineid", ENGINEID },
> - { "filter-pf-addresses",PFADDRFILTER },
> { "filter-routes", RTFILTER },
> { "group", GROUP },
> { "handle", HANDLE },
>
snmpd: remove filter-pf-addresses support
OpenBSD 7.4 is here. upgrade72.html already mentions it's deprecation.
OK?
martijn@
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/snmpd/parse.y,v
retrieving revision 1.78
diff -u -p -r1.78 parse.y
--- parse.y 6 Oct 2022 14:41:08 - 1.78
+++ parse.y 19 Oct 2023 14:12:55 -
@@ -140,7 +140,7 @@ typedef struct {
%token SYSTEM CONTACT DESCR LOCATION NAME OBJECTID SERVICES RTFILTER
%token READONLY READWRITE OCTETSTRING INTEGER COMMUNITY TRAP RECEIVER
%token SECLEVEL NONE AUTH ENC USER AUTHKEY ENCKEY ERROR
-%token HANDLE DEFAULT SRCADDR TCP UDP PFADDRFILTER BLOCKLIST PORT
+%token HANDLE DEFAULT SRCADDR TCP UDP BLOCKLIST PORT
%token STRING
%token NUMBER
%typeusmuser community optcommunity
@@ -336,26 +336,6 @@ main : LISTEN ON listen_udptcp
else
conf->sc_rtfilter = 0;
}
- /* XXX Remove after 7.4 */
- | PFADDRFILTER yesno{
- struct ber_oid *blocklist;
-
- log_warnx("filter-pf-addresses is deprecated. "
- "Please use blocklist pfTblAddrTable instead.");
- if ($2) {
- blocklist = recallocarray(conf->sc_blocklist,
- conf->sc_nblocklist,
- conf->sc_nblocklist + 1,
- sizeof(*blocklist));
- if (blocklist == NULL) {
- yyerror("malloc");
- YYERROR;
- }
- conf->sc_blocklist = blocklist;
- smi_string2oid("pfTblAddrTable",
- &(blocklist[conf->sc_nblocklist++]));
- }
- }
| seclevel {
conf->sc_min_seclevel = $1;
}
@@ -1195,7 +1175,6 @@ lookup(char *s)
{ "enc",ENC },
{ "enckey", ENCKEY },
{ "engineid", ENGINEID },
- { "filter-pf-addresses",PFADDRFILTER },
{ "filter-routes", RTFILTER },
{ "group", GROUP },
{ "handle", HANDLE },
Re: IPv4 on ix(4) slow/nothing - 7.4
On Wed, Oct 18, 2023 at 08:53:44PM +0200, Alexander Bluhm wrote:
> On Wed, Oct 18, 2023 at 08:19:29PM +0200, Mischa wrote:
> > It's indeed something like that: ix -> vlan (tagged) -> veb
>
> When vlan is added to veb, kernel should disable LRO on ix.
> All testing before release did not find this code path :-(
>
> Is it possible to add vlan to veb first, and then add or change the
> vlan parent to ix? If it works, that should also disable LRO.
>
> Jan said he will have a look tomorrow.
>
> trunk, carp, ... in veb or bridge might have the same issue.
First round of fixes for vlan(4), vxlan(4), nvgre(4) and bpe(4).
ok?
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.708
diff -u -p -r1.708 if.c
--- net/if.c16 Sep 2023 09:33:27 - 1.708
+++ net/if.c19 Oct 2023 13:03:33 -
@@ -3243,6 +3243,17 @@ ifsetlro(struct ifnet *ifp, int on)
struct ifreq ifrq;
int error = 0;
int s = splnet();
+ struct if_parent parent;
+
+ memset(, 0, sizeof(parent));
+ if ((*ifp->if_ioctl)(ifp, SIOCGIFPARENT, (caddr_t)) != -1) {
+ struct ifnet *ifp0 = if_unit(parent.ifp_parent);
+
+ if (ifp0 != NULL) {
+ ifsetlro(ifp0, on);
+ if_put(ifp0);
+ }
+ }
if (!ISSET(ifp->if_capabilities, IFCAP_LRO)) {
error = ENOTSUP;
Index: net/if_bpe.c
===
RCS file: /cvs/src/sys/net/if_bpe.c,v
retrieving revision 1.19
diff -u -p -r1.19 if_bpe.c
--- net/if_bpe.c8 Nov 2021 04:54:44 - 1.19
+++ net/if_bpe.c19 Oct 2023 13:20:18 -
@@ -631,6 +631,9 @@ bpe_set_parent(struct bpe_softc *sc, con
goto put;
}
+ if (ether_brport_isset(ifp))
+ ifsetlro(ifp0, 0);
+
/* commit */
sc->sc_key.k_if = ifp0->if_index;
etherbridge_flush(>sc_eb, IFBF_FLUSHALL);
Index: net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.174
diff -u -p -r1.174 if_gre.c
--- net/if_gre.c13 May 2023 13:35:17 - 1.174
+++ net/if_gre.c19 Oct 2023 13:24:56 -
@@ -3544,6 +3544,9 @@ nvgre_set_parent(struct nvgre_softc *sc,
return (EPROTONOSUPPORT);
}
+ if (ether_brport_isset(>sc_ac.ac_if))
+ ifsetlro(ifp0, 0);
+
/* commit */
sc->sc_ifp0 = ifp0->if_index;
if_put(ifp0);
Index: net/if_vlan.c
===
RCS file: /cvs/src/sys/net/if_vlan.c,v
retrieving revision 1.215
diff -u -p -r1.215 if_vlan.c
--- net/if_vlan.c 16 May 2023 14:32:54 - 1.215
+++ net/if_vlan.c 19 Oct 2023 11:08:23 -
@@ -937,6 +937,9 @@ vlan_set_parent(struct vlan_softc *sc, c
if (error != 0)
goto put;
+ if (ether_brport_isset(ifp))
+ ifsetlro(ifp0, 0);
+
/* commit */
sc->sc_ifidx0 = ifp0->if_index;
if (!ISSET(sc->sc_flags, IFVF_LLADDR))
Index: net/if_vxlan.c
===
RCS file: /cvs/src/sys/net/if_vxlan.c,v
retrieving revision 1.93
diff -u -p -r1.93 if_vxlan.c
--- net/if_vxlan.c 3 Aug 2023 09:49:08 - 1.93
+++ net/if_vxlan.c 19 Oct 2023 13:18:47 -
@@ -1582,6 +1582,9 @@ vxlan_set_parent(struct vxlan_softc *sc,
goto put;
}
+ if (ether_brport_isset(ifp))
+ ifsetlro(ifp0, 0);
+
/* commit */
sc->sc_if_index0 = ifp0->if_index;
etherbridge_flush(>sc_eb, IFBF_FLUSHALL);
Re: HAMMER2 filesystem for OpenBSD
Le Tue, Oct 17, 2023 at 10:14:25PM +0100, Chris Narkiewicz a écrit : > Hi, > > Tomohiro Kusumi is currently working on HAMMER2 implementation > for OpenBSD, FreeBSD and NetBSD. > > The repository is here: > https://github.com/kusumi/openbsd_hammer2 > > > He maintains repositories for NetBSD, FreeBSD and OpenBSD, which > suggests that the implementation is portable. He also > provides a patch for OpenBSD 7.3: > > https://github.com/kusumi/openbsd_hammer2/blob/master/patch/openbsd73.patch > > The patch looks very minimal to me, with no deeper changes to the > kernel. > > I haven't found any discussion about HAMMER2 in list archives, so I'd > like to bring it to devs attention, kindly asking for your opinion. https://marc.info/?l=openbsd-misc=169272174500676=2 > Does it look like it's worth bringing in? Does it require more work? > > I'd appreciate any opinions from more knowledgable crowd. > > Cheers, > Chris >
Re: bgpd convert rtr_proto.c to new ibuf API
On Thu, Oct 19, 2023 at 01:26:49PM +0200, Claudio Jeker wrote:
> On Thu, Oct 19, 2023 at 12:59:17PM +0200, Theo Buehler wrote:
> > On Thu, Oct 19, 2023 at 10:41:07AM +0200, Claudio Jeker wrote:
> > > More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
> > > the new functions.
> > >
> > > Two bits I'm unsure about:
> > > - I had to change some sizeof() to use native types (I especially dislike
> > > the sizeof(struct rtr_header).
> >
> > Yes, that's not very nice.
> >
> > > - ibuf_add_nXX() can fail if the value is too large. Which should be
> > > impossible but still maybe it would be better to check for errors.
> >
> > While it should be impossible, the length calculations are non-trivial,
> > so it seems wiser to check.
> >
> > It's a bit longer than what you have now, but maybe it's an option
> > to combine the length calculation with the errs += idiom.
> >
> > len += sizeof(rs->version);
> > len += sizeof(type);
> > len += sizeof(session_id);
> > len += sizeof(len);
> >
> > if ((buf = ibuf_open(len)) == NULL)
> > return NULL;
> >
> > errs += ibuf_add_n8(buf, rs->version);
> > errs += ibuf_add_n8(buf, type);
> > errs += ibuf_add_n16(buf, session_id);
> > errs += ibuf_add_n32(buf, len);
> >
> > if (errs) {
> > ibuf_free(ibuf);
> > return NULL;
> > }
> >
> > I'm ok with the diff as it is and you can ponder how you want to shave
> > this particular Yak.
>
> I like my yaks shaved like this...
I like this better than the errs dance, too.
ok tb
> The sizeof yak is still in queue... not sure about it.
It's not that horrible.
> --
> :wq Claudio
>
> Index: rtr_proto.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 rtr_proto.c
> --- rtr_proto.c 19 Oct 2023 11:12:10 - 1.18
> +++ rtr_proto.c 19 Oct 2023 11:24:56 -
> @@ -233,6 +233,7 @@ rtr_newmsg(struct rtr_session *rs, enum
> uint16_t session_id)
> {
> struct ibuf *buf;
> + int saved_errno;
>
> if (len > RTR_MAX_LEN) {
> errno = ERANGE;
> @@ -240,15 +241,23 @@ rtr_newmsg(struct rtr_session *rs, enum
> }
> len += sizeof(struct rtr_header);
> if ((buf = ibuf_open(len)) == NULL)
> - return NULL;
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n8(buf, rs->version);
> - ibuf_add_n8(buf, type);
> - ibuf_add_n16(buf, session_id);
> - ibuf_add_n32(buf, len);
> + goto fail;
> + if (ibuf_add_n8(buf, rs->version) == -1)
> + goto fail;
> + if (ibuf_add_n8(buf, type) == -1)
> + goto fail;
> + if (ibuf_add_n16(buf, session_id) == -1)
> + goto fail;
> + if (ibuf_add_n32(buf, len) == -1)
> + goto fail;
>
> return buf;
> +
> + fail:
> + saved_errno = errno;
> + ibuf_free(buf);
> + errno = saved_errno;
> + return NULL;
> }
>
> /*
> @@ -271,22 +280,27 @@ rtr_send_error(struct rtr_session *rs, e
>
> buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
> err);
> - if (buf == NULL) {
> - log_warn("rtr %s: send error report", log_rtr(rs));
> - return;
> - }
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n32(buf, len);
> - ibuf_add(buf, pdu, len);
> - ibuf_add_n32(buf, mlen);
> - ibuf_add(buf, msg, mlen);
> + if (buf == NULL)
> + goto fail;
> + if (ibuf_add_n32(buf, len) == -1)
> + goto fail;
> + if (ibuf_add(buf, pdu, len) == -1)
> + goto fail;
> + if (ibuf_add_n32(buf, mlen) == -1)
> + goto fail;
> + if (ibuf_add(buf, msg, mlen) == -1)
> + goto fail;
> ibuf_close(>w, buf);
>
> log_warnx("rtr %s: sending error report[%u] %s", log_rtr(rs), err,
> msg ? msg : "");
>
> rtr_fsm(rs, RTR_EVNT_SEND_ERROR);
> + return;
> +
> + fail:
> + log_warn("rtr %s: send error report", log_rtr(rs));
> + ibuf_free(buf);
> }
>
> static void
> @@ -309,15 +323,17 @@ rtr_send_serial_query(struct rtr_session
> struct ibuf *buf;
>
> buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
> - if (buf == NULL) {
> - log_warn("rtr %s: send serial query", log_rtr(rs));
> - rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> - return;
> - }
> -
> - /* cannot fail with fixed buffers */
> - ibuf_add_n32(buf, rs->serial);
> + if (buf == NULL)
> + goto fail;
> + if (ibuf_add_n32(buf, rs->serial) == -1)
> + goto fail;
> ibuf_close(>w, buf);
> + return;
> +
> + fail:
> + log_warn("rtr %s: send serial query", log_rtr(rs));
> + ibuf_free(buf);
> + rtr_send_error(rs, INTERNAL_ERROR, "out of memory",
Re: bgpd convert rtr_proto.c to new ibuf API
On Thu, Oct 19, 2023 at 12:59:17PM +0200, Theo Buehler wrote:
> On Thu, Oct 19, 2023 at 10:41:07AM +0200, Claudio Jeker wrote:
> > More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
> > the new functions.
> >
> > Two bits I'm unsure about:
> > - I had to change some sizeof() to use native types (I especially dislike
> > the sizeof(struct rtr_header).
>
> Yes, that's not very nice.
>
> > - ibuf_add_nXX() can fail if the value is too large. Which should be
> > impossible but still maybe it would be better to check for errors.
>
> While it should be impossible, the length calculations are non-trivial,
> so it seems wiser to check.
>
> It's a bit longer than what you have now, but maybe it's an option
> to combine the length calculation with the errs += idiom.
>
> len += sizeof(rs->version);
> len += sizeof(type);
> len += sizeof(session_id);
> len += sizeof(len);
>
> if ((buf = ibuf_open(len)) == NULL)
> return NULL;
>
> errs += ibuf_add_n8(buf, rs->version);
> errs += ibuf_add_n8(buf, type);
> errs += ibuf_add_n16(buf, session_id);
> errs += ibuf_add_n32(buf, len);
>
> if (errs) {
> ibuf_free(ibuf);
> return NULL;
> }
>
> I'm ok with the diff as it is and you can ponder how you want to shave
> this particular Yak.
I like my yaks shaved like this...
The sizeof yak is still in queue... not sure about it.
--
:wq Claudio
Index: rtr_proto.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
retrieving revision 1.18
diff -u -p -r1.18 rtr_proto.c
--- rtr_proto.c 19 Oct 2023 11:12:10 - 1.18
+++ rtr_proto.c 19 Oct 2023 11:24:56 -
@@ -233,6 +233,7 @@ rtr_newmsg(struct rtr_session *rs, enum
uint16_t session_id)
{
struct ibuf *buf;
+ int saved_errno;
if (len > RTR_MAX_LEN) {
errno = ERANGE;
@@ -240,15 +241,23 @@ rtr_newmsg(struct rtr_session *rs, enum
}
len += sizeof(struct rtr_header);
if ((buf = ibuf_open(len)) == NULL)
- return NULL;
-
- /* cannot fail with fixed buffers */
- ibuf_add_n8(buf, rs->version);
- ibuf_add_n8(buf, type);
- ibuf_add_n16(buf, session_id);
- ibuf_add_n32(buf, len);
+ goto fail;
+ if (ibuf_add_n8(buf, rs->version) == -1)
+ goto fail;
+ if (ibuf_add_n8(buf, type) == -1)
+ goto fail;
+ if (ibuf_add_n16(buf, session_id) == -1)
+ goto fail;
+ if (ibuf_add_n32(buf, len) == -1)
+ goto fail;
return buf;
+
+ fail:
+ saved_errno = errno;
+ ibuf_free(buf);
+ errno = saved_errno;
+ return NULL;
}
/*
@@ -271,22 +280,27 @@ rtr_send_error(struct rtr_session *rs, e
buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
err);
- if (buf == NULL) {
- log_warn("rtr %s: send error report", log_rtr(rs));
- return;
- }
-
- /* cannot fail with fixed buffers */
- ibuf_add_n32(buf, len);
- ibuf_add(buf, pdu, len);
- ibuf_add_n32(buf, mlen);
- ibuf_add(buf, msg, mlen);
+ if (buf == NULL)
+ goto fail;
+ if (ibuf_add_n32(buf, len) == -1)
+ goto fail;
+ if (ibuf_add(buf, pdu, len) == -1)
+ goto fail;
+ if (ibuf_add_n32(buf, mlen) == -1)
+ goto fail;
+ if (ibuf_add(buf, msg, mlen) == -1)
+ goto fail;
ibuf_close(>w, buf);
log_warnx("rtr %s: sending error report[%u] %s", log_rtr(rs), err,
msg ? msg : "");
rtr_fsm(rs, RTR_EVNT_SEND_ERROR);
+ return;
+
+ fail:
+ log_warn("rtr %s: send error report", log_rtr(rs));
+ ibuf_free(buf);
}
static void
@@ -309,15 +323,17 @@ rtr_send_serial_query(struct rtr_session
struct ibuf *buf;
buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
- if (buf == NULL) {
- log_warn("rtr %s: send serial query", log_rtr(rs));
- rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
- return;
- }
-
- /* cannot fail with fixed buffers */
- ibuf_add_n32(buf, rs->serial);
+ if (buf == NULL)
+ goto fail;
+ if (ibuf_add_n32(buf, rs->serial) == -1)
+ goto fail;
ibuf_close(>w, buf);
+ return;
+
+ fail:
+ log_warn("rtr %s: send serial query", log_rtr(rs));
+ ibuf_free(buf);
+ rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
}
/*
Re: bgpd convert rtr_proto.c to new ibuf API
On Thu, Oct 19, 2023 at 10:41:07AM +0200, Claudio Jeker wrote:
> More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
> the new functions.
>
> Two bits I'm unsure about:
> - I had to change some sizeof() to use native types (I especially dislike
> the sizeof(struct rtr_header).
Yes, that's not very nice.
> - ibuf_add_nXX() can fail if the value is too large. Which should be
> impossible but still maybe it would be better to check for errors.
While it should be impossible, the length calculations are non-trivial,
so it seems wiser to check.
It's a bit longer than what you have now, but maybe it's an option
to combine the length calculation with the errs += idiom.
len += sizeof(rs->version);
len += sizeof(type);
len += sizeof(session_id);
len += sizeof(len);
if ((buf = ibuf_open(len)) == NULL)
return NULL;
errs += ibuf_add_n8(buf, rs->version);
errs += ibuf_add_n8(buf, type);
errs += ibuf_add_n16(buf, session_id);
errs += ibuf_add_n32(buf, len);
if (errs) {
ibuf_free(ibuf);
return NULL;
}
I'm ok with the diff as it is and you can ponder how you want to shave
this particular Yak.
>
> --
> :wq Claudio
>
> Index: rtr_proto.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 rtr_proto.c
> --- rtr_proto.c 16 Aug 2023 08:26:35 - 1.17
> +++ rtr_proto.c 19 Oct 2023 08:35:49 -
> @@ -233,24 +233,21 @@ rtr_newmsg(struct rtr_session *rs, enum
> uint16_t session_id)
> {
> struct ibuf *buf;
> - struct rtr_header rh;
>
> if (len > RTR_MAX_LEN) {
> errno = ERANGE;
> return NULL;
> }
> - len += sizeof(rh);
> + len += sizeof(struct rtr_header);
> if ((buf = ibuf_open(len)) == NULL)
> return NULL;
>
> - memset(, 0, sizeof(rh));
> - rh.version = rs->version;
> - rh.type = type;
> - rh.session_id = htons(session_id);
> - rh.length = htonl(len);
> -
> /* cannot fail with fixed buffers */
> - ibuf_add(buf, , sizeof(rh));
> + ibuf_add_n8(buf, rs->version);
> + ibuf_add_n8(buf, type);
> + ibuf_add_n16(buf, session_id);
> + ibuf_add_n32(buf, len);
> +
> return buf;
> }
>
> @@ -264,7 +261,6 @@ rtr_send_error(struct rtr_session *rs, e
> {
> struct ibuf *buf;
> size_t mlen = 0;
> - uint32_t hdrlen;
>
> rs->last_sent_error = err;
> if (msg) {
> @@ -273,7 +269,7 @@ rtr_send_error(struct rtr_session *rs, e
> } else
> memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
>
> - buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(hdrlen) + len + mlen,
> + buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
> err);
> if (buf == NULL) {
> log_warn("rtr %s: send error report", log_rtr(rs));
> @@ -281,11 +277,9 @@ rtr_send_error(struct rtr_session *rs, e
> }
>
> /* cannot fail with fixed buffers */
> - hdrlen = ntohl(len);
> - ibuf_add(buf, , sizeof(hdrlen));
> + ibuf_add_n32(buf, len);
> ibuf_add(buf, pdu, len);
> - hdrlen = ntohl(mlen);
> - ibuf_add(buf, , sizeof(hdrlen));
> + ibuf_add_n32(buf, mlen);
> ibuf_add(buf, msg, mlen);
> ibuf_close(>w, buf);
>
> @@ -313,9 +307,8 @@ static void
> rtr_send_serial_query(struct rtr_session *rs)
> {
> struct ibuf *buf;
> - uint32_t s;
>
> - buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(s), rs->session_id);
> + buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
> if (buf == NULL) {
> log_warn("rtr %s: send serial query", log_rtr(rs));
> rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
> @@ -323,8 +316,7 @@ rtr_send_serial_query(struct rtr_session
> }
>
> /* cannot fail with fixed buffers */
> - s = htonl(rs->serial);
> - ibuf_add(buf, , sizeof(s));
> + ibuf_add_n32(buf, rs->serial);
> ibuf_close(>w, buf);
> }
>
>
bgpd convert rtr_proto.c to new ibuf API
More ibuf cleanup. rtr_proto.c still uses ibuf_add() where it could use
the new functions.
Two bits I'm unsure about:
- I had to change some sizeof() to use native types (I especially dislike
the sizeof(struct rtr_header).
- ibuf_add_nXX() can fail if the value is too large. Which should be
impossible but still maybe it would be better to check for errors.
--
:wq Claudio
Index: rtr_proto.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rtr_proto.c,v
retrieving revision 1.17
diff -u -p -r1.17 rtr_proto.c
--- rtr_proto.c 16 Aug 2023 08:26:35 - 1.17
+++ rtr_proto.c 19 Oct 2023 08:35:49 -
@@ -233,24 +233,21 @@ rtr_newmsg(struct rtr_session *rs, enum
uint16_t session_id)
{
struct ibuf *buf;
- struct rtr_header rh;
if (len > RTR_MAX_LEN) {
errno = ERANGE;
return NULL;
}
- len += sizeof(rh);
+ len += sizeof(struct rtr_header);
if ((buf = ibuf_open(len)) == NULL)
return NULL;
- memset(, 0, sizeof(rh));
- rh.version = rs->version;
- rh.type = type;
- rh.session_id = htons(session_id);
- rh.length = htonl(len);
-
/* cannot fail with fixed buffers */
- ibuf_add(buf, , sizeof(rh));
+ ibuf_add_n8(buf, rs->version);
+ ibuf_add_n8(buf, type);
+ ibuf_add_n16(buf, session_id);
+ ibuf_add_n32(buf, len);
+
return buf;
}
@@ -264,7 +261,6 @@ rtr_send_error(struct rtr_session *rs, e
{
struct ibuf *buf;
size_t mlen = 0;
- uint32_t hdrlen;
rs->last_sent_error = err;
if (msg) {
@@ -273,7 +269,7 @@ rtr_send_error(struct rtr_session *rs, e
} else
memset(rs->last_sent_msg, 0, sizeof(rs->last_sent_msg));
- buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(hdrlen) + len + mlen,
+ buf = rtr_newmsg(rs, ERROR_REPORT, 2 * sizeof(uint32_t) + len + mlen,
err);
if (buf == NULL) {
log_warn("rtr %s: send error report", log_rtr(rs));
@@ -281,11 +277,9 @@ rtr_send_error(struct rtr_session *rs, e
}
/* cannot fail with fixed buffers */
- hdrlen = ntohl(len);
- ibuf_add(buf, , sizeof(hdrlen));
+ ibuf_add_n32(buf, len);
ibuf_add(buf, pdu, len);
- hdrlen = ntohl(mlen);
- ibuf_add(buf, , sizeof(hdrlen));
+ ibuf_add_n32(buf, mlen);
ibuf_add(buf, msg, mlen);
ibuf_close(>w, buf);
@@ -313,9 +307,8 @@ static void
rtr_send_serial_query(struct rtr_session *rs)
{
struct ibuf *buf;
- uint32_t s;
- buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(s), rs->session_id);
+ buf = rtr_newmsg(rs, SERIAL_QUERY, sizeof(uint32_t), rs->session_id);
if (buf == NULL) {
log_warn("rtr %s: send serial query", log_rtr(rs));
rtr_send_error(rs, INTERNAL_ERROR, "out of memory", NULL, 0);
@@ -323,8 +316,7 @@ rtr_send_serial_query(struct rtr_session
}
/* cannot fail with fixed buffers */
- s = htonl(rs->serial);
- ibuf_add(buf, , sizeof(s));
+ ibuf_add_n32(buf, rs->serial);
ibuf_close(>w, buf);
}
Re: log.c use buffered IO
On Wed, Oct 18, 2023 at 11:34:09AM +0200, Claudio Jeker wrote:
> On Tue, Oct 17, 2023 at 10:06:54AM +0200, Sebastian Benoit wrote:
> > Theo Buehler(t...@theobuehler.org) on 2023.10.17 09:13:15 +0200:
> > > On Mon, Oct 16, 2023 at 12:19:17PM +0200, Claudio Jeker wrote:
> > > > I dislike how log.c does all these asprintf() calls with dubious
> > > > workaround calls in case asprintf() fails.
> > >
> > > You're not alone.
> > >
> > > > IMO it is easier to use the stdio provided buffers and fflush() to get
> > > > "atomic" writes on stderr. At least from my understanding this is the
> > > > reason to go all this lenght to do a single fprintf call.
> > >
> > > This makes sense, but I don't know the history here.
> >
> > as far as i can remember, it was done so it would still be able to work
> > somewhat when out of memeory.
> >
>
> After some input off-list here another idea.
> Require that logit() is called with a \n and by that simplify a lot of
> code around it. vsyslog() handles both so having the \n should not cause
> any breakage.
Really hard to say which is the lesser evil: requiring \n or the ugly
asprintf dances. I don't think it is flat out objectionable, but I can
see people disliking this. The churn you outline below seems indeed
rather limited, so I guess we can go this way.
Definitely ok with using a stack buffer rather than a static one.
>
> Now logit() is mostly used internally but in bgpd it is also used in
> logmsg.c and parse.y. Fixing those is simple.
>
> Also this uses a stack buffer for all the log_* cases now. This should
> make the code more thread safe.
>
> Also this removes vlog() from the API. I had a quick look at all the
> other log.c users and apart from ldapd this can be added to all of them
> without much issues. Nothing else uses vlog() and the logit() is also very
> minimal (mostly the same parse.y change as below).
>
> --
> :wq Claudio
>
> Index: log.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/log.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 log.c
> --- log.c 21 Mar 2017 12:06:55 - 1.64
> +++ log.c 18 Oct 2023 07:10:32 -
> @@ -26,6 +26,8 @@
>
> #include "log.h"
>
> +#define MAX_LOGLEN 4096
> +
> static intdebug;
> static intverbose;
> static const char*log_procname;
> @@ -68,30 +70,15 @@ void
> logit(int pri, const char *fmt, ...)
> {
> va_list ap;
> + int saved_errno = errno;
>
> va_start(ap, fmt);
> - vlog(pri, fmt, ap);
> - va_end(ap);
> -}
> -
> -void
> -vlog(int pri, const char *fmt, va_list ap)
> -{
> - char*nfmt;
> - int saved_errno = errno;
> -
> if (debug) {
> - /* best effort in out of mem situations */
> - if (asprintf(, "%s\n", fmt) == -1) {
> - vfprintf(stderr, fmt, ap);
> - fprintf(stderr, "\n");
> - } else {
> - vfprintf(stderr, nfmt, ap);
> - free(nfmt);
> - }
> + vfprintf(stderr, fmt, ap);
> fflush(stderr);
> } else
> vsyslog(pri, fmt, ap);
> + va_end(ap);
>
> errno = saved_errno;
> }
> @@ -99,26 +86,18 @@ vlog(int pri, const char *fmt, va_list a
> void
> log_warn(const char *emsg, ...)
> {
> - char*nfmt;
> - va_list ap;
> - int saved_errno = errno;
> + charfmtbuf[MAX_LOGLEN];
> + va_list ap;
> + int saved_errno = errno;
>
> /* best effort to even work in out of memory situations */
> if (emsg == NULL)
> - logit(LOG_ERR, "%s", strerror(saved_errno));
> + logit(LOG_ERR, "%s\n", strerror(saved_errno));
> else {
> va_start(ap, emsg);
> -
> - if (asprintf(, "%s: %s", emsg,
> - strerror(saved_errno)) == -1) {
> - /* we tried it... */
> - vlog(LOG_ERR, emsg, ap);
> - logit(LOG_ERR, "%s", strerror(saved_errno));
> - } else {
> - vlog(LOG_ERR, nfmt, ap);
> - free(nfmt);
> - }
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> va_end(ap);
> + logit(LOG_ERR, "%s: %s\n", fmtbuf, strerror(saved_errno));
> }
>
> errno = saved_errno;
> @@ -127,53 +106,65 @@ log_warn(const char *emsg, ...)
> void
> log_warnx(const char *emsg, ...)
> {
> - va_list ap;
> + charfmtbuf[MAX_LOGLEN];
> + va_list ap;
> + int saved_errno = errno;
>
> va_start(ap, emsg);
> - vlog(LOG_ERR, emsg, ap);
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> va_end(ap);
> + logit(LOG_ERR, "%s\n", fmtbuf);
> +
> + errno = saved_errno;
> }
>
> void
> log_info(const char *emsg, ...)
> {
> - va_list ap;
> + char
