Add support for standalone binaries as ublk targets #91
Conversation
The first is now the main binary for ublk and the latter is helper functions that ublk and in-tree targets needs. The aim is to move the built-in helpers to be standalone binaries, one binary for each backend target. With ublk.null to replace tgt_null.cpp and have it be spawned off the main ublk binary when adding a device. Like how mount -t cifs will spawn off to mount.cifs when mounting a cifs filesystem. Signed-off-by: Ronnie Sahlberg <[email protected]>
sahlberg
force-pushed
the
execve-3
branch
2 times, most recently
from
aaa39c7 to
ebd59b3
Compare
|
Hi @sahlberg I think this approach is super nice, thanks! Just a minor point, can we remove ublksrv_find_tgt_type() and always call ublksrv_execve_helper() for starting null, loop, and nbd? Meantime cmd_dev_add()/mkpath() or anything common can be put in ublksrv_tgt.cpp will become one second level library for all built-in targets. Finally each target can be started independently or run from ublk just like now. Thanks |
|
Yes to everything you said. Great that you approve of the approach. I will do this and update the pull request over the next few days. (There will be a lot of additional simplifications once that lands. For example, do we really need (struct ublk_tgt_type *)->type |
|
BTW, the standalone ublk.* executable can be switched to run in foreground Then we can simplify & cleanup more. This way can be done after the execve cleanup is completed. Thanks, |
|
Still a work in progress. loop and nbd are converted too but need to decide how to handle "ublk recover". Until now it has only worked for built-in targets. |
Signed-off-by: Ronnie Sahlberg <[email protected]>
If -t <target> does not represent a built-in target, then
instead try to pass everything on to execve("ublk.<target>", ...)
Signed-off-by: Ronnie Sahlberg <[email protected]>
The standard options are a set of common options that apply to all types of backends. Move the parsing and populating of struct ublksrv_dev_data into a helper function so that we do not ave to duplicate this code for all targets. Signed-off-by: Ronnie Sahlberg <[email protected]>
A common helper that prints all the standard options when adding a target. Signed-off-by: Ronnie Sahlberg <[email protected]>
Make the loop backend a standalone binary
that are invoked by execve("ublk.loop", ...) from
the main control binary when adding loop targets.
Signed-off-by: Ronnie Sahlberg <[email protected]>
that is is invoked by execve("ublk.nbd", ...) from the main
control binary when creating nbd targets.
Signed-off-by: Ronnie Sahlberg <[email protected]>
This function is now agnostic to target type and identical for the current three targets. Move it into the helper library. Signed-off-by: Ronnie Sahlberg <[email protected]>
We need to pass the command to the target binaries so that we can call it with different commands such as "add", "recover", ... Signed-off-by: Ronnie Sahlberg <[email protected]>
So that we can spawn the correct -t <type> backend and have it print the usage information, including type specific arguments. Signed-off-by: Ronnie Sahlberg <[email protected]>
All targets are now invoked via execve("ublk.<target>", ...)
so we can trim down cmd_dev_add() to only handle the execve case.
Signed-off-by: Ronnie Sahlberg <[email protected]>
This change does introduce a user-visible change. It is now required that you must specify -t <type> when using "ublk recover" Signed-off-by: Ronnie Sahlberg <[email protected]>
sahlberg
changed the title
|
Please review. I think it is tarting to come together. |
Yes, recover is basically same with 'add'. |
| @@ -476,6 +296,22 @@ static int cmd_dev_help(int argc, char *argv[]) | |||
| return EXIT_FAILURE; | |||
| } | |||
|
|
|||
| static int cmd_dev_recover(int argc, char *argv[]) | |||
There was a problem hiding this comment.
We can avoid the user-visible 'recover' command line change, the target type can be retrieved from the pid file from device number, and pid file is required for recover feature.
ublk.cpp
Outdated
| for (i = 1; i < argc; i++) | ||
| nargv[i + 1] = argv[i]; | ||
|
|
||
| return execve(nargv[0], nargv, nenv); |
There was a problem hiding this comment.
Here the whole path of ublk need to be taken into account, such as:
cd tests
/$HOME/git/ublksrv/tests/../ublk add -t null
nothing is dumped, and $? is 255, and this way actually fails all built test, such as:
make test T=generic/003
I think this PR is ready to go after the two comments are addressed. Thanks! |
In the execve helper, ensure that when we spawn a target binary that we always run it as an absolute path and that the target is in the same binary as the ublk helper itself. For example if the ublk binary is in /home/sahlberg/ublksrv/.libs/ublk then for type "loop" the binary we will execve will be /home/sahlberg/ublksrv/.libs/ublk.loop Additionally, copy the environment variable LD_LIBRARY_PATH from the ublk binary to the environment for the target binary. In a local build environment, when you call the binary directly without going through libtool the spwaned binary might not find the libublksrv.so library. In test systems where you have not installed ublksrv you may or may not need to set LD_LIBRARY_PATH=`pwd`/lib/.libs Signed-off-by: Ronnie Sahlberg <[email protected]>
|
BTW, I add commit 6ae5566 ("ublksrv_tgt: don't require recover command to provide tgt_type") to remove '-t target_type' command line from 'ublk recovery', so that all tests can run successfully as before. Thanks! |
This series breaks the main ublk functionality oout from ublksrv_tgt into a new file ulbk.cpp, leaving ublksrv_tgt as set of helper functions.
It converts tgt_null into a standalone binary, ublk.null, and adds support in ublk to execve("ublk.", ...)
if is not a built-in target.
If/when this is an agreeable approach, I can convert the loop and the nbd targets the same way. And by then there are no longer any built-in targets in ublk and we can simplify ublk.cpp greatly.