caesar(6) documents incorrect frequencies
The man page documents frequencies that are different than the code uses e.g. C (3.61 vs 2.7) and D (4.78 vs 3.8). This seems a bit much for a man page. If anyone prefers the letter ordering be kept, the correct order is ETSAORINDHLCPMUYFWGBVKXQZJ . - Matthew Martin diff --git caesar.6 caesar.6 index 9dc040a7a6d..889f24c6548 100644 --- caesar.6 +++ caesar.6 @@ -61,16 +61,3 @@ and in some of the databases used by the program to .Dq disguise their content. -.Pp -The frequency (from most common to least) of English letters is as follows: -.Bd -filled -offset indent -ETAONRISHDLFCMUGPYWBVKXJQZ -.Ed -.Pp -Their frequencies as a percentage are as follows: -.Bd -filled -offset indent -E(13), T(10.5), A(8.1), O(7.9), N(7.1), R(6.8), I(6.3), S(6.1), H(5.2), -D(3.8), L(3.4), F(2.9), C(2.7), M(2.5), U(2.4), G(2), -P(1.9), Y(1.9), -W(1.5), B(1.4), V(.9), K(.4), X(.15), J(.13), Q(.11), Z(.07). -.Ed
Re: printf(3) return value on ENOMEM
Here's my take. Internally if a intentional errno is produced, the functions should cease motion and return -1 to indicate error. However, these functions should probably guard against unintentional errno changes. Using save_errno method. I thought snprintf should maybe be a little different. I wondered if it should still accumulate an "usage estimate" in this case. It does not need to malloc, because the storage buffer is provided. Maybe that case already works out fine. Years ago I made positional arguments signal-handler safe using mmap. I really hope this doesn't mean snprintf has another late-allocation circumstance which uses signal-unsafe malloc -- that would suck. Recently we use dprintf in signal handlers. I hope it is safe, and doesn't need to malloc transient data.
Re: printf(3) return value on ENOMEM
> yeah. the number of bytes returned seems like a mistake in the api design. sorry, but that comes off like a clever soundbite. the return value informs about the expansion size after the format strings processing, and i am sure someone has used that information in a place where it was useful. especially in the world before snprintf arrived [Torek, I'd guess around 1998?], after that you could snprintf / asprintf, and then fwrite, and know the size. I'd like to point out this API is probably older than you, and I've never read this type of criticism before. > there is almost nothing one can do with this information. I'm sure someone in the past has been happy to know the expansion size. i'm sure there are purposes for knowing it. 'retrying write' isn't the only possible reason. furthermore, 2 of the 3 errno *printf were only intruduced in the last 15 years, and I doubt ENOMEM was well documented before that time. > i mean, what? only in the case of snprintf can the return be used, and the > idea of "short" write there is only harmful. suspect you described that wrong. the return value from snprintf and asprintf have been used throughout the tree, and if anything the addition of -1 / EILSEQ by solaris has made things tricker. > returning -1 to indicate error, ignoring the possibility of short > output, seems like the option that results in less damage. as an > application author, it's the only behavior i can reasonably code > against. I don't believe that. It may be possible to come to a conclusion like that, if review of a large body of code found at least a few checks for -1 / ENOMEM, or just -1 on it's own. If no old instances are found, is that a case of people not being reasonable application authors? No, I think it is just a gap -- really nothing new.
Re: printf(3) return value on ENOMEM
Ingo Schwarze wrote: > So i say in all cases above, return -1, set ENOMEM, and it doesn't > matter much whether anything is printed, except that asprintf(3) > must of course free(3) any allocated memory before returning and > set the pointer to NULL. yeah. the number of bytes returned seems like a mistake in the api design. there is almost nothing one can do with this information. unlike write(), you can't call printf again after incrementing the pointer. while (n < whatever) n += printf(format + n, args); i mean, what? only in the case of snprintf can the return be used, and the idea of "short" write there is only harmful. returning -1 to indicate error, ignoring the possibility of short output, seems like the option that results in less damage. as an application author, it's the only behavior i can reasonably code against.
vmd: reset queue_size if queue_select is invalid
hello tech@, here is a diff that will follow the virtio spec a little closer, and allows 9front's (http://9front.org) virtio-blk driver to correctly find the number of queues. i know that virtio-blk only has one queue, but the virtio probing code is shared between virtio-blk and virtio-scsi. without this change, the size of the first queue is used for all subsequently probed queues. for completeness i've changed rng and net to do the same as blk. some bits from the spec: 4.1.4.3.1 - "The device MUST present a 0 in queue_size if the virtqueue corresponding to the current queue_select is unavailable." 4.1.5.1.3 - "Write the virtqueue index (first queue is 0) to queue_select. Read the virtqueue size from queue_size. This controls how big the virtqueue is (see 2.4 Virtqueues). If this field is 0, the virtqueue does not exist." Index: virtio.c === RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v retrieving revision 1.49 diff -u -p -u -p -r1.49 virtio.c --- virtio.c30 May 2017 17:56:47 - 1.49 +++ virtio.c27 Jul 2017 04:35:46 - @@ -150,8 +150,10 @@ void viornd_update_qs(void) { /* Invalid queue? */ - if (viornd.cfg.queue_select > 0) + if (viornd.cfg.queue_select > 0) { + viornd.cfg.queue_size = 0; return; + } /* Update queue address/size based on queue select */ viornd.cfg.queue_address = viornd.vq[viornd.cfg.queue_select].qa; @@ -324,8 +326,10 @@ void vioblk_update_qs(struct vioblk_dev *dev) { /* Invalid queue? */ - if (dev->cfg.queue_select > 0) + if (dev->cfg.queue_select > 0) { + dev->cfg.queue_size = 0; return; + } /* Update queue address/size based on queue select */ dev->cfg.queue_address = dev->vq[dev->cfg.queue_select].qa; @@ -1037,8 +1041,10 @@ void vionet_update_qs(struct vionet_dev *dev) { /* Invalid queue? */ - if (dev->cfg.queue_select > 1) + if (dev->cfg.queue_select > 1) { + dev->cfg.queue_size = 0; return; + } /* Update queue address/size based on queue select */ dev->cfg.queue_address = dev->vq[dev->cfg.queue_select].qa;
Re: printf(3) return value on ENOMEM
Hi Theo, Theo de Raadt wrote on Wed, Jul 26, 2017 at 08:07:53AM -0600: > Ingo Schwarze wrote: >> The current behaviour of our implementation is to return the number >> of characters printed *and* set errno = ENOMEM. > I expect it should not set errno. As a general rule, errno should > only be set if an error has been indicated. Other short operations > don't set errno. Ooops, i overlooked the last sentence, sorry. Some *do* set errno. For example, the PRINT() macro calls __sprint() which calls __sfvwrite() in fvwrite.c which contains: _base = recallocarray(fp->_bf._base, fp->_bf._size + 1, _size + 1, 1); if (_base == NULL) goto err; and w = (*fp->_write)(fp->_cookie, p, w); if (w <= 0) goto err; and err: fp->_flags |= __SERR; return (EOF); and then PRINT() does if (__sprint(fp, &uio)) \ goto error; \ error: va_end(orgap); if (__sferror(fp)) ret = -1; goto finish; And invalid multibyte sequences in the format string cause short operations, returning -1 and setting EILSEQ. Same for invalid wide character codes in %lc and %ls arguments. __find_arguments() in GETASTER() is yet another example of a case that can cause a short operation by mmap(2) failure, returning -1 and setting errno. Looking through the code, i failed to find any case of a short operation that allows printf to still succeed apart from the four dtoa() instances we are discussing right now, and none at all that do not set errno. (Not absolutely sure because the code is of substantial size.) So not only does errno get set on typical short operations, but -1 gets returned as well, both for malloc(3) and write(3) failure and EILSEQ and EOVERFLOW, even if something was already written earlier. That seems like yet another argument to always return -1 on malloc(3) failure, answering the good question that kettenis@ asked: Should *printf() fail or succeed? I say, fail. Yours, Ingo
calendar vs KOI8
Is 5.9 out yet? Index: io.c === RCS file: /cvs/src/usr.bin/calendar/io.c,v retrieving revision 1.44 diff -u -p -r1.44 io.c --- io.c31 Aug 2016 09:38:47 - 1.44 +++ io.c26 Jul 2017 20:21:09 - @@ -89,13 +89,9 @@ cal(void) if (strncmp(buf, "LANG=", 5) == 0) { (void) setlocale(LC_ALL, buf + 5); setnnames(); - /* XXX remove KOI8 lines after 5.9 is out */ if (!strcmp(buf + 5, "ru_RU.UTF-8") || !strcmp(buf + 5, "uk_UA.UTF-8") || - !strcmp(buf + 5, "by_BY.UTF-8") || - !strcmp(buf + 5, "ru_RU.KOI8-R") || - !strcmp(buf + 5, "uk_UA.KOI8-U") || - !strcmp(buf + 5, "by_BY.KOI8-B")) { + !strcmp(buf + 5, "by_BY.UTF-8")) { bodun_maybe++; bodun = 0; free(prefix);
sys/net/rtsock.c: typo in comment
Hi, Looks like a typo to me. Comments? OK? Index: rtsock.c === RCS file: /cvs/src/sys/net/rtsock.c,v retrieving revision 1.241 diff -u -p -r1.241 rtsock.c --- rtsock.c24 Jul 2017 09:20:32 - 1.241 +++ rtsock.c26 Jul 2017 20:18:04 - @@ -764,7 +764,7 @@ rtm_output(struct rt_msghdr *rtm, struct /* * We cannot go through a delete/create/insert cycle for * cached route because this can lead to races in the -* receive path. Instead we upade the L2 cache. +* receive path. Instead we update the L2 cache. */ if ((rt != NULL) && ISSET(rt->rt_flags, RTF_CACHED)) goto change;
LC_NUMERIC in awk
Does awk really need to set and reset LC_NUMERIC? Does it need to set locale at all? Jan Index: main.c === RCS file: /cvs/src/usr.bin/awk/main.c,v retrieving revision 1.19 diff -u -p -r1.19 main.c --- main.c 22 Oct 2015 04:08:17 - 1.19 +++ main.c 26 Jul 2017 20:15:48 - @@ -28,7 +28,6 @@ const char*version = "version 20110810" #define DEBUG #include #include -#include #include #include #include @@ -61,9 +60,6 @@ int main(int argc, char *argv[]) { const char *fs = NULL; - setlocale(LC_ALL, ""); - setlocale(LC_NUMERIC, "C"); /* for parsing cmdline & prog */ - if (pledge("stdio rpath wpath cpath proc exec", NULL) == -1) { fprintf(stderr, "%s: pledge: incorrect arguments\n", cmdname); @@ -185,7 +181,6 @@ int main(int argc, char *argv[]) if (!safe) envinit(environ); yyparse(); - setlocale(LC_NUMERIC, ""); /* back to whatever it is locally */ if (fs) *FS = qstring(fs, '\0'); dprintf( ("errorflag=%d\n", errorflag) );
Re: printf(3) return value on ENOMEM
Hi, now we have conflicting and incomplete opinions. What should "prefix %.500f postfix", 1.0 and "%s %.500f %s", "prefix", 1.0, "postfix" do if the %f fails with ENOMEM? Currently, 1. [f]printf(..., "prefix %.500f postfix", 1.0) prints nothing, returns 7, sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should print "prefix " and return 7? 2. [f]printf(..., "%s %.500f %s", "prefix", 1.0, "postfix") prints "prefix", returns 7 (sic!), sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should print "prefix " (one more blank) and return 7? 3. snprintf(..., "prefix %.500f postfix", 1.0) prints nothing, returns 7, sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should print "prefix " and return 7? 4. snprintf(..., "%s %.500f %s", "prefix", 1.0, "postfix") prints "prefix", returns 7 (sic!), sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should print "prefix " (one more blank) and return 7? 5. asprintf(..., "prefix %.500f postfix", 1.0) allocates "" (sic!), returns 7, sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should allocate "prefix " and return 7? millert@ says it should return -1 and set ENOMEM. I assume he means it should not allocate anything. 6. asprintf(..., "%s %.500f %s", "prefix", 1.0, "postfix") allocates "prefix", returns 7 (sic!), sets ENOMEM. deraadt@ says it should preserve errno. I assume he means it should print "prefix " (one more blank) and return 7? millert@ says it should return -1 and set ENOMEM. I assume he means it should not allocate anything. 7. printf("%.500f postfix", 1.0); prints nothing, returns 0, sets ENOMEM. So this reports partial success of printing zero bytes. That doesn't make sense to me either. I certainly agree with deraadt@ that we should never clobber errno, even though kettenis@ may be right that POSIX does not forbid it: being more careful than POSIX makes sense in this case. But i disagree with deraadt@ and agree with millert@ that we should return failure (-1) on *any* ENOMEM. Even if something was already printed. Even in the case of snprintf(3). Even though POSIX does not allow snprintf(3) to fail with ENOMEM, i have no idea how to implement that (with correct, untruncated results, and in particular the correct return value of the length that would actually be required if memory were unlimited). I think that sprintf(3) should better fail than produce wrong results (in particular a deceivingly small return value), and when it fails, i see no guarantee that the buffer content must remain untouched. So i say in all cases above, return -1, set ENOMEM, and it doesn't matter much whether anything is printed, except that asprintf(3) must of course free(3) any allocated memory before returning and set the pointer to NULL. Once we reach consensus, i'll implement that. A test program is appended. Yours, Ingo OpenBSD results: printf literal: >>><<< ret = 7 errno = 12 printf %s:>>>prefix<<< ret = 7 errno = 12 snprintf literal: >>><<< ret = 7 errno = 12 snprintf %s: >>>prefix<<< ret = 7 errno = 12 asprintf literal: >>><<< ret = 7 errno = 12 asprintf %s: >>>prefix<<< ret = 7 errno = 12 printf %f first: >>><<< ret = 0 errno = 12 glibc results: printf literal: >>>prefix <<< ret = -1 errno = 12 printf %s:>>>prefix <<< ret = -1 errno = 12 snprintf literal: >>>prefix <<< ret = -1 errno = 12 snprintf %s: >>>prefix <<< ret = -1 errno = 12 asprintf literal: >>>(null)<<< ret = -1 errno = 12 asprintf %s: >>>(null)<<< ret = -1 errno = 12 printf %f first: >>><<< ret = -1 errno = 12 Solaris 11: printf and fprintf never seem to fail from ENOMEM and happily print five million zeros with %.500f even with all rlimits clamped down. asprintf simply segfaults on %f ENOMEM. With my patch: printf literal: >>><<< ret = -1 errno = 12 printf %s:>>><<< ret = -1 errno = 12 snprintf literal: >>><<< ret = -1 errno = 12 snprintf %s: >>>prefix<<< ret = -1 errno = 12 asprintf literal: >>>(null)<<< ret = -1 errno = 12 asprintf %s: >>>(null)<<< ret = -1 errno = 12 printf %f first: >>><<< ret = -1 errno = 12 The reason why the "printf %s" output changes is that there is yet another layer of buffering in our code even for _IONBF. __vfprintf() sets up a temporary buffer with __sbprintf(), which gets printed for ret >= 0 but does not get printed for ret = -1, see vfprintf.c line 141. #include #include #include #include #include int main(int argc, char *argv[]) { char buf[128]; struct rlimit limit; char *cp; int ret; setvbuf(stdout, NULL, _IONBF, 0); if (getrlimit(RLIMIT_DATA, &limit) < 0) err(1, "getrlimit"); if (limit.rlim_max == RLIM
ioctl under route promise for pledging snmpd
snmpe calls kif_update on an interface change which performs an ioctl with SIOCGIFDESCR, currently disallowed by pledge. No other network daemons do this. The only other programs that make this call appear to be ifconfig and systat. ifnet.if_description simply contains an optional user defined interface description. vmd performs an ioctl with SIOCSIFDESCR to set ifnet.if_description, and this is done in a privileged process that is not pledged. The following diff proposal allows for an ioctl on SIOCGIFDESCR under a route promise. Thoughts? Rob Index: kern_pledge.c === RCS file: /cvs/src/sys/kern/kern_pledge.c,v retrieving revision 1.216 diff -u -p -r1.216 kern_pledge.c --- kern_pledge.c 29 Jun 2017 04:10:07 - 1.216 +++ kern_pledge.c 26 Jul 2017 18:14:04 - @@ -1305,6 +1305,7 @@ pledge_ioctl(struct proc *p, long com, s if ((p->p_p->ps_pledge & PLEDGE_ROUTE)) { switch (com) { case SIOCGIFADDR: + case SIOCGIFDESCR: case SIOCGIFFLAGS: case SIOCGIFMETRIC: case SIOCGIFGMEMB:
[patch/route] Allow short commands
Hi, I use route(8) a lot and I thought being able to use shorter commands/keywords could be nice. Like : route a default 192.0.2.1 route del default Regards, Denis Index: route.c === RCS file: /cvs/src/sbin/route/route.c,v retrieving revision 1.200 diff -u -p -r1.200 route.c --- route.c 23 Mar 2017 13:28:25 - 1.200 +++ route.c 26 Jul 2017 16:34:43 - @@ -1864,7 +1864,10 @@ bprintf(FILE *fp, int b, char *s) int keycmp(const void *key, const void *kt) { - return (strcmp(key, ((struct keytab *)kt)->kt_cp)); + size_t wordlen = 0; + + wordlen = strlen(key); + return (strncmp(key, ((struct keytab *)kt)->kt_cp, wordlen)); } int
Re: whois(1): follow ICANN change to field names
On 2017/07/26 09:24, Todd C. Miller wrote: > On Wed, 26 Jul 2017 17:19:42 +0200, Jeremie Courreges-Anglas wrote: > > > On Wed, Jul 26 2017, Stuart Henderson wrote: > > > the > > > https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-e > > n > > > changes have gone live (at least for com/net), so whois(1) no longer > > > chases > > > referrals. OK to change the string to the new one? > > > > Would it make sense to keep looking for "Whois Server:" but use > > strcasestr(3) instead, to support both key names? > > Can you find any server still using the old name? I could not. If there are, I don't think they will last for long, the icann document says "Effective Date: 1 August 2017".
Re: whois(1): follow ICANN change to field names
On Wed, Jul 26 2017, "Todd C. Miller" wrote: > On Wed, 26 Jul 2017 17:19:42 +0200, Jeremie Courreges-Anglas wrote: > >> On Wed, Jul 26 2017, Stuart Henderson wrote: >> > the https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-e >> n >> > changes have gone live (at least for com/net), so whois(1) no longer chases >> > referrals. OK to change the string to the new one? >> >> Would it make sense to keep looking for "Whois Server:" but use >> strcasestr(3) instead, to support both key names? > > Can you find any server still using the old name? I could not. I don't know; maybe Stuart does. The diff looks fine to me and indeed fixes referrals. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: whois(1): follow ICANN change to field names
On Wed, 26 Jul 2017 17:19:42 +0200, Jeremie Courreges-Anglas wrote: > On Wed, Jul 26 2017, Stuart Henderson wrote: > > the https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-e > n > > changes have gone live (at least for com/net), so whois(1) no longer chases > > referrals. OK to change the string to the new one? > > Would it make sense to keep looking for "Whois Server:" but use > strcasestr(3) instead, to support both key names? Can you find any server still using the old name? I could not. - todd
Re: whois(1): follow ICANN change to field names
On Wed, Jul 26 2017, Stuart Henderson wrote: > the https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-en > changes have gone live (at least for com/net), so whois(1) no longer chases > referrals. OK to change the string to the new one? Would it make sense to keep looking for "Whois Server:" but use strcasestr(3) instead, to support both key names? > diff --git usr.bin/whois/whois.c usr.bin/whois/whois.c > index 907d102b2f8..0e608295edf 100644 > --- usr.bin/whois/whois.c > +++ usr.bin/whois/whois.c > @@ -62,7 +62,7 @@ > #define QNICHOST_TAIL ".whois-servers.net" > > #define WHOIS_PORT "whois" > -#define WHOIS_SERVER_ID "Whois Server:" > +#define WHOIS_SERVER_ID "Registrar WHOIS Server:" > > #define WHOIS_RECURSE0x01 > #define WHOIS_QUICK 0x02 > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: whois(1): follow ICANN change to field names
On Wed, 26 Jul 2017 15:43:38 +0100, Stuart Henderson wrote: > the https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-en > changes have gone live (at least for com/net), so whois(1) no longer chases > referrals. OK to change the string to the new one? OK millert@ - todd
Re: printf(3) return value on ENOMEM
On Wed, 26 Jul 2017 12:10:53 +0200, Ingo Schwarze wrote: > As related data points, for EOVERFLOW, we do always return -1, > and for EILSEQ, we changed the code some time ago to return -1 - > even though in both of these cases, it is not completely obvious > whether those should be considered "output errors" in the POSIX > sense. > > For ENOMEM, both glibc and Solaris 11 return -1 according to my > testing, and NetBSD does the same according to code inspection. In > FreeBSD, my impression is that dtoa() uses malloc(3), too, but i > failed to find any error handling code, so i guess they chose to > simply segfault - not sure, though. > > > In summary, i think we ought to return -1. > > It's the only option that allows a sane usage pattern (and in > particular the one that people *are* actually using, if they check > for errors at all), POSIX at least doesn't forbid it, and most > others seem to do it, too. I agree. People assume that asprintf() will return -1 on malloc failure. Doing anything else is going to create subtle bugs. - todd
whois(1): follow ICANN change to field names
the https://www.icann.org/resources/pages/rdds-labeling-policy-2017-02-01-en changes have gone live (at least for com/net), so whois(1) no longer chases referrals. OK to change the string to the new one? diff --git usr.bin/whois/whois.c usr.bin/whois/whois.c index 907d102b2f8..0e608295edf 100644 --- usr.bin/whois/whois.c +++ usr.bin/whois/whois.c @@ -62,7 +62,7 @@ #defineQNICHOST_TAIL ".whois-servers.net" #defineWHOIS_PORT "whois" -#defineWHOIS_SERVER_ID "Whois Server:" +#defineWHOIS_SERVER_ID "Registrar WHOIS Server:" #define WHOIS_RECURSE 0x01 #define WHOIS_QUICK0x02
Re: printf(3) return value on ENOMEM
> > From: "Theo de Raadt" > > Date: Wed, 26 Jul 2017 08:07:53 -0600 > > > > > The current behaviour of our implementation is to return the number > > > of characters printed *and* set errno = ENOMEM. > > > > I expect it should not set errno. As a general rule, errno should > > only be set if an error has been indicated. Other short operations > > don't set errno. > > POSIX says: > > "The value of errno should only be examined when it is indicated to > be valid by a function's return value." > > So clobbering errno when not returning a negative number is allowed. I disagree. Many years ago, malloc would trash errno along the way. It was pretty disruptive, and it got fixed. save_errno changes went in throughout the tree, not just around signal handlers. We don't need more functions doing it wrong. Inverting what POSIX says, thread-local errno should not be changed unless the caller is told to examine it. It should only be changed if the caller is observing it. It is pointless to change errno if it isn't being inspected in relationship to the failed function, so now it can accidentally interferere in buggy code. How about we pick 50 libc functions, and have them set errno=EPERM even upon success. Do you think the software ecosystem would survive that? It's permitted by the rule you layed out, but I think it a vast number of bugs would surface, due to code authors inspecting errno not immediately upon error indication but later. What authors really should do in such circumstances is is assign errno to a temporary at the moment of errno notification, and inspect the temporary later on. But they won't in all cases, so bugs will surface. So unless we want to break existing code, I think my interpretation is safer: A function should only set errno if it is going to return an indicator which will cause inspection. > The real question here is if we should report (partial) success if we > encounter an error halfway through printing/formatting. Sure, but error indication should happen with return value -1.
Re: printf(3) return value on ENOMEM
> From: "Theo de Raadt" > Date: Wed, 26 Jul 2017 08:07:53 -0600 > > > The current behaviour of our implementation is to return the number > > of characters printed *and* set errno = ENOMEM. > > I expect it should not set errno. As a general rule, errno should > only be set if an error has been indicated. Other short operations > don't set errno. POSIX says: "The value of errno should only be examined when it is indicated to be valid by a function's return value." So clobbering errno when not returning a negative number is allowed. The real question here is if we should report (partial) success if we encounter an error halfway through printing/formatting.
Re: printf(3) return value on ENOMEM
> The current behaviour of our implementation is to return the number > of characters printed *and* set errno = ENOMEM. I expect it should not set errno. As a general rule, errno should only be set if an error has been indicated. Other short operations don't set errno.
Re: em link state change
wow, and ok benno@ Alexander Bluhm(alexander.bl...@gmx.net) on 2017.07.25 18:07:19 +0200: > Hi, > > The LINK_STATE_IS_UP() macro considers LINK_STATE_UNKNOWN as up. > So the em driver never gets out of that state. The change was in > sys/net/if.h > > revision 1.123 > date: 2011/07/03 17:41:50; author: claudio; state: Exp; lines: +3 -2; > LINK_STATE_IS_UP() should consider LINK_STATE_UNKNOWN as an up state. > This is now possible because carp no longer uses LINK_STATE_UNKNOWN > for a state that is considered down. This will simplify a lot of code. > OK mpf@ mcbride@ henning@ > > I have checked ix(4), bge(4), myx(4). They compare the new value > with the old. em(4) should do the same. > > ok? > > bluhm > > Index: dev/pci/if_em.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_em.c,v > retrieving revision 1.335 > diff -u -p -r1.335 if_em.c > --- dev/pci/if_em.c 19 Mar 2017 11:09:26 - 1.335 > +++ dev/pci/if_em.c 25 Jul 2017 15:37:31 - > @@ -1458,6 +1458,7 @@ void > em_update_link_status(struct em_softc *sc) > { > struct ifnet *ifp = &sc->sc_ac.ac_if; > + u_char link_state; > > if (E1000_READ_REG(&sc->hw, STATUS) & E1000_STATUS_LU) { > if (sc->link_active == 0) { > @@ -1480,11 +1481,10 @@ em_update_link_status(struct em_softc *s > sc->smartspeed = 0; > ifp->if_baudrate = IF_Mbps(sc->link_speed); > } > - if (!LINK_STATE_IS_UP(ifp->if_link_state)) { > - if (sc->link_duplex == FULL_DUPLEX) > - ifp->if_link_state = LINK_STATE_FULL_DUPLEX; > - else > - ifp->if_link_state = LINK_STATE_HALF_DUPLEX; > + link_state = (sc->link_duplex == FULL_DUPLEX) ? > + LINK_STATE_FULL_DUPLEX : LINK_STATE_HALF_DUPLEX; > + if (ifp->if_link_state != link_state) { > + ifp->if_link_state = link_state; > if_link_state_change(ifp); > } > } else { >
printf(3) return value on ENOMEM
Hi, what should printf(3) return on %e/%f/%g/%a malloc(3) failure? Neither POSIX nor our manual page seem fully conclusive. POSIX says: The fprintf() and printf() functions may fail if: [ENOMEM] Insufficient storage space is available. RETURN VALUE Upon successful completion, the fprintf() and printf() functions shall return the number of bytes transmitted. If an output error was encountered, these functions shall return a negative value and set errno to indicate the error. It is not obvious to me whether malloc(3) failure is an "output error". If not, then the return value might be unspecified for that case. Our manual page agrees with almost the same wording, so it doesn't help either. The current behaviour of our implementation is to return the number of characters printed *and* set errno = ENOMEM. In various cases, that yields really weird results. For example, printf("test %.500f", 1.0); sets ENOMEM and returns 5 but does not actually print anything because the PRINT() macro only adds "test " to the internal iov[] data structure and the FLUSH() macro does not get called before the %f bails out of the function. Even weirder, ret = asprintf(&cp, "%s%.500f", argv[1], 1.0); is equivalent, in our implementation, to ret = strlen(argv[1]); cp = strdup(argv[1]); errno = ENOMEM; so a buffer does get allocated and returned, but its content is incomplete. To use our implementation correctly, the following idiom would be required: char *s; double x; size_t minsz; intret; minsz = strlen(s) + 2; ret = asprintf(&cp, "%s%f", s, x); if (ret < 0 || ret < (int)minsz) err(1, NULL); Nobody does that. Note in particular that the "ret < 0" is required because minsz may be too large to be represented as an integer, and it is sufficient to guard the (int) cast because in that case, printf(3) returns -1/EOVERFLOW. Also note that the calculation of minsz can become arbitrarily complicated for more complicated format strings, to the point of being almost impossible. For example, for "%.1f%.1f", a return value of 6 may mean that both arguments were 1.0, or it may mean that the first one was 1.2345 and then memory was exhausted. Alternatively, you could do the "save errno, set errno = 0, call printf, inspect errno, restore errno" dance, but nobody does that either, and it would be insane. As related data points, for EOVERFLOW, we do always return -1, and for EILSEQ, we changed the code some time ago to return -1 - even though in both of these cases, it is not completely obvious whether those should be considered "output errors" in the POSIX sense. For ENOMEM, both glibc and Solaris 11 return -1 according to my testing, and NetBSD does the same according to code inspection. In FreeBSD, my impression is that dtoa() uses malloc(3), too, but i failed to find any error handling code, so i guess they chose to simply segfault - not sure, though. In summary, i think we ought to return -1. It's the only option that allows a sane usage pattern (and in particular the one that people *are* actually using, if they check for errors at all), POSIX at least doesn't forbid it, and most others seem to do it, too. What do you think? Ingo Index: stdio/vfprintf.c === RCS file: /cvs/src/lib/libc/stdio/vfprintf.c,v retrieving revision 1.77 diff -u -p -r1.77 vfprintf.c --- stdio/vfprintf.c29 Aug 2016 12:20:57 - 1.77 +++ stdio/vfprintf.c26 Jul 2017 07:29:33 - @@ -701,6 +701,7 @@ reswitch: switch (ch) { &expt, &signflag, &dtoaend); if (dtoaresult == NULL) { errno = ENOMEM; + ret = -1; goto error; } } else { @@ -710,6 +711,7 @@ reswitch: switch (ch) { &expt, &signflag, &dtoaend); if (dtoaresult == NULL) { errno = ENOMEM; + ret = -1; goto error; } } @@ -747,6 +749,7 @@ fp_begin: &expt, &signflag, &dtoaend); if (dtoaresult == NULL) { errno = ENOMEM; + ret = -1; goto error; } } else { @@ -756,6 +759,7 @@ fp_begin: &expt, &signflag, &dtoaend); if (dtoaresult == NULL) { errno = ENOMEM; + ret = -1;
Re: em link state change
On 25/07/17(Tue) 18:07, Alexander Bluhm wrote: > Hi, > > The LINK_STATE_IS_UP() macro considers LINK_STATE_UNKNOWN as up. > So the em driver never gets out of that state. The change was in > sys/net/if.h > > revision 1.123 > date: 2011/07/03 17:41:50; author: claudio; state: Exp; lines: +3 -2; > LINK_STATE_IS_UP() should consider LINK_STATE_UNKNOWN as an up state. > This is now possible because carp no longer uses LINK_STATE_UNKNOWN > for a state that is considered down. This will simplify a lot of code. > OK mpf@ mcbride@ henning@ > > I have checked ix(4), bge(4), myx(4). They compare the new value > with the old. em(4) should do the same. > > ok? Great this bug has finally been found! That mean we should be able to use rtisvalid(9) in netinet/ip_output.c without breaking naddy@'s setup. ok mpi@ > Index: dev/pci/if_em.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_em.c,v > retrieving revision 1.335 > diff -u -p -r1.335 if_em.c > --- dev/pci/if_em.c 19 Mar 2017 11:09:26 - 1.335 > +++ dev/pci/if_em.c 25 Jul 2017 15:37:31 - > @@ -1458,6 +1458,7 @@ void > em_update_link_status(struct em_softc *sc) > { > struct ifnet *ifp = &sc->sc_ac.ac_if; > + u_char link_state; > > if (E1000_READ_REG(&sc->hw, STATUS) & E1000_STATUS_LU) { > if (sc->link_active == 0) { > @@ -1480,11 +1481,10 @@ em_update_link_status(struct em_softc *s > sc->smartspeed = 0; > ifp->if_baudrate = IF_Mbps(sc->link_speed); > } > - if (!LINK_STATE_IS_UP(ifp->if_link_state)) { > - if (sc->link_duplex == FULL_DUPLEX) > - ifp->if_link_state = LINK_STATE_FULL_DUPLEX; > - else > - ifp->if_link_state = LINK_STATE_HALF_DUPLEX; > + link_state = (sc->link_duplex == FULL_DUPLEX) ? > + LINK_STATE_FULL_DUPLEX : LINK_STATE_HALF_DUPLEX; > + if (ifp->if_link_state != link_state) { > + ifp->if_link_state = link_state; > if_link_state_change(ifp); > } > } else { >