Page MenuHomeFreeBSD

libusb: replace DEBUG_LEVEL with LOG_LEVEL
AcceptedPublic

Authored by aokblast on Wed, Jun 4, 10:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 25, 7:14 PM
Unknown Object (File)
Wed, Jun 25, 6:58 PM
Unknown Object (File)
Wed, Jun 25, 1:13 PM
Unknown Object (File)
Wed, Jun 25, 7:07 AM
Unknown Object (File)
Tue, Jun 24, 6:07 PM
Unknown Object (File)
Tue, Jun 24, 5:51 PM
Unknown Object (File)
Tue, Jun 24, 5:48 PM
Unknown Object (File)
Tue, Jun 24, 5:15 PM
Subscribers

Details

Reviewers
bapt
kevans
Group Reviewers
USB
Summary

The function "libusb_set_debug" s et level level by libusb_log_level instead of
libusb_debug_level. To be batter compatible with libusb interface.
We use the libusb_log_level as the original libusb used.

Also, delete the debug_level enum to force maintainer upgrade from their
legacy source code.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 64816
Build 61699: arc lint + arc unit

Event Timeline

aokblast retitled this revision from libusb: replace LOG_LEVEL with DEBUG_LEVEL to libusb: replace DEBUG_LEVEL with LOG_LEVEL.Wed, Jun 4, 10:02 AM
aokblast added a reviewer: bapt.

DPRINTF usually is a FreeBSD specific debugging printf which needs to be enabled at compile time (not in this case) and is purely for developers. It is not uncommon to have different flags depending on what part to debug.

I do not fully understand the reasoning that prompted this change? Can you elaborate a bit more why this change is needed?

I am fixing https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277653.

In D50680#1156990, @bz wrote:

DPRINTF usually is a FreeBSD specific debugging printf which needs to be enabled at compile time (not in this case) and is purely for developers. It is not uncommon to have different flags depending on what part to debug.

I do not fully understand the reasoning that prompted this change? Can you elaborate a bit more why this change is needed?

The libusb provides libusb_set_debug to decide which level is the debug output.

See: https://github.com/libusb/libusb/blob/15a7ebb4d426c5ce196684347d2b7cafad862626/libusb/core.c#L2213

However, the meaning of "level" in FreeBSD libusb is different from the original implementation. The original implementation indicate the critical level on the message instead of the FreeBSD one, which indicates the source where the error emit.

This cause the application would like to set debug level (like stlink) unable to use the correct level because they do not have the same meaning.

I think this is a API incompatible between these two different version of libusb.

Also, it seems that the DPRINTF in FreeBSD libusb is not a compile time macro as

		if ((ctx)->debug & LIBUSB_DEBUG_FUNCTION) {	\
			printf("LIBUSB_FUNCTION: "		\
			       format "\n", ## __VA_ARGS__);	\
		}

ctx->debug can be set at runtime by libusb_set_debug.

Replace implementation as a function call to prepare support for log callback

Use vsprintf as callback request a string instead of a fmt + va_args

Rebase to a correct version

kevans added inline comments.
lib/libusb/libusb10.c
1802

va_start should move to after the ctx->debug check; I think this is technically UB as-is to have a return-path that doesn't va_end it, and we don't really need to start it this early.

1809

Is the round-trip through buffer and using fputs entirely because you get warnings/errors about format strings not being a literal when you try to use vfprintf with new_fmt?

lib/libusb/libusb10.h
49

This should likely be tagged with __printflike(3, 4) from <sys/cdefs.h>

Fix comment's error and rebase to main

lib/libusb/libusb10.c
1809

This is needed because libusb upstream provide a option to overwrite the log with user defined callback. In the prototype of callback, it accept a string instead of a va_args. So we should build the pure string before we put into the callback function.

See: https://reviews.freebsd.org/D50818

kevans added inline comments.
lib/libusb/libusb10.c
1809

Ahh, ok, that's incredibly helpful context, thanks!

This revision is now accepted and ready to land.Thu, Jun 12, 10:09 PM

@bz Is there still any concern about this patch? @bapt and what is your opinion about this?
If there is no any problem, I will prepare a github link for merging.