Conversation
Podman's --tmpfs command line option is currently ignored by libkrun. Let's add a limited support for it, just to mount the given directory as tmpfs. This is implemented by looking at `mounts' in /.krun_config.json and mounting the first matching tmpfs directory there, if any. "Matching" means it's a tmpfs entry and the directory is not yet mounted. The mount options are ignored. We refuse to mount already mounted directories; this is because tmpfs mounts can be requested by Podman itself also for other directories, for example /dev, which libkrun already mounts itself. This commit implements just the most basic easy part and is not fully compliant. Possible future improvements are: - Copy the files from the original directory to the tmpfs. This is requested by default from Podman (tmpcopyup option). It's not implemented right now because the copying is non-trivial and it's better to leave that for Rust implementation of init. - Honour the other mount options. - Honour other mounts from the configuration. Fixes: containers#515 Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
mz-pdm
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing and mounting tmpfs filesystems defined in a JSON configuration. It introduces helper functions for JSON navigation and mount point validation. Feedback suggests enhancing the JSON parser's robustness, eliminating redundant helper functions in favor of existing ones, and ensuring error output is sent to stderr.
| if (tmount->type != JSMN_OBJECT) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
If an element in the mounts array is not an object (e.g., a nested array or a primitive), the current logic only increments t by 1. This will cause the parser to lose track of the array structure because it won't skip all tokens belonging to that element. Using config_parse_skip ensures the entire element is skipped correctly.
| if (tmount->type != JSMN_OBJECT) { | |
| continue; | |
| } | |
| if (tmount->type != JSMN_OBJECT) { | |
| t += config_parse_skip(data, tmount) - 1; | |
| continue; | |
| } |
| static bool json_value_equal(char *data, jsmntok_t *token, const char *value) | ||
| { | ||
| char *token_value = data + token->start; | ||
| unsigned int token_len = token->end - token->start; | ||
| unsigned int len = strlen(value); | ||
|
|
||
| return token_len == len && strncmp(token_value, value, len) == 0; | ||
| } |
There was a problem hiding this comment.
This function is redundant as jsoneq (defined at line 691) already provides similar functionality, including type checking for JSMN_STRING. While jsoneq is case-insensitive, it is generally acceptable for OCI configuration values like "tmpfs". Using the existing function improves maintainability and aligns with the repository's focus on readability over redundant implementations.
References
- Prioritize code readability and maintainability over redundant function definitions.
| if (jsoneq(data, &token[t + 1], "destination") == 0) { | ||
| tdestination = &token[t + 2]; | ||
| t += 2; | ||
| } else if (jsoneq(data, &token[t + 1], "type") == 0) { | ||
| ttype = &token[t + 2]; | ||
| t += 2; | ||
| } else if (jsoneq(data, &token[t + 1], "source") == 0) { | ||
| tsource = &token[t + 2]; | ||
| t += 2; | ||
| } else { | ||
| t++; | ||
| t += config_parse_skip(data, &token[t + 1]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The manual increment t += 2 assumes that the value associated with the key is a single token (like a string). While this is true for valid OCI configurations, it is fragile. Unifying the skipping logic using config_parse_skip makes the parser more robust against unexpected JSON structures.
for (j = 0; j < tmount->size; j++) {
if (jsoneq(data, &token[t + 1], "destination") == 0) {
tdestination = &token[t + 2];
} else if (jsoneq(data, &token[t + 1], "type") == 0) {
ttype = &token[t + 2];
} else if (jsoneq(data, &token[t + 1], "source") == 0) {
tsource = &token[t + 2];
}
t++;
t += config_parse_skip(data, &token[t + 1]);
}| json_value_equal(data, ttype, "tmpfs") && | ||
| json_value_equal(data, tsource, "tmpfs")) { |
There was a problem hiding this comment.
Use jsoneq instead of json_value_equal for consistency and to benefit from the type checking performed by jsoneq. This improves code readability and maintainability.
| json_value_equal(data, ttype, "tmpfs") && | |
| json_value_equal(data, tsource, "tmpfs")) { | |
| jsoneq(data, ttype, "tmpfs") == 0 && | |
| jsoneq(data, tsource, "tmpfs") == 0) { |
References
- Prioritize code readability and consistency over minor variations in implementation.
2 participants
Podman's --tmpfs command line option is currently ignored by libkrun. Let's add a limited support for it, just to mount the given directory as tmpfs.
This is implemented by looking at `mounts' in /.krun_config.json and mounting the first matching tmpfs directory there, if any. "Matching" means it's a tmpfs entry and the directory is not yet mounted. The mount options are ignored.
We refuse to mount already mounted directories; this is because tmpfs mounts can be requested by Podman itself also for other directories, for example /dev, which libkrun already mounts itself.
This commit implements just the most basic easy part and is not fully compliant. Possible future improvements are:
Copy the files from the original directory to the tmpfs. This is requested by default from Podman (tmpcopyup option). It's not implemented right now because the copying is non-trivial and it's better to leave that for Rust implementation of init.
Honour the other mount options.
Honour other mounts from the configuration.
Fixes: #515