Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: init ovfs #4652

Merged
merged 7 commits into from
Jun 3, 2024
Merged

feat: init ovfs #4652

merged 7 commits into from
Jun 3, 2024

Conversation

zjregee
Copy link
Member

@zjregee zjregee commented May 28, 2024

Initialize ovfs project.

@zjregee
Copy link
Member Author

zjregee commented May 28, 2024

Hi, @Xuanwo, is there anything else should be added in this PR.

bin/ovfs/src/main.rs Outdated Show resolved Hide resolved
bin/ovfs/src/server.rs Outdated Show resolved Hide resolved
bin/ovfs/src/virtiofs.rs Outdated Show resolved Hide resolved
bin/ovfs/src/virtiofs.rs Outdated Show resolved Hide resolved
use vmm_sys_util::epoll::EventSet;
use vmm_sys_util::eventfd::EventFd;

const QUEUE_SIZE: usize = 32768;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide detailed descriptions for each const so we can understand their meanings.

bin/ovfs/src/virtiofs.rs Outdated Show resolved Hide resolved
#[derive(Debug)]
enum Error {
HandleEventNotEpollIn,
HandleEventUnknownEvent,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error messages we provide should clarify what went wrong and guide users on how to fix the issue. Please ensure that the errors are understandable to users. Initially, we can return Unexpected(source).

bin/ovfs/src/virtiofs.rs Show resolved Hide resolved
bin/ovfs/src/virtiofs_utils.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented May 28, 2024

is there anything else should be added in this PR.

I prefer to remove code rather than add more... 😄

Please all basic CI config like we do for oli:

name: Oli CI
on:
push:
branches:
- main
pull_request:
branches:
- main
paths:
- "bin/oay/**"
- "core/**"
- ".github/workflows/ci_bin_oli.yml"
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-${{ github.event_name }}
cancel-in-progress: true
jobs:
check_clippy:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup Rust toolchain
uses: ./.github/actions/setup
with:
need-rocksdb: true
need-protoc: true
github-token: ${{ secrets.GITHUB_TOKEN }}
- name: Cargo clippy
working-directory: bin/oli
run: cargo clippy --all-targets --all-features -- -D warnings

@zjregee zjregee marked this pull request as draft May 29, 2024 04:23
@zjregee zjregee marked this pull request as ready for review May 30, 2024 06:19
@zjregee
Copy link
Member Author

zjregee commented May 30, 2024

It's ready for review now.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many things to polish, but I think we should give it a go.

@Xuanwo Xuanwo merged commit 2962ab4 into apache:main Jun 3, 2024
147 of 150 checks passed
@zjregee zjregee deleted the add-ovfs-entry branch June 3, 2024 04:31
George-Miao pushed a commit to George-Miao/opendal that referenced this pull request Jun 5, 2024
* init ovfs

* typo

* add ci

* unifiy error

* add more comments

* fix ci first

* typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants