Redirect error messages

[hreinecke]

This patchset adds an 'nvme_root_t' argument to nvme_msg(), and allows to store a different filedescriptor than 'stderr' to store in nvme_root_t. With that applications can re-route the error messages.

[igaw]

I suppose we could also add

extern int nvme_log_level;                                                                                                                                                                                             
extern bool nvme_log_timestamp;                                                                                                                                                                                        
extern bool nvme_log_pid;          

and move the log.h header to private

[tbzatek]

One possible concern I can think of might be a situation where an user supplied filedescriptor's buffer gets full and any writes become blocking. Say if I provide a pipe then running a side mainloop to read the other end might not be always feasible in a library.

[hreinecke]

Whee. Now we're getting into obscure corner-cases. I guess one could always construct cases where any attempt will fail in one or the other way; eg the same argument could be brought if we would allow for a (user-provided) callback function to print out the messages.
And really, the only way preventing that would be to add asynchronous logging. Which, for this tiny library, is a bit of an overkill.
I'd rather keep is simple and see how far we get with this; once we have real failure cases we can get them fixed.
But trying to address theoretical concerns before we even have the first real-world user is not getting us anywhere.

[igaw]

I am working on adapting nvme-cli to these changes. One thing I noticed:

        if (devicename)                                                                                                                                    
                r = nvme_scan_filter(nvme_match_device_filter, stderr, cfg.verbose);                                                                       
        else                                                                                                                                               
                r = nvme_scan(NULL);                                                                                                                       
                                                                                                                                                           
        if (r) {                                                                                                                                           
                set_log_level(r, cfg.verbose, false);    

set_log_level() is new local helper to map the verbose levels to the right level. The thing which looks a bit strange is why nvme_scan_filter() wants a filedescriptor but nvme_scan() not. This bit looks a bit strange to me.

[hreinecke]

I am working on adapting nvme-cli to these changes. One thing I noticed:

        if (devicename)                                                                                                                                    
                r = nvme_scan_filter(nvme_match_device_filter, stderr, cfg.verbose);                                                                       
        else                                                                                                                                               
                r = nvme_scan(NULL);                                                                                                                       
                                                                                                                                                           
        if (r) {                                                                                                                                           
                set_log_level(r, cfg.verbose, false);    

set_log_level() is new local helper to map the verbose levels to the right level. The thing which looks a bit strange is why nvme_scan_filter() wants a filedescriptor but nvme_scan() not. This bit looks a bit strange to me.

That's because 'nvme_scan()' takes the json config file as an argument, whereas nvme_scan_filter() is using the filedescriptor to redirect the error messages to.

Prime reason here was that I didn't want to modify 'nvme_scan()', as this is the main entry point into libnvme, and modifying that would require a modification of all programs using libnvme. Which I've tried to avoid.

But if you're fine with it I can easily add a 'log_level' parameter to nvme_scan() to make things more obvious.

[igaw]

Well, it's just a bit irritating to me that one can set the log level and file descriptor directly and the other needs an extra step.
I suppose this here would calm my OCD:

nvme_scan(NULL, stderr, level);

And my obvious question was, what happens when nvme_scan wants to log something?

[hreinecke]

This should address it.
Idea is to have nvme_scan() as a high-level interface (basically a 'make fine' button).
If one want to have more control over it the various steps (nvme_init_root(), nvme_scan_topology(), and nvme_read_config()) can be called directly.

[hreinecke]

Of course, that kills 'nvme_scan_filter()', but I always found the name slightly counter-intuitive, so I'm not that bothered.

[igaw]

Looks good. I think we want to do the nvme_init_root nvme_scan_topology steps then in nvme-cli. This gives us the needed way to set the log level.

[igaw]

Just one nitpick. Why not nvme_create_root()? There is nvme_create_ctrl()?

[hreinecke]

That should do it.
I also wanted to rename nvme_free_tree() to nvme_free_root(), but didn't do it.
Should be a separate patch.

[igaw]

Thanks. If you want to be extra correct the commit message would also need an update :)

[igaw]

Let's get those things sorted out now. I'd like to spin the next release candidate today.

[igaw]

Thanks! I'll update my PRs now.