[Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Sascha Silbe]

C11 allows errno to be clobbered by pretty much any library function
call, so in general callers need to take care to save errno before
calling other functions.

However, for error reporting functions this is rather awkward and can
make the code on the caller side more complicated than
necessary. error_setg_errno() already takes care of preserving errno
and some functions rely on that, so just promise that we continue to
do so in the future.

Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
---

Alternative approach to "error: error_setg_errno(): errno may be
clobbered" [1].

[1] mid:1469558699-23314-1-git-send-email-silbe@linux.vnet.ibm.com
    "[Qemu-devel] [PATCH] error: error_setg_errno(): errno may be
    clobbered" by Sascha Silbe <silbe@linux.vnet.ibm.com>, sent on
    2016-07-26.

 include/qapi/error.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 0576659..7e532d0 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
  * Just like error_setg(), with @os_error info added to the message.
  * If @os_error is non-zero, ": " + strerror(os_error) is appended to
  * the human-readable error message.
+ *
+ * The value of errno (which usually can get clobbered by almost any
+ * function call) will be preserved.
  */
 #define error_setg_errno(errp, os_error, fmt, ...)                      \
     error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
-- 
1.9.1

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Markus Armbruster]

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

> C11 allows errno to be clobbered by pretty much any library function
> call, so in general callers need to take care to save errno before
> calling other functions.
>
> However, for error reporting functions this is rather awkward and can
> make the code on the caller side more complicated than
> necessary. error_setg_errno() already takes care of preserving errno
> and some functions rely on that, so just promise that we continue to
> do so in the future.
>
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

On 07/27/2016 03:24 AM, Sascha Silbe wrote:
> C11 allows errno to be clobbered by pretty much any library function
> call, so in general callers need to take care to save errno before
> calling other functions.
> 
> However, for error reporting functions this is rather awkward and can
> make the code on the caller side more complicated than
> necessary. error_setg_errno() already takes care of preserving errno
> and some functions rely on that, so just promise that we continue to
> do so in the future.
> 
> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> ---
> 
> Alternative approach to "error: error_setg_errno(): errno may be
> clobbered" [1].

I like this alternative better.


> +++ b/include/qapi/error.h
> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>   * Just like error_setg(), with @os_error info added to the message.
>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>   * the human-readable error message.
> + *
> + * The value of errno (which usually can get clobbered by almost any
> + * function call) will be preserved.
>   */
>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \

Do we need/want to make the guarantee of preserving errno across any of
the other functions and macros declared in error.h?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/27/2016 03:24 AM, Sascha Silbe wrote:
>> C11 allows errno to be clobbered by pretty much any library function
>> call, so in general callers need to take care to save errno before
>> calling other functions.
>> 
>> However, for error reporting functions this is rather awkward and can
>> make the code on the caller side more complicated than
>> necessary. error_setg_errno() already takes care of preserving errno
>> and some functions rely on that, so just promise that we continue to
>> do so in the future.
>> 
>> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
>> ---
>> 
>> Alternative approach to "error: error_setg_errno(): errno may be
>> clobbered" [1].
>
> I like this alternative better.
>
>
>> +++ b/include/qapi/error.h
>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>   * Just like error_setg(), with @os_error info added to the message.
>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>   * the human-readable error message.
>> + *
>> + * The value of errno (which usually can get clobbered by almost any
>> + * function call) will be preserved.
>>   */
>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>
> Do we need/want to make the guarantee of preserving errno across any of
> the other functions and macros declared in error.h?

I guess we should for the ones that preserve errno, to make that
preservation actually useful.  These are: error_setv(),
error_setg_errno_internal(), error_append_hint().  Indirectly:
error_set_internal(), error_set(), error_setg_internal(), error_setg(),
error_setg_file_open_internal(), error_setg_file_open(), possibly
error_setg_win32_internal() and error_setg_win32().

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic]

On 07/28/2016 12:19 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 07/27/2016 03:24 AM, Sascha Silbe wrote:
>>> C11 allows errno to be clobbered by pretty much any library function
>>> call, so in general callers need to take care to save errno before
>>> calling other functions.
>>>
>>> However, for error reporting functions this is rather awkward and can
>>> make the code on the caller side more complicated than
>>> necessary. error_setg_errno() already takes care of preserving errno
>>> and some functions rely on that, so just promise that we continue to
>>> do so in the future.
>>>
>>> Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
>>> ---
>>>
>>> Alternative approach to "error: error_setg_errno(): errno may be
>>> clobbered" [1].
>>
>> I like this alternative better.
>>
>>
>>> +++ b/include/qapi/error.h
>>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>>   * Just like error_setg(), with @os_error info added to the message.
>>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>>   * the human-readable error message.
>>> + *
>>> + * The value of errno (which usually can get clobbered by almost any
>>> + * function call) will be preserved.
>>>   */
>>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>>
>> Do we need/want to make the guarantee of preserving errno across any of
>> the other functions and macros declared in error.h?
> 
> I guess we should for the ones that preserve errno, to make that
> preservation actually useful.  These are: error_setv(),
> error_setg_errno_internal(), error_append_hint().  Indirectly:
> error_set_internal(), error_set(), error_setg_internal(), error_setg(),
> error_setg_file_open_internal(), error_setg_file_open(), possibly
> error_setg_win32_internal() and error_setg_win32().
> 

The implementation of preserve errno seems inconsistent to me.  The
function error_setv is static, and I guess it is supposed to provide
this indirect errno preservation and is used for both error_setg und
error_setg_errno, yet error_setg_ errno_internal does extra save-restore
itself while error_setg_iternal relies on 'indirect', what is not OK in
my opinion.

As Sascha pointed out, in C11 any library functions may change errno
unless explicitly told otherwise for the particular function.  Since
start_va and end_va has nothing on preserving errno it is guaranteed by
the standard that they persevere errno, and we should assume they don't.

I could prepare a patch for this. Should I?

Halil

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

On 07/28/2016 04:46 AM, Halil Pasic wrote:

> The implementation of preserve errno seems inconsistent to me.  The
> function error_setv is static, and I guess it is supposed to provide
> this indirect errno preservation and is used for both error_setg und
> error_setg_errno, yet error_setg_ errno_internal does extra save-restore
> itself while error_setg_iternal relies on 'indirect', what is not OK in
> my opinion.

As long as errno gets saved where it is documented as saved, I don't
care whether it is direct or indirect (indirect is probably more
efficient, where we can prove that nothing is called that is allowed to
clobber errno).

> 
> As Sascha pointed out, in C11 any library functions may change errno
> unless explicitly told otherwise for the particular function.  Since
> start_va and end_va has nothing on preserving errno it is guaranteed by
> the standard that they persevere errno, and we should assume they don't.

You mean va_start, not start_va.  And actually, C11 is clear that errno
is unspecified after library functions (but not macros) that don't
explicitly state otherwise.  Since va_start() is a macro and not a
library function, that means va_start does NOT have carte blanche
permission to modify errno.  For more reading on the topic:

http://austingroupbugs.net/view.php?id=384

There are several related POSIX bug reports of other functions that have
been requested to explicitly document that they don't modify errno, and
I'm happy to submit even more, if we find other standard interfaces
whose semantics are easier when they guarantee that errno is not clobbered.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]



On 07/28/2016 04:56 PM, Eric Blake wrote:
> On 07/28/2016 04:46 AM, Halil Pasic wrote:
> 
>> The implementation of preserve errno seems inconsistent to me.  The
>> function error_setv is static, and I guess it is supposed to provide
>> this indirect errno preservation and is used for both error_setg und
>> error_setg_errno, yet error_setg_ errno_internal does extra save-restore
>> itself while error_setg_iternal relies on 'indirect', what is not OK in
>> my opinion.
> 
> As long as errno gets saved where it is documented as saved, I don't
> care whether it is direct or indirect (indirect is probably more
> efficient, where we can prove that nothing is called that is allowed to
> clobber errno).
> 

It is still inconsistent (error_setg_errno_internal and
error_setg_iternal). If it's ok for error_setg_internal to omit saving
errno before calling va_start then there is no reason to do it in
error_setg_errno_internal.

>>
>> As Sascha pointed out, in C11 any library functions may change errno
>> unless explicitly told otherwise for the particular function.  Since
>> start_va and end_va has nothing on preserving errno it is guaranteed by
>> the standard that they persevere errno, and we should assume they don't.
> 
> You mean va_start, not start_va.  And actually, C11 is clear that errno
> is unspecified after library functions (but not macros) that don't
> explicitly state otherwise.  Since va_start() is a macro and not a
> library function, that means va_start does NOT have carte blanche
> permission to modify errno.  For more reading on the topic:

I also considered this function/macro thing but in the end I am not
aware of anything in C11 what would prohibit va_start to modify errno --
correct me if I'm wrong. With that it boils down to 'may' and relying on
'does not' means you are not covered by the standard C11 (but may
be covered by something else -- in which case this should be documented
in HACKING).

> 
> http://austingroupbugs.net/view.php?id=384
>

This got rejected, or? Means that there is no willingness to introduce
this guarantee at POSIX level?
 
> There are several related POSIX bug reports of other functions that have
> been requested to explicitly document that they don't modify errno, and
> I'm happy to submit even more, if we find other standard interfaces
> whose semantics are easier when they guarantee that errno is not clobbered.
> 

Regards,
Halil



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On 07/28/2016 09:29 AM, Halil Pasic wrote:

>> You mean va_start, not start_va.  And actually, C11 is clear that errno
>> is unspecified after library functions (but not macros) that don't
>> explicitly state otherwise.  Since va_start() is a macro and not a
>> library function, that means va_start does NOT have carte blanche
>> permission to modify errno.  For more reading on the topic:
> 
> I also considered this function/macro thing but in the end I am not
> aware of anything in C11 what would prohibit va_start to modify errno --
> correct me if I'm wrong. With that it boils down to 'may' and relying on
> 'does not' means you are not covered by the standard C11 (but may
> be covered by something else -- in which case this should be documented
> in HACKING).
> 
>>
>> http://austingroupbugs.net/view.php?id=384
>>
> 
> This got rejected, or? Means that there is no willingness to introduce
> this guarantee at POSIX level?
>  

That particular bug report was rejected because the POSIX folks decided
that the C11 wording was clear enough that va_start() was already
guaranteed to not mess with errno, so no additionally wording was needed
in POSIX.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]



On 07/28/2016 11:03 PM, Eric Blake wrote:
> On 07/28/2016 09:29 AM, Halil Pasic wrote:
> 
>>> You mean va_start, not start_va.  And actually, C11 is clear that errno
>>> is unspecified after library functions (but not macros) that don't
>>> explicitly state otherwise.  Since va_start() is a macro and not a
>>> library function, that means va_start does NOT have carte blanche
>>> permission to modify errno.  For more reading on the topic:
>>
>> I also considered this function/macro thing but in the end I am not
>> aware of anything in C11 what would prohibit va_start to modify errno --
>> correct me if I'm wrong. With that it boils down to 'may' and relying on
>> 'does not' means you are not covered by the standard C11 (but may
>> be covered by something else -- in which case this should be documented
>> in HACKING).
>>
>>>
>>> http://austingroupbugs.net/view.php?id=384
>>>
>>
>> This got rejected, or? Means that there is no willingness to introduce
>> this guarantee at POSIX level?
>>  
> 
> That particular bug report was rejected because the POSIX folks decided
> that the C11 wording was clear enough that va_start() was already
> guaranteed to not mess with errno, so no additionally wording was needed
> in POSIX.
> 

Sadly, I still do not get it. I have re-read the relevant parts of N1570
and even had a conversation with the in house compiler team. The
compiler guy's opinion was also that there is no guarantee provided by
C11. In http://austingroupbugs.net/view.php?id=384 you stated in the
description that the code example provided there is not conforming. Your
last reply I read like you were wrong with that statement. I still do
not understand why were you wrong there. In fact, I could argue that you
were right, but I'm afraid the argument would be somewhat lengthy and
confusing, and I'm already feeling bad about taking so much of your time
with this. Since I'm  admittedly quite inexperienced in this field I
decided to just accept your the conclusion you and the POSIX guys
reached -- without fully understanding it.

Thanks again for your time.

Regards,
Halil



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

On 07/29/2016 08:38 AM, Halil Pasic wrote:
> 
> 
> On 07/28/2016 11:03 PM, Eric Blake wrote:
>> On 07/28/2016 09:29 AM, Halil Pasic wrote:
>>
>>>> You mean va_start, not start_va.  And actually, C11 is clear that errno
>>>> is unspecified after library functions (but not macros) that don't
>>>> explicitly state otherwise.  Since va_start() is a macro and not a
>>>> library function, that means va_start does NOT have carte blanche
>>>> permission to modify errno.  For more reading on the topic:
>>>
>>> I also considered this function/macro thing but in the end I am not
>>> aware of anything in C11 what would prohibit va_start to modify errno --
>>> correct me if I'm wrong. With that it boils down to 'may' and relying on
>>> 'does not' means you are not covered by the standard C11 (but may
>>> be covered by something else -- in which case this should be documented
>>> in HACKING).
>>>
>>>>
>>>> http://austingroupbugs.net/view.php?id=384
>>>>
>>>
>>> This got rejected, or? Means that there is no willingness to introduce
>>> this guarantee at POSIX level?
>>>  
>>
>> That particular bug report was rejected because the POSIX folks decided
>> that the C11 wording was clear enough that va_start() was already
>> guaranteed to not mess with errno, so no additionally wording was needed
>> in POSIX.
>>
> 
> Sadly, I still do not get it. I have re-read the relevant parts of N1570
> and even had a conversation with the in house compiler team. The
> compiler guy's opinion was also that there is no guarantee provided by
> C11. In http://austingroupbugs.net/view.php?id=384 you stated in the
> description that the code example provided there is not conforming.

The description was the initial claim, before I had consulted with the
rest of the Austin Group.  Yes, my initial claim was that POSIX needed
tightening to guarantee something not provided by C.

> Your
> last reply I read like you were wrong with that statement.

Correct - after consultation with the full Austin Group, my initial
claim was invalidated, and the reason it was invalidated was that the
C99 standard only permits arbitrary changes to errno after function
calls, and that the C99 definition of a function call does NOT include
macro expansions.  Therefore, POSIX does not need tightening, because
the guarantee I wanted is already present; the initial description in
that bug report is well-defined, rather than my claim of undefined.
Any compliant C implementation, and therefore all POSIX-compliant
implementations, already leave errno unchanged after any use of the
varags macros.  (The current version of POSIX is still stuck on C99,
although the Austin Group is just barely starting work to incorporate
C11 for the next version of POSIX.)

> I still do
> not understand why were you wrong there. In fact, I could argue that you
> were right, but I'm afraid the argument would be somewhat lengthy and
> confusing, and I'm already feeling bad about taking so much of your time
> with this. Since I'm  admittedly quite inexperienced in this field I
> decided to just accept your the conclusion you and the POSIX guys
> reached -- without fully understanding it.

The C99 standard is annoying in that it does not use the usual RFC
wording, so where C99 uses "may", many other standards (including POSIX)
use "shall" or even "shall only".  So the fact that C99 states that "The
value of errno may be set to nonzero by a library function call" is a
requirement that C can permit arbitrary modification of errno ONLY after
a function call, and not for any other reason (including after a macro
expansion if that macro does not expand to a documented function call).
va_start() is usually not implemented as a function call, and even if it
is, it is not a publicly documented function call.

But you are certainly welcome to add further comments to the Austin
Group bug, if you think anything was misinterpreted - in the end, the
intent IS that va_* are safe to use without arbitrary changes to errno,
and it is now just a matter of whether that intent is already met by C
wording or whether POSIX indeed needs to add an additional requirement.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Halil Pasic]

[-- Attachment #1: Type: text/plain, Size: 6242 bytes --]



On 01/09/2017 03:50 PM, Eric Blake wrote:
> On 07/29/2016 08:38 AM, Halil Pasic wrote:
>>
>>
>> On 07/28/2016 11:03 PM, Eric Blake wrote:
>>> On 07/28/2016 09:29 AM, Halil Pasic wrote:
>>>
>>>>> You mean va_start, not start_va.  And actually, C11 is clear that errno
>>>>> is unspecified after library functions (but not macros) that don't
>>>>> explicitly state otherwise.  Since va_start() is a macro and not a
>>>>> library function, that means va_start does NOT have carte blanche
>>>>> permission to modify errno.  For more reading on the topic:
>>>>
>>>> I also considered this function/macro thing but in the end I am not
>>>> aware of anything in C11 what would prohibit va_start to modify errno --
>>>> correct me if I'm wrong. With that it boils down to 'may' and relying on
>>>> 'does not' means you are not covered by the standard C11 (but may
>>>> be covered by something else -- in which case this should be documented
>>>> in HACKING).
>>>>
>>>>>
>>>>> http://austingroupbugs.net/view.php?id=384
>>>>>
>>>>
>>>> This got rejected, or? Means that there is no willingness to introduce
>>>> this guarantee at POSIX level?
>>>>  
>>>
>>> That particular bug report was rejected because the POSIX folks decided
>>> that the C11 wording was clear enough that va_start() was already
>>> guaranteed to not mess with errno, so no additionally wording was needed
>>> in POSIX.
>>>
>>
>> Sadly, I still do not get it. I have re-read the relevant parts of N1570
>> and even had a conversation with the in house compiler team. The
>> compiler guy's opinion was also that there is no guarantee provided by
>> C11. In http://austingroupbugs.net/view.php?id=384 you stated in the
>> description that the code example provided there is not conforming.
> 
> The description was the initial claim, before I had consulted with the
> rest of the Austin Group.  Yes, my initial claim was that POSIX needed
> tightening to guarantee something not provided by C.
> 
>> Your
>> last reply I read like you were wrong with that statement.
> 
> Correct - after consultation with the full Austin Group, my initial
> claim was invalidated, and the reason it was invalidated was that the
> C99 standard only permits arbitrary changes to errno after function
> calls, and that the C99 definition of a function call does NOT include
> macro expansions.  Therefore, POSIX does not need tightening, because
> the guarantee I wanted is already present; the initial description in
> that bug report is well-defined, rather than my claim of undefined.
> Any compliant C implementation, and therefore all POSIX-compliant
> implementations, already leave errno unchanged after any use of the
> varags macros.  (The current version of POSIX is still stuck on C99,
> although the Austin Group is just barely starting work to incorporate
> C11 for the next version of POSIX.)
> 
>> I still do
>> not understand why were you wrong there. In fact, I could argue that you
>> were right, but I'm afraid the argument would be somewhat lengthy and
>> confusing, and I'm already feeling bad about taking so much of your time
>> with this. Since I'm  admittedly quite inexperienced in this field I
>> decided to just accept your the conclusion you and the POSIX guys
>> reached -- without fully understanding it.
> 
> The C99 standard is annoying in that it does not use the usual RFC
> wording, so where C99 uses "may", many other standards (including POSIX)
> use "shall" or even "shall only".  So the fact that C99 states that "The
> value of errno may be set to nonzero by a library function call" is a
> requirement that C can permit arbitrary modification of errno ONLY after
> a function call, and not for any other reason (including after a macro

Thanks for the clarification. As a non-native speaker I find that usage
of "may" highly non-intuitive. Especially since in chapter 4.
"Conformance" (from n1124) does define how "shall" and "shall not" but
there is nothing on "may".

This way of saying macros expand to stuff that does not touch errno is
IMHO quite unfriendly (if this was really the intention - I think it is
quite likely that it was), and IMHO a more straight forward formulation
would benefit the standard.

> expansion if that macro does not expand to a documented function call).

It's clear that macro expansion itself does not modify program state, so
the question is to what is a library macro allowed to expand to.

> va_start() is usually not implemented as a function call, and even if it
> is, it is not a publicly documented function call.

IMHO whether it is implemented as a function (call) or a macro is irrelevant
here. Apparently library functions may be additionally implemented as a macro,
and library macros may be implemented as functions (7.2 "The assert macro
shall  be  implemented  as  a  macro,  not  as  an  actual  function").

Library macros can expand to stuff calling documented library functions
(7.2.1.1 "It then calls the abort function."), but this is also
irrelevant if we interpret that "may" as you explained. 


> 
> But you are certainly welcome to add further comments to the Austin
> Group bug, if you think anything was misinterpreted - in the end, the
> intent IS that va_* are safe to use without arbitrary changes to errno,
> and it is now just a matter of whether that intent is already met by C
> wording or whether POSIX indeed needs to add an additional requirement.
> 

Thank you very much for making your point clear. I take away: "The value
of errno may be set to nonzero by a library function call" also
means/implies 'use of any library entity, which was not specified as a
library function, shall not set errno to nonzero'.  This really helps me
a lot because it answers the question which part of the standard
prohibits the va_* macros from clobbering errno.

I see this primarily as a C ISO standard problem, so I'm reluctant to
necro-bump that bug in order to start a discussion about how the C
standard is to be interpreted.  I'm going to ask some friends if it is
only me who finds it difficult to read that sentence as you propose.


Best Regards,
Halil


 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On 01/09/2017 12:27 PM, Halil Pasic wrote:

>>> I still do
>>> not understand why were you wrong there. In fact, I could argue that you
>>> were right, but I'm afraid the argument would be somewhat lengthy and
>>> confusing, and I'm already feeling bad about taking so much of your time
>>> with this. Since I'm  admittedly quite inexperienced in this field I
>>> decided to just accept your the conclusion you and the POSIX guys
>>> reached -- without fully understanding it.
>>
>> The C99 standard is annoying in that it does not use the usual RFC
>> wording, so where C99 uses "may", many other standards (including POSIX)
>> use "shall" or even "shall only".  So the fact that C99 states that "The
>> value of errno may be set to nonzero by a library function call" is a
>> requirement that C can permit arbitrary modification of errno ONLY after
>> a function call, and not for any other reason (including after a macro
> 
> Thanks for the clarification. As a non-native speaker I find that usage
> of "may" highly non-intuitive. Especially since in chapter 4.
> "Conformance" (from n1124) does define how "shall" and "shall not" but
> there is nothing on "may".
> 
> This way of saying macros expand to stuff that does not touch errno is
> IMHO quite unfriendly (if this was really the intention - I think it is
> quite likely that it was), and IMHO a more straight forward formulation
> would benefit the standard.

Sadly, I'm not responsible for the wording in the C standard; I also
find it confusing sometimes, even as a native speaker.

> 
> Thank you very much for making your point clear. I take away: "The value
> of errno may be set to nonzero by a library function call" also
> means/implies 'use of any library entity, which was not specified as a
> library function, shall not set errno to nonzero'.  This really helps me
> a lot because it answers the question which part of the standard
> prohibits the va_* macros from clobbering errno.

Or better: "Use of any library entity which was not specified as a
library function shall not modify errno".  While most interfaces in the
library are functions (because they have required linkage) and may also
be a macro, there are some (like assert() and the va_* macros) which are
explicitly documented to be macros only.

> 
> I see this primarily as a C ISO standard problem, so I'm reluctant to
> necro-bump that bug in order to start a discussion about how the C
> standard is to be interpreted.  I'm going to ask some friends if it is
> only me who finds it difficult to read that sentence as you propose.

Yes, you are probably right that raising an issue with the C authors may
be the best path forward on this topic, as it is certainly getting
beyond the bounds of what qemu cares about.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Sascha Silbe]

Dear Eric,

Eric Blake <eblake@redhat.com> writes:

>> +++ b/include/qapi/error.h
>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>   * Just like error_setg(), with @os_error info added to the message.
>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>   * the human-readable error message.
>> + *
>> + * The value of errno (which usually can get clobbered by almost any
>> + * function call) will be preserved.
>>   */
>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>
> Do we need/want to make the guarantee of preserving errno across any of
> the other functions and macros declared in error.h?

It would be more consistent to have all error reporting functions
promise this, even if they do not get passed the errno. In some cases
the errno might not matter to the user (so error_setg_errno() isn't
used), but still be passed on to the caller to signal an error (so
clobbering it could be problematic).

Can prepare a follow-up patch that makes sure error_setg(),
error_propagate(), error_setg_file_open(), error_set() preserve
errno. Optionally also the other functions listed in
include/qapi/error.h and include/qemu/error-report.h.

Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Markus Armbruster]

Sascha Silbe <silbe@linux.vnet.ibm.com> writes:

> Dear Eric,
>
> Eric Blake <eblake@redhat.com> writes:
>
>>> +++ b/include/qapi/error.h
>>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>>   * Just like error_setg(), with @os_error info added to the message.
>>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>>   * the human-readable error message.
>>> + *
>>> + * The value of errno (which usually can get clobbered by almost any
>>> + * function call) will be preserved.
>>>   */
>>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>>
>> Do we need/want to make the guarantee of preserving errno across any of
>> the other functions and macros declared in error.h?
>
> It would be more consistent to have all error reporting functions
> promise this, even if they do not get passed the errno. In some cases
> the errno might not matter to the user (so error_setg_errno() isn't
> used), but still be passed on to the caller to signal an error (so
> clobbering it could be problematic).
>
> Can prepare a follow-up patch that makes sure error_setg(),
> error_propagate(), error_setg_file_open(), error_set() preserve
> errno. Optionally also the other functions listed in
> include/qapi/error.h and include/qemu/error-report.h.

Suggest:

* A patch to document existing errno-preserving behavior.

* Patches to reduce inconsistency, if any.  E.g. say all but one
  error_setg() function preserve errno, make the exception preserve it,
  too.

* Optionally, patches to add more errno-preserving behavior you consider
  useful.  I can't promise such patches will be applied, only that they
  will be reviewed :)

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno gets preserved

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Sascha Silbe <silbe@linux.vnet.ibm.com> writes:
>
>> Dear Eric,
>>
>> Eric Blake <eblake@redhat.com> writes:
>>
>>>> +++ b/include/qapi/error.h
>>>> @@ -170,6 +170,9 @@ void error_setg_internal(Error **errp,
>>>>   * Just like error_setg(), with @os_error info added to the message.
>>>>   * If @os_error is non-zero, ": " + strerror(os_error) is appended to
>>>>   * the human-readable error message.
>>>> + *
>>>> + * The value of errno (which usually can get clobbered by almost any
>>>> + * function call) will be preserved.
>>>>   */
>>>>  #define error_setg_errno(errp, os_error, fmt, ...)                      \
>>>>      error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
>>>
>>> Do we need/want to make the guarantee of preserving errno across any of
>>> the other functions and macros declared in error.h?
>>
>> It would be more consistent to have all error reporting functions
>> promise this, even if they do not get passed the errno. In some cases
>> the errno might not matter to the user (so error_setg_errno() isn't
>> used), but still be passed on to the caller to signal an error (so
>> clobbering it could be problematic).
>>
>> Can prepare a follow-up patch that makes sure error_setg(),
>> error_propagate(), error_setg_file_open(), error_set() preserve
>> errno. Optionally also the other functions listed in
>> include/qapi/error.h and include/qemu/error-report.h.
>
> Suggest:
>
> * A patch to document existing errno-preserving behavior.
>
> * Patches to reduce inconsistency, if any.  E.g. say all but one
>   error_setg() function preserve errno, make the exception preserve it,
>   too.
>
> * Optionally, patches to add more errno-preserving behavior you consider
>   useful.  I can't promise such patches will be applied, only that they
>   will be reviewed :)

Hmm, looks like I held onto your patch in the hope of getting a few
more, then forgot about it.  Since it still applies, I'll take it now.
Doesn't mean I've lost my hope for more of them :)