Closed FaerieLee closed 8 months ago
Hello,If you think it makes sense, please reply, if not, please reply, looking forward to your reply,Thanks.
Hi @FaerieLee ,
Sorry I was out when this was created.
1 - Actually the only usage of fuse_parse_cmdline
is in fuse_setup->fuse_main_real
which always provide a valid variable.
https://github.com/dokan-dev/dokany/blob/ce8b8229ad8262f920ca8d284448c1fe10e46cae/dokan_fuse/src/dokanfuse.cpp#L810-L815
The code is "robust" but just does not makes sense now. It is probably a legacy of a previous logic.
2 - I understand but that's how fuse is designed even in the latest API version http://libfuse.github.io/doxygen/structfuse__operations.html#ae2d73c7b5604a9fddffec229001d4556
1 - you are right. but in our code, we direcly use fuse_setup, not fuse_main_real, accroding to the routine of libfuse. So if directly call fuse_setup in the case of mountPoint=nullptr, the process would crash. Is need to add logic to check mountpoint in exported funcion fuse_setup ?
2 - void (init) (void userdata, struct fuse_conn_info conn); belong to the lowlevel api in the libfuse. void ( fuse_operations::init) (struct fuse_conn_info conn, struct fuse_config *cfg) belong to high level api, in the function, it doesn't carry private_data, So current design of dokany is consistent with the high-level api of libfuse. But I still have doubts about the design of libfuse.
1 - Oh I missed that! Then yeah I believe in fuse_parse_cmdline
we should have:
if (mountpoint)
*mountpoint = hopts.mountpoint;
else
+ goto err;
if (multithreaded)
*multithreaded = !hopts.singlethread;
if (foreground)
*foreground = hopts.foreground;
return 0;
err:
+ if (hopts.mountpoint)
+ free(hopts.mountpoint);
return -1;
}
What do you think ? If you agree, I believe it is rightful that you open a pull request since you found this but I can also do it
I think that the modification is perfect. Can i submit the code using your above modification ? It's really a pleasure to be able to contribute to open source projects. I really like it.
Yes go for it!
@FaerieLee Have you been able to prepare a pull request ?
Feature request can skip this form. Bug report must complete it.
Check List
must be 100% match or it will be automatically closed without further discussion. Please remove this line.Environment
Check List
Description
Question1: The following is the implementation of the function fuse_parse_cmdline
if mountpoint == nullptr, the code wuld free the hots.mountpoint and return 0. In the implementation of funtion fuse_setup
So in the case of that mountpoint == nullptr, it will continue to execute function fuse_mount. the process would crash because of dereferencing null pointer. The dokanfuse.dll doesn't only recognize invalid argument, but also result to crash. I think the code is not robust. what do you think ?
Question2: In the struct fuse_operations, the parameters of init funtion pointer hasn't variable private_data.
void *(*init) (struct fuse_conn_info *conn);
In the case of using c++ to build filesystem, it's possible to call fuse_setup in the oridinary funtion. but call init only in the static function, how to setup variable private_data which is the member of c++ class in the static function without any information ? I think this design is unreasonable . In the library libfuse, the defination of init function pointer is following:void (*init) (void *userdata, struct fuse_conn_info *conn);
So, is it necessary to upgrade init funtion pointer ?Thanks
Logs
Only some doubts, no log.