Hi Juergen,
On 27/06/2022 16:13, Juergen Gross wrote:
On 27.06.22 17:03, Julien Grall wrote:
Hi Juergen,
On 27/06/2022 15:50, Juergen Gross wrote:
On 27.06.22 16:48, Julien Grall wrote:
Hi,
On 27/06/2022 15:31, Juergen Gross wrote:
On 27.06.22 14:36, Julien Grall wrote:
From: Julien Grall <jgr...@amazon.com>
Some tools (e.g. xenstored) always expect EINVAL to be first in
xsd_errors.
Document it so, one doesn't add a new entry before hand by mistake.
Signed-off-by: Julien Grall <jgr...@amazon.com>
----
I have tried to add a BUILD_BUG_ON() but GCC complained that the
value
was not a constant. I couldn't figure out a way to make GCC happy.
Changes in v2:
- New patch
---
xen/include/public/io/xs_wire.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/include/public/io/xs_wire.h
b/xen/include/public/io/xs_wire.h
index c1ec7c73e3b1..dd4c9c9b972d 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -76,6 +76,7 @@ static struct xsd_errors xsd_errors[]
__attribute__((unused))
#endif
= {
+ /* /!\ Some users (e.g. xenstored) expect EINVAL to be the
first entry. */
XSD_ERROR(EINVAL),
XSD_ERROR(EACCES),
XSD_ERROR(EEXIST),
What about another approach, like:
In place of what? I still think we need the comment because this
assumption is not part of the ABI (AFAICT xs_wire.h is meant to be
stable).
At which point, I see limited reason to fix xenstored_core.c.
But I would have really prefer to use a BUILD_BUG_ON() (or similar)
so we can catch any misue a build. Maybe I should write a small
program that is executed at compile time?
My suggestion removes the need for EINVAL being the first entry
xsd_errors[] is part of the stable ABI. If Xenstored is already
"misusing" it, then I wouldn't be surprised if other software rely on
this as well.
Xenstored is the only instance which needs a translation from value to
string, while all other users should need only the opposite direction.
The only other candidate would be oxenstored, but that seems not to use
xsd_errors[].
That's assuming this is the only two implementation of Xenstored
existing ;).
And in fact libxenstore will just return a plain EINVAL in case it
can't find a translation, while hvmloader will return EIO in that case. >
With your reasoning and the hvmloader use case you could argue that
the EIO entry needs to stay at the same position, too.
I have looked at the hmvloader code. It doesn't seem to expect EIO to be
at a specific position.
However, I do agree that it is probably best to keep each error at the
same position.
Therefore, I don't really see how fixing Xenstored would allow us to
remove this restriction.
The only reason this was spotted is by Jan reviewing C Xenstored.
Without that, it would have problably took a long time to notice
this change (I don't think there are many other errno used by Xenstored
and xsd_errors). So I think the risk is not worth the effort.
I don't see a real risk here, but if there is consensus that the risk
should
not be taken, then I'd rather add a comment that new entries are only
allowed
to be added at the end of the array.
I would be fine to mandate that new errors should be added at the end of
the array.
Cheersm
--
Julien Grall
