tcmu_dev_get_memory_info() #588
Conversation
|
Thank you everyone for your comments (and dedication to keeping the source and history clean). Note: I couldn't stomach making the goto err_* maze in add_device() even more complex, so the allocs are now at the start of the function. Both must succeed or -ENOMEM is returned. |
|
@dfsmith All the others looks good to me. |
Fix the unwinding so it works like the other code. It may be more complex to you, but not to the others that are used to the linux kernel style of goto error unwinding. |
libtcmu.c
Outdated
| mmap_name = tcmu_dev_get_memory_info(dev, NULL, NULL, &mmap_offset); | ||
| if (!mmap_name) { | ||
| free(dev); | ||
| dev = NULL; |
There was a problem hiding this comment.
None of the other code in this repo works like this, so when scanning and handling errors we would have to account for the goto style and your style. Use the goto style so it matches the existing code and so everyone else does not have to adjust for your style.
There was a problem hiding this comment.
No problem! This is a slightly more invasive patch*, but is more in keeping with the rest of the file.
Let me know if you need it rebased.
(On a philosophical note, my original coding choice was because the "goto unwind" style is error-prone when re-ordering resource allocations with many exit paths. add_device(), specifically, is long and complex. I'd quite like to split this procedure into separate nested "allocation" and "discovery" functions, but it would be a more disruptive patch.)
- Mostly because there was another (misnamed?) "ret" variable.
There was a problem hiding this comment.
I know what you mean. I agree with you that add_device is getting nasty. Feel free to break it up. It would be really nice.
There was a problem hiding this comment.
I broke up add_device() and renamed it device_add() for (sorting) consistency with its subroutines. The chiastic structure of the goto unwind is now more apparent. Since this is a trivial but substantial change, it might be worth another pair of eyes following the device_add() logic. I believe it to be equivalent to the master code (with some extra checks and one minor bug fixed).
|
Just some minor coding style nits. I swear I am not trying to be a pain in the butt about that :) Thanks for the updated patch with all the cleanups! |
Split add_device() into multiple functions. Rename add_device() -> device_add(). Rename remove_device() -> device_remove().
|
Thank you for spotting the nits. Keep it up! |
|
Thanks! |
Our handler/device needs more information about the tcmu shared memory block (so that it can pass offsets to another process). This patch adds a function to retrieve that information about the shared memory.
Also updated install_dep.sh so that it works with Debian installations.