Re: printf(3) return value on ENOMEM

[Theo de Raadt]

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

[Theo de Raadt]

> 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

[Ted Unangst]

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

[Nick Owens]

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

[Ingo Schwarze]

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, )) \
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

[Jan Stary]

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

[Anton Lindqvist]

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

[Jan Stary]

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

[Ingo Schwarze]

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, ) < 0)
err(1, "getrlimit");
if (limit.rlim_max == 

ioctl under route promise for pledging snmpd

[Rob Pierce]

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

[Denis Fondras]

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

[Stuart Henderson]

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

[Jeremie Courreges-Anglas]

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

[Todd C. Miller]

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

[Jeremie Courreges-Anglas]

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

[Todd C. Miller]

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

whois(1): follow ICANN change to field names

[Stuart Henderson]

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

[Theo de Raadt]

> > 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

[Mark Kettenis]

> 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

[Theo de Raadt]

> 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

[Sebastian Benoit]

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_ac.ac_if;
> + u_char link_state;
>  
>   if (E1000_READ_REG(>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

[Ingo Schwarze]

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(, "%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(, "%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) {
, , );
if (dtoaresult == NULL) {
errno = ENOMEM;
+   ret = -1;
goto error;
}
} else {
@@ -710,6 +711,7 @@ reswitch:   switch (ch) {
, , );
if (dtoaresult == NULL) {
errno = ENOMEM;
+   ret = -1;
goto error;
}
}
@@ -747,6 +749,7 @@ fp_begin:
, , );
if (dtoaresult == NULL) {
errno = ENOMEM;
+   ret = -1;
goto error;
}
} else {
@@ -756,6 +759,7 @@ fp_begin:
, , );
if (dtoaresult == NULL) {
errno = ENOMEM;
+   ret = -1;
goto error;
}

Re: em link state change

[Martin Pieuchot]

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_ac.ac_if;
> + u_char link_state;
>  
>   if (E1000_READ_REG(>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 {
>