| Message ID | 1512566955-4712-1-git-send-email-julien.boibessot@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | linux-tools: add usbip (USB/IP userspace tools) | expand |
Hello, +Yann in Cc, as I'd really like to have his feedback on this patch. Title should be just: "linux-tools: add support for usbip" On Wed, 6 Dec 2017 14:29:15 +0100, julien.boibessot@free.fr wrote: > From: Julien BOIBESSOT <julien.boibessot@armadeus.com> > > I did this work before knowing the existence of previous attempts: Please don't use first person sentences in commit logs. > * 12/2012: http://lists.busybox.net/pipermail/buildroot/2012-December/064070.html > * 12/2016: http://buildroot-busybox.2317881.n4.nabble.com/PATCH-usbip-add-a-new-package-td152317.html > > I choosed to duplicate some autostuff infra commands in linux-tools as it was > more logical to me to include usbip inside linux-tools instead of trying to make > it a "normal" BR package. Indeed, previous 2016 attempt finally never reached > BR mainline. > Since 2012 usbip have moved from an external tool to an inside kernel sources > one. > > 2 linux patches are added to fix issues encountered while build testing with > utils/test-pkg. These patches have been sent upstream (linux-usb). > > Config used for test-pkg is: > > BR2_LINUX_KERNEL=y > BR2_LINUX_KERNEL_DEFCONFIG="mxs" > BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y > BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_17=y > BR2_PACKAGE_LINUX_TOOLS_USBIP=y > BR2_LINUX_KERNEL_DTS_SUPPORT=y > BR2_LINUX_KERNEL_USE_INTREE_DTS=y > BR2_LINUX_KERNEL_INTREE_DTS_NAME="imx28evk" > > (some toolchains needs to select a device tree explicitly) > > Results are the following: > > ./utils/test-pkg -c usbip.config -p linux-tool > armv5-ctng-linux-gnueabi [ 1/47]: SKIPPED > armv7-ctng-linux-gnueabihf [ 2/47]: SKIPPED > br-aarch64-glibc [ 3/47]: OK > br-arcle-hs38 [ 4/47]: OK > br-arm-basic [ 5/47]: SKIPPED > br-arm-cortex-a9-glibc [ 6/47]: OK > br-arm-cortex-a9-musl [ 7/47]: OK > br-arm-cortex-m4-full [ 8/47]: SKIPPED > br-arm-full [ 9/47]: SKIPPED > br-arm-full-nothread [10/47]: OK > br-arm-full-static [11/47]: SKIPPED > br-bfin-full [12/47]: SKIPPED > br-i386-pentium4-full [13/47]: SKIPPED > br-i386-pentium-mmx-musl [14/47]: SKIPPED > br-m68k-5208-full [15/47]: SKIPPED > br-m68k-68040-full [16/47]: OK > br-microblazeel-full [17/47]: OK > br-mips32r6-el-hf-glibc [18/47]: OK > br-mips64-n64-full [19/47]: OK > br-mips64r6-el-hf-glibc [20/47]: OK > br-mipsel-o32-full [21/47]: OK > br-nios2-glibc [22/47]: OK > br-openrisc-uclibc [23/47]: OK > br-powerpc-603e-basic-cpp [24/47]: SKIPPED > br-powerpc64le-power8-glibc [25/47]: OK > br-powerpc64-power7-glibc [26/47]: OK > br-powerpc-e500mc-full [27/47]: OK > br-sh4-full [28/47]: OK > br-sparc64-glibc [29/47]: OK > br-sparc-uclibc [30/47]: OK > br-x86-64-core2-full [31/47]: OK > br-x86-64-musl [32/47]: OK > br-xtensa-full [33/47]: OK > i686-ctng-linux-gnu [34/47]: SKIPPED > linaro-aarch64 [35/47]: OK > linaro-arm [36/47]: OK > mips64el-ctng_n32-linux-gnu [37/47]: SKIPPED > mips64el-ctng_n64-linux-gnu [38/47]: SKIPPED > powerpc-ctng_e500v2-linux-gnuspe [39/47]: SKIPPED > sourcery-arm-armv4t [40/47]: SKIPPED > sourcery-arm [41/47]: SKIPPED > sourcery-arm-thumb2 [42/47]: SKIPPED > sourcery-mips64 [43/47]: OK > sourcery-mips [44/47]: OK > sourcery-nios2 [45/47]: OK > sourcery-x86-64 [46/47]: OK > x86_64-ctng_locales-linux-gnu [47/47]: SKIPPED > 47 builds, 19 skipped, 0 build failed, 0 legal-info failed Does it makes sense to include the test-pkg details in the commit log? This is a real open question. We don't do it today, but I might see some value in having it. What do others think ? > diff --git a/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional I'm wondering if those two patches should be in linux/ or in package/linux-tools/. Not sure. > diff --git a/linux/linux.mk b/linux/linux.mk > index bd5589b..8877a7b 100644 > --- a/linux/linux.mk > +++ b/linux/linux.mk > @@ -221,6 +221,24 @@ define LINUX_TRY_PATCH_TIMECONST > endef > LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_TIMECONST > > +# Fixes tools/usbip build with recent gcc when -Werror is used > +# Try a dry-run patch to see if this applies, if it does go ahead > +define LINUX_TRY_PATCH_USBIP_GCC > + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >/dev/null ; then \ > + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional ; \ > + fi > +endef > +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_GCC > + > +# Fixes tools/usbip build with musl toolchains > +# Try a dry-run patch to see if this applies, if it does go ahead > +define LINUX_TRY_PATCH_USBIP_MUSL > + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional >/dev/null ; then \ > + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional ; \ > + fi > +endef > +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_MUSL Should we have this logic in linux/linux.mk, or in package/linux-tools/linux-tool-usbip.mk.in, possibly via some additional extension of the linux-tools infrastructure to register some patches to be applied ? Again, not sure, I'm just thinking out loud. Since this "let's try to apply a patch, but it fails don't error out" logic is now duplicated three times, perhaps it calls for a TRY_APPLY_PATCHES function, or something like that ? > +config BR2_PACKAGE_LINUX_TOOLS_USBIP > + bool "usbip" > + depends on BR2_PACKAGE_HAS_UDEV > + depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_17 # moved out of staging/ dir in kernel 3.17 > + select BR2_PACKAGE_LINUX_TOOLS > + help > + USB/IP protocol allows to pass USB device from server to client over > + the network. > + You need to activate support for it in your kernel configuration. Listing the actual kernel config options that are needed would be more useful. > + This (usbip) is the set of userspace tools used to handle connection > + and management. > + > + You can optionally add hwdata package to your BR config to have > + better runtime experience. BR -> Buildroot config -> configuration "better runtime experience" is somewhat fuzzy. Can we have a better description ? > +LINUX_TOOLS += usbip > + > +USBIP_DEPENDENCIES = udev > +USBIP_CONF_OPTS = --with-tcp-wrappers=no > + > +define USBIP_BUILD_CMDS > + $(Q)if test ! -f $(LINUX_DIR)/tools/usb/usbip/Makefile.am ; then \ > + echo "Your kernel version is too old and does not have the usbip tool." ; \ > + echo "At least kernel 3.17 must be used." ; \ > + exit 1 ; \ > + fi > + > + cd $(LINUX_DIR)/tools/usb/usbip && PATH=$(BR_PATH) ./autogen.sh If you autogen, you need to add at least host-autoconf and host-automake as dependencies. > + cd $(LINUX_DIR)/tools/usb/usbip && rm -rf config.cache && \ > + $(TARGET_CONFIGURE_OPTS) \ > + $(TARGET_CONFIGURE_ARGS) \ > + $(USBIP_CONF_ENV) \ > + CONFIG_SITE=/dev/null \ > + ./configure \ > + --target=$(GNU_TARGET_NAME) \ > + --host=$(GNU_TARGET_NAME) \ > + --build=$(GNU_HOST_NAME) \ > + --prefix=/usr \ > + --exec-prefix=/usr \ > + --sysconfdir=/etc \ > + --localstatedir=/var \ > + --program-prefix="" \ > + $(QUIET) $(USBIP_CONF_OPTS) Shouldn't we add a configure step for linux-tools ? > + > + $(TARGET_MAKE_ENV) $(MAKE) $(USBIP_MAKE_OPTS) \ > + -C $(LINUX_DIR)/tools/usb/usbip/ > +endef > + > +# for libusbip > +define USBIP_INSTALL_STAGING_CMDS > + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb/usbip \ > + $(USBIP_MAKE_OPTS) \ > + DESTDIR=$(STAGING_DIR) \ > + install > +endef > + > +define USBIP_INSTALL_TARGET_CMDS > + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb/usbip \ > + $(USBIP_MAKE_OPTS) \ > + DESTDIR=$(TARGET_DIR) \ > + install > +endef Thanks! Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: Hi, >> sourcery-arm-armv4t [40/47]: SKIPPED >> sourcery-arm [41/47]: SKIPPED >> sourcery-arm-thumb2 [42/47]: SKIPPED >> sourcery-mips64 [43/47]: OK >> sourcery-mips [44/47]: OK >> sourcery-nios2 [45/47]: OK >> sourcery-x86-64 [46/47]: OK >> x86_64-ctng_locales-linux-gnu [47/47]: SKIPPED >> 47 builds, 19 skipped, 0 build failed, 0 legal-info failed > Does it makes sense to include the test-pkg details in the commit log? > This is a real open question. We don't do it today, but I might see > some value in having it. What do others think ? I think it does. It is useful information and if it is in the commit message then it is easy to find back.
Julien, All, On 2017-12-14 09:56 +0100, Thomas Petazzoni spake thusly: > +Yann in Cc, as I'd really like to have his feedback on this patch. Ah, yep, I'd have expected to be in Cc: for the patch. ;-) > Title should be just: > > "linux-tools: add support for usbip" > > On Wed, 6 Dec 2017 14:29:15 +0100, julien.boibessot@free.fr wrote: > > From: Julien BOIBESSOT <julien.boibessot@armadeus.com> > > > > I did this work before knowing the existence of previous attempts: > > Please don't use first person sentences in commit logs. Usually, use 'we' instead, like; We use the git version becasue blabla... Autoreconf does not work, so we trick it into believing it has been autoreconf by blabla... However, if you have a "personal" message (like what you write), you can keep speaking with 'I' but put it below the --- line (which marks the end of the commit message, and that git-am will ignore when the patch is applied). See for example: https://patchwork.ozlabs.org/patch/843728/ > > * 12/2012: http://lists.busybox.net/pipermail/buildroot/2012-December/064070.html > > * 12/2016: http://buildroot-busybox.2317881.n4.nabble.com/PATCH-usbip-add-a-new-package-td152317.html I find it interesting that your referenced the prior art on this, and it is defeintely something that I'd like to see below the ---. [--SNIP--] > > 47 builds, 19 skipped, 0 build failed, 0 legal-info failed > > Does it makes sense to include the test-pkg details in the commit log? > This is a real open question. We don't do it today, but I might see > some value in having it. What do others think ? If everything is workih as expected, then no, except maybe a note like "the defconfig below builds with test-pkg with no error". > > diff --git a/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional > > I'm wondering if those two patches should be in linux/ or in > package/linux-tools/. Not sure. If they go into linux-tools, they won;t be applied because linux-tools does not have a source. Rather, they piggyback onto hte kernel sources and build directly in $(LINUX_DIR). Yes, that's is ugly and a special case I'm investigating for OOT. And maybe something you'll also want to keep an eye on for TLPB as well... > > diff --git a/linux/linux.mk b/linux/linux.mk > > index bd5589b..8877a7b 100644 > > --- a/linux/linux.mk > > +++ b/linux/linux.mk > > @@ -221,6 +221,24 @@ define LINUX_TRY_PATCH_TIMECONST > > endef > > LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_TIMECONST > > > > +# Fixes tools/usbip build with recent gcc when -Werror is used > > +# Try a dry-run patch to see if this applies, if it does go ahead > > +define LINUX_TRY_PATCH_USBIP_GCC > > + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >/dev/null ; then \ > > + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional ; \ > > + fi > > +endef > > +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_GCC > > + > > +# Fixes tools/usbip build with musl toolchains > > +# Try a dry-run patch to see if this applies, if it does go ahead > > +define LINUX_TRY_PATCH_USBIP_MUSL > > + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional >/dev/null ; then \ > > + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional ; \ > > + fi > > +endef > > +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_MUSL > > Should we have this logic in linux/linux.mk, or in > package/linux-tools/linux-tool-usbip.mk.in, possibly via some > additional extension of the linux-tools infrastructure to register some > patches to be applied ? One would argue that those are the source of the kernel tree, so the tweaks belong to linux.mk. OTOH, we are fixing usbip, which is built by the linux-tools package. > Again, not sure, I'm just thinking out loud. I'm not covinced either way, but to be pragmatic, I'd keep those in linux.mk. > Since this "let's try to apply a patch, but it fails don't error out" > logic is now duplicated three times, perhaps it calls for a > TRY_APPLY_PATCHES function, or something like that ? Yep. Julien, would you volunteer? ;-) > > + This (usbip) is the set of userspace tools used to handle connection > > + and management. > > + > > + You can optionally add hwdata package to your BR config to have > > + better runtime experience. > > BR -> Buildroot > config -> configuration > > "better runtime experience" is somewhat fuzzy. Can we have a better > description ? If the exprience is "so much better", why don't we forcibly select it? Is it only because hwdata is big? [--SNIP--] > > + cd $(LINUX_DIR)/tools/usb/usbip && rm -rf config.cache && \ > > + $(TARGET_CONFIGURE_OPTS) \ > > + $(TARGET_CONFIGURE_ARGS) \ > > + $(USBIP_CONF_ENV) \ > > + CONFIG_SITE=/dev/null \ > > + ./configure \ > > + --target=$(GNU_TARGET_NAME) \ > > + --host=$(GNU_TARGET_NAME) \ > > + --build=$(GNU_HOST_NAME) \ > > + --prefix=/usr \ > > + --exec-prefix=/usr \ > > + --sysconfdir=/etc \ > > + --localstatedir=/var \ > > + --program-prefix="" \ > > + $(QUIET) $(USBIP_CONF_OPTS) > > Shouldn't we add a configure step for linux-tools ? Yes. That does not need to be a separate patch, but that'd be better if it were. Thanks! :-) Regards, Yann E. MORIN.
Hello, Yann, Thomas, Peter, thanks for review ! On 14/12/2017 22:59, Yann E. MORIN wrote: > Julien, All, > > On 2017-12-14 09:56 +0100, Thomas Petazzoni spake thusly: >> +Yann in Cc, as I'd really like to have his feedback on this patch. > Ah, yep, I'd have expected to be in Cc: for the patch. ;-) Yeap sorry, next version, I promise. > >> Title should be just: >> >> "linux-tools: add support for usbip" >> >> On Wed, 6 Dec 2017 14:29:15 +0100, julien.boibessot@free.fr wrote: >>> From: Julien BOIBESSOT <julien.boibessot@armadeus.com> >>> >>> I did this work before knowing the existence of previous attempts: >> Please don't use first person sentences in commit logs. > Usually, use 'we' instead, like; > > We use the git version becasue blabla... > Autoreconf does not work, so we trick it into believing it > has been autoreconf by blabla... > > However, if you have a "personal" message (like what you write), you can > keep speaking with 'I' but put it below the --- line (which marks the > end of the commit message, and that git-am will ignore when the patch is > applied). See for example: > https://patchwork.ozlabs.org/patch/843728/ > >>> * 12/2012: http://lists.busybox.net/pipermail/buildroot/2012-December/064070.html >>> * 12/2016: http://buildroot-busybox.2317881.n4.nabble.com/PATCH-usbip-add-a-new-package-td152317.html > I find it interesting that your referenced the prior art on this, and it > is defeintely something that I'd like to see below the ---. > > [--SNIP--] >>> 47 builds, 19 skipped, 0 build failed, 0 legal-info failed >> Does it makes sense to include the test-pkg details in the commit log? >> This is a real open question. We don't do it today, but I might see >> some value in having it. What do others think ? > If everything is workih as expected, then no, except maybe a note like > "the defconfig below builds with test-pkg with no error". > >>> diff --git a/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >> I'm wondering if those two patches should be in linux/ or in >> package/linux-tools/. Not sure. > If they go into linux-tools, they won;t be applied because linux-tools > does not have a source. Rather, they piggyback onto hte kernel > sources and build directly in $(LINUX_DIR). > > Yes, that's is ugly and a special case I'm investigating for OOT. And > maybe something you'll also want to keep an eye on for TLPB as well... > >>> diff --git a/linux/linux.mk b/linux/linux.mk >>> index bd5589b..8877a7b 100644 >>> --- a/linux/linux.mk >>> +++ b/linux/linux.mk >>> @@ -221,6 +221,24 @@ define LINUX_TRY_PATCH_TIMECONST >>> endef >>> LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_TIMECONST >>> >>> +# Fixes tools/usbip build with recent gcc when -Werror is used >>> +# Try a dry-run patch to see if this applies, if it does go ahead >>> +define LINUX_TRY_PATCH_USBIP_GCC >>> + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >/dev/null ; then \ >>> + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional ; \ >>> + fi >>> +endef >>> +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_GCC >>> + >>> +# Fixes tools/usbip build with musl toolchains >>> +# Try a dry-run patch to see if this applies, if it does go ahead >>> +define LINUX_TRY_PATCH_USBIP_MUSL >>> + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional >/dev/null ; then \ >>> + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional ; \ >>> + fi >>> +endef >>> +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_MUSL >> Should we have this logic in linux/linux.mk, or in >> package/linux-tools/linux-tool-usbip.mk.in, possibly via some >> additional extension of the linux-tools infrastructure to register some >> patches to be applied ? > One would argue that those are the source of the kernel tree, so the > tweaks belong to linux.mk. > > OTOH, we are fixing usbip, which is built by the linux-tools package. For your information, my patches have been accepted by Greg KH for next Linux version. > >> Again, not sure, I'm just thinking out loud. > I'm not covinced either way, but to be pragmatic, I'd keep those in > linux.mk. > >> Since this "let's try to apply a patch, but it fails don't error out" >> logic is now duplicated three times, perhaps it calls for a >> TRY_APPLY_PATCHES function, or something like that ? > Yep. Julien, would you volunteer? ;-) I will have a look at it between turkey and foie gras ;-) > >>> + This (usbip) is the set of userspace tools used to handle connection >>> + and management. >>> + >>> + You can optionally add hwdata package to your BR config to have >>> + better runtime experience. >> BR -> Buildroot >> config -> configuration >> >> "better runtime experience" is somewhat fuzzy. Can we have a better >> description ? > If the exprience is "so much better", why don't we forcibly select it? > Is it only because hwdata is big? yes, hwdata is big. "better experience" means you see vendors/devices real names and not USB VID/PID. Moreover hwdata install PCI and USB infos and PCI is not needed in that case. > > [--SNIP--] >>> + cd $(LINUX_DIR)/tools/usb/usbip && rm -rf config.cache && \ >>> + $(TARGET_CONFIGURE_OPTS) \ >>> + $(TARGET_CONFIGURE_ARGS) \ >>> + $(USBIP_CONF_ENV) \ >>> + CONFIG_SITE=/dev/null \ >>> + ./configure \ >>> + --target=$(GNU_TARGET_NAME) \ >>> + --host=$(GNU_TARGET_NAME) \ >>> + --build=$(GNU_HOST_NAME) \ >>> + --prefix=/usr \ >>> + --exec-prefix=/usr \ >>> + --sysconfdir=/etc \ >>> + --localstatedir=/var \ >>> + --program-prefix="" \ >>> + $(QUIET) $(USBIP_CONF_OPTS) >> Shouldn't we add a configure step for linux-tools ? > Yes. That does not need to be a separate patch, but that'd be better if > it were. Understood. Thanks and regards, and merry Christmas ;-) Julien
Hi Julien; Le 22/12/2017 à 10:44, Julien Boibessot a écrit : > Hello, > > Yann, Thomas, Peter, > > thanks for review ! > > On 14/12/2017 22:59, Yann E. MORIN wrote: >> Julien, All, >> >> On 2017-12-14 09:56 +0100, Thomas Petazzoni spake thusly: >>> +Yann in Cc, as I'd really like to have his feedback on this patch. >> Ah, yep, I'd have expected to be in Cc: for the patch. ;-) > > Yeap sorry, next version, I promise. > >> >>> Title should be just: >>> >>> "linux-tools: add support for usbip" >>> >>> On Wed, 6 Dec 2017 14:29:15 +0100, julien.boibessot@free.fr wrote: >>>> From: Julien BOIBESSOT <julien.boibessot@armadeus.com> >>>> >>>> I did this work before knowing the existence of previous attempts: >>> Please don't use first person sentences in commit logs. >> Usually, use 'we' instead, like; >> >> We use the git version becasue blabla... >> Autoreconf does not work, so we trick it into believing it >> has been autoreconf by blabla... >> >> However, if you have a "personal" message (like what you write), you can >> keep speaking with 'I' but put it below the --- line (which marks the >> end of the commit message, and that git-am will ignore when the patch is >> applied). See for example: >> https://patchwork.ozlabs.org/patch/843728/ >> >>>> * 12/2012: http://lists.busybox.net/pipermail/buildroot/2012-December/064070.html >>>> * 12/2016: http://buildroot-busybox.2317881.n4.nabble.com/PATCH-usbip-add-a-new-package-td152317.html >> I find it interesting that your referenced the prior art on this, and it >> is defeintely something that I'd like to see below the ---. >> >> [--SNIP--] >>>> 47 builds, 19 skipped, 0 build failed, 0 legal-info failed >>> Does it makes sense to include the test-pkg details in the commit log? >>> This is a real open question. We don't do it today, but I might see >>> some value in having it. What do others think ? >> If everything is workih as expected, then no, except maybe a note like >> "the defconfig below builds with test-pkg with no error". >> >>>> diff --git a/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >>> I'm wondering if those two patches should be in linux/ or in >>> package/linux-tools/. Not sure. >> If they go into linux-tools, they won;t be applied because linux-tools >> does not have a source. Rather, they piggyback onto hte kernel >> sources and build directly in $(LINUX_DIR). >> >> Yes, that's is ugly and a special case I'm investigating for OOT. And >> maybe something you'll also want to keep an eye on for TLPB as well... >> >>>> diff --git a/linux/linux.mk b/linux/linux.mk >>>> index bd5589b..8877a7b 100644 >>>> --- a/linux/linux.mk >>>> +++ b/linux/linux.mk >>>> @@ -221,6 +221,24 @@ define LINUX_TRY_PATCH_TIMECONST >>>> endef >>>> LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_TIMECONST >>>> >>>> +# Fixes tools/usbip build with recent gcc when -Werror is used >>>> +# Try a dry-run patch to see if this applies, if it does go ahead >>>> +define LINUX_TRY_PATCH_USBIP_GCC >>>> + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >/dev/null ; then \ >>>> + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional ; \ >>>> + fi >>>> +endef >>>> +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_GCC >>>> + >>>> +# Fixes tools/usbip build with musl toolchains >>>> +# Try a dry-run patch to see if this applies, if it does go ahead >>>> +define LINUX_TRY_PATCH_USBIP_MUSL >>>> + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional >/dev/null ; then \ >>>> + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional ; \ >>>> + fi >>>> +endef >>>> +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_MUSL >>> Should we have this logic in linux/linux.mk, or in >>> package/linux-tools/linux-tool-usbip.mk.in, possibly via some >>> additional extension of the linux-tools infrastructure to register some >>> patches to be applied ? >> One would argue that those are the source of the kernel tree, so the >> tweaks belong to linux.mk. >> >> OTOH, we are fixing usbip, which is built by the linux-tools package. > > For your information, my patches have been accepted by Greg KH for next > Linux version. Ok great :) > >> >>> Again, not sure, I'm just thinking out loud. >> I'm not covinced either way, but to be pragmatic, I'd keep those in >> linux.mk. >> >>> Since this "let's try to apply a patch, but it fails don't error out" >>> logic is now duplicated three times, perhaps it calls for a >>> TRY_APPLY_PATCHES function, or something like that ? >> Yep. Julien, would you volunteer? ;-) > > I will have a look at it between turkey and foie gras ;-) So, in the mean time I'll mark this patch "Changes requested" in patchwork. Best regards, Romain > >> >>>> + This (usbip) is the set of userspace tools used to handle connection >>>> + and management. >>>> + >>>> + You can optionally add hwdata package to your BR config to have >>>> + better runtime experience. >>> BR -> Buildroot >>> config -> configuration >>> >>> "better runtime experience" is somewhat fuzzy. Can we have a better >>> description ? >> If the exprience is "so much better", why don't we forcibly select it? >> Is it only because hwdata is big? > > yes, hwdata is big. "better experience" means you see vendors/devices > real names and not USB VID/PID. Moreover hwdata install PCI and USB > infos and PCI is not needed in that case. > >> >> [--SNIP--] >>>> + cd $(LINUX_DIR)/tools/usb/usbip && rm -rf config.cache && \ >>>> + $(TARGET_CONFIGURE_OPTS) \ >>>> + $(TARGET_CONFIGURE_ARGS) \ >>>> + $(USBIP_CONF_ENV) \ >>>> + CONFIG_SITE=/dev/null \ >>>> + ./configure \ >>>> + --target=$(GNU_TARGET_NAME) \ >>>> + --host=$(GNU_TARGET_NAME) \ >>>> + --build=$(GNU_HOST_NAME) \ >>>> + --prefix=/usr \ >>>> + --exec-prefix=/usr \ >>>> + --sysconfdir=/etc \ >>>> + --localstatedir=/var \ >>>> + --program-prefix="" \ >>>> + $(QUIET) $(USBIP_CONF_OPTS) >>> Shouldn't we add a configure step for linux-tools ? >> Yes. That does not need to be a separate patch, but that'd be better if >> it were. > > Understood. > > Thanks and regards, and merry Christmas ;-) > > Julien > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
diff --git a/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional new file mode 100644 index 0000000..921e83c --- /dev/null +++ b/linux/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional @@ -0,0 +1,39 @@ +From 51d880bf162e88db19eb26829021078c92ad3260 Mon Sep 17 00:00:00 2001 +From: Julien BOIBESSOT <julien.boibessot@armadeus.com> +Date: Tue, 5 Dec 2017 14:49:36 +0100 +Subject: [PATCH] tools/usbip: fixes potential (minor) "buffer overflow" + (detected on recent gcc with -Werror) + +Fixes following build error: +vhci_driver.c: In function 'refresh_imported_device_list': +vhci_driver.c:118:37: error: 'snprintf' output may be truncated before + the last format character [-Werror=format-truncation=] + snprintf(status, sizeof(status), "status.%d", i); + ^~~~~~~~~~~ +vhci_driver.c:118:4: note: 'snprintf' output between 9 and 18 bytes into + a destination of size 17 + snprintf(status, sizeof(status), "status.%d", i); + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +cc1: all warnings being treated as errors + +Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com> +--- + tools/usb/usbip/libsrc/vhci_driver.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/tools/usb/usbip/libsrc/vhci_driver.c b/tools/usb/usbip/libsrc/vhci_driver.c +index 5727dfb..a9ce431 100644 +--- a/tools/usb/usbip/libsrc/vhci_driver.c ++++ b/tools/usb/usbip/libsrc/vhci_driver.c +@@ -106,7 +106,7 @@ static int parse_status(const char *value) + return 0; + } + +-#define MAX_STATUS_NAME 16 ++#define MAX_STATUS_NAME 18 + + static int refresh_imported_device_list(void) + { +-- +2.1.4 + diff --git a/linux/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional b/linux/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional new file mode 100644 index 0000000..b2f5683 --- /dev/null +++ b/linux/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional @@ -0,0 +1,44 @@ +From f30ff30e80b977311d15ed1134f7e13c379601a5 Mon Sep 17 00:00:00 2001 +From: Julien BOIBESSOT <julien.boibessot@armadeus.com> +Date: Tue, 5 Dec 2017 18:10:07 +0100 +Subject: [PATCH] tools/usbip: fixes build with musl libc toolchain +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Indeed musl doesn't define old SIGCLD signal name but only new one SIGCHLD. +SIGCHLD is the new POSIX name for that signal so it doesn't change +anything on other libcs. + +This fixes this kind of build error: + +usbipd.c: In function ‘set_signal’: +usbipd.c:459:12: error: 'SIGCLD' undeclared (first use in this function) + sigaction(SIGCLD, &act, NULL); + ^~~~~~ +usbipd.c:459:12: note: each undeclared identifier is reported only once + for each function it appears in +Makefile:407: recipe for target 'usbipd.o' failed +make[3]: *** [usbipd.o] Error 1 + +Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com> +--- + tools/usb/usbip/src/usbipd.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/tools/usb/usbip/src/usbipd.c b/tools/usb/usbip/src/usbipd.c +index 009afb4..c6dad2a 100644 +--- a/tools/usb/usbip/src/usbipd.c ++++ b/tools/usb/usbip/src/usbipd.c +@@ -456,7 +456,7 @@ static void set_signal(void) + sigaction(SIGTERM, &act, NULL); + sigaction(SIGINT, &act, NULL); + act.sa_handler = SIG_IGN; +- sigaction(SIGCLD, &act, NULL); ++ sigaction(SIGCHLD, &act, NULL); + } + + static const char *pid_file; +-- +2.1.4 + diff --git a/linux/linux.mk b/linux/linux.mk index bd5589b..8877a7b 100644 --- a/linux/linux.mk +++ b/linux/linux.mk @@ -221,6 +221,24 @@ define LINUX_TRY_PATCH_TIMECONST endef LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_TIMECONST +# Fixes tools/usbip build with recent gcc when -Werror is used +# Try a dry-run patch to see if this applies, if it does go ahead +define LINUX_TRY_PATCH_USBIP_GCC + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional >/dev/null ; then \ + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0002-tools-usbip-fixes-potential-minor-buffer-overflow-de.patch.conditional ; \ + fi +endef +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_GCC + +# Fixes tools/usbip build with musl toolchains +# Try a dry-run patch to see if this applies, if it does go ahead +define LINUX_TRY_PATCH_USBIP_MUSL + @if patch -p1 --dry-run -f -s -d $(@D) <$(LINUX_PKGDIR)/0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional >/dev/null ; then \ + $(APPLY_PATCHES) $(@D) $(LINUX_PKGDIR) 0003-tools-usbip-fixes-build-with-musl-libc-toolchain.patch.conditional ; \ + fi +endef +LINUX_POST_PATCH_HOOKS += LINUX_TRY_PATCH_USBIP_MUSL + ifeq ($(BR2_LINUX_KERNEL_USE_DEFCONFIG),y) LINUX_KCONFIG_DEFCONFIG = $(call qstrip,$(BR2_LINUX_KERNEL_DEFCONFIG))_defconfig else ifeq ($(BR2_LINUX_KERNEL_USE_ARCH_DEFAULT_CONFIG),y) diff --git a/package/linux-tools/Config.in b/package/linux-tools/Config.in index e3ccd85..223045a 100644 --- a/package/linux-tools/Config.in +++ b/package/linux-tools/Config.in @@ -85,4 +85,25 @@ config BR2_PACKAGE_LINUX_TOOLS_TMON tmon is a terminal-based tool (using curses) that allows the user to access thermal information about the system. +config BR2_PACKAGE_LINUX_TOOLS_USBIP + bool "usbip" + depends on BR2_PACKAGE_HAS_UDEV + depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_17 # moved out of staging/ dir in kernel 3.17 + select BR2_PACKAGE_LINUX_TOOLS + help + USB/IP protocol allows to pass USB device from server to client over + the network. + You need to activate support for it in your kernel configuration. + + This (usbip) is the set of userspace tools used to handle connection + and management. + + You can optionally add hwdata package to your BR config to have + better runtime experience. + + https://github.com/torvalds/linux/blob/master/tools/usb/usbip/README + +comment "usbip needs udev /dev management and a toolchain w/ headers >= 3.17" + depends on !BR2_PACKAGE_HAS_UDEV || !BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_17 + endmenu diff --git a/package/linux-tools/linux-tool-usbip.mk.in b/package/linux-tools/linux-tool-usbip.mk.in new file mode 100644 index 0000000..9a599ee --- /dev/null +++ b/package/linux-tools/linux-tool-usbip.mk.in @@ -0,0 +1,54 @@ +################################################################################ +# +# usbip +# +################################################################################ + +LINUX_TOOLS += usbip + +USBIP_DEPENDENCIES = udev +USBIP_CONF_OPTS = --with-tcp-wrappers=no + +define USBIP_BUILD_CMDS + $(Q)if test ! -f $(LINUX_DIR)/tools/usb/usbip/Makefile.am ; then \ + echo "Your kernel version is too old and does not have the usbip tool." ; \ + echo "At least kernel 3.17 must be used." ; \ + exit 1 ; \ + fi + + cd $(LINUX_DIR)/tools/usb/usbip && PATH=$(BR_PATH) ./autogen.sh + + cd $(LINUX_DIR)/tools/usb/usbip && rm -rf config.cache && \ + $(TARGET_CONFIGURE_OPTS) \ + $(TARGET_CONFIGURE_ARGS) \ + $(USBIP_CONF_ENV) \ + CONFIG_SITE=/dev/null \ + ./configure \ + --target=$(GNU_TARGET_NAME) \ + --host=$(GNU_TARGET_NAME) \ + --build=$(GNU_HOST_NAME) \ + --prefix=/usr \ + --exec-prefix=/usr \ + --sysconfdir=/etc \ + --localstatedir=/var \ + --program-prefix="" \ + $(QUIET) $(USBIP_CONF_OPTS) + + $(TARGET_MAKE_ENV) $(MAKE) $(USBIP_MAKE_OPTS) \ + -C $(LINUX_DIR)/tools/usb/usbip/ +endef + +# for libusbip +define USBIP_INSTALL_STAGING_CMDS + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb/usbip \ + $(USBIP_MAKE_OPTS) \ + DESTDIR=$(STAGING_DIR) \ + install +endef + +define USBIP_INSTALL_TARGET_CMDS + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/usb/usbip \ + $(USBIP_MAKE_OPTS) \ + DESTDIR=$(TARGET_DIR) \ + install +endef