[PAL] Support direct mode in Windows WSL

[g2flyer]

Description of the changes

Currently Gramine does not work on Windows subsystem for linux (WSL) as WSL does not have a "/sys/devices/system/node/" folder, preventing the use of this common and convenient development platoform. This PR remedies that by using reasonable default values if that folder does not exist.

This is a resurrection of earlier abandoned PR #679, updated to the latest version and also a bit cleaned up.

How to test this PR?

Usual CI should test that it doesn't break anything existing and to test on WSL, just build and run usual regression tests and some of the examples.

BTW: I've tested (successfully ;-) on WSL version 2.2.0 with kernel 5.15.150.1-microsoft-standard-WSL2 on running ubuntu 23.10. That said, i don't think specific WSL2 versions shouldn't matter much and neither should ubuntu version (beyond what is or is not generally supported by gramine).

---

This change is 

[dimakuv]

Reviewed 1 of 1 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: ) (waiting on @g2flyer)

---

pal/src/host/linux-common/topo_info.c line 294 at r1 (raw file):

    bool cpu_nodes_file_exists = false;
    size_t nodes_cnt = 1;
    /* Get the CPU node number. By default, the number is 1.

CPU node number -> number of NUMA nodes on the system

---

pal/src/host/linux-common/topo_info.c line 307 at r1 (raw file):

    } else {
        cpu_nodes_file_exists = true;
    }

This cascade of ifs feels complex. Why not a flat structure:

    bool cpu_nodes_file_exists = true;
    ...
    if (ret < 0 && ret != -ENOENT)
        return ret;
    if (ret == -ENOENT)
        cpu_nodes_file_exists = false;

---

pal/src/host/linux-common/topo_info.c line 340 at r1 (raw file):

    if (cpu_nodes_file_exists) {
        ret = iterate_ranges_from_file("/sys/devices/system/node/online", set_numa_node_online, numa_nodes);

Please fix limit per line (100 chars max)

---

pal/src/host/linux-common/topo_info.c line 346 at r1 (raw file):

        /* If there is no node information, the (only) node must be online. */
        numa_nodes[0].is_online = true;
    }

I would remove these else branches scattered around the whole file, and instead put them in a single place:

if (!cpu_nodes_file_exists) {
    /* if there is no node information, imitate a single node */
    numa_nodes[0].is_online = true;
    numa_nodes[0].nr_hugepages[HUGEPAGES_2M] = 0;
    numa_nodes[0].nr_hugepages[HUGEPAGES_1G] = 0;
    distances[0] = 10; /* default on Linux 5.15 */
}

---

pal/src/host/linux-common/topo_info.c line 402 at r1 (raw file):

                goto fail;

            /* Since our sysfs doesn't support writes, set persistent hugepages to their default value

Please fix limit per line (100 chars max)

---

pal/src/host/linux-common/topo_info.c line 408 at r1 (raw file):

        }
    } else {
        /* As node-id is by default zero we do not have to call set_node_id for synthesized mnode

monde -> node

[g2flyer]

Dmitrii, thanks for your review. I hope i did address your comments although i took the liberty of changing in slightly different ways but hopefully still captured the intention of your concern

Reviewable status: 0 of 1 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)

---

pal/src/host/linux-common/topo_info.c line 294 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

CPU node number -> number of NUMA nodes on the system

Done.

---

pal/src/host/linux-common/topo_info.c line 307 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This cascade of ifs feels complex. Why not a flat structure:

    bool cpu_nodes_file_exists = true;
    ...
    if (ret < 0 && ret != -ENOENT)
        return ret;
    if (ret == -ENOENT)
        cpu_nodes_file_exists = false;

Yeah, i also didn't like it in its original form but was too lazy to change. I did now make it more concise and also split the comments to separate the two parts the comment contained

---

pal/src/host/linux-common/topo_info.c line 340 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please fix limit per line (100 chars max)

good catch. I was counting that my editor applies clang-format but as i haven't touched this part of the code it didn't reformat, should be done now

---

pal/src/host/linux-common/topo_info.c line 346 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would remove these else branches scattered around the whole file, and instead put them in a single place:

if (!cpu_nodes_file_exists) {
    /* if there is no node information, imitate a single node */
    numa_nodes[0].is_online = true;
    numa_nodes[0].nr_hugepages[HUGEPAGES_2M] = 0;
    numa_nodes[0].nr_hugepages[HUGEPAGES_1G] = 0;
    distances[0] = 10; /* default on Linux 5.15 */
}

I consolidated although i didn't like the if (X) followed by if (!X) but left it an alse (but commented on what the else is.
BTW: along the same consolidation argument one could also merge the two for loops inside the iff? But left it as is to keep patch more minimal

---

pal/src/host/linux-common/topo_info.c line 402 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please fix limit per line (100 chars max)

Done.

---

pal/src/host/linux-common/topo_info.c line 408 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

monde -> node

Done.

[dimakuv]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 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 @g2flyer)

---

pal/src/host/linux-common/topo_info.c line 292 at r2 (raw file):

        return ret;

    bool cpu_nodes_file_exists = false;

No need to declare the bool here, just declare & assign it below

---

pal/src/host/linux-common/topo_info.c line 297 at r2 (raw file):

    ret = iterate_ranges_from_file("/sys/devices/system/node/possible", get_ranges_end, &nodes_cnt);
    if ((ret < 0) && (ret != -ENOENT)) {
        /* Some systems do not have the file, for example, Windows Subsystem for Linux, for which we

for example -> e.g.

---

pal/src/host/linux-common/topo_info.c line 298 at r2 (raw file):

    if ((ret < 0) && (ret != -ENOENT)) {
        /* Some systems do not have the file, for example, Windows Subsystem for Linux, for which we
         * will ignore the -ENOENT error and synthesize later a corresponding (single) numa node

No need for the word will

---

pal/src/host/linux-common/topo_info.c line 298 at r2 (raw file):

    if ((ret < 0) && (ret != -ENOENT)) {
        /* Some systems do not have the file, for example, Windows Subsystem for Linux, for which we
         * will ignore the -ENOENT error and synthesize later a corresponding (single) numa node

numa -> NUMA

---

pal/src/host/linux-common/topo_info.c line 433 at r2 (raw file):

                goto fail;
        }
    } else {  // !cpu_nodes_file_exists

We don't use such a notation with // !. I suggest to just remove it.

---

pal/src/host/linux-common/topo_info.c line 435 at r2 (raw file):

    } else {  // !cpu_nodes_file_exists
        /* As node-id is by default zero we do not have to call set_node_id for synthesized node
         * and, as above, unsupported persistent huge pages to zero */

This is hard to read and understand -- it talks about two different things. What about this:

/* node_id field for each CPU core is by default zero, so no need to call set_node_id() for 
 * our single synthesized node 0 */
;

/* as above, set unsupported persistent huge pages to zero */
num_nodes[0]. ...

---

pal/src/host/linux-common/topo_info.c line 439 at r2 (raw file):

        numa_nodes[0].nr_hugepages[HUGEPAGES_1G] = 0;

        /* Set the distance with default value Ubuntu 22.04 kernel 5.15 for syntheized numa node */

I think Ubuntu 22.04 is not relevant (i.e. it's a default in the Linux kernel, not in OS distros)?

Also, syntheized has a typo. And NUMA should be all capital letters.

In the end:

/* Set the distance to a default value on Linux 5.15 for our synthesized NUMA node */

[g2flyer]

Reviewable status: 0 of 1 files reviewed, 7 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)

---

pal/src/host/linux-common/topo_info.c line 292 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No need to declare the bool here, just declare & assign it below

good point

---

pal/src/host/linux-common/topo_info.c line 297 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

for example -> e.g.

ok, moving to latin (which is probably what i would have used if i would have written the original text :-)

---

pal/src/host/linux-common/topo_info.c line 298 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No need for the word will

Done.

---

pal/src/host/linux-common/topo_info.c line 298 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

numa -> NUMA

Done.

---

pal/src/host/linux-common/topo_info.c line 433 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

We don't use such a notation with // !. I suggest to just remove it.

I added that comment as i was a longish if clause but i didn't like to break it into two separate if (X) and if(!X) clauses. But i'm fine removing it ...

---

pal/src/host/linux-common/topo_info.c line 435 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is hard to read and understand -- it talks about two different things. What about this:

/* node_id field for each CPU core is by default zero, so no need to call set_node_id() for 
 * our single synthesized node 0 */
;

/* as above, set unsupported persistent huge pages to zero */
num_nodes[0]. ...

Done.

---

pal/src/host/linux-common/topo_info.c line 439 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think Ubuntu 22.04 is not relevant (i.e. it's a default in the Linux kernel, not in OS distros)?

Also, syntheized has a typo. And NUMA should be all capital letters.

In the end:

/* Set the distance to a default value on Linux 5.15 for our synthesized NUMA node */

googling a bit, seems this 10 is the standard normalized relative cost coming from ACPI. Changed text accordingly

[dimakuv]

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

[dimakuv]

Jenkins, test this please

[mkow]

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

---

-- commits line 2 at r3:
"in" sounds a bit like "direct mode" was a mode of WSL

Suggestion:

[PAL] Support direct mode under Windows WSL

---

-- commits line 4 at r3:

Suggestion:

Windows Subsystem for Linux

---

-- commits line 5 at r3:

Suggestion:

and default to a synthetic, sane structure.

---

pal/src/host/linux-common/topo_info.c line 295 at r3 (raw file):

    /* Get the number of NUMA nodes on the system. By default, the number is 1. */
    ret = iterate_ranges_from_file("/sys/devices/system/node/possible", get_ranges_end, &nodes_cnt);
    if ((ret < 0) && (ret != -ENOENT)) {

Suggestion:

if (ret < 0 && ret != -ENOENT) {

---

pal/src/host/linux-common/topo_info.c line 433 at r3 (raw file):

        }
    } else {
        /* Node-id for each CPU core is by default zero, so no need to call set_node_id for

Is it?

Code quote:

Node-id for each CPU core is by default zero

---

pal/src/host/linux-common/topo_info.c line 434 at r3 (raw file):

so no need to call set_node_id for our single synthesized NUMA node

This comment is IMO misleading, set_node_id() sets node ID in cores, not nodes. And it's not required for setting the IDs, it's just a helper function used in the parser, you could as well just set cores[i].node_id manually.

---

pal/src/host/linux-common/topo_info.c line 435 at r3 (raw file):

        /* Node-id for each CPU core is by default zero, so no need to call set_node_id for
         * our single synthesized NUMA node. */
        ;

?

[dimakuv]

Reviewable status: all files reviewed, 7 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 @g2flyer and @mkow)

---

pal/src/host/linux-common/topo_info.c line 433 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Is it?

Hm, indeed, is it?

gramine/pal/src/host/linux-common/topo_info.c

Line 311 in 53f5511

cores[i].node_id = -1;

---

pal/src/host/linux-common/topo_info.c line 435 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

?

That was a suggestion from me, to make it explicit that we don't need to do anything in the code.

Anyway, it looks like we need a proper look over cores and setting their node_id = 0; explicitly.

[dimakuv]

Reviewable status: all files reviewed, 7 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 @g2flyer and @mkow)

---

pal/src/host/linux-common/topo_info.c line 435 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

That was a suggestion from me, to make it explicit that we don't need to do anything in the code.

Anyway, it looks like we need a proper look over cores and setting their node_id = 0; explicitly.

*look -> loop

[g2flyer]

Thanks for the thorough look at code and finding the -1 != 0 issue. With the exception of the commit log comments where i need further guidance, i hope i have addressed your comments

Reviewable status: all files reviewed, 7 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 @mkow)

---

-- commits line 2 at r3:

Previously, mkow (Michał Kowalczyk) wrote…

"in" sounds a bit like "direct mode" was a mode of WSL

I gladly change that but as we currently have fixups i can do that only by squashing them which is against standard practice: should i do squash or just table it and resolve once everything is approved and i will autosquash and rebase to latest master?

---

-- commits line 4 at r3:
ditto

---

-- commits line 5 at r3:
ditto

---

pal/src/host/linux-common/topo_info.c line 295 at r3 (raw file):

    /* Get the number of NUMA nodes on the system. By default, the number is 1. */
    ret = iterate_ranges_from_file("/sys/devices/system/node/possible", get_ranges_end, &nodes_cnt);
    if ((ret < 0) && (ret != -ENOENT)) {

i know they are unnecessary but i like the imho increased readability. But i've changed it now

---

pal/src/host/linux-common/topo_info.c line 433 at r3 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm, indeed, is it?

gramine/pal/src/host/linux-common/topo_info.c

Line 311 in 53f5511

cores[i].node_id = -1;

Aargh, good catch. I (wrongly) trusted the original version, just extended comment with what i thought the code relies on and every test seemed to work also with -1 thought to be 0. But that's clearly not the way it should be. Should be fixed now.

---

pal/src/host/linux-common/topo_info.c line 434 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

so no need to call set_node_id for our single synthesized NUMA node

This comment is IMO misleading, set_node_id() sets node ID in cores, not nodes. And it's not required for setting the IDs, it's just a helper function used in the parser, you could as well just set cores[i].node_id manually.

i guess I meant "to" instead of "for". But should be moot now anyway. BTW: also was debating on whether directly setting node_id instead of calling the function. But as the function also does slightly more, e.g., test whether thread is online and for more uniformity i decided to keep the function call,

[mkow]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 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 @g2flyer)

---

-- commits line 2 at r3:

Previously, g2flyer (Michael Steiner) wrote…

I gladly change that but as we currently have fixups i can do that only by squashing them which is against standard practice: should i do squash or just table it and resolve once everything is approved and i will autosquash and rebase to latest master?

We usually either leave the comment open until the final rebase and only then fix them, or add squash! commits (a Git feature similar to fixup!). Both are fine with me.

---

pal/src/host/linux-common/topo_info.c line 295 at r3 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

i know they are unnecessary but i like the imho increased readability. But i've changed it now

For me it decreases readability - there's more parentheses to parse and match with each other, and without them we'd have a standard, idiomatic C if with only the most popular operators inside. It could be useful if you used some operators with less obvious ordering, like mixing ^ and &.

---

pal/src/host/linux-common/topo_info.c line 435 at r3 (raw file):

That was a suggestion from me, to make it explicit that we don't need to do anything in the code.

That's a weird convention IMO :) If it's not clear from the comment then I'd reword the comment instead.

Anyways, not relevant anymore.

UPDATE: actually still relevant, lol :)

---

pal/src/host/linux-common/topo_info.c line 433 at r4 (raw file):

        }
    } else {
        /* Set node-id of active threads to synthesized NUME node with id 0. */

Suggestion:

/* Set node-id of active threads to the synthesized NUMA node with id 0. */

[g2flyer]

Aargh, i did see that typo but somehow the editor's save command got lost. That said, it also gave me an opportunity to use the git commit ---fixup=amend:<commit-id variant of fixups which i didn't know before that it existed :-)

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)

[g2flyer]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)

---

pal/src/host/linux-common/topo_info.c line 435 at r3 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

That was a suggestion from me, to make it explicit that we don't need to do anything in the code.

That's a weird convention IMO :) If it's not clear from the comment then I'd reword the comment instead.

Anyways, not relevant anymore.

UPDATE: actually still relevant, lol :)

Done.

---

pal/src/host/linux-common/topo_info.c line 433 at r4 (raw file):

        }
    } else {
        /* Set node-id of active threads to synthesized NUME node with id 0. */

Done.

[mkow]

Huh, I didn't know this one, we'll see if it works with --autosquash when rebasing :)

Reviewed 1 of 1 files at r5, 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) (waiting on @dimakuv)

[kailun-qin]

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

---

pal/src/host/linux-common/topo_info.c line 446 at r5 (raw file):

        numa_nodes[0].nr_hugepages[HUGEPAGES_1G] = 0;

        /* Set  distance for synthesized NUMA node to standard node-local value provided by ACPI

2x white spaces?

Code quote:

Set  distance

[dimakuv]

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

---

-- commits line 31 at r5:
Interestingly, this line wrap is at ~90 characters. What editor does this?

Just curious, since we'll rewrap this text anyway at 75 chars per line during final merge.

---

-- commits line 34 at r5:
Shouldn't Zongmin Gu also have Signed-off-by line? He should, please add (I hope that the original PR had this line, so you don't need to re-ask his permission.)

---

pal/src/host/linux-common/topo_info.c line 301 at r5 (raw file):

        return ret;
    }
    bool cpu_nodes_file_exists = (ret >= 0);

Why such a name? Wouldn't it be clearer if the name was sys_node_dir_exists? There is no "file nodes", as the name implies currently.

[g2flyer]

Reviewable status: 0 of 1 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: ) (waiting on @dimakuv, @kailun-qin, and @mkow)

---

-- commits line 31 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Interestingly, this line wrap is at ~90 characters. What editor does this?

Just curious, since we'll rewrap this text anyway at 75 chars per line during final merge.

git commit --amend and vi (with no auto-wrap) as editor, inherited from original version, reformated now

---

-- commits line 34 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Shouldn't Zongmin Gu also have Signed-off-by line? He should, please add (I hope that the original PR had this line, so you don't need to re-ask his permission.)

I actually tried that initially but git(hub) refused as it seems to check that signed-off-by matches who pushes to have more than one signed-off, hence the move to co-authored-by.

---

pal/src/host/linux-common/topo_info.c line 301 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why such a name? Wouldn't it be clearer if the name was sys_node_dir_exists? There is no "file nodes", as the name implies currently.

historical reasons -- keeping close to original version ... -- but i see your (good) point. Changed ..

---

pal/src/host/linux-common/topo_info.c line 446 at r5 (raw file):

Previously, kailun-qin (Kailun Qin) wrote…

2x white spaces?

eagle eye :-) (fixed now)

[dimakuv]

Reviewed 1 of 1 files at r6, 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: ) (waiting on @g2flyer and @kailun-qin)

---

-- commits line 34 at r5:

Previously, g2flyer (Michael Steiner) wrote…

I actually tried that initially but git(hub) refused as it seems to check that signed-off-by matches who pushes to have more than one signed-off, hence the move to co-authored-by.

I think the order of signed-off lines matters here. If you specify your sign-off as the last item, then GitHub won't complain. At least that's how we did it before, and we never experienced issues.

---

pal/src/host/linux-common/topo_info.c line 301 at r5 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

historical reasons -- keeping close to original version ... -- but i see your (good) point. Changed ..

Still wrong... Why _file_? I wanted _dir_, that was my main point.

[g2flyer]

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: ) (waiting on @dimakuv and @kailun-qin)

---

-- commits line 34 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I think the order of signed-off lines matters here. If you specify your sign-off as the last item, then GitHub won't complain. At least that's how we did it before, and we never experienced issues.

I'm quite sure i was last at that time. But thinking of it, there is also a bit a philosophical question here? Doesn't 'Signed-off-by' also should reflect responsibility for the code and given that the code evolved quite a bit, it would at least to have to cross-check with Zongmin that he is fine with also signing off (or put differently, one could argue that the co-authored-by is the all the credit and none of the blame ;-)

---

pal/src/host/linux-common/topo_info.c line 301 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Still wrong... Why _file_? I wanted _dir_, that was my main point.

I missed that part. However, while true that WSL does not have the whole directory, the current code doesn't not test for the directory but only a file in that directory. So unless we also test for the (absence of the) directory i still think "file" is more appropriate than "directory" ? And adding an additional test for absence of directory doesn't seem to me to really add anything?

[dimakuv]

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @guzongmin and @kailun-qin)

---

-- commits line 34 at r5:

Previously, g2flyer (Michael Steiner) wrote…

I'm quite sure i was last at that time. But thinking of it, there is also a bit a philosophical question here? Doesn't 'Signed-off-by' also should reflect responsibility for the code and given that the code evolved quite a bit, it would at least to have to cross-check with Zongmin that he is fine with also signing off (or put differently, one could argue that the co-authored-by is the all the credit and none of the blame ;-)

@guzongmin Would you be Ok to put the Signed-off-by line in this PR?

Otherwise we'll merge this PR without your explicit sign off (which is no problem too).

---

pal/src/host/linux-common/topo_info.c line 301 at r5 (raw file):

Previously, g2flyer (Michael Steiner) wrote…

I missed that part. However, while true that WSL does not have the whole directory, the current code doesn't not test for the directory but only a file in that directory. So unless we also test for the (absence of the) directory i still think "file" is more appropriate than "directory" ? And adding an additional test for absence of directory doesn't seem to me to really add anything?

Ok, that's fair enough. I'm resolving; also seems that no other reviewers cared about the name, so I'm just being too picky.

[dimakuv]

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: Intel) (waiting on @kailun-qin)

---

-- commits line 34 at r5:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@guzongmin Would you be Ok to put the Signed-off-by line in this PR?

Otherwise we'll merge this PR without your explicit sign off (which is no problem too).

In private conversation, @guzongmin told me that he doesn't use WSL and didn't check if this PR works. So I guess we shouldn't add his sign off. Let's keep as is.

[kailun-qin]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: Intel)

[dimakuv]

Jenkins, test this please

[mkow]

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

---

-- commits line 1 at r6:
I hope I merged the commits messages correctly, as it was tricky: these amend! commits added some changes but also (accidentally) reversed some others, so it's not clear which version was intended to be the final.

[mkow]

Jenkins, retest this please

[kailun-qin]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved