Change in ...coreboot[master]: libpayload/sys/types.h: Add definition for off_t

[Thomas Heijligen (Code Review)]

Hello Nico Huber,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/31346

to review the following change.

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

libpayload/sys/types.h: Add definition for off_t

`off_t` is supposed to be signed, but has no (minimum) width
specified. We'll assume 32-bit minimum, like a `signed long int`.

Change-Id: I6c0c1bc1a959db7863cbad2ba29318da162431be
Signed-off-by: Nico Huber 
---
M payloads/libpayload/include/sys/types.h
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/31346/1

diff --git a/payloads/libpayload/include/sys/types.h b/payloads/libpayload/include/sys/types.h
index ae143d7..0ed4975 100644
--- a/payloads/libpayload/include/sys/types.h
+++ b/payloads/libpayload/include/sys/types.h
@@ -27,4 +27,11 @@
  * SUCH DAMAGE.
  */

+#ifndef _SYS_TYPES_H
+#define _SYS_TYPES_H
+
 #include <arch/types.h>
+
+typedef signed long int off_t;
+
+#endif /* _SYS_TYPES_H */

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 1:

I don't mind adding the type, but I'm not sure why we need a whole new header file for it. Why not just throw it into stddef.h or something? I don't think libpayload ever intended to be fully POSIX-compliant in terms of which definitions are in which headers (rather, all the standard stuff is supposed to be included as some part of libpayload.h).

Also, why not define it to ssize_t?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 1:
> I don't mind adding the type, but I'm not sure why we need a whole new header file for it.

It's not a new file, I just realized...
> Why not just throw it into stddef.h or something? I don't think libpayload ever intended to be fully POSIX-compliant in terms of which definitions are in which headers (rather, all the standard stuff is supposed to be included as some part of libpayload.h).

I don't know the original intention. Having a single `libpayload.h` works for code that is solely written for libpayload. But I have a lot of code around that is not. Most famous example would be libflashrom which makes use of off_t somewhere now.

Generally, implementing standard things in non-standard headers can only lead to one thing, IMHO: At some point, you'll realize that you have to use the standard headers (like in this case, to be able to compile code that doesn't target libpayload explicitly) and when you try to fix the mess too late, you'll cause breakage in the code that does target the non-standard environment explicitly.
> Also, why not define it to ssize_t?

Good question, have to look that ones definition up... ok, nothing special. I see no reason against it, also no reason to use it. May I ask why?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31346/1/payloads/libpayload/include/sys/type... 
File payloads/libpayload/include/sys/types.h:

https://review.coreboot.org/#/c/31346/1/payloads/libpayload/include/sys/type... 
PS1, Line 33: #include <arch/types.h>
We could get rid of this include by drawing `time_t` and
`suseconds_t` in here. It's the same definition for all arches.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 1: Code-Review+1

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 1:

(1 comment)
> Generally, implementing standard things in non-standard headers can only lead to one thing, IMHO: At some point, you'll realize that you have to use the standard headers (like in this case, to be able to compile code that doesn't target libpayload explicitly) and when you try to fix the mess too late, you'll cause breakage in the code that does target the non-standard environment explicitly.

Okay, I don't really care that much. But we should at least make sure that the "include libpayload.h to get everything" use case also still works, so you should add an include for this to that (maybe in place of <arch/types.h>).
> > Also, why not define it to ssize_t?
>
> Good question, have to look that ones definition up... ok, nothing special. I see no reason against it, also no reason to use it. May I ask why?

Just because I like relying on the exact definitions of what a "long" is as little as possible, maybe that's just a personal preference. With the whole LP64/ILP64/LLP64 thing I'm always getting terribly confused about what the standard integer types mean on a certain architecture, so I think it's nice to rely on well-defined types that are set up in a single place in arch/types.h as much as possible. (I know libpayload already isn't too great with that to begin with, including the definitions of size_t and ssize_t... I'd just prefer to not add more.)

https://review.coreboot.org/#/c/31346/1/payloads/libpayload/include/sys/type... 
File payloads/libpayload/include/sys/types.h:

https://review.coreboot.org/#/c/31346/1/payloads/libpayload/include/sys/type... 
PS1, Line 33: #include <arch/types.h>
> We could get rid of this include by drawing `time_t` and […]

Probably a good idea.

[Nico Huber (Code Review)]

Nico Huber has uploaded a new patch set (#2) to the change originally created by Thomas Heijligen. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

libpayload/sys/types.h: Add definition for off_t

`off_t` is supposed to be signed, but has no (minimum) width
specified. We'll assume 32-bit minimum, like a `signed long int`.

Also include `sys/types.h` in `libpayload.h` so everything is
available through the latter.

Change-Id: I6c0c1bc1a959db7863cbad2ba29318da162431be
Signed-off-by: Nico Huber 
---
M payloads/libpayload/include/libpayload.h
M payloads/libpayload/include/sys/types.h
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/31346/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 2:
> > Generally, implementing standard things in non-standard headers
> > can only lead to one thing, IMHO: At some point, you'll realize
> > that you have to use the standard headers (like in this case, to be
> > able to compile code that doesn't target libpayload explicitly) and
> > when you try to fix the mess too late, you'll cause breakage in the
> > code that does target the non-standard environment explicitly.
>
> Okay, I don't really care that much. But we should at least make
> sure that the "include libpayload.h to get everything" use case
> also still works, so you should add an include for this to that
> (maybe in place of <arch/types.h>).

Done. But as an independent include, as the indirect include may
not live long (follow-up).
> > > Also, why not define it to ssize_t?
> >
> > Good question, have to look that ones definition up... ok,
> > nothing special. I see no reason against it, also no reason to use
> > it. May I ask why?
>
> Just because I like relying on the exact definitions of what a
> "long" is as little as possible, maybe that's just a personal
> preference. With the whole LP64/ILP64/LLP64 thing I'm always
> getting terribly confused about what the standard integer types
> mean on a certain architecture, so I think it's nice to rely on
> well-defined types that are set up in a single place in
> arch/types.h as much as possible. (I know libpayload already isn't
> too great with that to begin with, including the definitions of
> size_t and ssize_t... I'd just prefer to not add more.)

But wouldn't a relation between `ssize_t` and `off_t` make
it even more confusing? Generally, you don't know their size.
So planting the idea that they are the same here, might con-
fuse us again when we work with other definitions.

I think it's best to never assume to know the width of these
types.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 2: Code-Review+2
> But wouldn't a relation between `ssize_t` and `off_t` make
> it even more confusing? Generally, you don't know their size.
> So planting the idea that they are the same here, might con-
> fuse us again when we work with other definitions.

ssize_t is defined to be machine-word size (i.e. the same as uintptr_t)... at least on all systems that are and ever will be relevant to coreboot, I think. So for me that's a pretty clear one and way more tangible than "long".

Anyway, was just a suggestion, I'm fine with this as well if you prefer it.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 2:
> > But wouldn't a relation between `ssize_t` and `off_t` make
> > it even more confusing? Generally, you don't know their size.
> > So planting the idea that they are the same here, might con-
> > fuse us again when we work with other definitions.
>
> ssize_t is defined to be machine-word size (i.e. the same as uintptr_t)... at least on all systems that are and ever will be relevant to coreboot, I think. So for me that's a pretty clear one and way more tangible than "long".

I don't understand how this information can help during
programming? ;) maybe when you write arch-specific code
_and_ want to use an ssize_t _and_ want to be sure that
more than 32 bits fit? Might be personal taste, I just
wouldn't use ssize_t in such a case.

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

libpayload/sys/types.h: Add definition for off_t

`off_t` is supposed to be signed, but has no (minimum) width
specified. We'll assume 32-bit minimum, like a `signed long int`.

Also include `sys/types.h` in `libpayload.h` so everything is
available through the latter.

Change-Id: I6c0c1bc1a959db7863cbad2ba29318da162431be
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/31346
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M payloads/libpayload/include/libpayload.h
M payloads/libpayload/include/sys/types.h
2 files changed, 8 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h
index c2510b7..0b9ab0d 100644
--- a/payloads/libpayload/include/libpayload.h
+++ b/payloads/libpayload/include/libpayload.h
@@ -58,6 +58,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
+#include <sys/types.h>
 #include <arch/types.h>
 #include <arch/io.h>
 #include <arch/virtual.h>
diff --git a/payloads/libpayload/include/sys/types.h b/payloads/libpayload/include/sys/types.h
index ae143d7..0ed4975 100644
--- a/payloads/libpayload/include/sys/types.h
+++ b/payloads/libpayload/include/sys/types.h
@@ -27,4 +27,11 @@
  * SUCH DAMAGE.
  */

+#ifndef _SYS_TYPES_H
+#define _SYS_TYPES_H
+
 #include <arch/types.h>
+
+typedef signed long int off_t;
+
+#endif /* _SYS_TYPES_H */

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 3:
> I don't understand how this information can help during
> programming? ;)

Isn't it always helpful to know what size the types you're using are? I mean, sometimes it doesn't matter, but sometimes it does. When I see an ssize_t I know immediately how many bytes it must have on every architecture. When I see a 'long' (or some variation of it), I need to start digging to be sure.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31346 )

Change subject: libpayload/sys/types.h: Add definition for off_t
......................................................................

Patch Set 3:
> Patch Set 3:
> > I don't understand how this information can help during
> > programming? ;)
>
> Isn't it always helpful to know what size the types you're using are?

It is helpful to know the minimum size (which is tricky in case of
`ssize_t` and `off_t`) but knowing the exact size and taking it into
account leads to unportable code. If you need exact sizes, just use
the stdint types?
> I mean, sometimes it doesn't matter, but sometimes it does. When I see an ssize_t I know immediately how many bytes it must have on every architecture. When I see a 'long' (or some variation of it), I need to start digging to be sure.

In that case you already consider unportable code, don't you? If
we see a `long` somewhere where 32 bits aren't enough, sure we
can start digging to be sure if it's ok in that particular case.
But shouldn't we fix the code (to be portable) instead?