Re: CVS commit: src/sys/coda
In article <20131123234556.1b4ac14a...@mail.netbsd.org>, Mindaugas Rasiukevicius wrote: >chris...@zoulas.com (Christos Zoulas) wrote: >> On Nov 23, 11:24pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote: >> -- Subject: Re: CVS commit: src/sys/coda >> >> | mp = mp == TAILQ_END(&mountlist) ? NULL : mp; >> | >> | Over: >> | >> | return mp; >> | >> | Everybody understood NULL, so why obfuscate the code? >> >> The point (as I understand it) is so that if you want to change to another >> ADT where end != NULL (are there any now, that we have banned CIRCLEQ?) >> using the END macro lets you s/FOOQ/BARQ/ and have it mostly work. > >Since 1990s (or even earlier) none of the lists in queue(3) had _END(), >apart from the circle queue (for a very obvious reason it makes sense). >Now that CIRCLEQ is banned - you added _END() for LIST and TAILQ. I do >not follow the logic. :) *I* did appear do be adding it from the commit message, but in reality I was syncing with OpenBSD/FreeBSD. >I would remove _END() macros to keep the way it always was. Perhaps that's a good idea now that it is always NULL. >If the goal >is to improve the interface, then now is a good time to design a new API, >but we already have a long thread on tech-userlevel for this.. Yes, so let's leave it the way it is now, and we'll make all the changes in one go. christos
Re: CVS commit: src/sys/coda
chris...@zoulas.com (Christos Zoulas) wrote: > On Nov 23, 11:24pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote: > -- Subject: Re: CVS commit: src/sys/coda > > | mp = mp == TAILQ_END(&mountlist) ? NULL : mp; > | > | Over: > | > | return mp; > | > | Everybody understood NULL, so why obfuscate the code? > > The point (as I understand it) is so that if you want to change to another > ADT where end != NULL (are there any now, that we have banned CIRCLEQ?) > using the END macro lets you s/FOOQ/BARQ/ and have it mostly work. Since 1990s (or even earlier) none of the lists in queue(3) had _END(), apart from the circle queue (for a very obvious reason it makes sense). Now that CIRCLEQ is banned - you added _END() for LIST and TAILQ. I do not follow the logic. :) I would remove _END() macros to keep the way it always was. If the goal is to improve the interface, then now is a good time to design a new API, but we already have a long thread on tech-userlevel for this.. -- Mindaugas
Re: CVS commit: src/sys/coda
On Nov 23, 11:24pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote: -- Subject: Re: CVS commit: src/sys/coda | mp = mp == TAILQ_END(&mountlist) ? NULL : mp; | | Over: | | return mp; | | Everybody understood NULL, so why obfuscate the code? The point (as I understand it) is so that if you want to change to another ADT where end != NULL (are there any now, that we have banned CIRCLEQ?) using the END macro lets you s/FOOQ/BARQ/ and have it mostly work. christos
Re: CVS commit: src/sys/coda
chris...@zoulas.com (Christos Zoulas) wrote: > On Nov 23, 6:14pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote: > -- Subject: Re: CVS commit: src/sys/coda > > | Although this is correct, TAILQ_END(&mountlist) *suggests* access of > | mountlist. If it would be true, it would be unlocked and buggy access. > | Why not just "return mp;" ? > | > | It seems to me that TAILQ_END() makes the code more missleading without > | doing any good. What was the rationale behind adding and using it? > > I think it was provided so that the macros look the same across different > ADT's. Don't shoot the messenger, I did not add it. But I will fix the > locking non-issue... Revision 1.54 of queue.h shows you. Thanks for improving it, but I still do not see the benefit of: mp = mp == TAILQ_END(&mountlist) ? NULL : mp; Over: return mp; Everybody understood NULL, so why obfuscate the code? -- Mindaugas
Re: CVS commit: src/sys/coda
On Nov 23, 6:14pm, rm...@netbsd.org (Mindaugas Rasiukevicius) wrote: -- Subject: Re: CVS commit: src/sys/coda | Although this is correct, TAILQ_END(&mountlist) *suggests* access of | mountlist. If it would be true, it would be unlocked and buggy access. | Why not just "return mp;" ? | | It seems to me that TAILQ_END() makes the code more missleading without | doing any good. What was the rationale behind adding and using it? I think it was provided so that the macros look the same across different ADT's. Don't shoot the messenger, I did not add it. But I will fix the locking non-issue... christos
Re: CVS commit: src/sys/coda
"Christos Zoulas" wrote: > Module Name: src > Committed By: christos > Date: Sat Nov 23 17:57:23 UTC 2013 > > Modified Files: > src/sys/coda: coda_vfsops.c > > Log Message: > replace open-coded scan with macro; fix locking > -/* mount structure wasn't found */ -return(NULL); +mutex_exit(&mountlist_lock); +return mp == TAILQ_END(&mountlist) ? NULL : mp; Although this is correct, TAILQ_END(&mountlist) *suggests* access of mountlist. If it would be true, it would be unlocked and buggy access. Why not just "return mp;" ? It seems to me that TAILQ_END() makes the code more missleading without doing any good. What was the rationale behind adding and using it? -- Mindaugas
Re: CVS commit: src/sys/arch/i386/conf
On 11/23/13 4:50 AM, Marc Balmer wrote: Zitat von Jeff Rizzo : Module Name:src Committed By:riz Date:Fri Nov 22 18:58:01 UTC 2013 Modified Files: src/sys/arch/i386/conf: ALL Log Message: Comment out npf for now, as we can't have both NPF and PF in the same kernel - rmind has said he'll address this eventually, and for now PF is more likely to have unnoticed breakage. ALL now builds again! Wouldn't it make a more sense to disable pf since npf sees actual development, so testing it would actually help (and ALL is not meant to be run anyways)? To me compile testing npf would make much more sense than pf... ymmv. I did think about it, and I believe I laid out my reasoning in the commit message. +j
Re: CVS commit: src/sys/arch/i386/conf
Zitat von Jeff Rizzo : Module Name:src Committed By: riz Date: Fri Nov 22 18:58:01 UTC 2013 Modified Files: src/sys/arch/i386/conf: ALL Log Message: Comment out npf for now, as we can't have both NPF and PF in the same kernel - rmind has said he'll address this eventually, and for now PF is more likely to have unnoticed breakage. ALL now builds again! Wouldn't it make a more sense to disable pf since npf sees actual development, so testing it would actually help (and ALL is not meant to be run anyways)? To me compile testing npf would make much more sense than pf... ymmv. To generate a diff of this commit: cvs rdiff -u -r1.364 -r1.365 src/sys/arch/i386/conf/ALL Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. This message was sent using IMP, the Internet Messaging Program.
