krun: allow configuring the virtiofs device

[slp]

Allow users to configure the tag and the shm_size of the virtiofs device exposing the root of the container.

This also fixes a problem where, when booting with an initramfs, dracut wasn't able to find the virtio-fs root filesystem. By having the ability to specify the tag of the virtiofs device, it's also possible to coordinate with the kernel command line.

[gemini-code-assist]

Code Review

This pull request updates libkrun_exec to use krun_add_virtiofs2 instead of krun_set_root, enabling configuration of the virtiofs tag and shared memory size. The review identifies a potential breaking change regarding the default tag value, suggests using const char * for type safety, and recommends implementing a fallback mechanism for compatibility with older versions of libkrun.

[gemini-code-assist]

Changing the default virtiofs tag from krunroot (the default used by libkrun's krun_set_root) to "/dev/root" is a breaking change for existing containers that rely on the default kernel command line provided by libkrun. Unless there is a specific reason to change the default for all users, it is safer to stick with "krunroot" as the default and allow users who need "/dev/root" to set it via the new configuration field.

        virtiofs_tag = "krunroot";

[slp]

Dear AI overlord. The default virtiofs tag in libkrun has never, ever been krunroot. It's always been /dev/root. Do you know how I know it? Because I wrote it.

[gemini-code-assist]

The variable virtiofs_tag should be declared as const char * since it is assigned a string literal at line 494 and is passed to krun_add_virtiofs2 which expects a const char *. This avoids discarding the const qualifier.

      const char *virtiofs_tag = NULL;

[giuseppe]

maybe this one makes sense :-)

[slp]

Ack, let's do it.

[gemini-code-assist]

The code now has a hard dependency on the krun_add_virtiofs2 symbol, which was introduced in recent versions of libkrun. If libcrun is linked against an older version of libkrun.so, it will fail to start the container. Consider adding a fallback to the older krun_set_root symbol if krun_add_virtiofs2 is not found to maintain compatibility with older library versions.

[giuseppe]

LGTM

[slp]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces the krun_set_root function with krun_add_virtiofs2 to support configurable virtiofs tags and SHM sizes. Review feedback highlights several areas for improvement: adding support for OCI annotations to maintain consistency with other parameters, validating that the SHM size is non-negative before casting to an unsigned integer, and addressing potential backward compatibility issues caused by changing the default root tag and requiring the new library symbol.

[gemini-code-assist]

For consistency with other krun configuration parameters (such as cpus, ram_mib, and gpu_flags), it would be beneficial to allow configuring virtiofs_tag and virtiofs_shm_size via OCI annotations (e.g., krun.virtiofs_tag and krun.virtiofs_shm_size). This would allow users to override these settings directly from the container engine (like Podman) without needing to provide a .krun_vm.json file inside the container image.

[slp]

Given this is in a static property of the workload, I don't think it'd make sense overriding it via an annotation. That said, I want to consolidate the whole way in which configuration parameters are parsed to make it more consistent. Possibly after the yyjson transition?

[giuseppe]

sure, we can do that later

[gemini-code-assist]

The virtiofs_shm_size value from JSON is cast to uint64_t without checking if it is negative. If a negative value is provided in the configuration, it will result in an extremely large value due to the unsigned cast, which could lead to unexpected behavior or failures when calling krun_add_virtiofs2. Consider validating that the value is non-negative.

          val_virtiofs_shm_size = yajl_tree_get (kconf->config_tree, path_virtiofs_shm_size, yajl_t_number);
          if (val_virtiofs_shm_size != NULL && YAJL_IS_INTEGER (val_virtiofs_shm_size))
            {
              long long val = YAJL_GET_INTEGER (val_virtiofs_shm_size);
              if (UNLIKELY (val < 0))
                error (EXIT_FAILURE, 0, "virtiofs_shm_size must be a non-negative integer");
              virtiofs_shm_size = (uint64_t) val;
            }

[giuseppe]

let's ignore json stuff for now, I am working on a port to yyjson

[gemini-code-assist]

The default tag is changed to "/dev/root". Note that the previous krun_set_root implementation in libkrun used "krunroot" as the default tag. If the kernel command line (either the default one in libkrunfw or a user-provided one) expects the old tag, this change might break the boot process unless the command line is also updated to match. Consider if "krunroot" should remain the default for better backward compatibility.

[gemini-code-assist]

This change replaces the use of krun_set_root with krun_add_virtiofs2, which introduces a hard dependency on the new symbol. This will cause a fatal error for users running against older versions of libkrun.so that do not export krun_add_virtiofs2. To maintain backward compatibility, consider falling back to krun_set_root if krun_add_virtiofs2 is not found, particularly when default values are being used.