[LibOS] Remove shim_qstr

[pwmarcz]

Description of the changes

This finishes the conversion: all usages of shim_qstr use regular heap-allocated C strings instead.

How to test this PR?

CI.

---

This change is 

[boryspoplawski]

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)

a discussion (no related file):
Nice!

---

---

LibOS/shim/include/shim_handle.h, line 220 at r1 (raw file):

    bool needs_et_poll_out;

    char* uri; /* PAL URI for this handle (if any). Does not change. */

Is it possible to add const here or do we use it as char* somewhere?

---

LibOS/shim/src/sys/shim_pipe.c, line 58 at r1 (raw file):

    cli->uri = strdup(uri);
    if (!srv->uri || !cli->uri) {
        ret = -ENOENT;

NP: we should free both uris and set them to NULL here

[boryspoplawski]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @pwmarcz)

---

LibOS/shim/src/sys/shim_pipe.c, line 58 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

NP: we should free both uris and set them to NULL here

Scratch that, it's ok (we free these uris together with handles).

[pwmarcz]

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski)

---

LibOS/shim/include/shim_handle.h, line 220 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Is it possible to add const here or do we use it as char* somewhere?

We don't modify it, but it's owned by this object and freed by it. I think using const char* (and later free((char*)hdl->uri)) would make that ownership less visible, suggest you can assign string literals to it, etc.

(The same happened earlier with shim_dentry.name, which also newer changes, and is also an owned char* field).

[mkow]

Reviewed 17 of 17 files at r1, 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 (1 more required, approved so far: ITL) (waiting on @boryspoplawski and @pwmarcz)

---

LibOS/shim/src/bookkeep/shim_handle.c, line 727 at r1 (raw file):

        if (hdl->uri)
            DO_CP_MEMBER(str, hdl, new_hdl, uri);

How actually is uri buffer migrated? Here we only copy the pointer. I'm also not sure how this code worked before, but I'm probably missing something.

---

LibOS/shim/src/sys/shim_pipe.c, line 78 at r1 (raw file):

    ret = 0;

out:

Don't we need to free srv->uri and cli->uri here on error? (if so, then be careful, they may be uninitialized at some points)

[pwmarcz]

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

---

LibOS/shim/src/bookkeep/shim_handle.c, line 727 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

How actually is uri buffer migrated? Here we only copy the pointer. I'm also not sure how this code worked before, but I'm probably missing something.

DO_CP_MEMBER(str, ...) calls the checkpoint function for "str", see here.

---

LibOS/shim/src/sys/shim_pipe.c, line 78 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Don't we need to free srv->uri and cli->uri here on error? (if so, then be careful, they may be uninitialized at some points)

They should be cleaned up together with handles. But since you're the second person to ask, it's probably less confusing to do it here.

[mkow]

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

---

LibOS/shim/src/bookkeep/shim_handle.c, line 727 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

DO_CP_MEMBER(str, ...) calls the checkpoint function for "str", see here.

Ah, I see, thanks.

---

LibOS/shim/src/sys/shim_pipe.c, line 78 at r1 (raw file):

Previously, pwmarcz (Paweł Marczewski) wrote…

They should be cleaned up together with handles. But since you're the second person to ask, it's probably less confusing to do it here.

Ah, I see now, my confusion stems from my incorrect assumption that srv and cli are out-only arguments. But now I see that they are in-out, so the caller will need to free them anyways.

[dimakuv]

Reviewed 16 of 17 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Nice!

Awesome PR! It's interesting how we finally removed a feature that was added as a "performance optimization".

---

[boryspoplawski]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners

[dimakuv]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)

[boryspoplawski]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[mkow]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved