Re: CVS commit: src/lib/libwrap
Date:Thu, 10 Jan 2019 20:21:52 -0500 From:chris...@zoulas.com (Christos Zoulas) Message-ID: <20190111012152.cc59917f...@rebar.astron.com> | I understand, this is why I grepped the source for %m and saw that there | was no use other than " %m\0". The point was most that libwrap is a public library, and in its tcpd.h it declares tcpd_warn() as a public function, and notes it as __syslog_like which means that it handles %m. Anyone, anywhere (who links against libwrap) can call that function. That's why I susggested that perhaps what we should do is document the issue, so that if there is anyone who decides that they need to include a message that has something like "%d%%mbufs" that they can expect something quite different than they hoped. Similarly, if they were even just to use (for some obscure reason) two %m conversions in one message. I am not sure where that doc ought be put however, as tcpd_warn doesn't seem to be documented anywhere (than in tcpd.h). [This also applies to tcpd_jump() of course, but that one is kind of hard to imagine anyone wanting to use.] | Yes, that is something I wished for many times. But then there are | trade-offs (such as compile time checking) and other safety guarantees. Of course ... perhaps we could make it so that the act of specifying a new format conversion creates a new printf clone function (which gcc would then know nothing about ...) which also solves the "lifetime of the conversion routine" issue (the function which would be provided to handle the added format would need to exist just as long as the returned function pointer is kept alive). Then only calls which need to use the added format would use the created function, all others would just use the existing ones, with all the gcc validity checking, and only the others would use the new one and miss out ... which is certainly no worse than calls which process the format string, modifying it to insert the data wanted by the new conversion, then passing the newly constructed format to printf. kre
Re: CVS commit: src/lib/libwrap
On Jan 11, 7:39am, k...@munnari.oz.au (Robert Elz) wrote: -- Subject: Re: CVS commit: src/lib/libwrap | | PR/53851: Andreas Gustafsson: libwrap prints "m" instead of errno | | Handle %m inline if needed, otherwise vasprintf strips the %m and... | | That change handles the simple case of %m but fails on %%m (though | that is, I assume, unlikely, in libwrap error messages). This is not intended to be a general solution. In fact it is a compromise: I thought of changing all %m's to %s, strerror(errno) in the messages and I decided that it was more intrusive than the simplistic change. | The simple change, making the test be ... | | if ((ptr = strstr(fmt, "%m")) != NULL && | (ptr == fmt || ptr[-1] != '%')) | | fails on %%%m (though I would guess that is even less likely). | | There's no good way to correctly fix this using this technique, what | is needed is to do as the syslog() functions do, and actually do a | (restricted) parse of the format string. I understand, this is why I grepped the source for %m and saw that there was no use other than " %m\0". | For this though, I suspect it would be overkill. The point of this message | is just to make sure everyone is aware of this, and perhaps, if there is | some suitable place to document it, that that be done. Yes, the perfect being the enemy of the good. | Oh for the ability to simply add application specific format specifiers | to printf - I think plan9 has something along those lines. It would be | farily easy to add, if only gcc didn't go and do printf format string | analysis and complain when it sees something it does not understand. Yes, that is something I wished for many times. But then there are trade-offs (such as compile time checking) and other safety guarantees. christos
Re: CVS commit: src/lib/libwrap
Date:Thu, 10 Jan 2019 08:53:27 -0500 From:"Christos Zoulas" Message-ID: <20190110135327.189a1f...@cvs.netbsd.org> | PR/53851: Andreas Gustafsson: libwrap prints "m" instead of errno | Handle %m inline if needed, otherwise vasprintf strips the %m and... That change handles the simple case of %m but fails on %%m (though that is, I assume, unlikely, in libwrap error messages). The simple change, making the test be ... if ((ptr = strstr(fmt, "%m")) != NULL && (ptr == fmt || ptr[-1] != '%')) fails on %%%m (though I would guess that is even less likely). There's no good way to correctly fix this using this technique, what is needed is to do as the syslog() functions do, and actually do a (restricted) parse of the format string. For this though, I suspect it would be overkill. The point of this message is just to make sure everyone is aware of this, and perhaps, if there is some suitable place to document it, that that be done. Oh for the ability to simply add application specific format specifiers to printf - I think plan9 has something along those lines. It would be farily easy to add, if only gcc didn't go and do printf format string analysis and complain when it sees something it does not understand. kre
Re: CVS commit: src/sys/kern
On Thu, Jan 10, 2019 at 05:50:30PM +0100, Christoph Badura wrote: > > If you hadn't reversed the order of > > tftproot_dhcpboot() > if (md_is_root) ... > rootspec = bootspec > > that would wouldn't have need fixing because tftproot_dhcpboot() sets > md_is_root. It needed fixing because: - md0 doesn't exist before it is opened for the first time. - that's one reason why setting rootspec didn't work as intendend > Why was the order reversed anyway? Can you please explain? It's not about the order but about separating different cases. The setroot code is riddled with side effects. > Can you also please respond to the review remarks? I asked you for a review, it never appeared. > And what about the timing? Can you please explain what exactly the > idea was to jump in, make these changes, and rush in after you noticed > someone else was working on that code? That seemed pretty rude to me. > These changes don't look like they were particularly urgent. > > I ask you to kindly refrain from making uncoordinated changes to code that > other people are working on. I am working on this since a few months and committed it now before the branch to -9 after asking (several times) for a review. Greetings, -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: CVS commit: src/sys/kern
On Sat, Jan 05, 2019 at 06:03:41PM +, Michael van Elst wrote: > Modified Files: > src/sys/kern: kern_subr.c > Log Message: > Restore code to create md0, this fixes booting an INSTALL kernel. This still does not restore the original functionality that was removed without explanation or discussion. If you hadn't reversed the order of tftproot_dhcpboot() if (md_is_root) ... rootspec = bootspec that would wouldn't have need fixing because tftproot_dhcpboot() sets md_is_root. Why was the order reversed anyway? Can you please explain? Can you also please respond to the review remarks? And what about the timing? Can you please explain what exactly the idea was to jump in, make these changes, and rush in after you noticed someone else was working on that code? That seemed pretty rude to me. These changes don't look like they were particularly urgent. I ask you to kindly refrain from making uncoordinated changes to code that other people are working on. --chris
