Re: [cip-dev][isar-cip-core][PATCH v2 1/4] swupdate: check output of sign-swu

From: Gylstorff Quirin <quirin.gylstorff@siemens.com>
To: Jan Kiszka <jan.kiszka@siemens.com>,
	cip-dev@lists.cip-project.org, felix.moessbauer@siemens.com
Subject: Re: [cip-dev][isar-cip-core][PATCH v2 1/4] swupdate: check output of sign-swu
Date: Tue, 5 Mar 2024 15:33:28 +0100	[thread overview]
Message-ID: <76dc4aef-74d6-4110-8d5a-e2828dfbfe65@siemens.com> (raw)
In-Reply-To: <9b294320-d609-4b93-9675-627d26f69b46@siemens.com>


On 3/5/24 2:50 PM, Jan Kiszka wrote:
> On 05.03.24 12:02, Quirin Gylstorff wrote:
>> From: Quirin Gylstorff <quirin.gylstorff@siemens.com>
>>
>> Check for signing errors to avoid an unusable swu file.
>>
>> This also moves the siging out of the loop to generate
>> the cpio archive *.swu as the Messages from the signing
>> can lead to errors in the archive generation. The cpio
>> options are no longer using the short form.
>>
>> Signed-off-by: Quirin Gylstorff <quirin.gylstorff@siemens.com>
>> ---
>>   classes/swupdate.bbclass | 43 ++++++++++++++++++++++++++++------------
>>   1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/classes/swupdate.bbclass b/classes/swupdate.bbclass
>> index aaff072..c62f43f 100644
>> --- a/classes/swupdate.bbclass
>> +++ b/classes/swupdate.bbclass
>> @@ -191,24 +191,41 @@ IMAGE_CMD:swu() {
>>                       "${PP_WORK}/$swu_file_base/${SWU_DESCRIPTION_FILE}"
>>               done
>>               cd "${PP_WORK}/$swu_file_base"
>> -            for file in "${SWU_DESCRIPTION_FILE}" ${SWU_ADDITIONAL_FILES}; do
>> -                if [ "$file" = "${SWU_DESCRIPTION_FILE}" ] || \
>> -                    grep -q "$file" "${PP_WORK}/$swu_file_base/${SWU_DESCRIPTION_FILE}"; then
>> +            cpio_files="${SWU_DESCRIPTION_FILE}"
>> +
>> +            if [ -n "$sign" ]; then
>> +                if ! /usr/bin/sign-swu \
>> +                    "${SWU_DESCRIPTION_FILE}" "${SWU_DESCRIPTION_FILE}.${SWU_SIGNATURE_EXT}" \
>> +                    > /dev/null 2>&1 || \
>> +                    [ ! -f "${SWU_DESCRIPTION_FILE}.${SWU_SIGNATURE_EXT}" ]; then
>> +                    echo "Could not create swupdate signature file '${SWU_DESCRIPTION_FILE}.${SWU_SIGNATURE_EXT}'" 1>&2
>> +                    exit 1
>> +                fi
>> +                cpio_files="$cpio_files ${SWU_DESCRIPTION_FILE}.${SWU_SIGNATURE_EXT}"
> 
> Can we use a local short-hand variable for
> "${SWU_DESCRIPTION_FILE}.${SWU_SIGNATURE_EXT}"? One that makes the lines
> shorter and more readable?

Sure will add in a v3.
> 
>> +            fi
>> +
>> +            # sw-description must be first file in *.swu
>> +            for cpio_file in $cpio_files ${SWU_ADDITIONAL_FILES}; do
>> +                if [ -f "$cpio_file" ]; then
>>                       # Set file timestamps for reproducible builds
>>                       if [ -n "${SOURCE_DATE_EPOCH}" ]; then
>>                           touch -d@"${SOURCE_DATE_EPOCH}" "$file"
>>                       fi
>> -                    echo "$file"
>> -                    if [ -n "$sign" -a "${SWU_DESCRIPTION_FILE}" = "$file" ]; then
>> -                        sign-swu "$file" "$file.${SWU_SIGNATURE_EXT}"
>> -                        # Set file timestamps for reproducible builds
>> -                        if [ -n "${SOURCE_DATE_EPOCH}" ]; then
>> -                            touch -d@"${SOURCE_DATE_EPOCH}" "$file.${SWU_SIGNATURE_EXT}"
>> -                        fi
> 
> Why is this timestamp tuning no longer needed with the new approach?
I moved it in the for loop for creating the cpio container. That way the 
timestamp tuning is in one location.

Quirin

> 
>> -                        echo "$file.${SWU_SIGNATURE_EXT}"
>> -                    fi
>> +                    case "$cpio_file" in
>> +                        sw-description*)
>> +                            echo "$cpio_file"
>> +                            ;;
>> +                        *)
>> +                            if grep -q "$cpio_file" \
>> +                                "${WORKDIR}/$swu_file_base/${SWU_DESCRIPTION_FILE}"; then
>> +                                echo "$cpio_file"
>> +                            fi
>> +                            ;;
>> +                    esac
>>                   fi
>> -            done | cpio -ovL --reproducible -H crc > "${PP_DEPLOY}/${SWU_IMAGE_FILE}$swu_file_extension.swu"
>> +            done | cpio \
>> +                --verbose --dereference --create --reproducible --format=crc \
>> +                > "${PP_DEPLOY}/${SWU_IMAGE_FILE}$swu_file_extension.swu"
>>   EOIMAGER
>>       done
>>   }
> 
> Jan
>