Xuanwo commented on code in PR #4652: URL: https://github.com/apache/opendal/pull/4652#discussion_r1617562369
########## bin/ovfs/src/virtiofs.rs: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; +use vhost::vhost_user::Backend; +use vhost_user_backend::{VhostUserBackend, VringMutex, VringState, VringT}; +use virtio_bindings::bindings::virtio_config::*; Review Comment: Please don't expose `*` from 3rd crates. ########## bin/ovfs/src/main.rs: ########## @@ -0,0 +1,27 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +mod server; +mod virtiofs; +mod virtiofs_utils; + +#[macro_use] +extern crate log; Review Comment: We don't need this after edition 2021. ########## bin/ovfs/src/server.rs: ########## @@ -0,0 +1,56 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::ffi::FromBytesWithNulError; +use std::io; + +use crate::virtiofs_utils::{Reader, Writer}; + +/// The Error here represents filesystem message related errors. +#[derive(Debug)] +pub enum Error { + DecodeMessage(io::Error), + EncodeMessage(io::Error), + MissingParameter, + InvalidHeaderLength, + InvalidCString(FromBytesWithNulError), +} + +impl std::fmt::Display for Error { Review Comment: Please use [`snafu`](https://docs.rs/snafu/latest/snafu/) instead of implement Error by hand. ########## bin/ovfs/src/virtiofs.rs: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; +use vhost::vhost_user::Backend; +use vhost_user_backend::{VhostUserBackend, VringMutex, VringState, VringT}; +use virtio_bindings::bindings::virtio_config::*; +use virtio_bindings::bindings::virtio_ring::{ + VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, +}; +use vm_memory::{ByteValued, GuestMemoryAtomic, GuestMemoryMmap, Le32}; +use vmm_sys_util::epoll::EventSet; +use vmm_sys_util::eventfd::EventFd; + +const QUEUE_SIZE: usize = 32768; +const REQUEST_QUEUES: u32 = 1; +const NUM_QUEUES: usize = REQUEST_QUEUES as usize + 1; +const HIPRIO_QUEUE_EVENT: u16 = 0; +const REQ_QUEUE_EVENT: u16 = 1; +const MAX_TAG_LEN: usize = 36; + +#[derive(Debug)] +enum Error { + HandleEventNotEpollIn, + HandleEventUnknownEvent, Review Comment: 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: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; Review Comment: The same. We can use `message::Abc` in code if there are many messages. ########## bin/ovfs/src/virtiofs_utils.rs: ########## @@ -0,0 +1,241 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::cmp; +use std::collections::VecDeque; +use std::io::{self, Read, Write}; +use std::mem::{size_of, MaybeUninit}; +use std::ops::Deref; +use std::ptr::copy_nonoverlapping; + +use virtio_queue::DescriptorChain; +use vm_memory::bitmap::{Bitmap, BitmapSlice}; +use vm_memory::{ + Address, ByteValued, GuestMemory, GuestMemoryMmap, GuestMemoryRegion, VolatileMemory, + VolatileMemoryError, VolatileSlice, +}; + +/// The Error here represents shared memory related errors. +#[derive(Debug)] +pub enum Error { Review Comment: Please unify the error we return to users. ########## bin/ovfs/src/virtiofs.rs: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; +use vhost::vhost_user::Backend; +use vhost_user_backend::{VhostUserBackend, VringMutex, VringState, VringT}; +use virtio_bindings::bindings::virtio_config::*; +use virtio_bindings::bindings::virtio_ring::{ + VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, +}; +use vm_memory::{ByteValued, GuestMemoryAtomic, GuestMemoryMmap, Le32}; +use vmm_sys_util::epoll::EventSet; +use vmm_sys_util::eventfd::EventFd; + +const QUEUE_SIZE: usize = 32768; Review Comment: Please provide detailed descriptions for each `const` so we can understand their meanings. ########## bin/ovfs/src/virtiofs.rs: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; +use vhost::vhost_user::Backend; +use vhost_user_backend::{VhostUserBackend, VringMutex, VringState, VringT}; +use virtio_bindings::bindings::virtio_config::*; +use virtio_bindings::bindings::virtio_ring::{ + VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, +}; +use vm_memory::{ByteValued, GuestMemoryAtomic, GuestMemoryMmap, Le32}; +use vmm_sys_util::epoll::EventSet; +use vmm_sys_util::eventfd::EventFd; + +const QUEUE_SIZE: usize = 32768; +const REQUEST_QUEUES: u32 = 1; +const NUM_QUEUES: usize = REQUEST_QUEUES as usize + 1; +const HIPRIO_QUEUE_EVENT: u16 = 0; +const REQ_QUEUE_EVENT: u16 = 1; +const MAX_TAG_LEN: usize = 36; + +#[derive(Debug)] +enum Error { Review Comment: There are two `Error` in our crates, it's better to avoid this. ########## bin/ovfs/src/virtiofs.rs: ########## @@ -0,0 +1,213 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::io; +use std::sync::RwLock; + +use vhost::vhost_user::message::*; +use vhost::vhost_user::Backend; +use vhost_user_backend::{VhostUserBackend, VringMutex, VringState, VringT}; +use virtio_bindings::bindings::virtio_config::*; +use virtio_bindings::bindings::virtio_ring::{ + VIRTIO_RING_F_EVENT_IDX, VIRTIO_RING_F_INDIRECT_DESC, +}; +use vm_memory::{ByteValued, GuestMemoryAtomic, GuestMemoryMmap, Le32}; +use vmm_sys_util::epoll::EventSet; +use vmm_sys_util::eventfd::EventFd; + +const QUEUE_SIZE: usize = 32768; +const REQUEST_QUEUES: u32 = 1; +const NUM_QUEUES: usize = REQUEST_QUEUES as usize + 1; +const HIPRIO_QUEUE_EVENT: u16 = 0; +const REQ_QUEUE_EVENT: u16 = 1; +const MAX_TAG_LEN: usize = 36; + +#[derive(Debug)] +enum Error { + HandleEventNotEpollIn, + HandleEventUnknownEvent, + CreateKillEventFd(io::Error), +} + +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{self:?}") + } +} + +impl std::convert::From<Error> for io::Error { + fn from(e: Error) -> Self { + io::Error::new(io::ErrorKind::Other, e) + } +} + +type Result<T> = std::result::Result<T, Error>; + +type VhostUserBackendResult<T> = std::result::Result<T, std::io::Error>; + +impl std::error::Error for Error {} + +struct VhostUserFsThread { Review Comment: For each struct, please provide a brief description of its purpose. Refer to the opendal core for guidance. If this PR is too large, we can add the descriptions gradually. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
