Reinstall libtcmu API and header files #444
Conversation
This reverts commit 8727a38.
This reverts commit a00cb71.
Separate tcmu-runner.h from libtcmu_priv.h and include it explicitly where it's necessary. We will export libtcmu_priv.h as part of libtcmu in the next patch. Signed-off-by: Yaowei Bai <[email protected]>
Signed-off-by: Yaowei Bai <[email protected]>
Signed-off-by: Yaowei Bai <[email protected]>
|
Hi Mike, could you please take a look at this patchset at your convenience? Thanks. @mikechristie |
|
Sorry. I misread it the first time. I thought it depended on your discussion with Xiubo on the PR. Will get to it today. |
|
@mikechristie If we change the whole code one time in a PR it will be very hard to review, it should be okay for me to split this in different small PRs to make it easier. And IMO, the goal is to make the libtcmu.so include and provide the following features and APIs: |
|
Yes, small PRs should be more easier to review. We could start from this PR and implement other features one by one. This should make sense. @mikechristie |
| @@ -272,7 +272,7 @@ configure_file ( | |||
| ) | |||
| install(SCRIPT tcmu.conf_install.cmake) | |||
|
|
|||
| install(FILES libtcmu.h libtcmu_common.h tcmu-runner.h | |||
| install(FILES libtcmu.h libtcmu_common.h libtcmu_priv.h darray.h | |||
There was a problem hiding this comment.
For the final lib we are not going to have users use/include the private definitions. The lib user should also not need the darray.h header unless we expose an api that uses a darrary.
There was a problem hiding this comment.
While the tcmulib_context and tcmu_device structs in libtcmu_priv.h are needed by users and tcmulib_context depends on darray.h currently. Maybe we should rename libtcmu_priv.h as it will not 'private'.
There was a problem hiding this comment.
The lib users should not need to ever directly interact with tcmu_device. It is internal to the lib. The only interact with it via libtcmu helpers.
There was a problem hiding this comment.
Check out how consumer.c or tcmu-synthesizer worked with libtcmu originally for an example. They never directly interacted with tcmu_device so we could make internal changes and not have to update every libtcmu user.
There was a problem hiding this comment.
Oh yeah for libtcmu_context and darray that should not be an issue. The users of libtcmu only pass around a tcmulib_context struct pointer. They never access it, so the forward declaration in the libtcmu.h should be enough.
There was a problem hiding this comment.
I see. I'll tune this as your suggestion. Thansk.
| #include "darray.h" | ||
| #include "ccan/list/list.h" | ||
| #include "tcmur_aio.h" | ||
|
|
| @@ -23,6 +23,7 @@ | |||
| #include "libtcmu_log.h" | |||
| #include "libtcmu_common.h" | |||
There was a problem hiding this comment.
This patch seems ok.
CMakeLists.txt
Outdated
| @@ -272,6 +272,8 @@ configure_file ( | |||
| ) | |||
| install(SCRIPT tcmu.conf_install.cmake) | |||
|
|
|||
| install(FILES libtcmu.h libtcmu_common.h tcmu-runner.h | |||
There was a problem hiding this comment.
Won't the final new lib have a new header name so it does not conflict with users of the old libtcmu?
Also wether the final lib needs something like tcmu-runner.h is still up in the air.
|
I agree small patchsets are best. The include related cleanup patches in this patches are nice on their own. So these patches: [PATCH 3/5] libtcmu: explicitly include tcmu-runner.h where necessary I will merge now. The final piece would be to install the new libtcmuXYZ and the headers again. I do not think we can install them now because they are going to change in the end, and also the libtcmu that would be installed now will not be supported in the end, so we do not want releases installing libs that are not supported. So these patches like these should be sent at the end when you have a lib to support: [PATCH 1/5] Revert "build: drop versionless libtcmu.so symlink" you would do at the end. |
The separation work of libtcmu and tcmu-runner seems harder than we thought and is going to take a bit more time to achieve. Let's reinstall libtcmu header files and API first for now as the future sepatation work can still base on this to move on and qemu-tcmu can also make a little forward with this change. @mikechristie @amarts @lxbsz
Both tcmu-runner and qemu-tcmu can build successfully with this patchset. Please review.