Details

Reviewers

bapt
kevans
lwhsu
markj

Group Reviewers

USB

Commits

rGbca5df45652a: libusb: replace LOG_LEVEL with DEBUG_LEVEL

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 64834
Build 61717: arc lint + arc unit

Event Timeline

aokblast created this revision.Jun 4 2025, 10:01 AM

Herald added a subscriber: imp. · View Herald TranscriptJun 4 2025, 10:01 AM

aokblast requested review of this revision.Jun 4 2025, 10:01 AM

Harbormaster completed remote builds in B64635: Diff 156524.Jun 4 2025, 10:01 AM

aokblast retitled this revision from libusb: replace LOG_LEVEL with DEBUG_LEVEL to libusb: replace DEBUG_LEVEL with LOG_LEVEL.Jun 4 2025, 10:02 AM

aokblast added a reviewer: bapt.

bapt added a reviewer: USB.Jun 4 2025, 11:37 AM

Use >= instead of or

Harbormaster completed remote builds in B64637: Diff 156527.Jun 4 2025, 12:28 PM

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

Harbormaster completed remote builds in B64793: Diff 156875.Jun 12 2025, 12:44 PM

aokblast edited the summary of this revision. (Show Details)Jun 12 2025, 3:20 PM

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

Harbormaster completed remote builds in B64811: Diff 156908.Jun 12 2025, 3:22 PM

Fix indent

Harbormaster completed remote builds in B64813: Diff 156910.Jun 12 2025, 3:33 PM

Output to stdout

Harbormaster completed remote builds in B64816: Diff 156915.Jun 12 2025, 3:59 PM

libusb: add option

Harbormaster completed remote builds in B64817: Diff 156916.Jun 12 2025, 4:03 PM

Rebase to a correct version

Harbormaster completed remote builds in B64819: Diff 156918.Jun 12 2025, 4:04 PM

aokblast added a child revision: D50818: libusb: implement libusb_set_option.Jun 12 2025, 4:09 PM

kevans added a subscriber: kevans.Jun 12 2025, 8:54 PM

kevans added inline comments.

lib/libusb/libusb10.c
1832	`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.
1839	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
51	This should likely be tagged with `__printflike(3, 4)` from `<sys/cdefs.h>`

Fix comment's error and rebase to main

Harbormaster completed remote builds in B64834: Diff 156944.Jun 12 2025, 10:03 PM

aokblast added inline comments.Jun 12 2025, 10:03 PM

lib/libusb/libusb10.c
1839	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 accepted this revision as: kevans.Jun 12 2025, 10:09 PM

kevans added inline comments.

lib/libusb/libusb10.c
1839	Ahh, ok, that's incredibly helpful context, thanks!

This revision is now accepted and ready to land.Jun 12 2025, 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.

Please use this patch to merge. Thanks! https://github.com/aokblast/freebsd-src/commit/e404814eed946bcd6551d163dbfeeeb951bf935b.patch

fix format

This revision now requires review to proceed.Jul 8 2025, 11:03 AM

Harbormaster completed remote builds in B65267: Diff 158148.Jul 8 2025, 11:03 AM

aokblast added reviewers: lwhsu, markj.Jul 8 2025, 11:05 AM

Approved, but see the inline comment.

lib/libusb/libusb10.h
49–55	Unrelated whitespace change?

Fix whitespace

Harbormaster completed remote builds in B65275: Diff 158168.Jul 8 2025, 3:25 PM

Just checking the metadata on https://github.com/aokblast/freebsd-src/commit/e404814eed946bcd6551d163dbfeeeb951bf935b.patch , some nits:

Check your Author line matches your Commit line
In convention we'll have a empty line before the Reviewed by (and don't forget to add markj)
To be batter compatible with libusb interface, We use libusb_log_level as the original libusb does. -> we use ...
The upstream libusb has a option to allow user to overwrite -> has an option, allows users to, and I guess to override might be more suitable here.
do all va_arg parser inside -> parsing

This revision is now accepted and ready to land.Jul 8 2025, 5:12 PM

Closed by commit rGbca5df45652a: libusb: replace LOG_LEVEL with DEBUG_LEVEL (authored by aokblast). · Explain WhyJul 9 2025, 2:21 AM

This revision was automatically updated to reflect the committed changes.

aokblast added a commit: rGbca5df45652a: libusb: replace LOG_LEVEL with DEBUG_LEVEL.

libusb: replace DEBUG_LEVEL with LOG_LEVEL
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 156944

lib/libusb/libusb.h

lib/libusb/libusb10.h

lib/libusb/libusb10.c

lib/libusb/libusb10_io.c

libusb: replace DEBUG_LEVEL with LOG_LEVELClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 156944

lib/libusb/libusb.h

lib/libusb/libusb10.h

lib/libusb/libusb10.c

lib/libusb/libusb10_io.c

libusb: replace DEBUG_LEVEL with LOG_LEVEL
ClosedPublic
Actions

Revision Contents
Changeset List