Skip to content

hidraw: Implement hid_error()#23

Merged
Youw merged 5 commits intolibusb:masterfrom
z3ntu:pr_125
Jul 15, 2019
Merged

hidraw: Implement hid_error()#23
Youw merged 5 commits intolibusb:masterfrom
z3ntu:pr_125

Conversation

@z3ntu
Copy link
Collaborator

@z3ntu z3ntu commented Jun 17, 2019

Here is my first try at implementing error messages for the Linux/hidraw implementation.

A few extra questions:

  • I am not sure why in utf8_to_wchar_t the input is called utf8. As far as I know, the encoding used in mbstowcs depends on the current locale, if if I haven't set my locale to use UTF8, then the input is not UTF8.
  • The register_error equivalent in windows/hid.c doesn't use its argument const char *op. Is that intended?
  • In the Windows implementation, do functions like HidD_GetPreparsedData actually set the error code for GetLastError()?

Originally signal11/hidapi#125

@Youw
Copy link
Member

Youw commented Jun 17, 2019

When a client code gets an "error string" as char* (or, in our case wchar_t*), it likely assumes, that that is a valid c-string, and for instance try to:

printf("Last HIDAPI call status: %S", hide_error());

and that would immediately lead to crash, since hide_error() sometime returns NULL.

I'd suggest always return a valid c-string (non-NULL) instead.
If there is no error, we might return "" or "Success", or smth like that.

@Youw
Copy link
Member

Youw commented Jun 24, 2019

my suggestion stands

@z3ntu if you accept - please update PR accordingly, otherwise give your note why you wouldn't do it within this PR; maybe we'll have a separate task for it

@z3ntu
Copy link
Collaborator Author

z3ntu commented Jun 25, 2019

Yes, will update it. I've just been travelling a lot in the past couple of days and spending time on other projects inbetween.

Do you think it's better that if the register_global_error function receives a NULL argument, that it sets the last_global_error_str variable to Success or that the register_global_error function should be called with parameter Success? I'm personally for the first option.

@Youw
Copy link
Member

Youw commented Jun 25, 2019

  • first looks OK
  • register_global_success is an alternative
  • hid_error could return "Success" if last_global_error_str is NULL

to be honest I'm more concerned of usage on the user side, and I'll support any reasonable good implementation, so it is up to you to decide
(and yes, I think passing "Success" in each place wouldn't look very nice)

@z3ntu
Copy link
Collaborator Author

z3ntu commented Jun 25, 2019

The last sounds the best imo

@z3ntu z3ntu force-pushed the pr_125 branch 2 times, most recently from 9683cba to 06a534f Compare July 3, 2019 15:04
@z3ntu
Copy link
Collaborator Author

z3ntu commented Jul 3, 2019

Should be good for a review now

@Youw
Copy link
Member

Youw commented Jul 9, 2019

@jdk, @todbot, @Qbicz your comments?

@Youw Youw merged commit 7df3973 into libusb:master Jul 15, 2019
@z3ntu z3ntu deleted the pr_125 branch July 15, 2019 12:08
Copy link
Member

@Youw Youw left a comment

Choose a reason for hiding this comment

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

Looks like documentation has to be updated(


Whether a function sets the last error is noted in its
documentation. These functions will reset the last error
to NULL before their execution.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks to comment from @canselcik, we see, that the part about NULL isn't true.


Strings returned from hid_error() must not be freed by the user!

This function is thread-safe, and error messages are thread-local.
Copy link
Member

Choose a reason for hiding this comment

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

And it is not thread-safe, when you're using same device from different threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hidraw Related to Linux/hidraw backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments