Weird loop in ftp client
Hi, a thing I spotted some weeks ago - - - usr.bin/ftp/ftp.c l.1090 - - - d = 0; do { wr = write(fileno(fout), buf + d, rd); if (wr == -1 errno == EPIPE) break; d += wr; rd -= wr; } while (d c); If write() fails without EPIPE, d is decremented, and the function keeps looping. If write() succeeds after several loops, d will be negative, and the function will write from buf-XX. Since d is later used to display a proper error message, I'd suggest this patch: Index: ftp.c === RCS file: /cvs/src/usr.bin/ftp/ftp.c,v retrieving revision 1.83 diff -u -r1.83 ftp.c --- ftp.c 13 Nov 2013 20:41:14 - 1.83 +++ ftp.c 22 Nov 2013 06:23:32 - @@ -1094,7 +1094,7 @@ break; d += wr; rd -= wr; - } while (d c); + } while (d c d 0); if (rd != 0) break; bytes += c; Any opinion?
Re: Don't link multicast records to the first address
On 18/11/13(Mon) 11:43, Martin Pieuchot wrote: Diff below changes the way protocol multicast addresses are linked to an interface. Right now they are added to a list attached to the first protocol address of an interface. That makes this address descriptor and its position in the global list special. Plus in the IPv6 case, a special kludge is used to move multicast records from one address to another. So this diff reuse the design of the protocol agnostic struct ifaddr and adds a new list of multicast addresses, struct ifmaddr, to the interface descriptor. It solves the problems described previously and as a bonus properly free the IPv4 multicast records of an interface when it is detached, thus plugging some more leaks. I tested it with a carp setup, I appreciate any multicast related tests. Here's an updated diff after recent header changes. Did anybody tested it? Does it break one of your use cases? Comments? Index: net/if.c === RCS file: /cvs/src/sys/net/if.c,v retrieving revision 1.277 diff -u -p -r1.277 if.c --- net/if.c19 Nov 2013 09:00:43 - 1.277 +++ net/if.c22 Nov 2013 08:17:11 - @@ -437,8 +437,9 @@ if_attach(struct ifnet *ifp) void if_attach_common(struct ifnet *ifp) { - TAILQ_INIT(ifp-if_addrlist); + TAILQ_INIT(ifp-if_maddrlist); + ifp-if_addrhooks = malloc(sizeof(*ifp-if_addrhooks), M_TEMP, M_WAITOK); TAILQ_INIT(ifp-if_addrhooks); Index: net/if_var.h === RCS file: /cvs/src/sys/net/if_var.h,v retrieving revision 1.1 diff -u -p -r1.1 if_var.h --- net/if_var.h21 Nov 2013 17:32:12 - 1.1 +++ net/if_var.h22 Nov 2013 08:17:11 - @@ -123,6 +123,7 @@ struct ifnet { /* and the entries */ TAILQ_ENTRY(ifnet) if_list; /* all struct ifnets are chained */ TAILQ_ENTRY(ifnet) if_txlist; /* list of ifnets ready to tx */ TAILQ_HEAD(, ifaddr) if_addrlist; /* linked list of addresses per if */ + TAILQ_HEAD(, ifmaddr) if_maddrlist; /* list of multicast records */ TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ struct hook_desc_head *if_addrhooks; /* address change callbacks */ struct hook_desc_head *if_linkstatehooks; /* link change callbacks */ @@ -302,6 +303,16 @@ struct ifaddr_item { struct ifaddr *ifai_ifa; struct ifaddr_item *ifai_next; u_intifai_rdomain; +}; + +/* + * Interface multicast address. + */ +struct ifmaddr { + struct sockaddr *ifma_addr; /* Protocol address */ + struct ifnet*ifma_ifp; /* Back pointer to ifnet */ + unsigned int ifma_refcnt; /* Count of references */ + TAILQ_ENTRY(ifmaddr) ifma_list; /* Per-interface list */ }; /* Index: netinet/igmp.c === RCS file: /cvs/src/sys/netinet/igmp.c,v retrieving revision 1.35 diff -u -p -r1.35 igmp.c --- netinet/igmp.c 18 Oct 2013 09:04:02 - 1.35 +++ netinet/igmp.c 22 Nov 2013 08:17:12 - @@ -83,6 +83,7 @@ #include sys/sysctl.h #include net/if.h +#include net/if_var.h #include net/route.h #include netinet/in.h @@ -193,6 +194,7 @@ igmp_input(struct mbuf *m, ...) struct igmp *igmp; int igmplen; int minlen; + struct ifmaddr *ifma; struct in_multi *inm; struct router_info *rti; struct in_ifaddr *ia; @@ -266,7 +268,10 @@ igmp_input(struct mbuf *m, ...) * except those that are already running and those * that belong to a local group (224.0.0.X). */ - IN_FOREACH_MULTI(ia, ifp, inm) { + TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) { + if (ifma-ifma_addr-sa_family != AF_INET) + continue; + inm = ifmatoinm(ifma); if (inm-inm_timer == 0 !IN_LOCAL_GROUP(inm-inm_addr.s_addr)) { inm-inm_state = IGMP_DELAYING_MEMBER; @@ -294,7 +299,10 @@ igmp_input(struct mbuf *m, ...) * timers already running, check if they need to be * reset. */ - IN_FOREACH_MULTI(ia, ifp, inm) { + TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) { + if (ifma-ifma_addr-sa_family != AF_INET) + continue; + inm = ifmatoinm(ifma); if (!IN_LOCAL_GROUP(inm-inm_addr.s_addr)
Re: Don't link multicast records to the first address
I tried the old version (I'm using igmpproxy on my firewall and have native multicast over pppoe), no problems, but I haven't carefully read the diff yet.
Kill IF_LEN() and IF_IS_EMPTY()
One kill a day, keeps... So, here's a diff that replaces these two macros by their IFQ_* equivalent and kill them. No object change. ok? Index: dev/ic/ar5008.c === RCS file: /home/ncvs/src/sys/dev/ic/ar5008.c,v retrieving revision 1.23 diff -u -p -r1.23 ar5008.c --- dev/ic/ar5008.c 7 Aug 2013 01:06:28 - 1.23 +++ dev/ic/ar5008.c 22 Nov 2013 08:50:14 - @@ -1062,7 +1062,7 @@ ar5008_swba_intr(struct athn_softc *sc) int error, totlen; if (ic-ic_tim_mcast_pending - IF_IS_EMPTY(ni-ni_savedq) + IFQ_IS_EMPTY(ni-ni_savedq) SIMPLEQ_EMPTY(sc-txq[ATHN_QID_CAB].head)) ic-ic_tim_mcast_pending = 0; @@ -1146,7 +1146,7 @@ ar5008_swba_intr(struct athn_softc *sc) IF_DEQUEUE(ni-ni_savedq, m); if (m == NULL) break; - if (!IF_IS_EMPTY(ni-ni_savedq)) { + if (!IFQ_IS_EMPTY(ni-ni_savedq)) { /* more queued frames, set the more data bit */ wh = mtod(m, struct ieee80211_frame *); wh-i_fc[1] |= IEEE80211_FC1_MORE_DATA; Index: dev/ic/ar9003.c === RCS file: /home/ncvs/src/sys/dev/ic/ar9003.c,v retrieving revision 1.27 diff -u -p -r1.27 ar9003.c --- dev/ic/ar9003.c 7 Aug 2013 01:06:28 - 1.27 +++ dev/ic/ar9003.c 22 Nov 2013 08:50:14 - @@ -1194,7 +1194,7 @@ ar9003_swba_intr(struct athn_softc *sc) int error, totlen; if (ic-ic_tim_mcast_pending - IF_IS_EMPTY(ni-ni_savedq) + IFQ_IS_EMPTY(ni-ni_savedq) SIMPLEQ_EMPTY(sc-txq[ATHN_QID_CAB].head)) ic-ic_tim_mcast_pending = 0; @@ -1290,7 +1290,7 @@ ar9003_swba_intr(struct athn_softc *sc) IF_DEQUEUE(ni-ni_savedq, m); if (m == NULL) break; - if (!IF_IS_EMPTY(ni-ni_savedq)) { + if (!IFQ_IS_EMPTY(ni-ni_savedq)) { /* more queued frames, set the more data bit */ wh = mtod(m, struct ieee80211_frame *); wh-i_fc[1] |= IEEE80211_FC1_MORE_DATA; Index: dev/ic/rtw.c === RCS file: /home/ncvs/src/sys/dev/ic/rtw.c,v retrieving revision 1.82 diff -u -p -r1.82 rtw.c --- dev/ic/rtw.c5 Dec 2012 23:20:19 - 1.82 +++ dev/ic/rtw.c22 Nov 2013 08:50:14 - @@ -2700,7 +2700,7 @@ rtw_80211_dequeue(struct rtw_softc *sc, { struct mbuf *m; - if (IF_IS_EMPTY(ifq)) + if (IFQ_IS_EMPTY(ifq)) return NULL; if (rtw_txring_choose(sc, tsbp, tdbp, pri) == -1) { DPRINTF(sc, RTW_DEBUG_XMIT_RSRC, (%s: no ring %d descriptor\n, Index: dev/pci/if_de.c === RCS file: /home/ncvs/src/sys/dev/pci/if_de.c,v retrieving revision 1.111 diff -u -p -r1.111 if_de.c --- dev/pci/if_de.c 7 Aug 2013 01:06:34 - 1.111 +++ dev/pci/if_de.c 22 Nov 2013 08:50:14 - @@ -3204,7 +3204,7 @@ tulip_rx_intr(tulip_softc_t * const sc) bus_dmamap_t map; int error; - if (fillok IF_LEN(sc-tulip_rxq) TULIP_RXQ_TARGET) + if (fillok IFQ_LEN(sc-tulip_rxq) TULIP_RXQ_TARGET) goto queue_mbuf; #if defined(TULIP_DEBUG) @@ -3225,7 +3225,7 @@ tulip_rx_intr(tulip_softc_t * const sc) TULIP_RXDESC_POSTSYNC(sc, eop, sizeof(*eop)); if volatile tulip_desc_t *) eop)-d_status (TULIP_DSTS_OWNER|TULIP_DSTS_RxFIRSTDESC|TULIP_DSTS_RxLASTDESC)) == (TULIP_DSTS_RxFIRSTDESC|TULIP_DSTS_RxLASTDESC)) { #ifdef DIAGNOSTIC - if (IF_IS_EMPTY(sc-tulip_rxq)) + if (IFQ_IS_EMPTY(sc-tulip_rxq)) panic(%s: tulip_rxq empty, sc-tulip_if.if_xname); #endif IF_DEQUEUE(sc-tulip_rxq, ms); @@ -3461,7 +3461,7 @@ tulip_rx_intr(tulip_softc_t * const sc) IF_ENQUEUE(sc-tulip_rxq, ms); } while ((ms = me) != NULL); - if (IF_LEN(sc-tulip_rxq) = TULIP_RXQ_TARGET) + if (IFQ_LEN(sc-tulip_rxq) = TULIP_RXQ_TARGET) sc-tulip_flags = ~TULIP_RXBUFSLOW; TULIP_PERFEND(rxget); } @@ -3894,7 +3894,7 @@ tulip_txput(tulip_softc_t * const sc, st struct mbuf *tmp; if (!notonqueue) { #ifdef DIAGNOSTIC - if (IF_IS_EMPTY(ifp-if_snd)) + if (IFQ_IS_EMPTY(ifp-if_snd)) panic(%s: if_snd queue empty, ifp-if_xname); #endif IFQ_DEQUEUE(ifp-if_snd, tmp); @@ -3978,7 +3978,7 @@ tulip_txput(tulip_softc_t * const sc, st /* remove the mbuf from the queue */ struct mbuf *tmp; #ifdef DIAGNOSTIC - if (IF_IS_EMPTY(ifp-if_snd)) + if (IFQ_IS_EMPTY(ifp-if_snd)) panic(%s: if_snd queue empty, ifp-if_xname);
Re: Weird loop in ftp client
On 2013/11/22 07:25, Maxime Villard wrote: Hi, a thing I spotted some weeks ago - - - usr.bin/ftp/ftp.c l.1090 - - - d = 0; do { wr = write(fileno(fout), buf + d, rd); if (wr == -1 errno == EPIPE) break; d += wr; rd -= wr; } while (d c); If write() fails without EPIPE, d is decremented, and the function keeps looping. If write() succeeds after several loops, d will be negative, and the function will write from buf-XX. Since d is later used to display a proper error message, I'd suggest this patch: Index: ftp.c === RCS file: /cvs/src/usr.bin/ftp/ftp.c,v retrieving revision 1.83 diff -u -r1.83 ftp.c --- ftp.c 13 Nov 2013 20:41:14 - 1.83 +++ ftp.c 22 Nov 2013 06:23:32 - @@ -1094,7 +1094,7 @@ break; d += wr; rd -= wr; - } while (d c); + } while (d c d 0); if (rd != 0) break; bytes += c; Any opinion? Shouldn't it be something more like this? Otherwise if the write() fails, we attempt writing one byte fewer for every retry. Index: ftp.c === RCS file: /cvs/src/usr.bin/ftp/ftp.c,v retrieving revision 1.83 diff -u -p -r1.83 ftp.c --- ftp.c 13 Nov 2013 20:41:14 - 1.83 +++ ftp.c 22 Nov 2013 10:08:16 - @@ -1090,10 +1090,13 @@ recvrequest(const char *cmd, const char d = 0; do { wr = write(fileno(fout), buf + d, rd); - if (wr == -1 errno == EPIPE) - break; - d += wr; - rd -= wr; + if (wr == -1) { + if (errno == EPIPE) + break; + } else { + d += wr; + rd -= wr; + } } while (d c); if (rd != 0) break;
Re: Weird loop in ftp client
On Fri, 22 Nov 2013, Stuart Henderson wrote: do { wr = write(fileno(fout), buf + d, rd); - if (wr == -1 errno == EPIPE) - break; - d += wr; - rd -= wr; + if (wr == -1) { + if (errno == EPIPE) + break; + } else { + d += wr; + rd -= wr; + } } while (d c); That still loops endlessly for errors other than EPIPE.
uhub2: device problem, disabling port 2
uhub2: port 2, set config at addr 3 failed uhub2: device problem, disabling port 2 When this happens, is there any way to get the port back without rebooting?
Re: Weird loop in ftp client
On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote: On 2013/11/22 07:25, Maxime Villard wrote: If write() fails without EPIPE, d is decremented, and the function keeps looping. If write() succeeds after several loops, d will be negative, and the function will write from buf-XX. When does write() fail and do we want to keep writing? (If I fill up the filesystem, do I really want to spin here?) Shouldn't it be something more like this? Otherwise if the write() fails, we attempt writing one byte fewer for every retry. That looks better to me.
Re: remove disksort()
An emphatic ok from me for this one. On Wed, Nov 20, 2013 at 3:21 AM, David Gwynne da...@gwynne.id.au wrote: the subject says it all really. this is sort of inspired by 5d2ecd5224 in bitrig except this brings all the architectures and device drivers forward (i didnt get to delete any to prepare for this), and maintains an algorithm for trying to order io on spinning rust (nscan). ok? Index: kern/kern_bufq.c === RCS file: /cvs/src/sys/kern/kern_bufq.c,v retrieving revision 1.25 diff -u -p -r1.25 kern_bufq.c --- kern/kern_bufq.c10 Apr 2013 01:35:55 - 1.25 +++ kern/kern_bufq.c20 Nov 2013 10:02:47 - @@ -42,13 +42,6 @@ struct bufq_impl { int (*impl_peek)(void *); }; -void *bufq_disksort_create(void); -voidbufq_disksort_destroy(void *); -voidbufq_disksort_queue(void *, struct buf *); -struct buf *bufq_disksort_dequeue(void *); -voidbufq_disksort_requeue(void *, struct buf *); -int bufq_disksort_peek(void *); - void *bufq_fifo_create(void); voidbufq_fifo_destroy(void *); voidbufq_fifo_queue(void *, struct buf *); @@ -65,14 +58,6 @@ int bufq_nscan_peek(void *); const struct bufq_impl bufq_impls[BUFQ_HOWMANY] = { { - bufq_disksort_create, - bufq_disksort_destroy, - bufq_disksort_queue, - bufq_disksort_dequeue, - bufq_disksort_requeue, - bufq_disksort_peek - }, - { bufq_fifo_create, bufq_fifo_destroy, bufq_fifo_queue, @@ -323,61 +308,6 @@ bufq_restart(void) mtx_leave(bufqs_mtx); } -/* - * disksort implementation. - */ - -void * -bufq_disksort_create(void) -{ - return (malloc(sizeof(struct buf), M_DEVBUF, M_NOWAIT | M_ZERO)); -} - -void -bufq_disksort_destroy(void *data) -{ - free(data, M_DEVBUF); -} - -void -bufq_disksort_queue(void *data, struct buf *bp) -{ - disksort((struct buf *)data, bp); -} - -struct buf * -bufq_disksort_dequeue(void *data) -{ - struct buf *bufq = data; - struct buf *bp; - - bp = bufq-b_actf; - if (bp != NULL) - bufq-b_actf = bp-b_actf; - if (bufq-b_actf == NULL) - bufq-b_actb = bufq-b_actf; - - return (bp); -} - -void -bufq_disksort_requeue(void *data, struct buf *bp) -{ - struct buf *bufq = data; - - bp-b_actf = bufq-b_actf; - bufq-b_actf = bp; - if (bp-b_actf == NULL) - bufq-b_actb = bp-b_actf; -} - -int -bufq_disksort_peek(void *data) -{ - struct buf *bufq = data; - - return (bufq-b_actf != NULL); -} /* * fifo implementation @@ -444,9 +374,7 @@ bufq_fifo_peek(void *data) * nscan implementation */ -#define BUF_INORDER(ba, bb) \ -(((ba)-b_cylinder (bb)-b_cylinder) || \ -((ba)-b_cylinder == (bb)-b_cylinder (ba)-b_blkno (bb)-b_blkno)) +#define BUF_INORDER(ba, bb) ((ba)-b_blkno (bb)-b_blkno) #define dsentries b_bufq.bufq_data_nscan.bqf_entries Index: kern/subr_disk.c === RCS file: /cvs/src/sys/kern/subr_disk.c,v retrieving revision 1.158 diff -u -p -r1.158 subr_disk.c --- kern/subr_disk.c18 Nov 2013 17:45:01 - 1.158 +++ kern/subr_disk.c20 Nov 2013 10:02:47 - @@ -91,99 +91,6 @@ void sr_map_root(void); void disk_attach_callback(void *, void *); /* - * Seek sort for disks. We depend on the driver which calls us using b_resid - * as the current cylinder number. - * - * The argument ap structure holds a b_actf activity chain pointer on which we - * keep two queues, sorted in ascending cylinder order. The first queue holds - * those requests which are positioned after the current cylinder (in the first - * request); the second holds requests which came in after their cylinder number - * was passed. Thus we implement a one way scan, retracting after reaching the - * end of the drive to the first request on the second queue, at which time it - * becomes the first queue. - * - * A one-way scan is natural because of the way UNIX read-ahead blocks are - * allocated. - */ - -void -disksort(struct buf *ap, struct buf *bp) -{ - struct buf *bq; - - /* If the queue is empty, then it's easy. */ - if (ap-b_actf == NULL) { - bp-b_actf = NULL; - ap-b_actf = bp; - return; - } - - /* -* If we lie after the first (currently active) request, then we -* must locate the second request list and add ourselves to it. -*/ - bq = ap-b_actf; - if (bp-b_cylinder bq-b_cylinder) { - while
NSD 4
I've got an update of NSD to v4.0, it's a fairly large diff so I've put it at http://junkpile.org/nsd4.diff (apply diff in /usr/src, it includes changes to etc/rc.d/nsd which must also be installed - to build NSD itself use make -f Makefile.bsd-wrapper obj make -f Makefile.bsd-wrapper sudo make -f Makefile.bsd-wrapper install.) I have not yet updated the sample config file (except as noted below, an old working config file will still be OK). Tested so far on powerpc and amd64, it would be particularly nice to have some reports from people using this on other arch, comments about the upgrade procedure would also be good. Update notes (also repeated in the diff file): cd /usr/sbin rm nsd-notify nsd-patch nsd-xfer nsd-zonec nsdc cd /usr/share/man/man8 rm nsd-notify.8 nsd-patch.8 nsd-xfer.8 nsd-zonec.8 nsdc.8 chown _nsd /var/nsd/db/nsd.db install -o _nsd -g _nsd -d 750 /var/nsd/run/xfr mv /etc/nsd.conf to /var/nsd/etc/nsd.conf - needed to support reloads while in chroot printf '\nremote-control:\n\tcontrol-enable: yes\n' /var/nsd/etc.nsd.conf $EDITOR /var/nsd/etc/nsd.conf - if you have include lines, edit them to specify the *full* path e.g. include /var/nsd/etc/nsd.local - nsd strips the chroot prefix as needed - remove any old cronjobs that run nsdc patch, this is no longer needed N.B. NSD now uses mmap() to access its database. From what I have read so far access is done just via the mmap rather than a mixture of that and write(), but I may have missed something, more eyes on this would be very welcome.
Re: Weird loop in ftp client
Le 22/11/2013 17:48, Ted Unangst a écrit : On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote: On 2013/11/22 07:25, Maxime Villard wrote: If write() fails without EPIPE, d is decremented, and the function keeps looping. If write() succeeds after several loops, d will be negative, and the function will write from buf-XX. When does write() fail and do we want to keep writing? (If I fill up the filesystem, do I really want to spin here?) I must say that I actually fail to see why EPIPE is the only case handled; it should stop looping regardless of the errno code, shouldn't it? Shouldn't it be something more like this? Otherwise if the write() fails, we attempt writing one byte fewer for every retry. That looks better to me.
[patch] start adding support for compcert compiler
I'm trying to port compcert to openbsd. Here's a first patch to allow jot to be compiled with compcert. Before the patch is applied compcert fails because _Bool is predefined as per C99: # ccomp -fall -c /usr/src/usr.bin/jot/jot.c /usr/include/stdbool.h:20: Error: illegal combination of type specifiers. Fatal error. 1 error detected. *** Error 2 in /usr/src/usr.bin/jot (sys.mk:87 'jot.o') # After the patch below is applied: # ccomp -fall -c /usr/src/usr.bin/jot/jot.c # Index: include/stdbool.h === RCS file: /home/cvs/src/include/stdbool.h,v retrieving revision 1.5 diff -u -p -u -r1.5 stdbool.h --- include/stdbool.h 24 Jul 2010 22:17:03 - 1.5 +++ include/stdbool.h 23 Nov 2013 22:38:23 - @@ -10,7 +10,7 @@ #ifndef __cplusplus -#if (defined(__GNUC__) __GNUC__ = 3) || defined(__PCC__) || defined(lint) +#if (defined(__GNUC__) __GNUC__ = 3) || defined(__COMPCERT__) || defined(__PCC__) || defined(lint) /* Support for _C99: type _Bool is already built-in. */ #define false 0 #define true 1 Index: sys/sys/types.h === RCS file: /home/cvs/src/sys/sys/types.h,v retrieving revision 1.39 diff -u -p -u -r1.39 types.h --- sys/sys/types.h 14 Sep 2013 01:35:02 - 1.39 +++ sys/sys/types.h 23 Nov 2013 22:40:59 - @@ -241,7 +241,7 @@ struct uio; #endif #ifdef _KERNEL -#if (defined(__GNUC__) __GNUC__ = 3) || defined(__PCC__) || defined(lint) +#if (defined(__GNUC__) __GNUC__ = 3) || defined(__COMPCERT__) || defined(__PCC__) || defined(lint) /* Support for _C99: type _Bool is already built-in. */ #define false 0 #define true 1