Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans wrote: > On Mon, 19 Nov 2012, Attilio Rao wrote: > >> Log: >> r16312 is not any longer real since many years (likely since when VFS >> received granular locking) but the comment present in UFS has been >> copied all over other filesystems code incorrectly for several times. >> >> Removes comments that makes no sense now. > > > It still made sense (except for bitrot in the function name), but might not > be true). The code made sense with it. Now the code makes no sense. > > >> Modified: head/sys/ufs/ffs/ffs_vfsops.c >> >> == >> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 >> (r243310) >> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 >> (r243311) >> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags >> ump = VFSTOUFS(mp); >> dev = ump->um_dev; >> fs = ump->um_fs; >> - >> - /* >> -* If this malloc() is performed after the getnewvnode() > > > This malloc() didn't match the code, which uses uma_zalloc(). Old > versions used MALLOC() in both the comment and the code. ffs's comment > was updated to say malloc() when the code was changed to use malloc(), > then rotted when the code was changed to use uma_zalloc(). In some > other file systems, the comment still said MALLOC(). > > >> -* it might block, leaving a vnode with a NULL v_data to be >> -* found by ffs_sync() if a sync happens to fire right then, >> -* which will cause a panic because ffs_sync() blindly >> -* dereferences vp->v_data (as well it should). >> -*/ >> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); >> >> /* Allocate a new vnode/inode. */ >> > > The code makes no sense now. The comment explains why ip is allocated > before vp, instead of in the natural, opposite order like it used to > be. Allocating things in an unnatural order requires extra code to > free ip when the allocation of vp fails. "Used to be" is very arguably. The code has been like its current form many more years than the opposite (16 against 3 I think). And the code makes perfectly sense if you know the history. So I don't agree with you. Thanks for the other comments, I will try to squeeze a patch about it. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243320 - head/usr.bin/cut
On Tue, Nov 20, 2012 at 01:57:21AM +, Eitan Adler wrote: > Author: eadler > Date: Tue Nov 20 01:57:21 2012 > New Revision: 243320 > URL: http://svnweb.freebsd.org/changeset/base/243320 > > Log: > Add 'w' flag to: > > Use whitespace (spaces and tabs) as the delimiter. > Consecutive spaces and tabs count as one single field > separator. Awesome, long desired. Thanks! ./danfe ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243324 - head/etc
On 11/20/2012 10:14 AM, Hiroki Sato wrote: > rc_fast, rc_force, and rc_quiet are not intended to be used in > /etc/rc.conf or other config files. These variables have no default > value and will be defined forcibly via {fast,force,quiet}start. If > we use checkyesno() here, it checks whether a variable is defined or > not and then put an warning message when not defined. It is a bad > side-effect for them. Ok. Thanks for the explanation. Cheers, Mike. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243228 - head/etc
Hi Chris, On Sun, Nov 18, 2012 at 02:21:05PM +, Chris Rees wrote: > Author: crees (ports committer) > Date: Sun Nov 18 14:21:05 2012 > New Revision: 243228 > URL: http://svnweb.freebsd.org/changeset/base/243228 > > Log: > cp -R misses out dotfiles; use pax instead to copy file hierarchies > > PR: conf/99721 (based on) > Submitted by: Florian Zavatzki > Approved by:hrs > MFC after: 1 month > > Modified: > head/etc/rc.initdiskless > > Modified: head/etc/rc.initdiskless > == > --- head/etc/rc.initdiskless Sun Nov 18 14:05:28 2012(r243227) > +++ head/etc/rc.initdiskless Sun Nov 18 14:21:05 2012(r243228) > @@ -354,7 +354,7 @@ for i in ${templates} ; do > subdir=${j##*/} > if [ -d $j -a ! -f $j.cpio.gz ]; then > create_md $subdir > - cp -Rp $j/ /$subdir > + (cd $j && pax -rw . /$subdir) > fi > done > for j in /conf/$i/*.cpio.gz ; do Have you tested this on a diskless and readonly system? It looks like pax need to write something in /tmp and it might not be writeable yet. I got an error, after the first of /bin/pax not found and having to add that to the list of files needed. John -- John Hay -- j...@meraka.csir.co.za / j...@freebsd.org ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243330 - head/usr.sbin/pw
Author: bapt Date: Tue Nov 20 10:59:41 2012 New Revision: 243330 URL: http://svnweb.freebsd.org/changeset/base/243330 Log: Correctly set the password file mode after renaming in NIS mode Modified: head/usr.sbin/pw/pw_nis.c Modified: head/usr.sbin/pw/pw_nis.c == --- head/usr.sbin/pw/pw_nis.c Tue Nov 20 09:27:56 2012(r243329) +++ head/usr.sbin/pw/pw_nis.c Tue Nov 20 10:59:41 2012(r243330) @@ -68,6 +68,8 @@ pw_nisupdate(const char * path, struct p } if (rename(pw_tempname(), path) == -1) err(1, "rename()"); + if (chmod(path, 0644) == -1) + err(1, "chmod()"); free(pw); pw_fini(); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On Tue, 20 Nov 2012, Attilio Rao wrote: On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans wrote: On Mon, 19 Nov 2012, Attilio Rao wrote: Log: r16312 is not any longer real since many years (likely since when VFS received granular locking) but the comment present in UFS has been copied all over other filesystems code incorrectly for several times. Removes comments that makes no sense now. It still made sense (except for bitrot in the function name), but might not be true). The code made sense with it. Now the code makes no sense. Modified: head/sys/ufs/ffs/ffs_vfsops.c == --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 (r243310) +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 (r243311) @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags ump = VFSTOUFS(mp); dev = ump->um_dev; fs = ump->um_fs; - - /* -* If this malloc() is performed after the getnewvnode() This malloc() didn't match the code, which uses uma_zalloc(). Old versions used MALLOC() in both the comment and the code. ffs's comment was updated to say malloc() when the code was changed to use malloc(), then rotted when the code was changed to use uma_zalloc(). In some other file systems, the comment still said MALLOC(). -* it might block, leaving a vnode with a NULL v_data to be -* found by ffs_sync() if a sync happens to fire right then, -* which will cause a panic because ffs_sync() blindly -* dereferences vp->v_data (as well it should). -*/ ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); /* Allocate a new vnode/inode. */ The code makes no sense now. The comment explains why ip is allocated before vp, instead of in the natural, opposite order like it used to be. Allocating things in an unnatural order requires extra code to free ip when the allocation of vp fails. "Used to be" is very arguably. The code has been like its current form many more years than the opposite (16 against 3 I think). And the code makes perfectly sense if you know the history. So I don't agree with you. But it shouldn't be necessary to know the history of the code to understand it. The code only makes sense if its comment is not removed, or if you know the history of the code so that you can restore the removed comment. However, if the comment makes no sense as you claim, then the code that it it describes makes no sense. I didn't point out before that the comment "/* Allocate a new vnode/inode. */" does less than echo the code, since the code obviously allocates a new vnode/inode and only the extent of the part which does that is unclear, and the comment is disorganized so as to make the scope unclear. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On 11/20/12, Bruce Evans wrote: > On Tue, 20 Nov 2012, Attilio Rao wrote: > >> On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans >> wrote: >>> On Mon, 19 Nov 2012, Attilio Rao wrote: >>> Log: r16312 is not any longer real since many years (likely since when VFS received granular locking) but the comment present in UFS has been copied all over other filesystems code incorrectly for several times. Removes comments that makes no sense now. >>> >>> >>> It still made sense (except for bitrot in the function name), but might >>> not >>> be true). The code made sense with it. Now the code makes no sense. >>> >>> Modified: head/sys/ufs/ffs/ffs_vfsops.c == --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 (r243310) +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 (r243311) @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags ump = VFSTOUFS(mp); dev = ump->um_dev; fs = ump->um_fs; - - /* -* If this malloc() is performed after the getnewvnode() >>> >>> >>> This malloc() didn't match the code, which uses uma_zalloc(). Old >>> versions used MALLOC() in both the comment and the code. ffs's comment >>> was updated to say malloc() when the code was changed to use malloc(), >>> then rotted when the code was changed to use uma_zalloc(). In some >>> other file systems, the comment still said MALLOC(). >>> >>> -* it might block, leaving a vnode with a NULL v_data to be -* found by ffs_sync() if a sync happens to fire right then, -* which will cause a panic because ffs_sync() blindly -* dereferences vp->v_data (as well it should). -*/ ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); /* Allocate a new vnode/inode. */ >>> >>> The code makes no sense now. The comment explains why ip is allocated >>> before vp, instead of in the natural, opposite order like it used to >>> be. Allocating things in an unnatural order requires extra code to >>> free ip when the allocation of vp fails. >> >> "Used to be" is very arguably. The code has been like its current form >> many more years than the opposite (16 against 3 I think). >> And the code makes perfectly sense if you know the history. So I don't >> agree with you. > > But it shouldn't be necessary to know the history of the code to > understand it. The code only makes sense if its comment is not removed, > or if you know the history of the code so that you can restore the > removed comment. However, if the comment makes no sense as you claim, > then the code that it it describes makes no sense. The "code that makes no sense" is basically the justification to have the allocation before the getnewvnode(). It makes no sense because the order makes no sense (you can allocate before or after getnewvnode(), you won't have v_data corruption as the comment claims). Hence the code makes no sense. I don't understand what is the point you are trying to make honestly. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On 11/20/12, Attilio Rao wrote: > On 11/20/12, Bruce Evans wrote: >> On Tue, 20 Nov 2012, Attilio Rao wrote: >> >>> On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans >>> wrote: On Mon, 19 Nov 2012, Attilio Rao wrote: > Log: > r16312 is not any longer real since many years (likely since when VFS > received granular locking) but the comment present in UFS has been > copied all over other filesystems code incorrectly for several times. > > Removes comments that makes no sense now. It still made sense (except for bitrot in the function name), but might not be true). The code made sense with it. Now the code makes no sense. > Modified: head/sys/ufs/ffs/ffs_vfsops.c > > == > --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 > (r243310) > +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 > (r243311) > @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags > ump = VFSTOUFS(mp); > dev = ump->um_dev; > fs = ump->um_fs; > - > - /* > -* If this malloc() is performed after the getnewvnode() This malloc() didn't match the code, which uses uma_zalloc(). Old versions used MALLOC() in both the comment and the code. ffs's comment was updated to say malloc() when the code was changed to use malloc(), then rotted when the code was changed to use uma_zalloc(). In some other file systems, the comment still said MALLOC(). > -* it might block, leaving a vnode with a NULL v_data to be > -* found by ffs_sync() if a sync happens to fire right then, > -* which will cause a panic because ffs_sync() blindly > -* dereferences vp->v_data (as well it should). > -*/ > ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); > > /* Allocate a new vnode/inode. */ > The code makes no sense now. The comment explains why ip is allocated before vp, instead of in the natural, opposite order like it used to be. Allocating things in an unnatural order requires extra code to free ip when the allocation of vp fails. >>> >>> "Used to be" is very arguably. The code has been like its current form >>> many more years than the opposite (16 against 3 I think). >>> And the code makes perfectly sense if you know the history. So I don't >>> agree with you. >> >> But it shouldn't be necessary to know the history of the code to >> understand it. The code only makes sense if its comment is not removed, >> or if you know the history of the code so that you can restore the >> removed comment. However, if the comment makes no sense as you claim, >> then the code that it it describes makes no sense. > > The "code that makes no sense" is basically the justification to have > the allocation before the getnewvnode(). It makes no sense because the > order makes no sense (you can allocate before or after getnewvnode(), > you won't have v_data corruption as the comment claims). > > Hence the code makes no sense. Herm, s/code/comment. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243328 - head/lib/libutil
Hi! On 2012-11-20, Baptiste Daroussin wrote: > change mode the group file to 0644 after a successfull rename(2) > > int > gr_mkdb(void) > { > - return (rename(tempname, group_file)); > + int ret; > + > + ret = rename(tempname, group_file); > + > + if (ret == 0) > + chmod(group_file, 0644); > + > + return (ret); > } Rename+chmod is not an atomic operation. There is a window when the file has wrong permissions. Also, you don't check the return value of chmod(). Maybe chmod first and then rename? -- Jaakko ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243333 - in head/sys: dev/fdc geom geom/bde geom/cache geom/label geom/mountver geom/multipath geom/nop geom/sched vm
Author: jh Date: Tue Nov 20 12:32:18 2012 New Revision: 24 URL: http://svnweb.freebsd.org/changeset/base/24 Log: - Don't pass geom and provider names as format strings. - Add __printflike() attributes. - Remove an extra argument for the g_new_geomf() call in swapongeom_ev(). Reviewed by: pjd Modified: head/sys/dev/fdc/fdc.c head/sys/geom/bde/g_bde.c head/sys/geom/cache/g_cache.c head/sys/geom/geom.h head/sys/geom/geom_aes.c head/sys/geom/geom_dev.c head/sys/geom/geom_mbr.c head/sys/geom/geom_slice.c head/sys/geom/geom_slice.h head/sys/geom/label/g_label.c head/sys/geom/mountver/g_mountver.c head/sys/geom/multipath/g_multipath.c head/sys/geom/nop/g_nop.c head/sys/geom/sched/g_sched.c head/sys/vm/swap_pager.c Modified: head/sys/dev/fdc/fdc.c == --- head/sys/dev/fdc/fdc.c Tue Nov 20 12:21:02 2012(r243332) +++ head/sys/dev/fdc/fdc.c Tue Nov 20 12:32:18 2012(r24) @@ -865,7 +865,7 @@ fdc_worker(struct fdc_data *fdc) g_orphan_provider(fd->fd_provider, ENXIO); fd->fd_provider->flags |= G_PF_WITHER; fd->fd_provider = - g_new_providerf(fd->fd_geom, fd->fd_geom->name); + g_new_providerf(fd->fd_geom, "%s", fd->fd_geom->name); g_error_provider(fd->fd_provider, 0); g_topology_unlock(); return (fdc_biodone(fdc, ENXIO)); @@ -2011,7 +2011,7 @@ fd_attach2(void *arg, int flag) fd->fd_geom = g_new_geomf(&g_fd_class, "fd%d", device_get_unit(fd->dev)); - fd->fd_provider = g_new_providerf(fd->fd_geom, fd->fd_geom->name); + fd->fd_provider = g_new_providerf(fd->fd_geom, "%s", fd->fd_geom->name); fd->fd_geom->softc = fd; g_error_provider(fd->fd_provider, 0); } Modified: head/sys/geom/bde/g_bde.c == --- head/sys/geom/bde/g_bde.c Tue Nov 20 12:21:02 2012(r243332) +++ head/sys/geom/bde/g_bde.c Tue Nov 20 12:32:18 2012(r24) @@ -184,7 +184,7 @@ g_bde_create_geom(struct gctl_req *req, /* XXX: error check */ kproc_create(g_bde_worker, gp, &sc->thread, 0, 0, "g_bde %s", gp->name); - pp = g_new_providerf(gp, gp->name); + pp = g_new_providerf(gp, "%s", gp->name); pp->stripesize = kp->zone_cont; pp->stripeoffset = 0; pp->mediasize = sc->mediasize; Modified: head/sys/geom/cache/g_cache.c == --- head/sys/geom/cache/g_cache.c Tue Nov 20 12:21:02 2012 (r243332) +++ head/sys/geom/cache/g_cache.c Tue Nov 20 12:32:18 2012 (r24) @@ -502,7 +502,7 @@ g_cache_create(struct g_class *mp, struc return (NULL); } - gp = g_new_geomf(mp, md->md_name); + gp = g_new_geomf(mp, "%s", md->md_name); sc = g_malloc(sizeof(*sc), M_WAITOK | M_ZERO); sc->sc_type = type; sc->sc_bshift = bshift; Modified: head/sys/geom/geom.h == --- head/sys/geom/geom.hTue Nov 20 12:21:02 2012(r243332) +++ head/sys/geom/geom.hTue Nov 20 12:32:18 2012(r24) @@ -271,8 +271,10 @@ int g_handleattr_int(struct bio *bp, con int g_handleattr_off_t(struct bio *bp, const char *attribute, off_t val); int g_handleattr_str(struct bio *bp, const char *attribute, const char *str); struct g_consumer * g_new_consumer(struct g_geom *gp); -struct g_geom * g_new_geomf(struct g_class *mp, const char *fmt, ...); -struct g_provider * g_new_providerf(struct g_geom *gp, const char *fmt, ...); +struct g_geom * g_new_geomf(struct g_class *mp, const char *fmt, ...) +__printflike(2, 3); +struct g_provider * g_new_providerf(struct g_geom *gp, const char *fmt, ...) +__printflike(2, 3); void g_resize_provider(struct g_provider *pp, off_t size); int g_retaste(struct g_class *mp); void g_spoil(struct g_provider *pp, struct g_consumer *cp); Modified: head/sys/geom/geom_aes.c == --- head/sys/geom/geom_aes.cTue Nov 20 12:21:02 2012(r243332) +++ head/sys/geom/geom_aes.cTue Nov 20 12:32:18 2012(r24) @@ -342,7 +342,7 @@ g_aes_taste(struct g_class *mp, struct g } } g_topology_lock(); - pp = g_new_providerf(gp, gp->name); + pp = g_new_providerf(gp, "%s", gp->name); pp->mediasize = mediasize - sectorsize; pp->sectorsize = sectorsize; g_error_provider(pp, 0); Modified: head/sys/geom/geom_dev.c =
Re: svn commit: r243328 - head/lib/libutil
On Tue, Nov 20, 2012 at 02:22:26PM +0200, Jaakko Heinonen wrote: > > Hi! > > On 2012-11-20, Baptiste Daroussin wrote: > > change mode the group file to 0644 after a successfull rename(2) > > > > int > > gr_mkdb(void) > > { > > - return (rename(tempname, group_file)); > > + int ret; > > + > > + ret = rename(tempname, group_file); > > + > > + if (ret == 0) > > + chmod(group_file, 0644); > > + > > + return (ret); > > } > > Rename+chmod is not an atomic operation. There is a window when the file > has wrong permissions. Also, you don't check the return value of > chmod(). Maybe chmod first and then rename? > > -- > Jaakko Yes you are right chmod should probably be first. regards, Bapt pgp2j1SIOZgIB.pgp Description: PGP signature
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On Tue, 20 Nov 2012, Attilio Rao wrote: On 11/20/12, Attilio Rao wrote: On 11/20/12, Bruce Evans wrote: On Tue, 20 Nov 2012, Attilio Rao wrote: On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans wrote: On Mon, 19 Nov 2012, Attilio Rao wrote: Log: r16312 is not any longer real since many years (likely since when VFS received granular locking) but the comment present in UFS has been copied all over other filesystems code incorrectly for several times. Removes comments that makes no sense now. It still made sense (except for bitrot in the function name), but might not be true). The code made sense with it. Now the code makes no sense. Modified: head/sys/ufs/ffs/ffs_vfsops.c == --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 (r243310) +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 (r243311) @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags ump = VFSTOUFS(mp); dev = ump->um_dev; fs = ump->um_fs; - - /* -* If this malloc() is performed after the getnewvnode() This malloc() didn't match the code, which uses uma_zalloc(). Old versions used MALLOC() in both the comment and the code. ffs's comment was updated to say malloc() when the code was changed to use malloc(), then rotted when the code was changed to use uma_zalloc(). In some other file systems, the comment still said MALLOC(). -* it might block, leaving a vnode with a NULL v_data to be -* found by ffs_sync() if a sync happens to fire right then, -* which will cause a panic because ffs_sync() blindly -* dereferences vp->v_data (as well it should). -*/ ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); /* Allocate a new vnode/inode. */ The code makes no sense now. The comment explains why ip is allocated before vp, instead of in the natural, opposite order like it used to be. Allocating things in an unnatural order requires extra code to free ip when the allocation of vp fails. "Used to be" is very arguably. The code has been like its current form many more years than the opposite (16 against 3 I think). And the code makes perfectly sense if you know the history. So I don't agree with you. But it shouldn't be necessary to know the history of the code to understand it. The code only makes sense if its comment is not removed, or if you know the history of the code so that you can restore the removed comment. However, if the comment makes no sense as you claim, then the code that it it describes makes no sense. The "code that makes no sense" is basically the justification to have the allocation before the getnewvnode(). It makes no sense because the order makes no sense (you can allocate before or after getnewvnode(), you won't have v_data corruption as the comment claims). Hence the code makes no sense. Herm, s/code/comment. I think you are right that the comment makes no sense. A preemptible kernel may be preempted without it calling malloc() where it may block. Thus ffs_fsync() and anything else that looks at the inode must be doing something to avoid dereferencing v_data if the vnode is not fully constructed. This seems to be done by iterating over vnodes using MNT_VNODE_FOREACH_ACTIVE*() and not making incomplete vnodes active. ffs_fsync() still just blindly dereferences the inode. I think I am right that the code makes no sense. It is ordered like it is because placing the allocation of ip before the allocation of vp used to be enough to prevent v_data being dereferenced. This makes no sense when it isn't enough. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On 11/20/12, Bruce Evans wrote: > On Tue, 20 Nov 2012, Attilio Rao wrote: > >> On 11/20/12, Attilio Rao wrote: >>> On 11/20/12, Bruce Evans wrote: On Tue, 20 Nov 2012, Attilio Rao wrote: > On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans > wrote: >> On Mon, 19 Nov 2012, Attilio Rao wrote: >> >>> Log: >>> r16312 is not any longer real since many years (likely since when >>> VFS >>> received granular locking) but the comment present in UFS has been >>> copied all over other filesystems code incorrectly for several >>> times. >>> >>> Removes comments that makes no sense now. >> >> >> It still made sense (except for bitrot in the function name), but >> might >> not >> be true). The code made sense with it. Now the code makes no sense. >> >> >>> Modified: head/sys/ufs/ffs/ffs_vfsops.c >>> >>> == >>> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 >>> (r243310) >>> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 >>> (r243311) >>> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags >>> ump = VFSTOUFS(mp); >>> dev = ump->um_dev; >>> fs = ump->um_fs; >>> - >>> - /* >>> -* If this malloc() is performed after the getnewvnode() >> >> >> This malloc() didn't match the code, which uses uma_zalloc(). Old >> versions used MALLOC() in both the comment and the code. ffs's >> comment >> was updated to say malloc() when the code was changed to use >> malloc(), >> then rotted when the code was changed to use uma_zalloc(). In some >> other file systems, the comment still said MALLOC(). >> >> >>> -* it might block, leaving a vnode with a NULL v_data to be >>> -* found by ffs_sync() if a sync happens to fire right then, >>> -* which will cause a panic because ffs_sync() blindly >>> -* dereferences vp->v_data (as well it should). >>> -*/ >>> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); >>> >>> /* Allocate a new vnode/inode. */ >>> >> >> The code makes no sense now. The comment explains why ip is >> allocated >> before vp, instead of in the natural, opposite order like it used to >> be. Allocating things in an unnatural order requires extra code to >> free ip when the allocation of vp fails. > > "Used to be" is very arguably. The code has been like its current form > many more years than the opposite (16 against 3 I think). > And the code makes perfectly sense if you know the history. So I don't > agree with you. But it shouldn't be necessary to know the history of the code to understand it. The code only makes sense if its comment is not removed, or if you know the history of the code so that you can restore the removed comment. However, if the comment makes no sense as you claim, then the code that it it describes makes no sense. >>> >>> The "code that makes no sense" is basically the justification to have >>> the allocation before the getnewvnode(). It makes no sense because the >>> order makes no sense (you can allocate before or after getnewvnode(), >>> you won't have v_data corruption as the comment claims). >>> >>> Hence the code makes no sense. >> >> Herm, s/code/comment. > > I think you are right that the comment makes no sense. A preemptible > kernel may be preempted without it calling malloc() where it may block. > Thus ffs_fsync() and anything else that looks at the inode must be > doing something to avoid dereferencing v_data if the vnode is not fully > constructed. This seems to be done by iterating over vnodes using > MNT_VNODE_FOREACH_ACTIVE*() and not making incomplete vnodes active. > ffs_fsync() still just blindly dereferences the inode. > > I think I am right that the code makes no sense. It is ordered like > it is because placing the allocation of ip before the allocation of > vp used to be enough to prevent v_data being dereferenced. This makes > no sense when it isn't enough. In the past, before VFS got locking and kernel was single-threaded, the comment and code arranged in this way were sensitive and effective. As now this is not true anymore, there is no strict relationship between the getnewvnode() and sleeping points. It is important to remove stale comments because they confuse people, the porters and as you can see the code/comment has been cut&paste quite a bit around. Attilio -- Peace can only be achieved by understanding - A. Einstein ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243328 - head/lib/libutil
On Tue, Nov 20, 2012 at 02:22:26PM +0200, Jaakko Heinonen wrote: > > Hi! > > On 2012-11-20, Baptiste Daroussin wrote: > > change mode the group file to 0644 after a successfull rename(2) > > > > int > > gr_mkdb(void) > > { > > - return (rename(tempname, group_file)); > > + int ret; > > + > > + ret = rename(tempname, group_file); > > + > > + if (ret == 0) > > + chmod(group_file, 0644); > > + > > + return (ret); > > } > > Rename+chmod is not an atomic operation. There is a window when the file > has wrong permissions. Also, you don't check the return value of > chmod(). Maybe chmod first and then rename? > > -- > Jaakko Does this looks better to you? http://people.freebsd.org/~bapt/gr_util.diff regards, Bapt pgpNUVRv360fH.pgp Description: PGP signature
Re: svn commit: r243228 - head/etc
On Tue, 20 Nov 2012, John Hay wrote: On Sun, Nov 18, 2012 at 02:21:05PM +, Chris Rees wrote: Log: cp -R misses out dotfiles; use pax instead to copy file hierarchies PR: conf/99721 (based on) Submitted by: Florian Zavatzki Approved by: hrs MFC after:1 month Modified: head/etc/rc.initdiskless Modified: head/etc/rc.initdiskless == --- head/etc/rc.initdisklessSun Nov 18 14:05:28 2012(r243227) +++ head/etc/rc.initdisklessSun Nov 18 14:21:05 2012(r243228) @@ -354,7 +354,7 @@ for i in ${templates} ; do subdir=${j##*/} if [ -d $j -a ! -f $j.cpio.gz ]; then create_md $subdir - cp -Rp $j/ /$subdir + (cd $j && pax -rw . /$subdir) fi done for j in /conf/$i/*.cpio.gz ; do Have you tested this on a diskless and readonly system? It looks like pax need to write something in /tmp and it might not be writeable yet. I got an error, after the first of /bin/pax not found and having to add that to the list of files needed. It uses mkstemp(3), normally in /tmp but it honors $TMPDIR. It seems to always create 1 temporary file (even for copying a single regular file), and sometimes 2 temporary files. Both of the temporary files seem to be to hold metadata for file times and hashes, in case it is too large for memory. cp -Rp probably needs to do the same (except it is imperfect to unnecessarily assume that /tmp is writable), to fix its link and timestamp handling. BTW, I think it is a large bug that ed and vi create temporary files even before you change anything. Even view(1) (vi -R) wants to scribble on /var/tmp/vi.recover. At least it doesn't refuse to start if this is not writeable. ed(1) is considerably more broken. It - hard codes /tmp and doesn't use _PATH_TMP or honor $TMPDIR - always scribbles in /tmp - refuses to start if /tmp is not writeable. This makes ed(1) wlays broken in single user shells until '/' is mounted rw, although ed is the only editor that is sure to be there and the reason for using a single user shell is often that there is a problem with mounting '/' rw. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243328 - head/lib/libutil
On 2012-11-20, Baptiste Daroussin wrote: > Does this looks better to you? > http://people.freebsd.org/~bapt/gr_util.diff Yes. Thank you. -- Jaakko ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243334 - head/lib/libutil
Author: bapt Date: Tue Nov 20 14:03:09 2012 New Revision: 243334 URL: http://svnweb.freebsd.org/changeset/base/243334 Log: only rename(2) after chmod(2) has succeed report error if chmod(2) fails Reported by: jh Modified: head/lib/libutil/gr_util.c Modified: head/lib/libutil/gr_util.c == --- head/lib/libutil/gr_util.c Tue Nov 20 12:32:18 2012(r24) +++ head/lib/libutil/gr_util.c Tue Nov 20 14:03:09 2012(r243334) @@ -318,14 +318,10 @@ gr_copy(int ffd, int tfd, const struct g int gr_mkdb(void) { - int ret; + if (chmod(tempname, 0644) != 0) + return (-1); - ret = rename(tempname, group_file); - - if (ret == 0) - chmod(group_file, 0644); - - return (ret); + return (rename(tempname, group_file)); } /* ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243335 - head/usr.sbin/pw
Author: bapt Date: Tue Nov 20 14:05:46 2012 New Revision: 243335 URL: http://svnweb.freebsd.org/changeset/base/243335 Log: In NIS mode first chmod(2) the temporary file and is succeed then rename(2) Modified: head/usr.sbin/pw/pw_nis.c Modified: head/usr.sbin/pw/pw_nis.c == --- head/usr.sbin/pw/pw_nis.c Tue Nov 20 14:03:09 2012(r243334) +++ head/usr.sbin/pw/pw_nis.c Tue Nov 20 14:05:46 2012(r243335) @@ -66,10 +66,10 @@ pw_nisupdate(const char * path, struct p pw_fini(); err(1, "pw_copy()"); } + if (chmod(pw_tempname(), 0644) == -1) + err(1, "chmod()"); if (rename(pw_tempname(), path) == -1) err(1, "rename()"); - if (chmod(path, 0644) == -1) - err(1, "chmod()"); free(pw); pw_fini(); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243336 - head/sys/netinet6
Author: ae Date: Tue Nov 20 14:09:37 2012 New Revision: 243336 URL: http://svnweb.freebsd.org/changeset/base/243336 Log: Remove opt_inet.h, it isn't required here. MFC after:1 week Modified: head/sys/netinet6/ip6_mroute.c Modified: head/sys/netinet6/ip6_mroute.c == --- head/sys/netinet6/ip6_mroute.c Tue Nov 20 14:05:46 2012 (r243335) +++ head/sys/netinet6/ip6_mroute.c Tue Nov 20 14:09:37 2012 (r243336) @@ -81,7 +81,6 @@ #include __FBSDID("$FreeBSD$"); -#include "opt_inet.h" #include "opt_inet6.h" #include ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243337 - head/sys/modules
Author: ae Date: Tue Nov 20 14:11:27 2012 New Revision: 243337 URL: http://svnweb.freebsd.org/changeset/base/243337 Log: Connect ip6_mroute kernel module to the build. MFC after:1 week Modified: head/sys/modules/Makefile Modified: head/sys/modules/Makefile == --- head/sys/modules/Makefile Tue Nov 20 14:09:37 2012(r243336) +++ head/sys/modules/Makefile Tue Nov 20 14:11:27 2012(r243337) @@ -150,6 +150,7 @@ SUBDIR= \ ${_ipfw} \ ipfw_nat \ ${_ipmi} \ + ip6_mroute_mod \ ip_mroute_mod \ ${_ips} \ ${_ipw} \ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On Tue, Nov 20, 2012 at 01:27:07PM +, Attilio Rao wrote: > On 11/20/12, Bruce Evans wrote: > > On Tue, 20 Nov 2012, Attilio Rao wrote: > > > >> On 11/20/12, Attilio Rao wrote: > >>> On 11/20/12, Bruce Evans wrote: > On Tue, 20 Nov 2012, Attilio Rao wrote: > > > On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans > > wrote: > >> On Mon, 19 Nov 2012, Attilio Rao wrote: > >> > >>> Log: > >>> r16312 is not any longer real since many years (likely since when > >>> VFS > >>> received granular locking) but the comment present in UFS has been > >>> copied all over other filesystems code incorrectly for several > >>> times. > >>> > >>> Removes comments that makes no sense now. > >> > >> > >> It still made sense (except for bitrot in the function name), but > >> might > >> not > >> be true). The code made sense with it. Now the code makes no sense. > >> > >> > >>> Modified: head/sys/ufs/ffs/ffs_vfsops.c > >>> > >>> == > >>> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 > >>> (r243310) > >>> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 > >>> (r243311) > >>> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags > >>> ump = VFSTOUFS(mp); > >>> dev = ump->um_dev; > >>> fs = ump->um_fs; > >>> - > >>> - /* > >>> -* If this malloc() is performed after the getnewvnode() > >> > >> > >> This malloc() didn't match the code, which uses uma_zalloc(). Old > >> versions used MALLOC() in both the comment and the code. ffs's > >> comment > >> was updated to say malloc() when the code was changed to use > >> malloc(), > >> then rotted when the code was changed to use uma_zalloc(). In some > >> other file systems, the comment still said MALLOC(). > >> > >> > >>> -* it might block, leaving a vnode with a NULL v_data to be > >>> -* found by ffs_sync() if a sync happens to fire right then, > >>> -* which will cause a panic because ffs_sync() blindly > >>> -* dereferences vp->v_data (as well it should). > >>> -*/ > >>> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); > >>> > >>> /* Allocate a new vnode/inode. */ > >>> > >> > >> The code makes no sense now. The comment explains why ip is > >> allocated > >> before vp, instead of in the natural, opposite order like it used to > >> be. Allocating things in an unnatural order requires extra code to > >> free ip when the allocation of vp fails. > > > > "Used to be" is very arguably. The code has been like its current form > > many more years than the opposite (16 against 3 I think). > > And the code makes perfectly sense if you know the history. So I don't > > agree with you. > > But it shouldn't be necessary to know the history of the code to > understand it. The code only makes sense if its comment is not > removed, > or if you know the history of the code so that you can restore the > removed comment. However, if the comment makes no sense as you claim, > then the code that it it describes makes no sense. > >>> > >>> The "code that makes no sense" is basically the justification to have > >>> the allocation before the getnewvnode(). It makes no sense because the > >>> order makes no sense (you can allocate before or after getnewvnode(), > >>> you won't have v_data corruption as the comment claims). > >>> > >>> Hence the code makes no sense. > >> > >> Herm, s/code/comment. > > > > I think you are right that the comment makes no sense. A preemptible > > kernel may be preempted without it calling malloc() where it may block. > > Thus ffs_fsync() and anything else that looks at the inode must be > > doing something to avoid dereferencing v_data if the vnode is not fully > > constructed. This seems to be done by iterating over vnodes using > > MNT_VNODE_FOREACH_ACTIVE*() and not making incomplete vnodes active. > > ffs_fsync() still just blindly dereferences the inode. > > > > I think I am right that the code makes no sense. It is ordered like > > it is because placing the allocation of ip before the allocation of > > vp used to be enough to prevent v_data being dereferenced. This makes > > no sense when it isn't enough. > > In the past, before VFS got locking and kernel was single-threaded, > the comment and code arranged in this way were sensitive and > effective. > As now this is not true anymore, there is no strict relationship > between the getnewvnode() and sleeping points. > It is important to remove stale comments because they confuse people, > the porters and as you can see the code/comment has been cut&paste > quite a bit around. > Putting the issue of
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On 11/20/12, Konstantin Belousov wrote: > On Tue, Nov 20, 2012 at 01:27:07PM +, Attilio Rao wrote: >> On 11/20/12, Bruce Evans wrote: >> > On Tue, 20 Nov 2012, Attilio Rao wrote: >> > >> >> On 11/20/12, Attilio Rao wrote: >> >>> On 11/20/12, Bruce Evans wrote: >> On Tue, 20 Nov 2012, Attilio Rao wrote: >> >> > On Tue, Nov 20, 2012 at 6:20 AM, Bruce Evans >> > wrote: >> >> On Mon, 19 Nov 2012, Attilio Rao wrote: >> >> >> >>> Log: >> >>> r16312 is not any longer real since many years (likely since >> >>> when >> >>> VFS >> >>> received granular locking) but the comment present in UFS has >> >>> been >> >>> copied all over other filesystems code incorrectly for several >> >>> times. >> >>> >> >>> Removes comments that makes no sense now. >> >> >> >> >> >> It still made sense (except for bitrot in the function name), but >> >> might >> >> not >> >> be true). The code made sense with it. Now the code makes no >> >> sense. >> >> >> >> >> >>> Modified: head/sys/ufs/ffs/ffs_vfsops.c >> >>> >> >>> == >> >>> --- head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 21:58:14 2012 >> >>> (r243310) >> >>> +++ head/sys/ufs/ffs/ffs_vfsops.c Mon Nov 19 22:43:45 2012 >> >>> (r243311) >> >>> @@ -1676,14 +1676,6 @@ ffs_vgetf(mp, ino, flags, vpp, ffs_flags >> >>> ump = VFSTOUFS(mp); >> >>> dev = ump->um_dev; >> >>> fs = ump->um_fs; >> >>> - >> >>> - /* >> >>> -* If this malloc() is performed after the getnewvnode() >> >> >> >> >> >> This malloc() didn't match the code, which uses uma_zalloc(). Old >> >> versions used MALLOC() in both the comment and the code. ffs's >> >> comment >> >> was updated to say malloc() when the code was changed to use >> >> malloc(), >> >> then rotted when the code was changed to use uma_zalloc(). In >> >> some >> >> other file systems, the comment still said MALLOC(). >> >> >> >> >> >>> -* it might block, leaving a vnode with a NULL v_data to >> >>> be >> >>> -* found by ffs_sync() if a sync happens to fire right >> >>> then, >> >>> -* which will cause a panic because ffs_sync() blindly >> >>> -* dereferences vp->v_data (as well it should). >> >>> -*/ >> >>> ip = uma_zalloc(uma_inode, M_WAITOK | M_ZERO); >> >>> >> >>> /* Allocate a new vnode/inode. */ >> >>> >> >> >> >> The code makes no sense now. The comment explains why ip is >> >> allocated >> >> before vp, instead of in the natural, opposite order like it used >> >> to >> >> be. Allocating things in an unnatural order requires extra code >> >> to >> >> free ip when the allocation of vp fails. >> > >> > "Used to be" is very arguably. The code has been like its current >> > form >> > many more years than the opposite (16 against 3 I think). >> > And the code makes perfectly sense if you know the history. So I >> > don't >> > agree with you. >> >> But it shouldn't be necessary to know the history of the code to >> understand it. The code only makes sense if its comment is not >> removed, >> or if you know the history of the code so that you can restore the >> removed comment. However, if the comment makes no sense as you >> claim, >> then the code that it it describes makes no sense. >> >>> >> >>> The "code that makes no sense" is basically the justification to have >> >>> the allocation before the getnewvnode(). It makes no sense because >> >>> the >> >>> order makes no sense (you can allocate before or after getnewvnode(), >> >>> you won't have v_data corruption as the comment claims). >> >>> >> >>> Hence the code makes no sense. >> >> >> >> Herm, s/code/comment. >> > >> > I think you are right that the comment makes no sense. A preemptible >> > kernel may be preempted without it calling malloc() where it may block. >> > Thus ffs_fsync() and anything else that looks at the inode must be >> > doing something to avoid dereferencing v_data if the vnode is not fully >> > constructed. This seems to be done by iterating over vnodes using >> > MNT_VNODE_FOREACH_ACTIVE*() and not making incomplete vnodes active. >> > ffs_fsync() still just blindly dereferences the inode. >> > >> > I think I am right that the code makes no sense. It is ordered like >> > it is because placing the allocation of ip before the allocation of >> > vp used to be enough to prevent v_data being dereferenced. This makes >> > no sense when it isn't enough. >> >> In the past, before VFS got locking and kernel was single-threaded, >> the comment and code arranged in this way were sensitive and >> effective. >> As now this is not true anymore, ther
Re: svn commit: r242847 - in head/sys: i386/include kern
On 11/18/12 6:13 PM, Andre Oppermann wrote: > On 18.11.2012 15:05, Andrey Zonov wrote: >> On 11/11/12 3:04 AM, Andre Oppermann wrote: >>> On 10.11.2012 23:24, Alfred Perlstein wrote: On 11/10/12 11:18 AM, Andre Oppermann wrote: > On 10.11.2012 19:04, Peter Wemm wrote: >> This is complicated but we need a simple user visible view of it. It >> really needs to be something like "nmbclusters defaults to 6% of >> physical ram, with machine dependent limits". The MD limits are bad >> enough, and using bogo-units like "maxusers" just makes it worse. > > Yes, that would be optimal. > No it would not. I used to be able to tell people "hey just try increasing maxusers" and they would and suddenly the box would be OK. Now I'll have to remember 3,4,5,10,20x tunable to increase? >>> >>> No. The whole mbuf and cluster stuff isn't allocated or reserved >>> at boot time. We simply need a limit to prevent it from exhausting >>> all available kvm / physical memory whichever is less. >>> >> >> For now, we have limit which does not allow to run even one igb(4) NIC >> in 9k jumbo configuration. > > My patch for mbuf* zone auto-sizing does fix that, or not? > Are you talking about r242910? I have not tried it. -- Andrey Zonov signature.asc Description: OpenPGP digital signature
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On Tue, 20 Nov 2012, Attilio Rao wrote: On 11/20/12, Konstantin Belousov wrote: On Tue, Nov 20, 2012 at 01:27:07PM +, Attilio Rao wrote: On 11/20/12, Bruce Evans wrote: ... I think I am right that the code makes no sense. It is ordered like it is because placing the allocation of ip before the allocation of vp used to be enough to prevent v_data being dereferenced. This makes no sense when it isn't enough. In the past, before VFS got locking and kernel was single-threaded, the comment and code arranged in this way were sensitive and effective. As now this is not true anymore, there is no strict relationship between the getnewvnode() and sleeping points. It is important to remove stale comments because they confuse people, the porters and as you can see the code/comment has been cut&paste quite a bit around. Putting the issue of the comment making no sense at all aside, which is true but tangential to what I want to note. Although malloc(9) is not ordered with the vnode locks in the lock order, it is still good practice to not perform allocations under the vnode lock. The reason is that pagedaemon might need to clean and write pages, in particular, belonging to the vnodes. Generally, if you own vnode lock and do something which might kick pagedaemon, you are creating troubles for pagedaemon, which would attempt to lock the vnode with timeout, spending the precious time waiting for lock it cannot obtain. This is less an issue for the new vnode instantiation locations, because vnode cannot have resident pages yet. But it is good to follow the same pattern anywhere. The comment could have been changed to one about this. Except it doesn't really apply here. Neither of the allocations is onder the vnode lock. Yes, I completely agree. Infact the code is not changed also to avoid pagedaemon hosing. However this is a different situation respect a "if you allocate while you hold the lock you will get a deadlock/race/corruption". The code is still nonsense since its order is unrelated to this too :-). The natural code and order is: o // no lock held here o allocate vp o allocate ip and any other stuff needed to complete vp o aquire exclusive lock o check that vp, ip and other stuff didn't go away o wire ip and other stuff into vp // shouldn't do more allocations here o release lock on return (?) but the current order for at least ffs is: o // no lock held here o // oops, actually we always (?) hold at least a shared lock here. Often // we need to promote to an exclusive lock. o allocate ip o allocate vp o aquire exclusive lock // no check that nothing went away?? A comment earlier still says that // we intentionally allow races earlier. Is that correct when we hold // at leasr a shared lock throughout? o wire ip into vp. Includes futher initializations of both. Hopefully these can't block. bread() the inode. Can block now. No worse than blocking for write(2) (?). o further zalloc()s for dinodes. Breaks kib's rule. It would be painful to have to release all the zalloc()ated data on earlier failures. At this point, we can still fail, but have initialized enough of the vnode to just fail without cleaning it up -- ufs_reclaim() will clean it. o further initializations, mostly not involving operations that can block. release lock on return (?) The allocation of ip is clearly special -- it contains state info that hopefully records the state of the initialization, and it must be wired into vp first so that desrtructors can see this info. Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243339 - head/sys/modules/ufs
Author: kib Date: Tue Nov 20 15:23:22 2012 New Revision: 243339 URL: http://svnweb.freebsd.org/changeset/base/243339 Log: Fix module build after r243245. Modified: head/sys/modules/ufs/Makefile Modified: head/sys/modules/ufs/Makefile == --- head/sys/modules/ufs/Makefile Tue Nov 20 15:19:55 2012 (r243338) +++ head/sys/modules/ufs/Makefile Tue Nov 20 15:23:22 2012 (r243339) @@ -7,7 +7,8 @@ SRCS= opt_ddb.h opt_directio.h opt_ffs.h vnode_if.h ufs_acl.c ufs_bmap.c ufs_dirhash.c ufs_extattr.c \ ufs_gjournal.c ufs_inode.c ufs_lookup.c ufs_quota.c ufs_vfsops.c \ ufs_vnops.c ffs_alloc.c ffs_balloc.c ffs_inode.c ffs_snapshot.c \ - ffs_softdep.c ffs_subr.c ffs_tables.c ffs_vfsops.c ffs_vnops.c + ffs_softdep.c ffs_subr.c ffs_suspend.c ffs_tables.c ffs_vfsops.c \ + ffs_vnops.c .if !defined(KERNBUILDDIR) CFLAGS+= -DSOFTUPDATES -DUFS_DIRHASH ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243340 - head/sys/fs/nullfs
Author: kib Date: Tue Nov 20 15:25:00 2012 New Revision: 243340 URL: http://svnweb.freebsd.org/changeset/base/243340 Log: Remove the check and panic for an impossible condition. The NULL lowervp vnode v_vnlock would cause panic due to NULL pointer dereference much earlier. MFC after:1 week Modified: head/sys/fs/nullfs/null_subr.c Modified: head/sys/fs/nullfs/null_subr.c == --- head/sys/fs/nullfs/null_subr.c Tue Nov 20 15:23:22 2012 (r243339) +++ head/sys/fs/nullfs/null_subr.c Tue Nov 20 15:25:00 2012 (r243340) @@ -251,8 +251,6 @@ null_nodeget(mp, lowervp, vpp) vp->v_type = lowervp->v_type; vp->v_data = xp; vp->v_vnlock = lowervp->v_vnlock; - if (vp->v_vnlock == NULL) - panic("null_nodeget: Passed a NULL vnlock.\n"); error = insmntque1(vp, mp, null_insmntque_dtr, xp); if (error != 0) return (error); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243341 - head/sys/kern
Author: kib Date: Tue Nov 20 15:33:48 2012 New Revision: 243341 URL: http://svnweb.freebsd.org/changeset/base/243341 Log: Add a special meaning to the negative ticks argument for taskqueue_enqueue_timeout(). Do not rearm the callout if it is already armed and the ticks is negative. Otherwise rearm it to fire in abs(ticks) ticks in the future. The intended use is to call taskqueue_enqueue_timeout() for the given timeout_task with the same negative ticks argument. As result, the task is scheduled to execute not further than abs(ticks) ticks in future, and the consequent enqueues are coalesced until the already scheduled task is finished. Reviewed by: rwatson Tested by:Markus Gebert MFC after:2 weeks Modified: head/sys/kern/subr_taskqueue.c Modified: head/sys/kern/subr_taskqueue.c == --- head/sys/kern/subr_taskqueue.c Tue Nov 20 15:25:00 2012 (r243340) +++ head/sys/kern/subr_taskqueue.c Tue Nov 20 15:33:48 2012 (r243341) @@ -252,9 +252,13 @@ taskqueue_enqueue_timeout(struct taskque } else { queue->tq_callouts++; timeout_task->f |= DT_CALLOUT_ARMED; + if (ticks < 0) + ticks = -ticks; /* Ignore overflow. */ + } + if (ticks > 0) { + callout_reset(&timeout_task->c, ticks, + taskqueue_timeout_func, timeout_task); } - callout_reset(&timeout_task->c, ticks, taskqueue_timeout_func, - timeout_task); } TQ_UNLOCK(queue); return (res); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243342 - head/sys/kern
Author: kib Date: Tue Nov 20 15:45:48 2012 New Revision: 243342 URL: http://svnweb.freebsd.org/changeset/base/243342 Log: Schedule garbage collection run for the in-flight rights passed over the unix domain sockets to the next tick, coalescing the serial calls until the collection fires. The thought is that more work for the collector could arise in the near time, allowing to clean more and not spend too much CPU on repeated collection when there is no garbage. Currently the collection task is fired immediately upon unix domain socket close if there are any rights in flight, which caused excessive CPU usage and too long blocking of the threads waiting for unp_list_lock and unp_link_rwlock in write mode. Robert noted that it would be nice if we could find some heuristic by which we decide whether to run GC a bit more quickly. E.g., if the number of UNIX domain sockets is close to its resource limit, but not quite. Reported and tested by: Markus Gebert Reviewed by: rwatson MFC after:2 weeks Modified: head/sys/kern/uipc_usrreq.c Modified: head/sys/kern/uipc_usrreq.c == --- head/sys/kern/uipc_usrreq.c Tue Nov 20 15:33:48 2012(r243341) +++ head/sys/kern/uipc_usrreq.c Tue Nov 20 15:45:48 2012(r243342) @@ -131,7 +131,7 @@ static const struct sockaddrsun_noname * reentrance in the UNIX domain socket, file descriptor, and socket layer * code. See unp_gc() for a full description. */ -static struct task unp_gc_task; +static struct timeout_task unp_gc_task; /* * The close of unix domain sockets attached as SCM_RIGHTS is @@ -672,7 +672,7 @@ uipc_detach(struct socket *so) if (vp) vrele(vp); if (local_unp_rights) - taskqueue_enqueue(taskqueue_thread, &unp_gc_task); + taskqueue_enqueue_timeout(taskqueue_thread, &unp_gc_task, -1); } static int @@ -1784,7 +1784,7 @@ unp_init(void) LIST_INIT(&unp_shead); LIST_INIT(&unp_sphead); SLIST_INIT(&unp_defers); - TASK_INIT(&unp_gc_task, 0, unp_gc, NULL); + TIMEOUT_TASK_INIT(taskqueue_thread, &unp_gc_task, 0, unp_gc, NULL); TASK_INIT(&unp_defer_task, 0, unp_process_defers, NULL); UNP_LINK_LOCK_INIT(); UNP_LIST_LOCK_INIT(); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243321 - head/usr.sbin/edquota
On 20 November 2012 02:03, Bruce Evans wrote: > On Tue, 20 Nov 2012, Eitan Adler wrote: > >> Log: >> Remove unneeded includes. >> >> Tested with "make universe"; there are no conditional features. > > > "make universe" can't find such features. Except inversely -- when it > doesn't find them, it usually means that there is a bug in the headers. [ thanks for the info. I'll go through it and parse it fully soon ] Just to explain. The above sentence means "I tested this with make universe" AND I checked to make sure that there were no #ifdefs or the like which might rely on these headers. -- Eitan Adler Source, Ports, Doc committer Bugmeister, Ports Security teams ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243311 - in head/sys: fs/ext2fs fs/msdosfs fs/nfsclient fs/nullfs fs/unionfs gnu/fs/reiserfs nfsclient ufs/ffs
On 20 November 2012 03:17, Attilio Rao wrote: > And the code makes perfectly sense if you know the history. One shouldn't have to know the history of the code to understand the present state. -- Eitan Adler Source, Ports, Doc committer Bugmeister, Ports Security teams ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r243328 - head/lib/libutil
On 20 November 2012 08:38, Baptiste Daroussin wrote: > On Tue, Nov 20, 2012 at 02:22:26PM +0200, Jaakko Heinonen wrote: >> >> Hi! >> >> On 2012-11-20, Baptiste Daroussin wrote: >> > change mode the group file to 0644 after a successfull rename(2) >> > >> > int >> > gr_mkdb(void) >> > { >> > - return (rename(tempname, group_file)); >> > + int ret; >> > + >> > + ret = rename(tempname, group_file); >> > + >> > + if (ret == 0) >> > + chmod(group_file, 0644); >> > + >> > + return (ret); >> > } >> >> Rename+chmod is not an atomic operation. There is a window when the file >> has wrong permissions. Also, you don't check the return value of >> chmod(). Maybe chmod first and then rename? >> >> -- >> Jaakko > > Does this looks better to you? > http://people.freebsd.org/~bapt/gr_util.diff This makes more sense. -- Eitan Adler Source, Ports, Doc committer Bugmeister, Ports Security teams ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243346 - head/tools/regression/lib/libc/resolv
Author: emaste Date: Tue Nov 20 19:23:44 2012 New Revision: 243346 URL: http://svnweb.freebsd.org/changeset/base/243346 Log: Non-void function should return a value. Found by: clang Modified: head/tools/regression/lib/libc/resolv/resolv.c Modified: head/tools/regression/lib/libc/resolv/resolv.c == --- head/tools/regression/lib/libc/resolv/resolv.c Tue Nov 20 17:08:37 2012(r243345) +++ head/tools/regression/lib/libc/resolv/resolv.c Tue Nov 20 19:23:44 2012(r243346) @@ -226,7 +226,7 @@ resolvloop(void *p) { int *nhosts = (int *)p; if (*nhosts == 0) - return; + return NULL; do resolvone(*nhosts); while (--(*nhosts)); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243347 - in head: sys/conf sys/contrib/dev/acpica sys/contrib/dev/acpica/common sys/contrib/dev/acpica/compiler sys/contrib/dev/acpica/components/debugger sys/contrib/dev/acpica/compon...
Author: jkim Date: Tue Nov 20 21:01:59 2012 New Revision: 243347 URL: http://svnweb.freebsd.org/changeset/base/243347 Log: Merge ACPICA 20121114. Added: head/sys/contrib/dev/acpica/components/disassembler/dmdeferred.c - copied, changed from r243045, vendor-sys/acpica/dist/source/components/disassembler/dmdeferred.c Modified: head/sys/conf/files head/sys/contrib/dev/acpica/changes.txt (contents, props changed) head/sys/contrib/dev/acpica/common/adfile.c head/sys/contrib/dev/acpica/common/adisasm.c head/sys/contrib/dev/acpica/common/dmextern.c head/sys/contrib/dev/acpica/common/dmrestag.c head/sys/contrib/dev/acpica/compiler/aslcompile.c head/sys/contrib/dev/acpica/compiler/aslcompiler.h head/sys/contrib/dev/acpica/compiler/aslerror.c head/sys/contrib/dev/acpica/compiler/aslfiles.c head/sys/contrib/dev/acpica/compiler/aslglobal.h head/sys/contrib/dev/acpica/compiler/asllisting.c head/sys/contrib/dev/acpica/compiler/asllookup.c head/sys/contrib/dev/acpica/compiler/aslmain.c head/sys/contrib/dev/acpica/compiler/aslstartup.c head/sys/contrib/dev/acpica/compiler/dttemplate.c head/sys/contrib/dev/acpica/compiler/prutils.c head/sys/contrib/dev/acpica/components/debugger/dbfileio.c head/sys/contrib/dev/acpica/components/debugger/dbinput.c head/sys/contrib/dev/acpica/components/debugger/dbmethod.c head/sys/contrib/dev/acpica/components/disassembler/dmopcode.c head/sys/contrib/dev/acpica/components/disassembler/dmresrc.c head/sys/contrib/dev/acpica/components/disassembler/dmresrcl.c head/sys/contrib/dev/acpica/components/disassembler/dmresrcl2.c head/sys/contrib/dev/acpica/components/disassembler/dmresrcs.c head/sys/contrib/dev/acpica/components/dispatcher/dsmethod.c head/sys/contrib/dev/acpica/components/executer/exregion.c head/sys/contrib/dev/acpica/components/namespace/nsutils.c head/sys/contrib/dev/acpica/components/namespace/nsxfname.c head/sys/contrib/dev/acpica/components/resources/rscalc.c head/sys/contrib/dev/acpica/components/resources/rscreate.c head/sys/contrib/dev/acpica/components/resources/rsdump.c head/sys/contrib/dev/acpica/components/resources/rslist.c head/sys/contrib/dev/acpica/components/resources/rsmisc.c head/sys/contrib/dev/acpica/components/resources/rsxface.c head/sys/contrib/dev/acpica/components/utilities/utdelete.c head/sys/contrib/dev/acpica/components/utilities/utresrc.c head/sys/contrib/dev/acpica/components/utilities/utstate.c head/sys/contrib/dev/acpica/components/utilities/uttrack.c head/sys/contrib/dev/acpica/include/acdisasm.h head/sys/contrib/dev/acpica/include/acmacros.h head/sys/contrib/dev/acpica/include/acpixf.h head/sys/contrib/dev/acpica/include/acrestyp.h head/sys/contrib/dev/acpica/include/acutils.h head/sys/modules/acpi/acpi/Makefile head/usr.sbin/acpi/acpidb/Makefile head/usr.sbin/acpi/iasl/Makefile Directory Properties: head/sys/contrib/dev/acpica/ (props changed) head/sys/contrib/dev/acpica/common/ (props changed) head/sys/contrib/dev/acpica/compiler/ (props changed) head/sys/contrib/dev/acpica/components/debugger/ (props changed) head/sys/contrib/dev/acpica/components/disassembler/ (props changed) head/sys/contrib/dev/acpica/components/dispatcher/ (props changed) head/sys/contrib/dev/acpica/components/executer/ (props changed) head/sys/contrib/dev/acpica/components/namespace/ (props changed) head/sys/contrib/dev/acpica/components/resources/ (props changed) head/sys/contrib/dev/acpica/components/utilities/ (props changed) head/sys/contrib/dev/acpica/include/ (props changed) Modified: head/sys/conf/files == --- head/sys/conf/files Tue Nov 20 19:23:44 2012(r243346) +++ head/sys/conf/files Tue Nov 20 21:01:59 2012(r243347) @@ -294,6 +294,7 @@ contrib/dev/acpica/components/debugger/d contrib/dev/acpica/components/debugger/dbutils.c optional acpi acpi_debug contrib/dev/acpica/components/debugger/dbxface.c optional acpi acpi_debug contrib/dev/acpica/components/disassembler/dmbuffer.c optional acpi acpi_debug +contrib/dev/acpica/components/disassembler/dmdeferred.coptional acpi acpi_debug contrib/dev/acpica/components/disassembler/dmnames.c optional acpi acpi_debug contrib/dev/acpica/components/disassembler/dmopcode.c optional acpi acpi_debug contrib/dev/acpica/components/disassembler/dmobject.c optional acpi acpi_debug Modified: head/sys/contrib/dev/acpica/changes.txt == --- head/sys/contrib/dev/acpica/changes.txt Tue Nov 20 19:23:44 2012 (r243346) +++ head/sys/contrib/dev/acpica/changes.txt Tue Nov 20 21:01:59 2012 (r243347) @@ -1,4 +1,94 @@ +14 November 2012. Summary of changes for version 20121114: + +This release is available at https://www.acpica.org/downl
svn commit: r243348 - head/share/mk
Author: dim Date: Tue Nov 20 21:26:13 2012 New Revision: 243348 URL: http://svnweb.freebsd.org/changeset/base/243348 Log: Do not expose LIBCXXRT and LIBCPLUSPLUS in bsd.libnames.mk, if WITHOUT_LIBCPLUSPLUS is specified. Submitted by: Garrett Cooper MFC after:3 days Modified: head/share/mk/bsd.libnames.mk Modified: head/share/mk/bsd.libnames.mk == --- head/share/mk/bsd.libnames.mk Tue Nov 20 21:01:59 2012 (r243347) +++ head/share/mk/bsd.libnames.mk Tue Nov 20 21:26:13 2012 (r243348) @@ -28,8 +28,10 @@ LIBBSDXML?= ${DESTDIR}${LIBDIR}/libbsdxm LIBBSM?= ${DESTDIR}${LIBDIR}/libbsm.a LIBBSNMP?= ${DESTDIR}${LIBDIR}/libbsnmp.a LIBBZ2?= ${DESTDIR}${LIBDIR}/libbz2.a +.if ${MK_LIBCPLUSPLUS} != "no" LIBCXXRT?= ${DESTDIR}${LIBDIR}/libcxxrt.a LIBCPLUSPLUS?= ${DESTDIR}${LIBDIR}/libc++.a +.endif LIBC?= ${DESTDIR}${LIBDIR}/libc.a LIBC_PIC?= ${DESTDIR}${LIBDIR}/libc_pic.a LIBCALENDAR?= ${DESTDIR}${LIBDIR}/libcalendar.a ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243359 - head/sys/arm/arm
Author: cognet Date: Wed Nov 21 01:38:40 2012 New Revision: 243359 URL: http://svnweb.freebsd.org/changeset/base/243359 Log: Make sure the address starts on a cache line boundary. Modified: head/sys/arm/arm/pl310.c Modified: head/sys/arm/arm/pl310.c == --- head/sys/arm/arm/pl310.cWed Nov 21 01:01:19 2012(r243358) +++ head/sys/arm/arm/pl310.cWed Nov 21 01:38:40 2012(r243359) @@ -180,9 +180,13 @@ static void pl310_wbinv_range(vm_paddr_t start, vm_size_t size) { + if (start & g_l2cache_align_mask) { + size += start & g_l2cache_align_mask; + start &= ~g_l2cache_align_mask; + } if (size & g_l2cache_align_mask) { size &= ~g_l2cache_align_mask; - size += g_l2cache_line_size; + size += g_l2cache_line_size; } #if 1 @@ -217,6 +221,10 @@ static void pl310_wb_range(vm_paddr_t start, vm_size_t size) { + if (start & g_l2cache_align_mask) { + size += start & g_l2cache_align_mask; + start &= ~g_l2cache_align_mask; + } if (size & g_l2cache_align_mask) { size &= ~g_l2cache_align_mask; size += g_l2cache_line_size; @@ -235,6 +243,10 @@ static void pl310_inv_range(vm_paddr_t start, vm_size_t size) { + if (start & g_l2cache_align_mask) { + size += start & g_l2cache_align_mask; + start &= ~g_l2cache_align_mask; + } if (size & g_l2cache_align_mask) { size &= ~g_l2cache_align_mask; size += g_l2cache_line_size; ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r243366 - head/sys/vm
Author: alc Date: Wed Nov 21 06:26:18 2012 New Revision: 243366 URL: http://svnweb.freebsd.org/changeset/base/243366 Log: Correct an error in r230623. When both VM_ALLOC_NODUMP and VM_ALLOC_ZERO were specified to vm_page_alloc(), PG_NODUMP wasn't being set on the allocated page when it happened to be pre-zeroed. MFC after:5 days Modified: head/sys/vm/vm_page.c Modified: head/sys/vm/vm_page.c == --- head/sys/vm/vm_page.c Wed Nov 21 04:54:02 2012(r243365) +++ head/sys/vm/vm_page.c Wed Nov 21 06:26:18 2012(r243366) @@ -1481,13 +1481,13 @@ vm_page_alloc(vm_object_t object, vm_pin * must be cleared before the free page queues lock is released. */ flags = 0; - if (req & VM_ALLOC_NODUMP) - flags |= PG_NODUMP; if (m->flags & PG_ZERO) { vm_page_zero_count--; if (req & VM_ALLOC_ZERO) flags = PG_ZERO; } + if (req & VM_ALLOC_NODUMP) + flags |= PG_NODUMP; m->flags = flags; mtx_unlock(&vm_page_queue_free_mtx); m->aflags = 0; ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"