WIP Convert PalStreamOpen to return pal_error_t

[oshogbo]

How to test this PR?

Ci

---

This change is 

[dimakuv]

Reviewed 43 of 43 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)

a discussion (no related file):
Generally looks uncontroversial to me. Two notes:

1. Why do we negate the error code? I think we should keep the semblance to UNIX error codes (e.g. errno == -EBADF).
2. Do you really want to convert PAL functions one by one? Should we do all of them in one go?

---

---

Documentation/pal/host-abi.rst line 53 at r1 (raw file):

   :project: pal

.. doxygenenum:: _pal_error_t

Can we remove this one? This is unused.

---

libos/src/libos_init.c line 573 at r1 (raw file):

                             /*options=*/0, &pipe);
        if (pret != PAL_ERROR_SUCCESS) {
            if (!use_vmid_for_name && pret == PAL_ERROR_STREAMEXIST) {

I highly dislike that error codes are positive now, and we need an additional negation when changing to UNIX error codes... Why do we do that?

---

libos/src/net/unix.c line 150 at r1 (raw file):

    PAL_HANDLE pal_handle = NULL;
    pal_error_t pret;
    pret = PalStreamOpen(pipe_name, PAL_ACCESS_RDWR, /*share_flags=*/0, PAL_CREATE_IGNORED, options,

Can we combine into one line?

---

libos/src/net/unix.c line 248 at r1 (raw file):

    PAL_HANDLE pal_handle = NULL;
    pal_error_t pret;
    pret = PalStreamOpen(pipe_name, PAL_ACCESS_RDWR, /*share_flags=*/0, PAL_CREATE_IGNORED,

Can we combine into one line?

---

pal/regression/HelloWorld.c line 12 at r1 (raw file):

    pal_error_t pret;

    pret = PalStreamOpen("console:", PAL_ACCESS_WRONLY, /*share_flags=*/0, PAL_CREATE_NEVER,

Can we combine into one line?

[oshogbo]

Reviewable status: 41 of 43 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Generally looks uncontroversial to me. Two notes:

1. Why do we negate the error code? I think we should keep the semblance to UNIX error codes (e.g. errno == -EBADF).
2. Do you really want to convert PAL functions one by one? Should we do all of them in one go?

1. The negative codes are discussed below.
2. I will try to do a few functions together. It's a first round so I don't want to convert 20-50 functions to decide that we don't agree about positive/negative and go over all of it. So first I would love to know that we agree with the directions that I took.
   We have discussed that with @mkow and we figured out that we should split it into a few commits (not as small as this one maybe be per module or something like that) because:

• it's a large patch to review, easy to miss some mistakes,
• it will block other PRs from merging in the meantime,
• it will take a lot of time to merge a single large commit and the maintenance of it will be hard (as new features are merged),
• It will take a lot of time to prepare single patch,
• Others can join in the effort.

---

---

Documentation/pal/host-abi.rst line 53 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we remove this one? This is unused.

No. The doxygen generates an error because the typedef definition uses it (the line above).
So the pal_error_t generates:
typedef _pal_error_t pal_error_t
And it complains that _pal_error_t is unknown. I was thinking about dropping _pal_error_t from the code and leaving only typedef. But I guess this is a separate issue.

---

libos/src/libos_init.c line 573 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I highly dislike that error codes are positive now, and we need an additional negation when changing to UNIX error codes... Why do we do that?

I think if the function returns pal_error_t you don't expect it to return a negative enum, you expect to return exactly the value as it is defined.

About the UNIX error codes - this is temporary because when we convert all functions to pal_error_t you either, will have an int with a negative UNIX error code, the positive pal_error_t, or a single function that converts positive pal_error_t to negative UNIX error code (the pal_to_unix). And I think this will be actually cleaner because currently, the pal_to_unix_error can return a positive or negative error code dependable on the value.

---

libos/src/net/unix.c line 150 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we combine into one line?

Done.

---

libos/src/net/unix.c line 248 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we combine into one line?

Done.

---

pal/regression/HelloWorld.c line 12 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can we combine into one line?

Done.

[dimakuv]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)

a discussion (no related file):

Previously, oshogbo (Mariusz Zaborski) wrote…

1. The negative codes are discussed below.
2. I will try to do a few functions together. It's a first round so I don't want to convert 20-50 functions to decide that we don't agree about positive/negative and go over all of it. So first I would love to know that we agree with the directions that I took.
   We have discussed that with @mkow and we figured out that we should split it into a few commits (not as small as this one maybe be per module or something like that) because:

• it's a large patch to review, easy to miss some mistakes,
• it will block other PRs from merging in the meantime,
• it will take a lot of time to merge a single large commit and the maintenance of it will be hard (as new features are merged),
• It will take a lot of time to prepare single patch,
• Others can join in the effort.

See my other comment, let's keep the conversation there. I'm resolving this one.

---

---

Documentation/pal/host-abi.rst line 53 at r1 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

No. The doxygen generates an error because the typedef definition uses it (the line above).
So the pal_error_t generates:
typedef _pal_error_t pal_error_t
And it complains that _pal_error_t is unknown. I was thinking about dropping _pal_error_t from the code and leaving only typedef. But I guess this is a separate issue.

Why not remove _pal_error_t in this PR? Seems related.

---

libos/src/libos_init.c line 573 at r1 (raw file):

I think if the function returns pal_error_t you don't expect it to return a negative enum, you expect to return exactly the value as it is defined.

Ok, I agree with this.

About the UNIX error codes - this is temporary because when we convert all functions to pal_error_t you either, will have an int with a negative UNIX error code, the positive pal_error_t, or a single function that converts positive pal_error_t to negative UNIX error code (the pal_to_unix). And I think this will be actually cleaner because currently, the pal_to_unix_error can return a positive or negative error code dependable on the value.

Partially agree. What I dislike in this proposal is the "temporary" word. Maybe we should revisit the strategy of PRs and just change everything in one (huge) PR? Then we won't have this "temporary" negation. Just imagine a developer who git-bisects and sees that in some commits there is a - and in some commits there is none. I think this will be way too hard to analyze commit history if we have a bunch of commits exposing different behavior of pal_to_unix_errno().

---

libos/src/net/unix.c line 246 at r2 (raw file):

    PAL_HANDLE pal_handle = NULL;
    pal_error_t pret = PalStreamOpen(pipe_name, PAL_ACCESS_RDWR, /*share_flags=*/0, PAL_CREATE_IGNORED,

Please re-wrap to 100 chars

[oshogbo]

Reviewable status: 42 of 43 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

Documentation/pal/host-abi.rst line 53 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why not remove _pal_error_t in this PR? Seems related.

Maybe I should do that in the NODSICARD PR?

---

libos/src/libos_init.c line 573 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think if the function returns pal_error_t you don't expect it to return a negative enum, you expect to return exactly the value as it is defined.

Ok, I agree with this.

About the UNIX error codes - this is temporary because when we convert all functions to pal_error_t you either, will have an int with a negative UNIX error code, the positive pal_error_t, or a single function that converts positive pal_error_t to negative UNIX error code (the pal_to_unix). And I think this will be actually cleaner because currently, the pal_to_unix_error can return a positive or negative error code dependable on the value.

Partially agree. What I dislike in this proposal is the "temporary" word. Maybe we should revisit the strategy of PRs and just change everything in one (huge) PR? Then we won't have this "temporary" negation. Just imagine a developer who git-bisects and sees that in some commits there is a - and in some commits there is none. I think this will be way too hard to analyze commit history if we have a bunch of commits exposing different behavior of pal_to_unix_errno().

I really wouldn't like to do one big PR as this will be messy to maintain, and we really are not good at quick merges. We will have to chase the upstream all the time with this patch :)
Also please notice, that many functions will require a change in the interface (not simply changing the return type). For example, the write function will return pal_error_t and in arguments, there will be a new parameter that indicates the actual size. I really wouldn't like to do that in one go.

Let me propose a middle ground - instead of doing -pal_to_unix_errno maybe let's do a new function palerror_to_unix or whatever that we will also convert to with these, and it will have a behavior that we expect it to have at the end? Please remember I have to check each time if the functions return a positive or negative value as this isn't standardized and pal_to_unix_errno can return both.

---

libos/src/net/unix.c line 246 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please re-wrap to 100 chars

Done.

[dimakuv]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)

---

Documentation/pal/host-abi.rst line 53 at r1 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

Maybe I should do that in the NODSICARD PR?

Sure, makes sense.

---

libos/src/libos_init.c line 573 at r1 (raw file):

I really wouldn't like to do one big PR as this will be messy to maintain, and we really are not good at quick merges. We will have to chase the upstream all the time with this patch :)

I still disagree. We had cases like this, e.g., when we renamed all our directories in the repo. In that case, we simply deliberately stopped all other development and merged all the related changes (I think it was still split in 2-3 commits) in a matter of days. I suggest we do the same in this case. Just need to agree on a week where all maintainers can do that.

Also please notice, that many functions will require a change in the interface (not simply changing the return type). For example, the write function will return pal_error_t and in arguments, there will be a new parameter that indicates the actual size.

What "write function" are you talking about? I see already-good function signature:

gramine/pal/include/pal/pal.h

Line 397 in f8eda80

int PalStreamWrite(PAL_HANDLE handle, uint64_t offset, size_t* count, void* buffer);

Do you mean the internal PAL function?

gramine/pal/include/pal_internal.h

Line 49 in f8eda80

int64_t (*write)(PAL_HANDLE handle, uint64_t offset, uint64_t count, const void* buffer);

Yes, that one would need to be changed...

---

Anyway, I don't mind having multiple commits (and multiple PRs), but all these PRs must be done simultaneously and merged in a manner of days.

[oshogbo]

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

libos/src/libos_init.c line 573 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I really wouldn't like to do one big PR as this will be messy to maintain, and we really are not good at quick merges. We will have to chase the upstream all the time with this patch :)

I still disagree. We had cases like this, e.g., when we renamed all our directories in the repo. In that case, we simply deliberately stopped all other development and merged all the related changes (I think it was still split in 2-3 commits) in a matter of days. I suggest we do the same in this case. Just need to agree on a week where all maintainers can do that.

Also please notice, that many functions will require a change in the interface (not simply changing the return type). For example, the write function will return pal_error_t and in arguments, there will be a new parameter that indicates the actual size.

What "write function" are you talking about? I see already-good function signature:

gramine/pal/include/pal/pal.h

Line 397 in f8eda80

int PalStreamWrite(PAL_HANDLE handle, uint64_t offset, size_t* count, void* buffer);

Do you mean the internal PAL function?

gramine/pal/include/pal_internal.h

Line 49 in f8eda80

int64_t (*write)(PAL_HANDLE handle, uint64_t offset, uint64_t count, const void* buffer);

Yes, that one would need to be changed...

---

Anyway, I don't mind having multiple commits (and multiple PRs), but all these PRs must be done simultaneously and merged in a manner of days.

@dimakuv One more idea ;) To make you happy and not do everything in one go.
What if I will change enums to be a negative value (PAL_ERROR_STREAMEXIST = -1 or so), and in one go convert all the code to drop -PAL_ERROR_* to _PAL_ERROR_* this is simply because its a mechanical change. Then the pal_error_to_unix would work as expected without -, and I can convert with smaller batches.

[mkow]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)

a discussion (no related file):

semblance to UNIX error codes (e.g. errno == -EBADF)

errno is actually positive on unix.

---

---

meson.build line 41 at r3 (raw file):

  add_global_arguments('-DGRAMINE_HAS_NODISCARD', language : 'c')
endif

This will be merged as #1879, we'll need to rebase afterwards.

[dimakuv]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)

a discussion (no related file):

errno is actually positive on unix.

Yeah, bad example. What I meant is Linux style checks, where functions return negative error codes on failure and 0/positive number on success.

---

---

libos/src/libos_init.c line 573 at r1 (raw file):

Previously, oshogbo (Mariusz Zaborski) wrote…

@dimakuv One more idea ;) To make you happy and not do everything in one go.
What if I will change enums to be a negative value (PAL_ERROR_STREAMEXIST = -1 or so), and in one go convert all the code to drop -PAL_ERROR_* to _PAL_ERROR_* this is simply because its a mechanical change. Then the pal_error_to_unix would work as expected without -, and I can convert with smaller batches.

Yes, changing all enums to negative ints makes me happy :)