Support virtio-fs #88
Conversation
| int32_t error; | ||
| uint64_t unique; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Remove redundant line.
| #define FUSE_RELEASE 18 | ||
| #define FUSE_FLUSH 25 | ||
| #define FUSE_DESTROY 38 | ||
|
|
There was a problem hiding this comment.
Remove redundant line.
fuse.h
Outdated
| }; | ||
|
|
||
| struct fuse_init_in { | ||
| uint32_t major; // FUSE major version supported by the guest (typically 7) |
There was a problem hiding this comment.
Follow the coding style guide from CONTRIBUTING.md, i.e., using /* ... */ instead of // ... for single-line comment.
fuse.h
Outdated
| uint32_t major; // FUSE major version supported by the device | ||
| uint32_t minor; // FUSE minor version supported by the device | ||
| uint32_t max_readahead; // Maximum readahead size accepted by the device | ||
| uint32_t |
There was a problem hiding this comment.
Rewrite like
/* Flags supported by the device (negotiated with the guest) */
uint32_t flags;
to avoid line breaking.
| #include <sys/uio.h> | ||
| #include <time.h> | ||
| #include <unistd.h> | ||
|
|
There was a problem hiding this comment.
Remove all redundant lines in the file.
| case FUSE_DESTROY: | ||
| virtio_fs_destroy_handler(vfs, vq_desc, plen); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
Add TODO for the OP codes to implement in the future.
|
Clean up the commit history by squashing related commits using |
|
Why |
|
Please improve the pull request description to make the content clearer and easier to understand. See: |
|
Please move |
|
Many thanks to @shengwen-tw . I rebased the branch, and split the changes into two commits: one for configs/linux.config, and the other for the rest. |
device.h
Outdated
|
|
||
| typedef struct { | ||
| uint64_t ino; | ||
| char path[4096]; |
There was a problem hiding this comment.
Instead of hard-coding the value, consider using PATH_MAX from the standard library (limits.h) for better portability and readability.
#include <stdio.h>
#include <limits.h>
int main(void) {
printf("PATH_MAX is defined: %d\n", PATH_MAX);
return 0;
}
main.c
Outdated
|
|
||
| #if SEMU_HAS(VIRTIOFS) | ||
| case 0x48: /* virtio-fs */ | ||
| // printf("width:%d\n",width); |
There was a problem hiding this comment.
Remove debugging code.
main.c
Outdated
| execpath); | ||
| fprintf(stderr, | ||
| "Usage: %s -k linux-image [-b dtb] [-i initrd-image] [-d " | ||
| "disk-image] [-s mount-folder]\n", |
There was a problem hiding this comment.
Use directory (UNIX terminology) instead of folder (Windows terminology).
| @@ -24,6 +24,45 @@ | |||
| #define VIRTIO_BLK_S_IOERR 1 | |||
| #define VIRTIO_BLK_S_UNSUPP 2 | |||
|
|
|||
| #define FUSE_INIT 26 | |||
There was a problem hiding this comment.
Could you point out the source for the FUSE OP codes and what are missing in the current implementation?
device.h
Outdated
| /* guest memory base */ | ||
| uint32_t *ram; | ||
|
|
||
| char *mount_tag; // guest sees this tag |
There was a problem hiding this comment.
Use /* ... */ instead.
virtio-fs.c
Outdated
| PACKED(struct virtio_fs_config { | ||
| char tag[36]; | ||
| uint32_t num_request_queues; | ||
| uint32_t notify_buf_size; // ignored |
There was a problem hiding this comment.
Rewrite all // ... with /* ... */ in this file.
virtio-fs.c
Outdated
|
|
||
| typedef struct { | ||
| DIR *dir; | ||
| char path[4096]; |
There was a problem hiding this comment.
Is it possible to use malloc() instead of a fixing path size?
virtio-fs.c
Outdated
| uint32_t *ram = vfs->ram; | ||
| void *priv = vfs->priv; | ||
| char *mount_tag = vfs->mount_tag; | ||
| char shared_dir[4096]; |
There was a problem hiding this comment.
Is it possible to use malloc() instead of a fixing path size?
virtio-fs.c
Outdated
| *plen = header_resp->out.len; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove redundant lines.
virtio-fs.c
Outdated
| fprintf(stderr, "unsupported virtio-fs operation!\n"); | ||
| return -1; | ||
| } | ||
| /* TO DO: FUSE_WRITE, FUSE_MKDIR, FUSE_RMDIR , FUSE_CREATE */ |
There was a problem hiding this comment.
Use TODO instead of TO DO.
Check Effectively Managing Technical Debt with TODO, FIXME, and Other Code Reminders.
README.md
Outdated
| @@ -77,13 +77,33 @@ You can exit the emulator using: \<Ctrl-a x\>. (press Ctrl+A, leave it, afterwar | |||
| ## Usage | |||
|
|
|||
| ```shell | |||
| ./semu -k linux-image [-b dtb-file] [-i initrd-image] [-d disk-image] | |||
| ./semu -k linux-image [-b dtb-file] [-i initrd-image] [-d disk-image] [-s mount-directory] | |||
There was a problem hiding this comment.
mount-directory might be ambiguous, especially in contexts involving virtio-blk devices. Consider renaming it to something more descriptive, such as shared-directory.
|
Drop |
MikazukiHikari
force-pushed
the
master
branch
2 times, most recently
from
9168250 to
9210af1
Compare
|
@MikazukiHikari Hi, could you provide a Linux image with virtio-fs enabled for quick testing? Thank you! |
|
Please rebase to the latest remote master branch. Otherwise, the build on macOS will fail. |
No problem! How should I send it to you? via Gmail? Or just commit the Linux image? |
You can simply upload to this code review thread. |
THX for the reminder. |
@MikazukiHikari After a quick test, it seems that the create operation is not yet implemented. However, you mentioned file creation in your earlier comment. I encountered the following error when trying to create a file using the touch command in the guest: My testing environment: |
You're right! I wrote the previous comment incorrectly — it's been corrected. Sorry for the confusion. |
Can you confirm if |
I confirm |
There was a problem hiding this comment.
Using uthash is overkill. Check https://github.com/sysprog21/rv32emu/blob/master/src/cache.c for a simple and usable implementation.
OK ! I use linked list instead of uthash for a simple and usable implementation. |
@@ -652,8 +652,22 @@ static int virtio_fs_desc_handler(virtio_fs_state_t *vfs,
virtio_fs_destroy_handler(vfs, vq_desc, plen);
break;
default:
- fprintf(stderr, "unsupported virtio-fs operation!\n");
- return -1;
+ struct fuse_out_header out = {
+ .len = sizeof(struct fuse_out_header),
+ .error = -EOPNOTSUPP,
+ .unique = header_req->in.unique,
+ };
+
+ /* Copy to output buffer */
+ if (vq_desc[2].len >= sizeof(out)) {
+ memcpy((void *)((uintptr_t)vfs->ram + vq_desc[2].addr),
+ &out, sizeof(out));
+ *plen = sizeof(out);
+ } else {
+ fprintf(stderr, "output buffer too small for error reply!\n");
+ *plen = 0;
+ }
+ break;
}
/* TODO: FUSE_WRITE, FUSE_MKDIR, FUSE_RMDIR, FUSE_CREATE */When the guest OS attempts an unsupported operation such as The proposed patch ensures that unsupported operations return a proper error response to the guest OS and terminates the corresponding process with an error code. You may want to wrap it into a static function for better readability and consistency compared to other operations. After this change, commands like
|
MikazukiHikari
force-pushed
the
master
branch
2 times, most recently
from
6fb6a37 to
4f3e035
Compare
Thanks for your contribution! I agree that wrapping the error handling into a static function would improve clarity and user experience. Accordingly, I update the patch in latest commit. |
I do not see the wrapped static function? BTW, if you don't mind, please add |
Sure! Thanks for the reminder! I've wrapped the error handling logic into a static helper function for better readability, and also added the Co-authored-by line to the end of the commit message as requested. Appreciate your suggestion! |
| #include "riscv_private.h" | ||
| #include "virtio.h" | ||
|
|
||
| #define VFS_DEV_CNT_MAX 1 |
There was a problem hiding this comment.
AFAIK, virtio-fs supports multiple mount points if the virtio-fs device supports it. Therefore, add a comment here to clarify this rationale that sets VFS_DEV_CNT_MAX to 1.
virtio-fs.c
Outdated
| #include "fuse.h" | ||
| #include "riscv.h" | ||
| #include "riscv_private.h" | ||
| #include "virtio.h" |
There was a problem hiding this comment.
--- a/virtio-fs.c
+++ b/virtio-fs.c
@@ -1,25 +1,14 @@
-#include <assert.h>
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
-#include <limits.h>
-#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
-#include <sys/ioctl.h>
-#include <sys/mman.h>
#include <sys/stat.h>
-#include <sys/uio.h>
-#include <time.h>
-#include <unistd.h>
-#include "common.h"
#include "device.h"
#include "fuse.h"
-#include "riscv.h"
#include "riscv_private.h"
-#include "virtio.h"Streamline the headers.
There was a problem hiding this comment.
--- a/virtio-fs.c +++ b/virtio-fs.c @@ -1,25 +1,14 @@ -#include <assert.h> #include <dirent.h> #include <errno.h> #include <fcntl.h> -#include <limits.h> -#include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/ioctl.h> -#include <sys/mman.h> #include <sys/stat.h> -#include <sys/uio.h> -#include <time.h> -#include <unistd.h> -#include "common.h" #include "device.h" #include "fuse.h" -#include "riscv.h" #include "riscv_private.h" -#include "virtio.h"Streamline the headers.
I’d prefer to keep common.h for now, since it defines the RANGE_CHECK macro, which may still be used in virtio_fs_reg_read & virtio_fs_reg_write.
There was a problem hiding this comment.
--- a/virtio-fs.c +++ b/virtio-fs.c @@ -1,25 +1,14 @@ -#include <assert.h> #include <dirent.h> #include <errno.h> #include <fcntl.h> -#include <limits.h> -#include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/ioctl.h> -#include <sys/mman.h> #include <sys/stat.h> -#include <sys/uio.h> -#include <time.h> -#include <unistd.h> -#include "common.h" #include "device.h" #include "fuse.h" -#include "riscv.h" #include "riscv_private.h" -#include "virtio.h"Streamline the headers.
I’d prefer to keep
common.hfor now, since it defines theRANGE_CHECKmacro, which may still be used invirtio_fs_reg_read&virtio_fs_reg_write.
The common.h is included during build (check here), so eliminating it is fine and cleaner.
MikazukiHikari
force-pushed
the
master
branch
2 times, most recently
from
cc80c8c to
67e1485
Compare
Map MMIO region at 0xF48____ for virtio-fs device in semu. Introduce special-case logic for char tag in virtio_fs config handling in semu. Add inode_map linked list for mapping inodes to file paths. Implement FUSE core operations: - INIT - GETATTR - OPENDIR - READDIRPLUS - LOOKUP - FORGET - RELEASEDIR - OPEN - READ - RELEASE - FLUSH - DESTROY to make semu supports commands:`cd`, `cat`, `ls`. Set default mount tag to "myfs" and mount directory to "./shared". When `make check`, if MOUNT_DIRECTORY is not exist, then create a new one. If -s parameter is empty, virtio-fs is unused. Add the explanation about virtio-fs in `Usage` of README.md. Add the introduction of how to mount and unmount in semu. When the guest OS attempts an unsupported operation, it should receive a "operation not supported" response instead of hanging to improve the user experience. Co-authored-by: ChinYikMing <yikming2222@gmail.com>
Overview
This commit introduces the VirtIO file system device (virtio-fs) to enable efficient and secure sharing of host directories with semu.
Implementation Details
Test Procedures
ls,cat, andcdto ensure correct operation.This project is implemented on 6.11.0-26-generic 24.04.1-Ubuntu GNU/Linux.