Date: Mon, 2 Apr 2012 20:10:04 GMT From: Matthew Story <matthewstory@gmail.com> To: freebsd-standards@FreeBSD.org Subject: Re: kern/164674: vsprintf/vswprintf return error (EOF) on success if __SERR flag is already set on file Message-ID: <201204022010.q32KA4sR050121@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/164674; it has been noted by GNATS. From: Matthew Story <matthewstory@gmail.com> To: freebsd-gnats-submit@freebsd.org, freebsd-standards@freebsd.org Cc: Subject: Re: kern/164674: vsprintf/vswprintf return error (EOF) on success if __SERR flag is already set on file Date: Mon, 2 Apr 2012 16:05:24 -0400 --20cf307813d4f19f8d04bcb7b558 Content-Type: multipart/alternative; boundary=20cf307813d4f19f8804bcb7b556 --20cf307813d4f19f8804bcb7b556 Content-Type: text/plain; charset=ISO-8859-1 [excerpts from other thread] On Wed, Mar 21, 2012 at 5:45 PM, David Schultz <das@freebsd.org> wrote: > On Mon, Mar 12, 2012, Matthew Story wrote: >> On Sun, Mar 4, 2012 at 2:03 PM, John Baldwin <jhb@freebsd.org> wrote: >> >> > My standard-parsing fu is weak. Can some other folks on this list look at >> > this PR, figure out what the correct semantics for *fprintf() in this case, >> > and evaluate the patch? Thanks. > [...] >> libc vfprintf currently behaves differently on un|buffered FILEs with >> ferror set: >> >> 1. Unbuffered FILEs >> >> -> does not transmit any bytes, and returns -1 >> >> 2. Buffered FILEs (fully buffered, or line buffered) >> >> -> transmits all bytes, and returns -1 >> >> I believe that this should be reconciled such that vfprintf (and all >> derivatives) returns sucess|failure based on transmission, not taking >> ferror state into account, but an alternate solution would be to reconcile >> the behavior of buffered FILEs with the current behavior of unbuffered >> FILEs (e.g. detect ferror state, transmit no bytes and return -1). > > I agree with this assessment. When the error indicator is set on > a stream, it means that an error has occurred at some point---not > that all subsequent operations on the stream should fail. > > There ought to be a less ugly fix than the one proposed. Probably > the PRINT macro and the various other evil macros in vfprintf() > should set ret to EOF, and the following lines in vfprintf.c should > be removed: > > if (__sferror(fp)) > ret = EOF; > > If vfprintf() is fixed so that printing to a buffered stream > always returns success after a successful write (regardless of the > prior state of the stream's error indicator), that should fix the > problem for unbuffered streams automatically. Unbuffered streams > go through __sbprintf(), which throws away the output if > vfprintf() returns -1. I've followed up on David's suggestion to remove this test and set ret = EOF prior to goto error in all relevant places. I audited all cases where __SERR could potentially be set, and determined that all cases are correctly setting __SERR, returning error in the bounding function, and properly going to error within __vf(w)printf already (I have an exhaustive list of these if anyone is curious ...). Original test program in the PR works with this patch, and the regressions for stdio all function as expected. I took the liberty of cleaning up a few return -1's, in favor of return EOF's. All in all, this is much less ugly than the previous solution. patch is against head ... -- regards, matt --20cf307813d4f19f8804bcb7b556 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable [excerpts from other thread]<div><div>On Wed, Mar 21, 2012 at 5:45 PM, Davi= d Schultz <<a href=3D"mailto:das@freebsd.org">das@freebsd.org</a>> wr= ote:</div><div>> On Mon, Mar 12, 2012, Matthew Story wrote:</div><div>&g= t;> On Sun, Mar 4, 2012 at 2:03 PM, John Baldwin <<a href=3D"mailto:j= hb@freebsd.org">jhb@freebsd.org</a>> wrote:</div> <div>>></div><div>>> > My standard-parsing fu is weak. =A0Ca= n some other folks on this list look at</div><div>>> > this PR, fi= gure out what the correct semantics for *fprintf() in this case,</div><div> >> > and evaluate the patch? =A0Thanks.</div><div>> [...]</div>= <div>>> libc vfprintf currently behaves differently on un|buffered FI= LEs with</div><div>>> ferror set:</div><div>>></div><div>>&g= t; 1. Unbuffered FILEs</div> <div>>></div><div>>> =A0 =A0 =A0 -> does not transmit any by= tes, and returns -1</div><div>>></div><div>>> 2. Buffered FILEs= (fully buffered, or line buffered)</div><div>>></div><div>>> = =A0 =A0 =A0 -> transmits all bytes, and returns -1</div> <div>>></div><div>>> I believe that this should be reconciled s= uch that vfprintf (and all</div><div>>> derivatives) returns sucess|f= ailure based on transmission, not taking</div><div>>> ferror state in= to account, but an alternate solution would be to reconcile</div> <div>>> the behavior of buffered FILEs with the current behavior of u= nbuffered</div><div>>> FILEs (e.g. detect ferror state, transmit no b= ytes and return -1).</div><div>></div><div>> I agree with this assess= ment. =A0When the error indicator is set on</div> <div>> a stream, it means that an error has occurred at some point---not= </div><div>> that all subsequent operations on the stream should fail.</= div><div>></div><div>> There ought to be a less ugly fix than the one= proposed. =A0Probably</div> <div>> the PRINT macro and the various other evil macros in vfprintf()</= div><div>> should set ret to EOF, and the following lines in vfprintf.c = should</div><div>> be removed:</div><div>></div><div>> =A0 =A0 =A0= =A0if (__sferror(fp))</div> <div>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D EOF;</div><div>></div><= div>> If vfprintf() is fixed so that printing to a buffered stream</div>= <div>> always returns success after a successful write (regardless of th= e</div><div>> prior state of the stream's error indicator), that sho= uld fix the</div> <div>> problem for unbuffered streams automatically. =A0Unbuffered strea= ms</div><div>> go through __sbprintf(), which throws away the output if<= /div><div>> vfprintf() returns -1.</div><div><br></div><div>I've fol= lowed up on David's suggestion to remove this test and set ret =3D EOF = prior to goto error in all relevant places. =A0I audited all cases where __= SERR could potentially be set, and determined that all cases are correctly = setting __SERR, returning error in the bounding function, and properly goin= g to error within __vf(w)printf already (I have an exhaustive list of these= if anyone is curious ...). =A0</div> <div><br></div><div>Original test program in the PR works with this patch, = and the regressions for stdio all function as expected. =A0I took the liber= ty of cleaning up a few return -1's, in favor of return EOF's. =A0<= /div> <div><br></div><div>All in all, this is much less ugly than the previous so= lution. =A0patch is against head ...</div><br>-- <br>regards,<br>matt<br><b= r></div> --20cf307813d4f19f8804bcb7b556-- --20cf307813d4f19f8d04bcb7b558 Content-Type: text/plain; charset=US-ASCII; name="freebsd.PR164674-better.patch.txt" Content-Disposition: attachment; filename="freebsd.PR164674-better.patch.txt" Content-Transfer-Encoding: base64 X-Attachment-Id: f_h0jy3ng60 SW5kZXg6IGxpYi9saWJjL3N0ZGlvL3Zmd3ByaW50Zi5jCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGxpYi9saWJj L3N0ZGlvL3Zmd3ByaW50Zi5jCShyZXZpc2lvbiAyMzM3ODApCisrKyBsaWIvbGliYy9zdGRpby92 ZndwcmludGYuYwkod29ya2luZyBjb3B5KQpAQCAtNDQ5LDIwICs0NDksMjggQEAKIAogCS8qIEJF V0FSRSwgdGhlc2UgYGdvdG8gZXJyb3InIG9uIGVycm9yLiAqLwogI2RlZmluZQlQUklOVChwdHIs IGxlbikJZG8gewkJCVwKLQlpZiAoaW9fcHJpbnQoJmlvLCAocHRyKSwgKGxlbiksIGxvY2FsZSkp CVwKLQkJZ290byBlcnJvcjsgXAorCWlmIChpb19wcmludCgmaW8sIChwdHIpLCAobGVuKSwgbG9j YWxlKSkgewlcCisJCXJldCA9IEVPRjsJXAorCQlnb3RvIGVycm9yOwlcCisJfQlcCiB9IHdoaWxl ICgwKQogI2RlZmluZQlQQUQoaG93bWFueSwgd2l0aCkgeyBcCi0JaWYgKGlvX3BhZCgmaW8sICho b3dtYW55KSwgKHdpdGgpLCBsb2NhbGUpKSBcCi0JCWdvdG8gZXJyb3I7IFwKKwlpZiAoaW9fcGFk KCZpbywgKGhvd21hbnkpLCAod2l0aCksIGxvY2FsZSkpIHsJXAorCQlyZXQgPSBFT0Y7CVwKKwkJ Z290byBlcnJvcjsJXAorCX0JXAogfQogI2RlZmluZQlQUklOVEFORFBBRChwLCBlcCwgbGVuLCB3 aXRoKSB7CVwKLQlpZiAoaW9fcHJpbnRhbmRwYWQoJmlvLCAocCksIChlcCksIChsZW4pLCAod2l0 aCksIGxvY2FsZSkpIFwKLQkJZ290byBlcnJvcjsgXAorCWlmIChpb19wcmludGFuZHBhZCgmaW8s IChwKSwgKGVwKSwgKGxlbiksICh3aXRoKSwgbG9jYWxlKSkgewlcCisJCXJldCA9IEVPRjsJXAor CQlnb3RvIGVycm9yOwlcCisJfQlcCiB9CiAjZGVmaW5lCUZMVVNIKCkgeyBcCi0JaWYgKGlvX2Zs dXNoKCZpbywgbG9jYWxlKSkgXAotCQlnb3RvIGVycm9yOyBcCisJaWYgKGlvX2ZsdXNoKCZpbywg bG9jYWxlKSkgewlcCisJCXJldCA9IEVPRjsJXAorCQlnb3RvIGVycm9yOwlcCisJfQlcCiB9CiAK IAkvKgpAQCAtODk3LDYgKzkwNSw3IEBACiAJCQkJCWNvbnZidWYgPSBfX21ic2NvbnYobWJwLCBw cmVjKTsKIAkJCQkJaWYgKGNvbnZidWYgPT0gTlVMTCkgewogCQkJCQkJZnAtPl9mbGFncyB8PSBf X1NFUlI7CisJCQkJCQlyZXQgPSBFT0Y7CiAJCQkJCQlnb3RvIGVycm9yOwogCQkJCQl9CiAJCQkJ CWNwID0gY29udmJ1ZjsKQEAgLTEwMzAsOCArMTAzOSwxMCBAQAogCQkJLyogbGVhZGluZyB6ZXJv ZXMgZnJvbSBkZWNpbWFsIHByZWNpc2lvbiAqLwogCQkJUEFEKGRwcmVjIC0gc2l6ZSwgemVyb2Vz KTsKIAkJCWlmIChncy5ncm91cGluZykgewotCQkJCWlmIChncm91cGluZ19wcmludCgmZ3MsICZp bywgY3AsIGJ1ZitCVUYsIGxvY2FsZSkgPCAwKQorCQkJCWlmIChncm91cGluZ19wcmludCgmZ3Ms ICZpbywgY3AsIGJ1ZitCVUYsIGxvY2FsZSkgPCAwKSB7CisJCQkJCXJldCA9IEVPRjsKIAkJCQkJ Z290byBlcnJvcjsKKwkJCQl9CiAJCQl9IGVsc2UgewogCQkJCVBSSU5UKGNwLCBzaXplKTsKIAkJ CX0KQEAgLTEwNDcsMTAgKzEwNTgsMTEgQEAKIAkJCQkJcHJlYyArPSBleHB0OwogCQkJCX0gZWxz ZSB7CiAJCQkJCWlmIChncy5ncm91cGluZykgewotCQkJCQkJbiA9IGdyb3VwaW5nX3ByaW50KCZn cywgJmlvLAotCQkJCQkJICAgIGNwLCBjb252YnVmICsgbmRpZywgbG9jYWxlKTsKLQkJCQkJCWlm IChuIDwgMCkKKwkJCQkJCWlmICgobiA9IGdyb3VwaW5nX3ByaW50KCZncywgJmlvLAorCQkJCQkJ ICAgIGNwLCBjb252YnVmICsgbmRpZywgbG9jYWxlKSkgPCAwKSB7CisJCQkJCQkJcmV0ID0gRU9G OwogCQkJCQkJCWdvdG8gZXJyb3I7CisJCQkJCQl9CiAJCQkJCQljcCArPSBuOwogCQkJCQl9IGVs c2UgewogCQkJCQkJUFJJTlRBTkRQQUQoY3AsIGNvbnZidWYgKyBuZGlnLApAQCAtMTA4OSw4ICsx MTAxLDYgQEAKIAl2YV9lbmQob3JnYXApOwogCWlmIChjb252YnVmICE9IE5VTEwpCiAJCWZyZWUo Y29udmJ1Zik7Ci0JaWYgKF9fc2ZlcnJvcihmcCkpCi0JCXJldCA9IEVPRjsKIAlpZiAoKGFyZ3Rh YmxlICE9IE5VTEwpICYmIChhcmd0YWJsZSAhPSBzdGF0YXJndGFibGUpKQogCQlmcmVlIChhcmd0 YWJsZSk7CiAJcmV0dXJuIChyZXQpOwpJbmRleDogbGliL2xpYmMvc3RkaW8vdmZwcmludGYuYwo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBsaWIvbGliYy9zdGRpby92ZnByaW50Zi5jCShyZXZpc2lvbiAyMzM3ODAp CisrKyBsaWIvbGliYy9zdGRpby92ZnByaW50Zi5jCSh3b3JraW5nIGNvcHkpCkBAIC0xMjcsNyAr MTI3LDcgQEAKIAljb25zdCBDSEFSICpjcDAgPSBjcDsKIAogCWlmIChpb19wcmludGFuZHBhZChp b3AsIGNwLCBlcCwgZ3MtPmxlYWQsIHplcm9lcywgbG9jYWxlKSkKLQkJcmV0dXJuICgtMSk7CisJ CXJldHVybiAoRU9GKTsKIAljcCArPSBncy0+bGVhZDsKIAl3aGlsZSAoZ3MtPm5zZXBzID4gMCB8 fCBncy0+bnJlcGVhdHMgPiAwKSB7CiAJCWlmIChncy0+bnJlcGVhdHMgPiAwKQpAQCAtMTM3LDkg KzEzNyw5IEBACiAJCQlncy0+bnNlcHMtLTsKIAkJfQogCQlpZiAoaW9fcHJpbnQoaW9wLCBncy0+ dGhvdXNhbmRzX3NlcCwgZ3MtPnRob3VzZXBfbGVuLCBsb2NhbGUpKQotCQkJcmV0dXJuICgtMSk7 CisJCQlyZXR1cm4gKEVPRik7CiAJCWlmIChpb19wcmludGFuZHBhZChpb3AsIGNwLCBlcCwgKmdz LT5ncm91cGluZywgemVyb2VzLCBsb2NhbGUpKQotCQkJcmV0dXJuICgtMSk7CisJCQlyZXR1cm4g KEVPRik7CiAJCWNwICs9ICpncy0+Z3JvdXBpbmc7CiAJfQogCWlmIChjcCA+IGVwKQpAQCAtMzY5 LDIwICszNjksMjggQEAKIAogCS8qIEJFV0FSRSwgdGhlc2UgYGdvdG8gZXJyb3InIG9uIGVycm9y LiAqLwogI2RlZmluZQlQUklOVChwdHIsIGxlbikgeyBcCi0JaWYgKGlvX3ByaW50KCZpbywgKHB0 ciksIChsZW4pLCBsb2NhbGUpKQlcCi0JCWdvdG8gZXJyb3I7IFwKKwlpZiAoaW9fcHJpbnQoJmlv LCAocHRyKSwgKGxlbiksIGxvY2FsZSkpIHsJXAorCQlyZXQgPSBFT0Y7CVwKKwkJZ290byBlcnJv cjsJXAorCX0JXAogfQogI2RlZmluZQlQQUQoaG93bWFueSwgd2l0aCkgeyBcCi0JaWYgKGlvX3Bh ZCgmaW8sIChob3dtYW55KSwgKHdpdGgpLCBsb2NhbGUpKSBcCisJaWYgKGlvX3BhZCgmaW8sICho b3dtYW55KSwgKHdpdGgpLCBsb2NhbGUpKSB7CVwKKwkJcmV0ID0gRU9GOwlcCiAJCWdvdG8gZXJy b3I7IFwKKwl9CVwKIH0KICNkZWZpbmUJUFJJTlRBTkRQQUQocCwgZXAsIGxlbiwgd2l0aCkgewlc Ci0JaWYgKGlvX3ByaW50YW5kcGFkKCZpbywgKHApLCAoZXApLCAobGVuKSwgKHdpdGgpLCBsb2Nh bGUpKSBcCi0JCWdvdG8gZXJyb3I7IFwKKwlpZiAoaW9fcHJpbnRhbmRwYWQoJmlvLCAocCksIChl cCksIChsZW4pLCAod2l0aCksIGxvY2FsZSkpIHsJXAorCQlyZXQgPSBFT0Y7CVwKKwkJZ290byBl cnJvcjsJXAorCX0JXAogfQogI2RlZmluZQlGTFVTSCgpIHsgXAotCWlmIChpb19mbHVzaCgmaW8s IGxvY2FsZSkpIFwKKwlpZiAoaW9fZmx1c2goJmlvLCBsb2NhbGUpKSB7CVwKKwkJcmV0ID0gRU9G OwlcCiAJCWdvdG8gZXJyb3I7IFwKKwl9CVwKIH0KIAogCS8qCkBAIC02MTcsNiArNjI1LDcgQEAK IAkJCQkgICAgKHdjaGFyX3QpR0VUQVJHKHdpbnRfdCksICZtYnMpOwogCQkJCWlmIChtYnNlcWxl biA9PSAoc2l6ZV90KS0xKSB7CiAJCQkJCWZwLT5fZmxhZ3MgfD0gX19TRVJSOworCQkJCQlyZXQg PSBFT0Y7CiAJCQkJCWdvdG8gZXJyb3I7CiAJCQkJfQogCQkJCXNpemUgPSAoaW50KW1ic2VxbGVu OwpAQCAtODI4LDYgKzgzNyw3IEBACiAJCQkJCWNvbnZidWYgPSBfX3djc2NvbnYod2NwLCBwcmVj KTsKIAkJCQkJaWYgKGNvbnZidWYgPT0gTlVMTCkgewogCQkJCQkJZnAtPl9mbGFncyB8PSBfX1NF UlI7CisJCQkJCQlyZXQgPSBFT0Y7CiAJCQkJCQlnb3RvIGVycm9yOwogCQkJCQl9CiAJCQkJCWNw ID0gY29udmJ1ZjsKQEAgLTk2Miw4ICs5NzIsMTAgQEAKIAkJCS8qIGxlYWRpbmcgemVyb2VzIGZy b20gZGVjaW1hbCBwcmVjaXNpb24gKi8KIAkJCVBBRChkcHJlYyAtIHNpemUsIHplcm9lcyk7CiAJ CQlpZiAoZ3MuZ3JvdXBpbmcpIHsKLQkJCQlpZiAoZ3JvdXBpbmdfcHJpbnQoJmdzLCAmaW8sIGNw LCBidWYrQlVGLCBsb2NhbGUpIDwgMCkKKwkJCQlpZiAoZ3JvdXBpbmdfcHJpbnQoJmdzLCAmaW8s IGNwLCBidWYrQlVGLCBsb2NhbGUpIDwgMCkgeworCQkJCQlyZXQgPSBFT0Y7CiAJCQkJCWdvdG8g ZXJyb3I7CisJCQkJfQogCQkJfSBlbHNlIHsKIAkJCQlQUklOVChjcCwgc2l6ZSk7CiAJCQl9CkBA IC05NzksMTAgKzk5MSwxMSBAQAogCQkJCQlwcmVjICs9IGV4cHQ7CiAJCQkJfSBlbHNlIHsKIAkJ CQkJaWYgKGdzLmdyb3VwaW5nKSB7Ci0JCQkJCQluID0gZ3JvdXBpbmdfcHJpbnQoJmdzLCAmaW8s Ci0JCQkJCQkgICAgY3AsIGR0b2FlbmQsIGxvY2FsZSk7Ci0JCQkJCQlpZiAobiA8IDApCisJCQkJ CQlpZiAoKG4gPSBncm91cGluZ19wcmludCgmZ3MsICZpbywKKwkJCQkJCSAgICBjcCwgZHRvYWVu ZCwgbG9jYWxlKSkgPCAwKSB7CisJCQkJCQkJcmV0ID0gRU9GOwogCQkJCQkJCWdvdG8gZXJyb3I7 CisJCQkJCQl9CiAJCQkJCQljcCArPSBuOwogCQkJCQl9IGVsc2UgewogCQkJCQkJUFJJTlRBTkRQ QUQoY3AsIGR0b2FlbmQsCkBAIC0xMDI0LDggKzEwMzcsNiBAQAogI2VuZGlmCiAJaWYgKGNvbnZi dWYgIT0gTlVMTCkKIAkJZnJlZShjb252YnVmKTsKLQlpZiAoX19zZmVycm9yKGZwKSkKLQkJcmV0 ID0gRU9GOwogCWlmICgoYXJndGFibGUgIT0gTlVMTCkgJiYgKGFyZ3RhYmxlICE9IHN0YXRhcmd0 YWJsZSkpCiAJCWZyZWUgKGFyZ3RhYmxlKTsKIAlyZXR1cm4gKHJldCk7Cg== --20cf307813d4f19f8d04bcb7b558--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204022010.q32KA4sR050121>