[1/6] check_if_required: Don't skip a file if not all related images are skipped

Message ID 1509002299-24631-1-git-send-email-stefan@herbrechtsmeier.net
State New
Headers show
Series
  • [1/6] check_if_required: Don't skip a file if not all related images are skipped
Related show

Commit Message

Stefan Herbrechtsmeier Oct. 26, 2017, 7:18 a.m.
From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---
 corelib/installer.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Stefano Babic Oct. 30, 2017, 2:53 p.m. | #1
On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> 
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
>  parser/parser.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/parser/parser.c b/parser/parser.c
> index d569c2b..a979d15 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -253,6 +253,7 @@ static int parse_partitions(parsertype p, void *cfg, struct swupdate_cfg *swcfg)
>  
>  		if (!strlen(partition->volname) || !strlen(partition->device)) {
>  			ERROR("Partition incompleted in description file");
> +			free(partition);
>  			return -1;
>  		}
>  
> @@ -432,6 +433,7 @@ static int parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua
>  			if (seek_str == endp || (image->seek == ULLONG_MAX && \
>  					errno == ERANGE)) {
>  				ERROR("offset argument: ustrtoull failed");
> +				free(image);
>  				return -1;
>  			}
>  		} else
> @@ -450,8 +452,11 @@ static int parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua
>  		get_field(p, elem, "install-if-different", &image->id.install_if_different);
>  		get_field(p, elem, "encrypted", &image->is_encrypted);
>  
> -		if (run_embscript(p, elem, image, L, swcfg->embscript))
> +		if (run_embscript(p, elem, image, L, swcfg->embscript)) {
> +			free(image);
>  			return -1;
> +		}
> +
>  		LIST_INSERT_HEAD(&swcfg->images, image, next);
>  
>  		TRACE("Found %sImage %s %s: %s in %s : %s for handler %s%s %s\n",
> @@ -520,8 +525,12 @@ static int parse_files(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua_
>  		get_field(p, elem, "installed-directly", &file->install_directly);
>  		get_field(p, elem, "install-if-different", &file->id.install_if_different);
>  		get_field(p, elem, "encrypted", &file->is_encrypted);
> -		if (run_embscript(p, elem, file, L, swcfg->embscript))
> +
> +		if (run_embscript(p, elem, file, L, swcfg->embscript)) {
> +			free(file);
>  			return -1;
> +		}
> +
>  		LIST_INSERT_HEAD(&swcfg->images, file, next);
>  
>  		TRACE("Found %sFile %s %s: %s --> %s (%s) %s\n",
> 

Agree, thanks for fixing this !

Applied to -master, thanks !

Best regards,
Stefano Babic
Stefano Babic Oct. 30, 2017, 3 p.m. | #2
Hi Stefan,

On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> 
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
>  corelib/installer.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/corelib/installer.c b/corelib/installer.c
> index f246953..1d84b2e 100644
> --- a/corelib/installer.c
> +++ b/corelib/installer.c
> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>  {
>  	int skip = SKIP_FILE;
>  	struct img_type *img;
> -	int img_skip = 0;
>  
>  	/*
>  	 * Check that not more as one image wnat to be streamed
> @@ -106,7 +105,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>  				 *  drop this from the list of images to be installed
>  				 */
>  				LIST_REMOVE(img, next);
> -				img_skip++;
>  				continue;
>  			}
>  
> @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>  		}
>  	}
>  
> -	if (img_skip > 0)
> -		skip = SKIP_FILE;
> -
>  	return skip;
>  }

I don't understand how it works. Your patch drops a return value equal
to SKIP_FILE, but this is a use case too. Let's think about a
multi-setup installation where artifacts for a different setup are in
the SWU (or they belong to another target), and they must not be
considered in the current installation. In your patch, file is never
skipped and it works in your setup (same file for multiple entries in
sw-description), but it does not work when file should be really
skipped. Have I misunderstood something ?

Best regards,
Stefano Babic
Stefan Herbrechtsmeier Oct. 30, 2017, 7:31 p.m. | #3
Hi Stefano,

Am 30.10.2017 um 16:00 schrieb Stefano Babic:
> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>> ---
>>   corelib/installer.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/corelib/installer.c b/corelib/installer.c
>> index f246953..1d84b2e 100644
>> --- a/corelib/installer.c
>> +++ b/corelib/installer.c
>> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>>   {
>>   	int skip = SKIP_FILE;
>>   	struct img_type *img;
>> -	int img_skip = 0;
>>   
>>   	/*
>>   	 * Check that not more as one image wnat to be streamed
>> @@ -106,7 +105,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>>   				 *  drop this from the list of images to be installed
>>   				 */
>>   				LIST_REMOVE(img, next);
>> -				img_skip++;
>>   				continue;
>>   			}
>>   
>> @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list, struct filehdr *pfdh,
>>   		}
>>   	}
>>   
>> -	if (img_skip > 0)
>> -		skip = SKIP_FILE;
>> -
>>   	return skip;
>>   }
> I don't understand how it works. Your patch drops a return value equal
> to SKIP_FILE, but this is a use case too.
The return value SKIP_FILE is the default return value. It is only 
overwritten if an image is found.

At the moment the code skips a file if one or more images are skipped 
even if one image needs the file. I think the function should skip a 
file only if no image needs the file. Maybe the function should 
immediately return from the loop with a COPY_FILE or INSTALL_FROM_STREAM 
if a valid image is found or return a SKIP_FILE after the loop.

> Let's think about a
> multi-setup installation where artifacts for a different setup are in
> the SWU (or they belong to another target), and they must not be
> considered in the current installation. In your patch, file is never
> skipped and it works in your setup (same file for multiple entries in
> sw-description), but it does not work when file should be really
> skipped. Have I misunderstood something ?
In a multi-setup installation the image list contains only images of the 
current installation. If no image referenced the file it should be 
skipped because of the default skip variable value SKIP_FILE.

Best regards
   Stefan
Stefano Babic Nov. 2, 2017, 10:04 a.m. | #4
Hi Stefan,

On 30/10/2017 20:31, Stefan Herbrechtsmeier wrote:
> Hi Stefano,
> 
> Am 30.10.2017 um 16:00 schrieb Stefano Babic:
>> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> Signed-off-by: Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier@weidmueller.com>
>>> ---
>>>   corelib/installer.c | 5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/corelib/installer.c b/corelib/installer.c
>>> index f246953..1d84b2e 100644
>>> --- a/corelib/installer.c
>>> +++ b/corelib/installer.c
>>> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list, struct
>>> filehdr *pfdh,
>>>   {
>>>       int skip = SKIP_FILE;
>>>       struct img_type *img;
>>> -    int img_skip = 0;
>>>         /*
>>>        * Check that not more as one image wnat to be streamed
>>> @@ -106,7 +105,6 @@ int check_if_required(struct imglist *list,
>>> struct filehdr *pfdh,
>>>                    *  drop this from the list of images to be installed
>>>                    */
>>>                   LIST_REMOVE(img, next);
>>> -                img_skip++;
>>>                   continue;
>>>               }
>>>   @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list,
>>> struct filehdr *pfdh,
>>>           }
>>>       }
>>>   -    if (img_skip > 0)
>>> -        skip = SKIP_FILE;
>>> -
>>>       return skip;
>>>   }
>> I don't understand how it works. Your patch drops a return value equal
>> to SKIP_FILE, but this is a use case too.
> The return value SKIP_FILE is the default return value. It is only
> overwritten if an image is found.
> 
> At the moment the code skips a file if one or more images are skipped
> even if one image needs the file.

Right.

> I think the function should skip a
> file only if no image needs the file.

Ok, agree - we could change behaviour, I do not see issues.

> Maybe the function should
> immediately return from the loop with a COPY_FILE or INSTALL_FROM_STREAM
> if a valid image is found or return

But we chould check that for multiple-install streaming is not allowed.

> a SKIP_FILE after the loop.
> 
>> Let's think about a
>> multi-setup installation where artifacts for a different setup are in
>> the SWU (or they belong to another target), and they must not be
>> considered in the current installation. In your patch, file is never
>> skipped and it works in your setup (same file for multiple entries in
>> sw-description), but it does not work when file should be really
>> skipped. Have I misunderstood something ?
> In a multi-setup installation the image list contains only images of the
> current installation. If no image referenced the file it should be
> skipped because of the default skip variable value SKIP_FILE.
> 

Best regards,
Stefano
Stefan Herbrechtsmeier Nov. 6, 2017, 7:34 a.m. | #5
Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic [mailto:sbabic@denx.de]
> Gesendet: Donnerstag, 2. November 2017 11:05
> An: Stefan Herbrechtsmeier; Stefano Babic; swupdate@googlegroups.com
> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> Betreff: Re: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
> file if not all related images are skipped
>
> > Am 30.10.2017 um 16:00 schrieb Stefano Babic:
> >> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
> >>> From: Stefan Herbrechtsmeier
> >>> <stefan.herbrechtsmeier@weidmueller.com>
> >>>
> >>> Signed-off-by: Stefan Herbrechtsmeier
> >>> <stefan.herbrechtsmeier@weidmueller.com>
> >>> ---
> >>>   corelib/installer.c | 5 -----
> >>>   1 file changed, 5 deletions(-)
> >>>
> >>> diff --git a/corelib/installer.c b/corelib/installer.c index
> >>> f246953..1d84b2e 100644
> >>> --- a/corelib/installer.c
> >>> +++ b/corelib/installer.c
> >>> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list,
> struct
> >>> filehdr *pfdh,
> >>>   {
> >>>       int skip = SKIP_FILE;
> >>>       struct img_type *img;
> >>> -    int img_skip = 0;
> >>>         /*
> >>>        * Check that not more as one image wnat to be streamed @@
> >>> -106,7 +105,6 @@ int check_if_required(struct imglist *list, struct
> >>> filehdr *pfdh,
> >>>                    *  drop this from the list of images to be
> >>> installed
> >>>                    */
> >>>                   LIST_REMOVE(img, next);
> >>> -                img_skip++;
> >>>                   continue;
> >>>               }
> >>>   @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list,
> >>> struct filehdr *pfdh,
> >>>           }
> >>>       }
> >>>   -    if (img_skip > 0)
> >>> -        skip = SKIP_FILE;
> >>> -
> >>>       return skip;
> >>>   }
> >> I don't understand how it works. Your patch drops a return value
> >> equal to SKIP_FILE, but this is a use case too.
> > The return value SKIP_FILE is the default return value. It is only
> > overwritten if an image is found.
> >
> > At the moment the code skips a file if one or more images are skipped
> > even if one image needs the file.
>
> Right.
>
> > I think the function should skip a
> > file only if no image needs the file.
>
> Ok, agree - we could change behaviour, I do not see issues.
>
> > Maybe the function should
> > immediately return from the loop with a COPY_FILE or
> > INSTALL_FROM_STREAM if a valid image is found or return
>
> But we chould check that for multiple-install streaming is not allowed.

You are right, we need to iterate over all images.

I introduce a evaluate_images function in an follow up patch. Maybe this check could be moved into this function to simplify the check_if_required function.

Best regards

Stefan Herbrechtsmeier
Software Developer Embedded Systems

Weidmüller – Your partner in Industrial Connectivity
We look forward to sharing ideas with you – Let’s connect.

Weidmueller Interface GmbH & Co. KG
Klingenbergstraße 16, 32758 Detmold, Germany
Stefan Herbrechtsmeier Nov. 13, 2017, 3:32 p.m. | #6
Hi Stefano,

> Gesendet: Montag, 6. November 2017 08:34
> An: 'Stefano Babic'; Stefan Herbrechtsmeier; swupdate@googlegroups.com
> Betreff: AW: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
> file if not all related images are skipped
>
> > -----Ursprüngliche Nachricht-----
> > Von: Stefano Babic [mailto:sbabic@denx.de]
> > Gesendet: Donnerstag, 2. November 2017 11:05
> > An: Stefan Herbrechtsmeier; Stefano Babic; swupdate@googlegroups.com
> > Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> > Betreff: Re: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
> > file if not all related images are skipped
> >
> > > Am 30.10.2017 um 16:00 schrieb Stefano Babic:
> > >> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
> > >>> From: Stefan Herbrechtsmeier
> > >>> <stefan.herbrechtsmeier@weidmueller.com>
> > >>>
> > >>> Signed-off-by: Stefan Herbrechtsmeier
> > >>> <stefan.herbrechtsmeier@weidmueller.com>
> > >>> ---
> > >>>   corelib/installer.c | 5 -----
> > >>>   1 file changed, 5 deletions(-)
> > >>>
> > >>> diff --git a/corelib/installer.c b/corelib/installer.c index
> > >>> f246953..1d84b2e 100644
> > >>> --- a/corelib/installer.c
> > >>> +++ b/corelib/installer.c
> > >>> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list,
> > struct
> > >>> filehdr *pfdh,
> > >>>   {
> > >>>       int skip = SKIP_FILE;
> > >>>       struct img_type *img;
> > >>> -    int img_skip = 0;
> > >>>         /*
> > >>>        * Check that not more as one image wnat to be streamed @@
> > >>> -106,7 +105,6 @@ int check_if_required(struct imglist *list,
> > >>> struct filehdr *pfdh,
> > >>>                    *  drop this from the list of images to be
> > >>> installed
> > >>>                    */
> > >>>                   LIST_REMOVE(img, next);
> > >>> -                img_skip++;
> > >>>                   continue;
> > >>>               }
> > >>>   @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list,
> > >>> struct filehdr *pfdh,
> > >>>           }
> > >>>       }
> > >>>   -    if (img_skip > 0)
> > >>> -        skip = SKIP_FILE;
> > >>> -
> > >>>       return skip;
> > >>>   }
> > >> I don't understand how it works. Your patch drops a return value
> > >> equal to SKIP_FILE, but this is a use case too.
> > > The return value SKIP_FILE is the default return value. It is only
> > > overwritten if an image is found.
> > >
> > > At the moment the code skips a file if one or more images are
> > > skipped even if one image needs the file.
> >
> > Right.
> >
> > > I think the function should skip a
> > > file only if no image needs the file.
> >
> > Ok, agree - we could change behaviour, I do not see issues.
> >
> > > Maybe the function should
> > > immediately return from the loop with a COPY_FILE or
> > > INSTALL_FROM_STREAM if a valid image is found or return
> >
> > But we chould check that for multiple-install streaming is not
> allowed.
>
> You are right, we need to iterate over all images.
>
> I introduce a evaluate_images function in an follow up patch. Maybe
> this check could be moved into this function to simplify the
> check_if_required function.

Is this patch okay for you? It still iterates over all images and returns an error if a multiple-install streaming is found.

Best regards

Stefan Herbrechtsmeier
Software Developer Embedded Systems

Weidmüller – Your partner in Industrial Connectivity
We look forward to sharing ideas with you – Let’s connect.

Weidmueller Interface GmbH & Co. KG
Klingenbergstraße 16, 32758 Detmold, Germany
Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
Stefano Babic Nov. 13, 2017, 3:51 p.m. | #7
Hi Stefan,

On 13/11/2017 16:32, Stefan.Herbrechtsmeier@weidmueller.com wrote:
> Hi Stefano,
> 
>> Gesendet: Montag, 6. November 2017 08:34
>> An: 'Stefano Babic'; Stefan Herbrechtsmeier; swupdate@googlegroups.com
>> Betreff: AW: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
>> file if not all related images are skipped
>>
>>> -----Ursprüngliche Nachricht-----
>>> Von: Stefano Babic [mailto:sbabic@denx.de]
>>> Gesendet: Donnerstag, 2. November 2017 11:05
>>> An: Stefan Herbrechtsmeier; Stefano Babic; swupdate@googlegroups.com
>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
>>> Betreff: Re: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
>>> file if not all related images are skipped
>>>
>>>> Am 30.10.2017 um 16:00 schrieb Stefano Babic:
>>>>> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote:
>>>>>> From: Stefan Herbrechtsmeier
>>>>>> <stefan.herbrechtsmeier@weidmueller.com>
>>>>>>
>>>>>> Signed-off-by: Stefan Herbrechtsmeier
>>>>>> <stefan.herbrechtsmeier@weidmueller.com>
>>>>>> ---
>>>>>>   corelib/installer.c | 5 -----
>>>>>>   1 file changed, 5 deletions(-)
>>>>>>
>>>>>> diff --git a/corelib/installer.c b/corelib/installer.c index
>>>>>> f246953..1d84b2e 100644
>>>>>> --- a/corelib/installer.c
>>>>>> +++ b/corelib/installer.c
>>>>>> @@ -86,7 +86,6 @@ int check_if_required(struct imglist *list,
>>> struct
>>>>>> filehdr *pfdh,
>>>>>>   {
>>>>>>       int skip = SKIP_FILE;
>>>>>>       struct img_type *img;
>>>>>> -    int img_skip = 0;
>>>>>>         /*
>>>>>>        * Check that not more as one image wnat to be streamed @@
>>>>>> -106,7 +105,6 @@ int check_if_required(struct imglist *list,
>>>>>> struct filehdr *pfdh,
>>>>>>                    *  drop this from the list of images to be
>>>>>> installed
>>>>>>                    */
>>>>>>                   LIST_REMOVE(img, next);
>>>>>> -                img_skip++;
>>>>>>                   continue;
>>>>>>               }
>>>>>>   @@ -139,9 +137,6 @@ int check_if_required(struct imglist *list,
>>>>>> struct filehdr *pfdh,
>>>>>>           }
>>>>>>       }
>>>>>>   -    if (img_skip > 0)
>>>>>> -        skip = SKIP_FILE;
>>>>>> -
>>>>>>       return skip;
>>>>>>   }
>>>>> I don't understand how it works. Your patch drops a return value
>>>>> equal to SKIP_FILE, but this is a use case too.
>>>> The return value SKIP_FILE is the default return value. It is only
>>>> overwritten if an image is found.
>>>>
>>>> At the moment the code skips a file if one or more images are
>>>> skipped even if one image needs the file.
>>>
>>> Right.
>>>
>>>> I think the function should skip a
>>>> file only if no image needs the file.
>>>
>>> Ok, agree - we could change behaviour, I do not see issues.
>>>
>>>> Maybe the function should
>>>> immediately return from the loop with a COPY_FILE or
>>>> INSTALL_FROM_STREAM if a valid image is found or return
>>>
>>> But we chould check that for multiple-install streaming is not
>> allowed.
>>
>> You are right, we need to iterate over all images.
>>
>> I introduce a evaluate_images function in an follow up patch. Maybe
>> this check could be moved into this function to simplify the
>> check_if_required function.
> 
> Is this patch okay for you? It still iterates over all images and returns an error if a multiple-install streaming is found.
> 

Hi Stefan,

I admit that I would like to test your patchset on my own before
merging, because I am unsure if there are side effects. That the reason
why I have not yet merged and your patchset is still in queue.

I hope I will come with some tests this week.

Thanks,
Stefano
Stefan Herbrechtsmeier Nov. 13, 2017, 4:16 p.m. | #8
Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic [mailto:sbabic@denx.de]
> Gesendet: Montag, 13. November 2017 16:51
> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de;
> stefan@herbrechtsmeier.net; swupdate@googlegroups.com
> Betreff: Re: AW: [swupdate] [PATCH 1/6] check_if_required: Don't skip a
> file if not all related images are skipped
>
> I admit that I would like to test your patchset on my own before
> merging, because I am unsure if there are side effects. That the reason
> why I have not yet merged and your patchset is still in queue.

No problem. I will resend my other patches after you have test the current patches.

Best regards,

Stefan Herbrechtsmeier
Software Developer Embedded Systems

Weidmüller – Your partner in Industrial Connectivity
We look forward to sharing ideas with you – Let’s connect.

Weidmueller Interface GmbH & Co. KG
Klingenbergstraße 16, 32758 Detmold, Germany
Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com

Patch

diff --git a/corelib/installer.c b/corelib/installer.c
index f246953..1d84b2e 100644
--- a/corelib/installer.c
+++ b/corelib/installer.c
@@ -86,7 +86,6 @@  int check_if_required(struct imglist *list, struct filehdr *pfdh,
 {
 	int skip = SKIP_FILE;
 	struct img_type *img;
-	int img_skip = 0;
 
 	/*
 	 * Check that not more as one image wnat to be streamed
@@ -106,7 +105,6 @@  int check_if_required(struct imglist *list, struct filehdr *pfdh,
 				 *  drop this from the list of images to be installed
 				 */
 				LIST_REMOVE(img, next);
-				img_skip++;
 				continue;
 			}
 
@@ -139,9 +137,6 @@  int check_if_required(struct imglist *list, struct filehdr *pfdh,
 		}
 	}
 
-	if (img_skip > 0)
-		skip = SKIP_FILE;
-
 	return skip;
 }