assertion in debug mode when destroying zvol (#65) #15
Conversation
lib/libzpool/uzfs_mgmt.c
Outdated
| @@ -323,6 +326,8 @@ uzfs_open_dataset_init(spa_t *spa, const char *ds_name, zvol_state_t **z) | |||
| disown_free: | |||
| dmu_objset_disown(zv->zv_objset, zv); | |||
| free_ret: | |||
| if (spa != NULL) | |||
There was a problem hiding this comment.
I think if we are here, we have spa non-null, so to me check is un-necessary.
|
@vishnuitta : we need data type change because if all spa, zvol, etc. pointers are declared as void pointers then compiler cannot check that we are passing the right pointer types to the functions. So basically it is about being able to uncover more bugs during compilation phase. More disastrous scenarios can happen if we mix data types which are more different (i.e. numeric types and pointers). And the c file which defines the functions must always include the prototypes from header file so that compiler can check that they are the same. If not that could lead to nasty bugs ... |
Signed-off-by: Jan Kryl <jan.kryl@cloudbyte.com>
|
@jkryl Idea of test framework was to go through uzfs_* APIs. If any functionality is required, those need to be provided from uzfs APIs so that zpool libraries become independent for making any testing tools. |
|
any suggestions @gila , @satbirchhikara on ^^^ ?? |
|
Vitta, those header files are not consumed only by test libraries but also contain prototypes of functions used in production code. So all the possible inconsistencies which are very difficult to discover unless you include the header file in c file apply to production API as well. So firstly if this hack was done to solve some problem in test API then we need two header files, strictly decoupling production from test header file. Secondly, I don't understand what abstraction layer we are trying to build here. What advantage does it have for our test code if for example instead of spa_t we can pass any pointer type? Everything in test library is then void * pointer and you can guess only by name what type of pointer it is. I don't see a single advantage of this approach. |
|
Got to see that these header files are used in production code also, which is not intended. We need to separate these header files for production and test code. |
|
thanks Vitta |
The core of the change is about fixing references placed on spas from uzfs code. Each hold should have a unique tag because it is done on behalf of a different zvol (instead of a global tag). The other change is that the reference should not be bound to zinfo structure but rather to zvol_state structure, because that is the structure containing the actual spa reference. The code needs to be very clear in this regard.
Minor change bundled to this fix has been done to header and test files. It is not acceptable to define function in header files with different prototypes than real definitions in c files. And of course the c file which defines the functions needs to include the header file where the function is declared. That was quite messy and now that has been fixed.