POC: Add logging function callback

[tasleson]

I spent some time today exploring if have a call back function which was embedded in the nvme_root_t to prevent global data would be possible. This is rough, and incomplete, and not even run once for testing.

I created a macro nvme_msg_n which takes an additional argument which is the nvme_root_t pointer. In some cases its use is pretty clean:

nvme_msg_n(r, LOG_ERR, "Failed to write to %s, %s\n", ...

In other cases, kind of ugly and I'm not even sure if all the embedded pointers are correctly setup for the context.

nvme_msg_n(c->s->h->r, LOG_ERR, "failed to resolve host %s info\n", c->traddr);

In some other cases we have nothing existing to get a handle to the nvme_root_t, so I added an argument to the function. In other cases I just removed the logging and returned an error code.

At the moment I'm not sure how to make this cleaner. Thoughts, suggestions?

[tasleson]

Could resolve: #187

[igaw]

Yes, this should just return the error code.

[igaw]

I don't mind these error handling cleanups in inet*(). But it would be better to go into a separate patch.

[igaw]

Indeed, looks a bit ugly. Though I don't have a good idea how to do it differently.

[igaw]

You can't remove this one. This is a public function (even documented) see

libnvme/src/nvme/linux.h

Line 83 in 5aea021

int __nvme_get_log_page(int fd, __u32 nsid, __u8 log_id, bool rae,

Bad name. We should rename it. Though I don't have a better name yet.

[tbzatek]

Interesting concept, however to be really useful I would need a context passed to log_fn - a custom pointer (user_data) set along with log_fn, passed back within the callback. From a library user perspective getting no context means I would still need to use TLS for temporary storage and in the case of nvme_root_t I would need an internal translation table. Setting logging function per-nvme_root_t could only help to differentiate between logging codepaths, but connection to my internal object would be still missing.

[tasleson]

@tbzatek

Interesting concept, however to be really useful I would need a context passed to log_fn - a custom pointer (user_data) set along with log_fn, passed back within the callback. From a library user perspective getting no context means I would still need to use TLS for temporary storage and in the case of nvme_root_t I would need an internal translation table. Setting logging function per-nvme_root_t could only help to differentiate between logging codepaths, but connection to my internal object would be still missing.

Your original comment on this #160 (comment) didn't have user supplied data. We could add a user supplied data pointer when registering the callback and store it in the nvme_root_t structure. I'm assuming each thread of execution is retrieving their own nvme_root_t correct? Otherwise the library caller could embed the callback in a structure and leverage things like container_of approach to associate the callback with some other data for correlation.

[tbzatek]

Yes, my original proposal was about general logging function with no context as everything was on a global level. But now since you would be operating on a local nvme_root_t instance, supplying an additional user data pointer when registering a particular logging callback shouldn't create much more mess and would greatly improve usability.

I'm assuming each thread of execution is retrieving their own nvme_root_t correct?

Not necessarily but I understood it should be a good practice. However heavy async app may create nvme_root_t somewhere and push an actual operation over it to a worker thread pool elsewhere. I expect the whole codepath should operate on a private nvme_root_t instance though.
--> And this should be documented somewhere...

[tasleson]

Not necessarily but I understood it should be a good practice. However heavy async app may create nvme_root_t somewhere and push an actual operation over it to a worker thread pool elsewhere. I expect the whole codepath should operate on a private nvme_root_t instance though.

I believe we are in agreement that a single root_nvme_t shouldn't be used concurrently. Obtaining a handle in one thread, passing it to another for processing would adhere to this.

If/when I update this PR I'll include a user supplied data pointer. I can see how that would be beneficial in a multi-threaded app.

[hreinecke]

Can you check PR #202 ? That does most what you want; if needed we could even add a logging function callback to nvme_root_t, but I guess with being able to redirect the message logging to a different filedescriptor we should be mostly there.

[igaw]

Let's close this PR as we have some good progress in #202.