diff mbox

[RFC] U-Boot: don't check hashes for U-Boot external patches

Message ID 1460644321-13849-1-git-send-email-julien.boibessot@free.fr
State Changes Requested
Headers show

Commit Message

Julien Boibessot April 14, 2016, 2:32 p.m. UTC 
From: Julien BOIBESSOT <julien.boibessot@armadeus.com>

BR2_TARGET_UBOOT_PATCH allows user to give URL of patches on the Web, but in
that case BR dl infra tries to compare hashes of the downloaded patches with the
infos from boot/uboot/uboot.hash.
So, for that case (user specified patches), tell BR not to check hashes.

Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com>
---
 boot/uboot/uboot.mk | 2 ++
 1 file changed, 2 insertions(+)

Comments

Arnout Vandecappelle April 14, 2016, 9 p.m. UTC | #1 
On 04/14/16 16:32, julien.boibessot@free.fr wrote:
> From: Julien BOIBESSOT <julien.boibessot@armadeus.com>
>
> BR2_TARGET_UBOOT_PATCH allows user to give URL of patches on the Web, but in
> that case BR dl infra tries to compare hashes of the downloaded patches with the
> infos from boot/uboot/uboot.hash.
> So, for that case (user specified patches), tell BR not to check hashes.
>
> Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com>
> ---
>   boot/uboot/uboot.mk | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 48f40c3..9e24b48 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -109,6 +109,8 @@ UBOOT_POST_RSYNC_HOOKS += UBOOT_COPY_OLD_LICENSE_FILE
>   # directories or files.
>   UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
>   UBOOT_PATCH = $(filter ftp://% http://% https://%,$(UBOOT_PATCHES))
> +UBOOT_PATCHES_NAME = $(notdir $(UBOOT_PATCHES))

  Since only the non-local patches will be downloaded, perhaps it's better to 
use UBOOT_PATCH instead of UBOOT_PATCHES.

  Also, there is no need to use an additional variable for this, it's used only 
once.

  Regards,
  Arnout

> +BR_NO_CHECK_HASH_FOR += $(UBOOT_PATCHES_NAME)
>
>   define UBOOT_APPLY_LOCAL_PATCHES
>   	for p in $(filter-out ftp://% http://% https://%,$(UBOOT_PATCHES)) ; do \
>
Arnout Vandecappelle April 14, 2016, 9:25 p.m. UTC | #2 
On 04/14/16 23:00, Arnout Vandecappelle wrote:
> On 04/14/16 16:32, julien.boibessot@free.fr wrote:
>> From: Julien BOIBESSOT <julien.boibessot@armadeus.com>
>>
>> BR2_TARGET_UBOOT_PATCH allows user to give URL of patches on the Web, but in
>> that case BR dl infra tries to compare hashes of the downloaded patches with the
>> infos from boot/uboot/uboot.hash.
>> So, for that case (user specified patches), tell BR not to check hashes.
>>
>> Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com>
>> ---
>>   boot/uboot/uboot.mk | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
>> index 48f40c3..9e24b48 100644
>> --- a/boot/uboot/uboot.mk
>> +++ b/boot/uboot/uboot.mk
>> @@ -109,6 +109,8 @@ UBOOT_POST_RSYNC_HOOKS += UBOOT_COPY_OLD_LICENSE_FILE
>>   # directories or files.
>>   UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
>>   UBOOT_PATCH = $(filter ftp://% http://% https://%,$(UBOOT_PATCHES))
>> +UBOOT_PATCHES_NAME = $(notdir $(UBOOT_PATCHES))
>
>   Since only the non-local patches will be downloaded, perhaps it's better to
> use UBOOT_PATCH instead of UBOOT_PATCHES.
>
>   Also, there is no need to use an additional variable for this, it's used only
> once.

  Oh, and something similar should be done for LINUX_PATCH as well.

  Regards,
  Arnout
Julien Boibessot April 18, 2016, 10:28 a.m. UTC | #3 
Hello,

Arnout,

thanks for the comments.

On 14/04/2016 23:25, Arnout Vandecappelle wrote:
> On 04/14/16 23:00, Arnout Vandecappelle wrote:
>> On 04/14/16 16:32, julien.boibessot@free.fr wrote:
>>> From: Julien BOIBESSOT <julien.boibessot@armadeus.com>
>>>
>>> BR2_TARGET_UBOOT_PATCH allows user to give URL of patches on the
>>> Web, but in
>>> that case BR dl infra tries to compare hashes of the downloaded
>>> patches with the
>>> infos from boot/uboot/uboot.hash.
>>> So, for that case (user specified patches), tell BR not to check
>>> hashes.
>>>
>>> Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com>
>>> ---
>>>   boot/uboot/uboot.mk | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
>>> index 48f40c3..9e24b48 100644
>>> --- a/boot/uboot/uboot.mk
>>> +++ b/boot/uboot/uboot.mk
>>> @@ -109,6 +109,8 @@ UBOOT_POST_RSYNC_HOOKS +=
>>> UBOOT_COPY_OLD_LICENSE_FILE
>>>   # directories or files.
>>>   UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
>>>   UBOOT_PATCH = $(filter ftp://% http://% https://%,$(UBOOT_PATCHES))
>>> +UBOOT_PATCHES_NAME = $(notdir $(UBOOT_PATCHES))
>>
>>   Since only the non-local patches will be downloaded, perhaps it's
>> better to
>> use UBOOT_PATCH instead of UBOOT_PATCHES.
>>
>>   Also, there is no need to use an additional variable for this, it's
>> used only
>> once.
>
>  Oh, and something similar should be done for LINUX_PATCH as well.

LINUX_PATCH is already working well with external patches that have to
be downloaded.

Regards,
Julien
Arnout Vandecappelle April 18, 2016, 11:18 p.m. UTC | #4 
On 04/18/16 12:28, Julien Boibessot wrote:
> Hello,
>
> Arnout,
>
> thanks for the comments.
>
> On 14/04/2016 23:25, Arnout Vandecappelle wrote:
[snip]
>>
>>   Oh, and something similar should be done for LINUX_PATCH as well.
>
> LINUX_PATCH is already working well with external patches that have to
> be downloaded.

  Of course, because we don't have a linux.hash and probably never will have one.

  Regards,
  Arnout
Julien Boibessot April 20, 2016, 8:47 a.m. UTC | #5 
On 19/04/2016 01:18, Arnout Vandecappelle wrote:
> On 04/18/16 12:28, Julien Boibessot wrote:
>> Hello,
>>
>> Arnout,
>>
>> thanks for the comments.
>>
>> On 14/04/2016 23:25, Arnout Vandecappelle wrote:
> [snip]
>>>
>>>   Oh, and something similar should be done for LINUX_PATCH as well.
>>
>> LINUX_PATCH is already working well with external patches that have to
>> be downloaded.
>
>  Of course, because we don't have a linux.hash and probably never will
> have one.

ah ok, got it now ! :-)
So why can't we just get rid of uboot.hash too ?

Regards,
Julien
Arnout Vandecappelle April 20, 2016, 8:18 p.m. UTC | #6 
On 04/20/16 10:47, Julien Boibessot wrote:
> On 19/04/2016 01:18, Arnout Vandecappelle wrote:
>> On 04/18/16 12:28, Julien Boibessot wrote:
>>> Hello,
>>>
>>> Arnout,
>>>
>>> thanks for the comments.
>>>
>>> On 14/04/2016 23:25, Arnout Vandecappelle wrote:
>> [snip]
>>>>
>>>>    Oh, and something similar should be done for LINUX_PATCH as well.
>>>
>>> LINUX_PATCH is already working well with external patches that have to
>>> be downloaded.
>>
>>   Of course, because we don't have a linux.hash and probably never will
>> have one.
>
> ah ok, got it now ! :-)
> So why can't we just get rid of uboot.hash too ?

  I don't think so.

  For the kernel, it is very unlikely that you just use the latest version, most 
projects will want a specific version and usually even a patched version. With 
device trees this becomes somewhat less true, but even so, most people just 
don't use the latest-and-greatest kernel. Therefore, having hashes is quite useless.

  For u-boot (and barebox for that matter), this also used to be the case, but 
nowadays you can very often get away with just using the upstream version. In 
this case, the hash is indeed valuable.

  Cc-ing Yann, our hash expert :-)

  Regards,
  Arnout
Yann E. MORIN April 24, 2016, 4:11 p.m. UTC | #7 
Arnout, Julien, All,

On 2016-04-20 22:18 +0200, Arnout Vandecappelle spake thusly:
> On 04/20/16 10:47, Julien Boibessot wrote:
> >On 19/04/2016 01:18, Arnout Vandecappelle wrote:
> >>On 04/18/16 12:28, Julien Boibessot wrote:
> >>>Hello,
> >>>
> >>>Arnout,
> >>>
> >>>thanks for the comments.
> >>>
> >>>On 14/04/2016 23:25, Arnout Vandecappelle wrote:
> >>[snip]
> >>>>
> >>>>   Oh, and something similar should be done for LINUX_PATCH as well.
> >>>
> >>>LINUX_PATCH is already working well with external patches that have to
> >>>be downloaded.
> >>
> >>  Of course, because we don't have a linux.hash and probably never will
> >>have one.
> >
> >ah ok, got it now ! :-)
> >So why can't we just get rid of uboot.hash too ?
> 
>  I don't think so.

Siding with Arnout on that one: we want to one day make .hash files
mandatory.

>  For the kernel, it is very unlikely that you just use the latest version,
> most projects will want a specific version and usually even a patched
> version. With device trees this becomes somewhat less true, but even so,
> most people just don't use the latest-and-greatest kernel. Therefore, having
> hashes is quite useless.

Yes and no. We could well have hashes for the latest release, at least.
Then, when bumping, we can leave the old one and just add the new one;
thus hashes would be piling up as time goes, at least for official
releases.

Plus, we could also bulk-add hashes for older official releases. Official
releases are also very often used, at least for "standard" architectures
like x86, so it would not be nonsense to have hashes for those releases.

Normally, when a .hash file does exist, an archive must have at least
one hash, otherwise this is considered an error. NO_CHECK_HASH_FOR makes
it so that this is not an error that an archive does not have a hash,
but won't bar the existing hashes to be checked.

It's on my TODO to add hashes for the kernel.

Regards,
Yann E. MORIN.

>  For u-boot (and barebox for that matter), this also used to be the case,
> but nowadays you can very often get away with just using the upstream
> version. In this case, the hash is indeed valuable.
> 
>  Cc-ing Yann, our hash expert :-)
> 
>  Regards,
>  Arnout
> 
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
Yann E. MORIN June 12, 2016, 9:36 p.m. UTC | #8 
Julien, All,

On 2016-04-14 16:32 +0200, julien.boibessot@free.fr spake thusly:
> From: Julien BOIBESSOT <julien.boibessot@armadeus.com>
> 
> BR2_TARGET_UBOOT_PATCH allows user to give URL of patches on the Web, but in
> that case BR dl infra tries to compare hashes of the downloaded patches with the
> infos from boot/uboot/uboot.hash.
> So, for that case (user specified patches), tell BR not to check hashes.

You've had some suggestions from Arnout. Care to update your patch and
re-submit it, please?

In the meantime, I'm marking it as "Changes Requestd" in our Patchwork.

Thanks!

Regards,
Yann E. MORIN.

> Signed-off-by: Julien BOIBESSOT <julien.boibessot@armadeus.com>
> ---
>  boot/uboot/uboot.mk | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 48f40c3..9e24b48 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -109,6 +109,8 @@ UBOOT_POST_RSYNC_HOOKS += UBOOT_COPY_OLD_LICENSE_FILE
>  # directories or files.
>  UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
>  UBOOT_PATCH = $(filter ftp://% http://% https://%,$(UBOOT_PATCHES))
> +UBOOT_PATCHES_NAME = $(notdir $(UBOOT_PATCHES))
> +BR_NO_CHECK_HASH_FOR += $(UBOOT_PATCHES_NAME)
>  
>  define UBOOT_APPLY_LOCAL_PATCHES
>  	for p in $(filter-out ftp://% http://% https://%,$(UBOOT_PATCHES)) ; do \
> -- 
> 2.1.4
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
diff mbox

Patch

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 48f40c3..9e24b48 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -109,6 +109,8 @@  UBOOT_POST_RSYNC_HOOKS += UBOOT_COPY_OLD_LICENSE_FILE
 # directories or files.
 UBOOT_PATCHES = $(call qstrip,$(BR2_TARGET_UBOOT_PATCH))
 UBOOT_PATCH = $(filter ftp://% http://% https://%,$(UBOOT_PATCHES))
+UBOOT_PATCHES_NAME = $(notdir $(UBOOT_PATCHES))
+BR_NO_CHECK_HASH_FOR += $(UBOOT_PATCHES_NAME)
 
 define UBOOT_APPLY_LOCAL_PATCHES
 	for p in $(filter-out ftp://% http://% https://%,$(UBOOT_PATCHES)) ; do \