[Qemu-devel] [PATCH] error: error_setg_errno(): errno may be clobbered

[Qemu-devel] [PATCH] error: error_setg_errno(): errno may be clobbered

[Sascha Silbe]

As a general policy, we want callers to save errno
themselves. error_setg_internal() currently goes out of its way to
preserve errno, so with the API documentation not mentioning it either
way, callers might come to rely on the current behaviour of the
implementation. Spell out that we don't want to make that promise.

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

This came up during review of Halil's patch "block: improve error
handling in raw_open" [1].

[1] mid:1469532873-78542-1-git-send-email-pasic@linux.vnet.ibm.com
    "[Qemu-devel] [PATCH v2 1/1] block: improve error handling in
    raw_open" by Halil Pasic <pasic@linux.vnet.ibm.com>, sent on
    2016-07-26.

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

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 0576659..e5417e9 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -170,6 +170,11 @@ 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.
+ *
+ * Reminder: errno may get clobbered by almost any function call. If
+ * you need the value of errno for another purpose, save it before
+ * invoking error_setg_errno() (or any other function for that
+ * matter).
  */
 #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 may be clobbered

[Eric Blake]

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

On 07/26/2016 12:44 PM, Sascha Silbe wrote:
> As a general policy, we want callers to save errno
> themselves. error_setg_internal() currently goes out of its way to
> preserve errno, so with the API documentation not mentioning it either
> way, callers might come to rely on the current behaviour of the
> implementation. Spell out that we don't want to make that promise.

Alternatively, we could explicitly document that we DO intend to
preserve errno, so that callers can rely on it.  Libvirt went with that
approach (error-reporting functions that preserve errno are easier to
think about than functions which may clobber errno).


-- 
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 may be clobbered

[Markus Armbruster]

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

> As a general policy, we want callers to save errno
> themselves. error_setg_internal() currently goes out of its way to
> preserve errno,

In other words, "error_setg_errno(): errno may be clobbered" is a lie :)

>                 so with the API documentation not mentioning it either
> way, callers might come to rely on the current behaviour of the
> implementation.

According to Max, we rely on the errno-saving behavior in several places
already, and don't intend to change them.

>                 Spell out that we don't want to make that promise.

Well, we already did, tacitly.  Let's make the promise explicit.  Could
you post the patch?

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

Re: [Qemu-devel] [PATCH] error: error_setg_errno(): errno may be clobbered

[Sascha Silbe]

Dear Markus,

Markus Armbruster <armbru@redhat.com> writes:

> Sascha Silbe <silbe@linux.vnet.ibm.com> writes:
>
>> As a general policy, we want callers to save errno
>> themselves. error_setg_internal() currently goes out of its way to
>> preserve errno,
>
> In other words, "error_setg_errno(): errno may be clobbered" is a lie :)

No, it's just standardese (though it should be in capital letters for
that) for "do not rely on it, we may change our mind any time". :)


>>                 so with the API documentation not mentioning it either
>> way, callers might come to rely on the current behaviour of the
>> implementation.
>
> According to Max, we rely on the errno-saving behavior in several places
> already, and don't intend to change them.
>
>>                 Spell out that we don't want to make that promise.
>
> Well, we already did, tacitly.  Let's make the promise explicit.  Could
> you post the patch?

Sure [1]. Like a good lawyer, I can argue either way, even within the
same case. ;)

Sascha

[1] mid:1469611466-31574-1-git-send-email-silbe@linux.vnet.ibm.com
    "[PATCH] error: error_setg_errno(): errno gets preserved" by Sascha
    Silbe <silbe@linux.vnet.ibm.com>, sent on 2016-07-27.
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641