Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libtcmu: allow multiple daemons co-exist #519

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

baihuahua
Copy link

@baihuahua baihuahua commented Dec 25, 2018

As we support libtcmu again users could want to run several
daemons at the same time. So don't fail the device addition
via netlink reply when can't find a handler in ctx to handle
the device. This just means the device addition event is not
for THIS ctx so give other daemons a chance to try.

This fix is for issue #516 .

Signed-off-by: Yaowei Bai [email protected]

libtcmu.c Outdated
@@ -432,8 +434,8 @@ static int add_device(struct tcmulib_context *ctx, char *dev_name,

dev->handler = find_handler(ctx, dev->cfgstring);
if (!dev->handler) {
tcmu_err("could not find handler for %s\n", dev->dev_name);
goto err_free;
tcmu_dbg("could not find handler for %s\n", dev->dev_name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a warning tcmu_warn () ?
For tcmu-runner case, it is helpful to have this msg at logs with non-debug log levels too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense to me, thanks.

libtcmu.c Outdated
err_nohandler:
free(dev);

return 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, It makes more sense to return -1/1 for all those str***() function errors, and return ENOENT for nohandler case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will fix it, thanks.

As we support libtcmu again users could want to run several
daemons at the same time. So don't fail the device addition
via netlink reply when can't find a handler in ctx to handle
the device. This just means the device addition event is not
for this ctx so give other daemons a chance to try.

Signed-off-by: Yaowei Bai <[email protected]>
@baihuahua
Copy link
Author

Updated, please review. Thanks. @pkalever @mikechristie

@baihuahua
Copy link
Author

Hey Mike, what do you think about this? @mikechristie

@mikechristie
Copy link
Collaborator

I am on PTO from Jan 28 - Feb 8. I am going to try and get to this next week while on vacation.

@bgly
Copy link
Contributor

bgly commented Mar 6, 2019

This looks fine to me.

@baihuahua
Copy link
Author

Thanks @bgly .Mike, what do you think about this? @mikechristie

@bno1
Copy link

bno1 commented Jun 22, 2020

targetcli hangs if the added device subtype is not found by any daemon. Restarting tcmu-runner solves the problem because it resets the netlink interface, but this is not a good solution. Also consider that a custom daemon might crash or be killed during the handling of TCMU_CMD_ADDED_DEVICE, which would also cause targetcli to hang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants