Attention is currently required from: Nico Huber, Thomas Heijligen.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70139 )
Change subject: libpayload: Outsource delay function into own header ......................................................................
Patch Set 8:
(1 comment)
File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/c/coreboot/+/70139/comment/73e0960d_a25f9868 PS2, Line 71: libpayload
And the straight-forward solution: move what we need into a smaller header file. If that's not right, we should discuss that first.
I think that's fine.
If we'd just name it delay.h it would be prone to further naming collisions. But still, we could just do that.
Yes, I think it should just be named delay.h.
A completely different solution could be to implement more standard library APIs in libpayload, e.g. POSIX clock_gettime(2) in this case.
I mean, clock_gettime() is pretty far away from delay() anyway (and not really achievable since libpayload doesn't have access to the real time clock). If you want an official POSIX equivalent, I think it would be sleep()/usleep(). So if somebody really wants that to make porting easier, I think it would be fine to add those functions to libpayload's <unistd.h> as wrappers for delay()/udelay(), but I still think delay should remain the "main" identifier for this functionality because it's more accurate (sleeping implies a scheduler which we don't have).
FWIW, if you just want a cheap out for this particular issue, I think it would also be fine and somewhat reasonable to move the delay functions out to unistd.h instead of making a separate header for them. But of course that only solves this instance of the problem and it's probably going to come up again elsewhere.
I guess the idea here was to avoid conflicts by not giving the new header file a too generic name, i.e. simply <delay.h>. If we would do that and any payload would already have a delay.h, we'd have another problem that can be avoided by prefixing the new file name (doesn't have to be <libpayload/delay.h>, could also be <libplayload-delay.h>).
I don't think one-offs like that are a good idea. This issue will keep coming up again over and over for other payloads, just like it does for userspace C libraries. Today it's delay.h, tomorrow some payload might be annoyed that we have a compiler.h or archive.h or queue.h or whatever that clashes with whatever they have. This should be an all-or-nothing decision: either libpayload is super careful about namespace isolation and anything that's not an official POSIX header name must go in a prefix directory (e.g. <lp/delay.h>) -- if we want to do that, we'd have to rename a ton of existing headers (and then we should technically do the same for function namespacing too, probably). Or we continue to use whatever names we want and say it is up to payload to deal with that. But whatever we choose we should do it consistently across the whole repo and not just as a one off for one header that caused a specific problem with one payload.
Like I said, I don't think there's a real consensus in the C ecosystem on this, there are projects doing it one way and those that do it another. (If you don't like the PNG example there are others, e.g. ncurses uses form.h, menu.h and names like that.) So in my opinion, since this has caused few problems over many years and it would be a big change that would cause disruption for most payloads, my vote would be to just leave things as they are, add a delay.h and let flashrom figure out how to deal with that. But if you really want to switch to clear namespacing I won't stop you either, I just think we need to go all the way then so we still have a consistent overall approach (and then clearly define that in a naming policy document for the future).
And when libpayload adds another, conflicting header file in the future? What we'd like to try is to avoid repeating the problem.
Then payloads will need to adjust again. I'd say that's part of the deal of being downstream to another project.
-
Julius Werner (Code Review)