| Message ID | 1509002299-24631-1-git-send-email-stefan@herbrechtsmeier.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/6] check_if_required: Don't skip a file if not all related images are skipped | expand |
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
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
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
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
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
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
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
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
Hi Hamish, 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; > } > This patch reverts a previous patch: commit 9224371491b9cd38b00a5fcde2e1e2845a8443ed Author: Hamish Guthrie <hamish.guthrie@kistler.com> Date: Tue Mar 14 12:31:39 2017 +0100 Skip all instances of filename in images list If multiple references are made to the same file (i.e. multiple copies of the same file are destined to different partitions or volumes) and the file does not need to be installed, then skip all instances of that filename in the images list. However, I tendentially agree with Stefan - it is ok to skip an already installed version, but if the image should be installed into another destination / partition / flash, the check should be done again. And only if all images are not required to be installed, file should be skipped. Now it is skipped if just one single image is already installed. This is exactly against Hamish' patch. Hamish, can you maybe comment the topic ? Best regards, Stefano
Hi Stefano and Stefan, > However, I tendentially agree with Stefan - it is ok to skip an already installed version, > but if the image should be installed into another destination / partition / flash, the > check should be done again. And only if all images are not required to be installed, > file should be skipped. Now it is skipped if just one single image is already installed. > > This is exactly against Hamish' patch. Hamish, can you maybe comment the topic ? This was for a particular use-case and can probably be improved, but I will have to look deeper into it and at the moment do not have much time available. My use-case was in particular for updating the SPL of our device. In our device we have 4 copies of the SPL, all of which are identical, and in this use-case, all are confined to be on the same flash device. We have 4 separate mtd partitions on the device, all of which contain the same binary, so when the binary is updated, all 4 partitions need to be updated. My patch simply enforced that if the first binary did not need to be updated (i.e. it was the same version of the previous binary), then none of the binaries would be updated. At the time I could not think of a use-case where this would be counter-productive, but I do not know all use-cases, so I could have been completely wrong in my assumptions. I hope this helps but I am available as always to discuss this further. With kind regards Hamish
Hi Hamish, On 21/11/2017 20:58, Hamish Guthrie wrote: > Hi Stefano and Stefan, > >> However, I tendentially agree with Stefan - it is ok to skip an already installed version, >> but if the image should be installed into another destination / partition / flash, the >> check should be done again. And only if all images are not required to be installed, >> file should be skipped. Now it is skipped if just one single image is already installed. >> >> This is exactly against Hamish' patch. Hamish, can you maybe comment the topic ? > > This was for a particular use-case and can probably be improved, but I will have to look deeper into it and at the moment do not have much time available. My use-case was in particular for updating the SPL of our device. In our device we have 4 copies of the SPL, all of which are identical, and in this use-case, all are confined to be on the same flash device. We have 4 separate mtd partitions on the device, all of which contain the same binary, so when the binary is updated, all 4 partitions need to be updated. My patch simply enforced that if the first binary did not need to be updated (i.e. it was the same version of the previous binary), then none of the binaries would be updated. Got it. Anyway, you can also add an explicit rules to skip all 4 copies, instead of having an implicit rule hard-coded in SWUpdate. SWUpdate will just iterate for the 4 images and it will find the same rule (based on version), and it will skip or accept all copies. Having the rule hard-coded in SWUpdate forbids other use case, as Stefan reported. I think that applying Stefan's patch does not block your use case and it is more flexible as now. > > At the time I could not think of a use-case where this would be counter-productive, but I do not know all use-cases, so I could have been completely wrong in my assumptions.> > I hope this helps but I am available as always to discuss this further. > Best regards, Stefano
Hi Stefan, On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > A variation is described in the sw-version file. It must match between > running images and software description artifacts. It could be used to > skip incompatible artifacts. > > Example: > > software = > { > version = "0.1"; > > hardware-compatibility: [ "revA"]; > > images: ( > { > id = "bootloader"; > variation = "A"; > filename = "boot.bin"; > device = "/dev/mtd1"; > type = "flash"; > }, > { > id = "bootloader"; > variation = "B"; > filename = "boot.bin"; > device = "/dev/mtd0"; > type = "flash"; > } > ); > > scripts: ( > { > filename = "bootloader.lua"; > type = "lua"; > }, > ); > > bootenv: ( > { > id = "bootloader"; > variation = "A"; > name = "bootloader_id"; > value = "B"; > }, > { > id = "bootloader"; > variation = "B"; > name = "bootloader_id"; > value = "A"; > } > ); > } > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > --- Patches cannot be applied anymore, but I will discuss about toimplement this and similar feature. I have thought about and I am not convinced this is the right way. In fact, this does not scale well. "Variations" is added to the internal structure, and it looks to me not general enough as other fields. If in future someone else needs something different, just to recognize if an artifact must be installed or not, there will be a new attribute, new fields in the structure, new logic into SWUpdate - the thing is that this special cases become hard-coded. I have found another solution that is more flexible. This uses no pre- post- install script, but the embedded script. That means, an image is inserted or removed from the list at parsing time. A possible sw-description for your case could be: software = { version = "0.1.0"; embedded-script =" --[[ require (\"swupdate\") function check_if_installable(image) <hier the code to check if image is in list> end "; images: ( { filename = "boot.bin"; device = "/dev/mtd1"; type = "flash"; hook = "check_if_installable"; properties = ( { name = "variation"; value = "A"; } }, { filename = "boot.bin"; device = "/dev/mtd0"; type = "flash"; hook = "check_if_installable"; properties = ( { name = "variation"; value = "B"; } } ); This has many advantages: special code is not linked to SWUpdate and can be even changed from release to release, and it is open for extension for the future. Near "variation" we can add any new attribute, and the name / value of the attributes (property) are defined by the release manager / developer, not by SWUpdate itself. To reach this, we need some changes, but they seem to me trivial and less invasive as last patches: - the embedded script must return a status (OK / NOT OK / SKIP Image), instead of just a boolean Yes / No - Properies should be pushed to the LUA stack as the rest of attributes. What do you think ? Best regards, Stefano Babic > corelib/artifacts_versions.c | 22 +++++++++++++++------- > corelib/installer.c | 33 ++++++++++++++++++++++++--------- > corelib/lua_interface.c | 5 +++++ > include/swupdate.h | 1 + > parser/parser.c | 16 ++++++++++++---- > 5 files changed, 57 insertions(+), 20 deletions(-) > > diff --git a/corelib/artifacts_versions.c b/corelib/artifacts_versions.c > index 40f7b96..85a4c4b 100644 > --- a/corelib/artifacts_versions.c > +++ b/corelib/artifacts_versions.c > @@ -48,7 +48,7 @@ static int read_sw_version_file(struct swupdate_cfg *sw) > { > FILE *fp; > int ret; > - char *name, *version; > + char *name, *version, *variation; > struct sw_version *swcomp; > > /* > @@ -61,9 +61,9 @@ static int read_sw_version_file(struct swupdate_cfg *sw) > return -EACCES; > > while (1) { > - ret = fscanf(fp, "%ms %ms", &name, &version); > + ret = fscanf(fp, "%ms %ms %ms", &name, &version, &variation); > /* pair component / version found */ > - if (ret == 2) { > + if (ret >= 2) { > swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version)); > if (!swcomp) { > ERROR("Allocation error"); > @@ -71,12 +71,17 @@ static int read_sw_version_file(struct swupdate_cfg *sw) > } > strncpy(swcomp->name, name, sizeof(swcomp->name)); > strncpy(swcomp->version, version, sizeof(swcomp->version)); > + if (ret == 3) > + strncpy(swcomp->variation, variation, sizeof(swcomp->variation)); > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); > - TRACE("Installed %s: Version %s", > + TRACE("Installed %s: Version %s%s%s", > swcomp->name, > - swcomp->version); > + swcomp->version, > + strlen(swcomp->variation) ? ", Variation " : "", > + strlen(swcomp->variation) ? swcomp->variation : ""); > free(name); > free(version); > + free(variation); > } else { > if (ret == EOF) > break; > @@ -122,11 +127,14 @@ static int versions_settings(void *setting, void *data) > > GET_FIELD_STRING(LIBCFG_PARSER, elem, "name", swcomp->name); > GET_FIELD_STRING(LIBCFG_PARSER, elem, "version", swcomp->version); > + GET_FIELD_STRING(LIBCFG_PARSER, elem, "variation", swcomp->variation); > > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); > - TRACE("Installed %s: Version %s", > + TRACE("Installed %s: Version %s%s%s", > swcomp->name, > - swcomp->version); > + swcomp->version, > + strlen(swcomp->variation) ? ", Variation " : "", > + strlen(swcomp->variation) ? swcomp->variation : ""); > } > > return 0; > diff --git a/corelib/installer.c b/corelib/installer.c > index 02cb467..7ab9537 100644 > --- a/corelib/installer.c > +++ b/corelib/installer.c > @@ -56,21 +56,36 @@ static int check_if_installable(struct swver *sw_ver_list, > if (!sw_ver_list) > return true; > > - if (!strlen(img->id.name) || (!strlen(img->id.version) || > - !img->id.install_if_different)) > + if (!strlen(img->id.name)) > + return true; > + > + if ((!strlen(img->id.version) || !img->id.install_if_different) && > + !strlen(img->id.variation)) > return true; > > LIST_FOREACH(swver, sw_ver_list, next) { > /* > - * Check if name and version are identical > + * Check if name and version are identical or > + * name identical and variation compatible > */ > - if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) && > - !strncmp(img->id.version, swver->version, sizeof(img->id.version))) { > - TRACE("%s(%s) already installed, skipping...", > - img->id.name, > - img->id.version); > + if (!strncmp(img->id.name, swver->name, sizeof(img->id.name))) { > + if (img->id.install_if_different && strlen(img->id.version) && > + !strncmp(img->id.version, swver->version, sizeof(img->id.version))) { > + TRACE("%s version %s already installed, skipping...", > + img->id.name, > + img->id.version); > + return false; > + } > + > + if (strlen(img->id.variation) && > + strncmp(img->id.variation, swver->variation, sizeof(img->id.variation))) { > + TRACE("%s variation %s not compatible, skipping...", > + img->id.name, > + img->id.variation); > + return false; > + } > > - return false; > + return true; > } > } > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index b472304..8fbf085 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -189,6 +189,10 @@ static void lua_string_to_img(struct img_type *img, const char *key, > strncpy(img->id.version, value, > sizeof(img->id.version)); > } > + if (!strcmp(key, "variation")) { > + strncpy(img->id.variation, value, > + sizeof(img->id.variation)); > + } > if (!strcmp(key, "filename")) { > strncpy(img->fname, value, > sizeof(img->fname)); > @@ -269,6 +273,7 @@ static void image2table(lua_State* L, struct img_type *img) { > lua_newtable (L); > LUA_PUSH_IMG_STRING(img, "id", id.name); > LUA_PUSH_IMG_STRING(img, "version", id.version); > + LUA_PUSH_IMG_STRING(img, "variation", id.variation); > LUA_PUSH_IMG_STRING(img, "filename", fname); > LUA_PUSH_IMG_STRING(img, "volume", volname); > LUA_PUSH_IMG_STRING(img, "type", type); > diff --git a/include/swupdate.h b/include/swupdate.h > index 96b10c8..407c7e5 100644 > --- a/include/swupdate.h > +++ b/include/swupdate.h > @@ -53,6 +53,7 @@ enum { > struct sw_version { > char name[SWUPDATE_GENERAL_STRING_SIZE]; > char version[SWUPDATE_GENERAL_STRING_SIZE]; > + char variation[SWUPDATE_GENERAL_STRING_SIZE]; > int install_if_different; > LIST_ENTRY(sw_version) next; > }; > diff --git a/parser/parser.c b/parser/parser.c > index 55e7b7f..f0a5d11 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -307,6 +307,7 @@ static int parse_scripts(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lu > > GET_FIELD_STRING(p, elem, "id", script->id.name); > GET_FIELD_STRING(p, elem, "version", script->id.version); > + GET_FIELD_STRING(p, elem, "variation", script->id.variation); > GET_FIELD_STRING(p, elem, "filename", script->fname); > GET_FIELD_STRING(p, elem, "type", script->type); > GET_FIELD_STRING(p, elem, "data", script->type_data); > @@ -328,9 +329,10 @@ static int parse_scripts(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lu > > LIST_INSERT_HEAD(&swcfg->scripts, script, next); > > - TRACE("Found Script%s%s%s%s: %s%s\n", > + TRACE("Found Script%s%s%s%s%s%s: %s%s\n", > strlen(script->id.name) ? " " : "", script->id.name, > strlen(script->id.version) ? " " : "", script->id.version, > + strlen(script->id.variation) ? " " : "", script->id.variation, > script->fname, > (strlen(script->id.name) && script->id.install_if_different) ? > "; Version must be checked" : ""); > @@ -376,6 +378,7 @@ static int parse_bootloader(parsertype p, void *cfg, struct swupdate_cfg *swcfg, > > GET_FIELD_STRING(p, elem, "id", boot->id.name); > GET_FIELD_STRING(p, elem, "version", boot->id.version); > + GET_FIELD_STRING(p, elem, "variation", boot->id.variation); > GET_FIELD_STRING(p, elem, "name", boot->name); > GET_FIELD_STRING(p, elem, "value", boot->value); > get_field(p, elem, "install-if-different", &boot->id.install_if_different); > @@ -387,9 +390,10 @@ static int parse_bootloader(parsertype p, void *cfg, struct swupdate_cfg *swcfg, > > LIST_INSERT_HEAD(&swcfg->bootloader, boot, next); > > - TRACE("Found boot loader variable%s%s%s%s: %s = '%s'%s\n", > + TRACE("Found boot loader variable%s%s%s%s%s%s: %s = '%s'%s\n", > strlen(boot->id.name) ? " " : "", boot->id.name, > strlen(boot->id.version) ? " " : "", boot->id.version, > + strlen(boot->id.variation) ? " " : "", boot->id.variation, > boot->name, > boot->value, > (strlen(boot->id.name) && boot->id.install_if_different) ? > @@ -442,6 +446,7 @@ static int parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua > > GET_FIELD_STRING(p, elem, "id", image->id.name); > GET_FIELD_STRING(p, elem, "version", image->id.version); > + GET_FIELD_STRING(p, elem, "variation", image->id.variation); > GET_FIELD_STRING(p, elem, "filename", image->fname); > GET_FIELD_STRING(p, elem, "volume", image->volname); > GET_FIELD_STRING(p, elem, "device", image->device); > @@ -490,10 +495,11 @@ static int parse_images(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua > > LIST_INSERT_HEAD(&swcfg->images, image, next); > > - TRACE("Found %sImage%s%s%s%s: %s in %s : %s for handler %s%s%s\n", > + TRACE("Found %sImage%s%s%s%s%s%s: %s in %s : %s for handler %s%s%s\n", > image->compressed ? "compressed " : "", > strlen(image->id.name) ? " " : "", image->id.name, > strlen(image->id.version) ? " " : "", image->id.version, > + strlen(image->id.variation) ? " " : "", image->id.variation, > image->fname, > strlen(image->volname) ? "volume" : "device", > strlen(image->volname) ? image->volname : > @@ -541,6 +547,7 @@ static int parse_files(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua_ > > GET_FIELD_STRING(p, elem, "id", file->id.name); > GET_FIELD_STRING(p, elem, "version", file->id.version); > + GET_FIELD_STRING(p, elem, "variation", file->id.variation); > GET_FIELD_STRING(p, elem, "filename", file->fname); > GET_FIELD_STRING(p, elem, "path", file->path); > GET_FIELD_STRING(p, elem, "device", file->device); > @@ -570,10 +577,11 @@ static int parse_files(parsertype p, void *cfg, struct swupdate_cfg *swcfg, lua_ > > LIST_INSERT_HEAD(&swcfg->images, file, next); > > - TRACE("Found %sFile%s%s%s%s: %s --> %s (%s)%s\n", > + TRACE("Found %sFile%s%s%s%s%s%s: %s --> %s (%s)%s\n", > file->compressed ? "compressed " : "", > strlen(file->id.name) ? " " : "", file->id.name, > strlen(file->id.version) ? " " : "", file->id.version, > + strlen(file->id.variation) ? " " : "", file->id.variation, > file->fname, > file->path, > strlen(file->device) ? file->device : "ROOTFS", >
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Freitag, 24. November 2017 15:08 > An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Cc: Herbrechtsmeier Dr.-Ing. , Stefan > Betreff: Re: [swupdate] [PATCH 6/6] parser: Add variation support > > On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > > > A variation is described in the sw-version file. It must match > between > > running images and software description artifacts. It could be used > to > > skip incompatible artifacts. > > > > Example: > > > > software = > > { > > version = "0.1"; > > > > hardware-compatibility: [ "revA"]; > > > > images: ( > > { > > id = "bootloader"; > > variation = "A"; > > filename = "boot.bin"; > > device = "/dev/mtd1"; > > type = "flash"; > > }, > > { > > id = "bootloader"; > > variation = "B"; > > filename = "boot.bin"; > > device = "/dev/mtd0"; > > type = "flash"; > > } > > ); > > > > scripts: ( > > { > > filename = "bootloader.lua"; > > type = "lua"; > > }, > > ); > > > > bootenv: ( > > { > > id = "bootloader"; > > variation = "A"; > > name = "bootloader_id"; > > value = "B"; > > }, > > { > > id = "bootloader"; > > variation = "B"; > > name = "bootloader_id"; > > value = "A"; > > } > > ); > > } > > > > Signed-off-by: Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier@weidmueller.com> > > --- > > Patches cannot be applied anymore, but I will discuss about toimplement > this and similar feature. I have thought about and I am not convinced > this is the right way. > > In fact, this does not scale well. "Variations" is added to the > internal structure, and it looks to me not general enough as other > fields. If in future someone else needs something different, just to > recognize if an artifact must be installed or not, there will be a new > attribute, new fields in the structure, new logic into SWUpdate - the > thing is that this special cases become hard-coded. As long as you can detect the difference at system startup and decode it in a string you could reuse the variation property. > I have found another solution that is more flexible. This uses no pre- > post- install script, but the embedded script. That means, an image is > inserted or removed from the list at parsing time. I used the embedded script at first but there are some thing I dislike and therefore I reuse the version check approach. - The embedded script is error-prone because you must escape the double quotes inside the script - You couldn’t share the embedded script between sw-description files - You lose the community aspect because the embedded script couldn’t be shared between users > A possible sw-description for your case could be: > software = > { > version = "0.1.0"; > > embedded-script =" > --[[ > > require (\"swupdate\") > function check_if_installable(image) > > <hier the code to check if image is in list> end "; > > images: ( > { > filename = "boot.bin"; > device = "/dev/mtd1"; > type = "flash"; > hook = "check_if_installable"; > properties = ( > { > name = "variation"; > value = "A"; > } > }, > { > filename = "boot.bin"; > device = "/dev/mtd0"; > type = "flash"; > hook = "check_if_installable"; > properties = ( > { > name = "variation"; > value = "B"; > } > } > ); I don’t like the name properties because also the filename is a property. Maybe parameters or attributes are better names. > This has many advantages: special code is not linked to SWUpdate and > can be even changed from release to release, and it is open for > extension for the future. Near "variation" we can add any new > attribute, and the name / value of the attributes (property) are > defined by the release manager / developer, not by SWUpdate itself. You advantage have the disadvantage that you need to ship the code with each image and that you lose some level of abstraction because the update need to know the details about the installed update image version. Additionally everybody will use its one “properties” and you lose any kind of best practice or common solutions. > To reach this, we need some changes, but they seem to me trivial and > less invasive as last patches: > > - the embedded script must return a status (OK / NOT OK / SKIP Image), > instead of just a boolean Yes / No What about an additional skip image type property to be backward compatible with older embedded scripts? > - Properies should be pushed to the LUA stack as the rest of > attributes. What happens if you use a name of an existing or future attribute? > What do you think ? I see your point that the embedded script is more flexible. Maybe we could rework the embedded script a little bit: - Add a tool or script to preprocess a sw-description.in file and automatically embedded different LUA scripts - Collect LUA scripts in the project 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
Hi Stefan, On 27/11/2017 14:37, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Freitag, 24. November 2017 15:08 >> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >> Betreff: Re: [swupdate] [PATCH 6/6] parser: Add variation support >> >> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote: >>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>> >>> A variation is described in the sw-version file. It must match >> between >>> running images and software description artifacts. It could be used >> to >>> skip incompatible artifacts. >>> >>> Example: >>> >>> software = >>> { >>> version = "0.1"; >>> >>> hardware-compatibility: [ "revA"]; >>> >>> images: ( >>> { >>> id = "bootloader"; >>> variation = "A"; >>> filename = "boot.bin"; >>> device = "/dev/mtd1"; >>> type = "flash"; >>> }, >>> { >>> id = "bootloader"; >>> variation = "B"; >>> filename = "boot.bin"; >>> device = "/dev/mtd0"; >>> type = "flash"; >>> } >>> ); >>> >>> scripts: ( >>> { >>> filename = "bootloader.lua"; >>> type = "lua"; >>> }, >>> ); >>> >>> bootenv: ( >>> { >>> id = "bootloader"; >>> variation = "A"; >>> name = "bootloader_id"; >>> value = "B"; >>> }, >>> { >>> id = "bootloader"; >>> variation = "B"; >>> name = "bootloader_id"; >>> value = "A"; >>> } >>> ); >>> } >>> >>> Signed-off-by: Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier@weidmueller.com> >>> --- >> >> Patches cannot be applied anymore, but I will discuss about toimplement >> this and similar feature. I have thought about and I am not convinced >> this is the right way. >> >> In fact, this does not scale well. "Variations" is added to the >> internal structure, and it looks to me not general enough as other >> fields. If in future someone else needs something different, just to >> recognize if an artifact must be installed or not, there will be a new >> attribute, new fields in the structure, new logic into SWUpdate - the >> thing is that this special cases become hard-coded. > > As long as you can detect the difference at system startup and decode it in a string you could reuse the variation property. > >> I have found another solution that is more flexible. This uses no pre- >> post- install script, but the embedded script. That means, an image is >> inserted or removed from the list at parsing time. > > I used the embedded script at first but there are some thing I dislike and therefore I reuse the version check approach. > - The embedded script is error-prone because you must escape the double quotes inside the script True, but the error should not happen in field as an upgrade was already tested in the development cycle. If we say that this requires some iterations to fix the script, this is also true, but the same happens for pre- and post- install scripts. The embedded script could be tested with the "-c" (check) parameter if there are not conditions that can be tested only on the device. > - You couldn’t share the embedded script between sw-description files This is another nice to have feature, that is a "include" to reuse some parts of sw-description. It could also generated at build time, the feature is not strictly required on the device. Yes, this is also in roadmap for project. Anyway, this is an independent issue. > - You lose the community aspect because the embedded script couldn’t be shared between users This mostly depends on the more permissive licence for LUA scripts. They are not GPLv2, and users are allowed to not share them, but I *strongly* recommend any users to share their scripts and LUA handlers. We could add a 3rdparty directory to collect them and include into the project. And after moving the utilities from examples to tools, much more sw-description examples can be collected into the project. Nothing against. And LUA scripts can be added, too. > >> A possible sw-description for your case could be: >> software = >> { >> version = "0.1.0"; >> >> embedded-script =" >> --[[ >> >> require (\"swupdate\") >> function check_if_installable(image) >> >> <hier the code to check if image is in list> end "; >> >> images: ( >> { >> filename = "boot.bin"; >> device = "/dev/mtd1"; >> type = "flash"; >> hook = "check_if_installable"; >> properties = ( >> { >> name = "variation"; >> value = "A"; >> } >> }, >> { >> filename = "boot.bin"; >> device = "/dev/mtd0"; >> type = "flash"; >> hook = "check_if_installable"; >> properties = ( >> { >> name = "variation"; >> value = "B"; >> } >> } >> ); > > I don’t like the name properties because also the filename is a property. Maybe parameters or attributes are better names. On the other side, the single entries are called "attribute" in whole documentation. I get rid of correct names, I confess. No problem to switch to "parameters" if this is better understood. > >> This has many advantages: special code is not linked to SWUpdate and >> can be even changed from release to release, and it is open for >> extension for the future. Near "variation" we can add any new >> attribute, and the name / value of the attributes (property) are >> defined by the release manager / developer, not by SWUpdate itself. > > You advantage have the disadvantage that you need to ship the code with each image and that you lose some level of abstraction because the update need to know the details about the installed update image version. Not so much. The logic (that is, the script) must be delivered, but the detail can be still known at runtime when script is running. It is not so different as now when we provide the full configuration for a dual-copy and device detects where is root. > Additionally everybody will use its one “properties” and you lose any kind of best practice or common solutions. This is also true if people / companies do not understand the added value to push their changes upstream and to work in an open source community. Their changes won't work maybe in future if they will not work together with community, and they have to maintain themselves a fork of a project. I saw this a lot of times in U-Boot or linux as well. > > >> To reach this, we need some changes, but they seem to me trivial and >> less invasive as last patches: >> >> - the embedded script must return a status (OK / NOT OK / SKIP Image), >> instead of just a boolean Yes / No > > What about an additional skip image type property to be backward compatible with older embedded scripts? So you want to pass the return code via the LUA stack with a new value. It looks more convoluted, I will prefer to let things easy, even if this creates an incompatibility with previous embedded scripts development. > > >> - Properies should be pushed to the LUA stack as the rest of >> attributes. > > What happens if you use a name of an existing or future attribute? All inside a "properties" in sw description should be in a separate namespace. This can be reached with a separate table or with a convention for name (like "property_<name>"). > >> What do you think ? > > I see your point that the embedded script is more flexible. Maybe we could rework the embedded script a little bit: > - Add a tool or script to preprocess a sw-description.in file and automatically embedded different LUA scripts +1 > - Collect LUA scripts in the project Fully agree, thanks for remember this. I ask all SWUpdate users to not be shy and send your LUA script examples or LUA handler. Best regards, Stefano Babic
Hi Stefano > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Mittwoch, 29. November 2017 15:19 > An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: [swupdate] [PATCH 6/6] parser: Add variation support > > Hi Stefan, > > On 27/11/2017 14:37, Stefan.Herbrechtsmeier@weidmueller.com wrote: > > Hi Stefano, > > > >> -----Ursprüngliche Nachricht----- > >> Von: Stefano Babic [mailto:sbabic@denx.de] > >> Gesendet: Freitag, 24. November 2017 15:08 > >> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >> Cc: Herbrechtsmeier Dr.-Ing. , Stefan > >> Betreff: Re: [swupdate] [PATCH 6/6] parser: Add variation support > >> > >> On 26/10/2017 09:18, stefan@herbrechtsmeier.net wrote: > >>> From: Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier@weidmueller.com> > >>> > >>> A variation is described in the sw-version file. It must match > >> between > >>> running images and software description artifacts. It could be used > >> to > >>> skip incompatible artifacts. > >>> > >>> Example: > >>> > >>> software = > >>> { > >>> version = "0.1"; > >>> > >>> hardware-compatibility: [ "revA"]; > >>> > >>> images: ( > >>> { > >>> id = "bootloader"; > >>> variation = "A"; > >>> filename = "boot.bin"; > >>> device = "/dev/mtd1"; > >>> type = "flash"; > >>> }, > >>> { > >>> id = "bootloader"; > >>> variation = "B"; > >>> filename = "boot.bin"; > >>> device = "/dev/mtd0"; > >>> type = "flash"; > >>> } > >>> ); > >>> > >>> scripts: ( > >>> { > >>> filename = "bootloader.lua"; > >>> type = "lua"; > >>> }, > >>> ); > >>> > >>> bootenv: ( > >>> { > >>> id = "bootloader"; > >>> variation = "A"; > >>> name = "bootloader_id"; > >>> value = "B"; > >>> }, > >>> { > >>> id = "bootloader"; > >>> variation = "B"; > >>> name = "bootloader_id"; > >>> value = "A"; > >>> } > >>> ); > >>> } > >>> > >>> Signed-off-by: Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier@weidmueller.com> > >>> --- > >> > >> Patches cannot be applied anymore, but I will discuss about > >> toimplement this and similar feature. I have thought about and I am > >> not convinced this is the right way. > >> > >> In fact, this does not scale well. "Variations" is added to the > >> internal structure, and it looks to me not general enough as other > >> fields. If in future someone else needs something different, just to > >> recognize if an artifact must be installed or not, there will be a > >> new attribute, new fields in the structure, new logic into SWUpdate > - > >> the thing is that this special cases become hard-coded. > > > > As long as you can detect the difference at system startup and decode > it in a string you could reuse the variation property. > > > >> I have found another solution that is more flexible. This uses no > >> pre- > >> post- install script, but the embedded script. That means, an image > >> is inserted or removed from the list at parsing time. > > > > I used the embedded script at first but there are some thing I > dislike and therefore I reuse the version check approach. > > - The embedded script is error-prone because you must escape the > > double quotes inside the script > > True, but the error should not happen in field as an upgrade was > already tested in the development cycle. If we say that this requires > some iterations to fix the script, this is also true, but the same > happens for pre- and post- install scripts. > > The embedded script could be tested with the "-c" (check) parameter if > there are not conditions that can be tested only on the device. > > > - You couldn’t share the embedded script between sw-description files > > This is another nice to have feature, that is a "include" to reuse some > parts of sw-description. It could also generated at build time, the > feature is not strictly required on the device. Yes, this is also in > roadmap for project. Anyway, this is an independent issue. > > > - You lose the community aspect because the embedded script couldn’t > > be shared between users > > This mostly depends on the more permissive licence for LUA scripts. > They are not GPLv2, and users are allowed to not share them, but I > *strongly* recommend any users to share their scripts and LUA handlers. > We could add a 3rdparty directory to collect them and include into the > project. > And after moving the utilities from examples to tools, much more sw- > description examples can be collected into the project. Nothing > against. > > And LUA scripts can be added, too. > > > > >> A possible sw-description for your case could be: > >> software = > >> { > >> version = "0.1.0"; > >> > >> embedded-script =" > >> --[[ > >> > >> require (\"swupdate\") > >> function check_if_installable(image) > >> > >> <hier the code to check if image is in list> end "; > >> > >> images: ( > >> { > >> filename = "boot.bin"; > >> device = "/dev/mtd1"; > >> type = "flash"; > >> hook = "check_if_installable"; > >> properties = ( > >> { > >> name = "variation"; > >> value = "A"; > >> } > >> }, > >> { > >> filename = "boot.bin"; > >> device = "/dev/mtd0"; > >> type = "flash"; > >> hook = "check_if_installable"; > >> properties = ( > >> { > >> name = "variation"; > >> value = "B"; > >> } > >> } > >> ); > > > > I don’t like the name properties because also the filename is a > property. Maybe parameters or attributes are better names. > > On the other side, the single entries are called "attribute" in whole > documentation. I get rid of correct names, I confess. No problem to > switch to "parameters" if this is better understood. I would prefer "parameters" or "hook-parameters". > >> This has many advantages: special code is not linked to SWUpdate and > >> can be even changed from release to release, and it is open for > >> extension for the future. Near "variation" we can add any new > >> attribute, and the name / value of the attributes (property) are > >> defined by the release manager / developer, not by SWUpdate itself. > > > > You advantage have the disadvantage that you need to ship the code > with each image and that you lose some level of abstraction because the > update need to know the details about the installed update image > version. > > Not so much. The logic (that is, the script) must be delivered, but the > detail can be still known at runtime when script is running. It is not > so different as now when we provide the full configuration for a dual- > copy and device detects where is root. > > > Additionally everybody will use its one “properties” and you lose any > kind of best practice or common solutions. > > This is also true if people / companies do not understand the added > value to push their changes upstream and to work in an open source > community. Their changes won't work maybe in future if they will not > work together with community, and they have to maintain themselves a > fork of a project. I saw this a lot of times in U-Boot or linux as > well. > > > > > > >> To reach this, we need some changes, but they seem to me trivial and > >> less invasive as last patches: > >> > >> - the embedded script must return a status (OK / NOT OK / SKIP > >> Image), instead of just a boolean Yes / No > > > > What about an additional skip image type property to be backward > compatible with older embedded scripts? > > So you want to pass the return code via the LUA stack with a new value. > It looks more convoluted, I will prefer to let things easy, even if > this creates an incompatibility with previous embedded scripts > development. I would add a new attribute "skip" to the sw-description with the default value "false". I wouldn't combine different information (skip image and parse status) into one variable. > >> - Properies should be pushed to the LUA stack as the rest of > >> attributes. > > > > What happens if you use a name of an existing or future attribute? > All inside a "properties" in sw description should be in a separate > namespace. This can be reached with a separate table or with a > convention for name (like "property_<name>"). I would go with a sub table to be conform with the sw-description. > >> What do you think ? > > > > I see your point that the embedded script is more flexible. Maybe we > could rework the embedded script a little bit: > > - Add a tool or script to preprocess a sw-description.in file and > > automatically embedded different LUA scripts > > +1 > > > - Collect LUA scripts in the project > > Fully agree, thanks for remember this. I ask all SWUpdate users to not > be shy and send your LUA script examples or LUA handler. Let's discuss the other optimization after we have integrate the variation support. After we agree on the approach I could adapt my system and post a new patch series. 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
Hi Stefan, I dropped most part and let just the open points: On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: >> On the other side, the single entries are called "attribute" in whole >> documentation. I get rid of correct names, I confess. No problem to >> switch to "parameters" if this is better understood. > > I would prefer "parameters" or "hook-parameters". They are not related to hook. They are already used for the "swuforwarder" handler that I pushed 10 days ago. As this is a very new feature, I do not care about compatibility and we could also change the name, but if we have not a real added value, I will let it as it is now ("properties"). We can say that this new set of attributes is specific for a later entity, such as the embedded script or a handler. A split can be thought that everything in "properties" or "parameters" (or whatever we choose) is not evaluated by the parser itself. This does not require to change "struct img_type", fact that does not scale well. > > I would add a new attribute "skip" to the sw-description with the default value "false". > > I wouldn't combine different information (skip image and parse status) into one variable. ok, we can do in this way. > >>>> - Properies should be pushed to the LUA stack as the rest of >>>> attributes. >>> >>> What happens if you use a name of an existing or future attribute? >> All inside a "properties" in sw description should be in a separate >> namespace. This can be reached with a separate table or with a >> convention for name (like "property_<name>"). > > I would go with a sub table to be conform with the sw-description. ok > >>>> What do you think ? >>> >>> I see your point that the embedded script is more flexible. Maybe we >> could rework the embedded script a little bit: >>> - Add a tool or script to preprocess a sw-description.in file and >>> automatically embedded different LUA scripts >> >> +1 >> >>> - Collect LUA scripts in the project >> >> Fully agree, thanks for remember this. I ask all SWUpdate users to not >> be shy and send your LUA script examples or LUA handler. > > Let's discuss the other optimization after we have integrate the variation support. > > After we agree on the approach I could adapt my system and post a new patch series. Fine with me. Best regards, Stefano Babic
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Freitag, 1. Dezember 2017 10:49 > An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation > support > > I dropped most part and let just the open points: > > On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: > > >> On the other side, the single entries are called "attribute" in > whole > >> documentation. I get rid of correct names, I confess. No problem to > >> switch to "parameters" if this is better understood. > > > > I would prefer "parameters" or "hook-parameters". > > They are not related to hook. They are already used for the > "swuforwarder" handler that I pushed 10 days ago. As this is a very new > feature, I do not care about compatibility and we could also change the > name, but if we have not a real added value, I will let it as it is now > ("properties"). > > We can say that this new set of attributes is specific for a later > entity, such as the embedded script or a handler. A split can be > thought that everything in "properties" or "parameters" (or whatever we > choose) is not evaluated by the parser itself. This does not require to > change "struct img_type", fact that does not scale well. The normal attributes could be manipulated and the private "properties" or "parameters" describe the immutable "characteristics" or "constants" of an image. I find the "properties" name to generic but you are right the "parameters" name don't match. Why do you use an array of name value elements instead of an list of arbitrary attributes? 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
Hi Stefan, On 01/12/2017 12:51, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Freitag, 1. Dezember 2017 10:49 >> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Betreff: Re: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation >> support >> >> I dropped most part and let just the open points: >> >> On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: >> >>>> On the other side, the single entries are called "attribute" in >> whole >>>> documentation. I get rid of correct names, I confess. No problem to >>>> switch to "parameters" if this is better understood. >>> >>> I would prefer "parameters" or "hook-parameters". >> >> They are not related to hook. They are already used for the >> "swuforwarder" handler that I pushed 10 days ago. As this is a very new >> feature, I do not care about compatibility and we could also change the >> name, but if we have not a real added value, I will let it as it is now >> ("properties"). >> >> We can say that this new set of attributes is specific for a later >> entity, such as the embedded script or a handler. A split can be >> thought that everything in "properties" or "parameters" (or whatever we >> choose) is not evaluated by the parser itself. This does not require to >> change "struct img_type", fact that does not scale well. > > The normal attributes could be manipulated and the private "properties" or "parameters" describe the immutable "characteristics" or "constants" of an image. I find the "properties" name to generic but you are right the "parameters" name don't match. > > Why do you use an array of name value elements instead of an list of arbitrary attributes? Good question. Instead of: properties = ( { name = "variation"; value = "B"; }, { name = "othername"; value = "other value"; }, it seems more logical: properties = { variation = "B"; othername = "other value"; } However, I have not found in the libconfig API a way to get the name of the attribute. I can always get the value when I know the name of the attribute, but I have not found (without patching libconfig, that I do not want) a way to iterate in the list and retrieving both name and value. I will thank you if you find the way (with json-c is possible) and I will switch to the list then. Best regards, Stefano Babic
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Freitag, 1. Dezember 2017 13:02 > An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation > support > > On 01/12/2017 12:51, Stefan.Herbrechtsmeier@weidmueller.com wrote: > > Hi Stefano, > > > >> -----Ursprüngliche Nachricht----- > >> Von: Stefano Babic [mailto:sbabic@denx.de] > >> Gesendet: Freitag, 1. Dezember 2017 10:49 > >> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >> Betreff: Re: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation > >> support > >> > >> I dropped most part and let just the open points: > >> > >> On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >> > >>>> On the other side, the single entries are called "attribute" in > >> whole > >>>> documentation. I get rid of correct names, I confess. No problem > to > >>>> switch to "parameters" if this is better understood. > >>> > >>> I would prefer "parameters" or "hook-parameters". > >> > >> They are not related to hook. They are already used for the > >> "swuforwarder" handler that I pushed 10 days ago. As this is a very > >> new feature, I do not care about compatibility and we could also > >> change the name, but if we have not a real added value, I will let > it > >> as it is now ("properties"). > >> > >> We can say that this new set of attributes is specific for a later > >> entity, such as the embedded script or a handler. A split can be > >> thought that everything in "properties" or "parameters" (or whatever > >> we > >> choose) is not evaluated by the parser itself. This does not require > >> to change "struct img_type", fact that does not scale well. > > > > The normal attributes could be manipulated and the private > "properties" or "parameters" describe the immutable "characteristics" > or "constants" of an image. I find the "properties" name to generic but > you are right the "parameters" name don't match. > > > > Why do you use an array of name value elements instead of an list of > arbitrary attributes? > > Good question. Instead of: > > properties = ( > { > name = "variation"; > value = "B"; > }, > { > name = "othername"; > value = "other value"; > }, > > it seems more logical: > > properties = { > variation = "B"; > othername = "other value"; > } > > However, I have not found in the libconfig API a way to get the name of > the attribute. I can always get the value when I know the name of the > attribute, but I have not found (without patching libconfig, that I do > not want) a way to iterate in the list and retrieving both name and > value. I will thank you if you find the way (with json-c is possible) > and I will switch to the list then. The config_setting_length and config_setting_get_elem functions allow the iteration over a group. The returned config_setting_t type has a name attribute. 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
On 01/12/2017 14:13, Stefan.Herbrechtsmeier@weidmueller.com wrote: > > > > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Freitag, 1. Dezember 2017 13:02 >> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Betreff: Re: AW: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation >> support >> >> On 01/12/2017 12:51, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>> Hi Stefano, >>> >>>> -----Ursprüngliche Nachricht----- >>>> Von: Stefano Babic [mailto:sbabic@denx.de] >>>> Gesendet: Freitag, 1. Dezember 2017 10:49 >>>> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; >>>> stefan@herbrechtsmeier.net; swupdate@googlegroups.com >>>> Betreff: Re: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation >>>> support >>>> >>>> I dropped most part and let just the open points: >>>> >>>> On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>> >>>>>> On the other side, the single entries are called "attribute" in >>>> whole >>>>>> documentation. I get rid of correct names, I confess. No problem >> to >>>>>> switch to "parameters" if this is better understood. >>>>> >>>>> I would prefer "parameters" or "hook-parameters". >>>> >>>> They are not related to hook. They are already used for the >>>> "swuforwarder" handler that I pushed 10 days ago. As this is a very >>>> new feature, I do not care about compatibility and we could also >>>> change the name, but if we have not a real added value, I will let >> it >>>> as it is now ("properties"). >>>> >>>> We can say that this new set of attributes is specific for a later >>>> entity, such as the embedded script or a handler. A split can be >>>> thought that everything in "properties" or "parameters" (or whatever >>>> we >>>> choose) is not evaluated by the parser itself. This does not require >>>> to change "struct img_type", fact that does not scale well. >>> >>> The normal attributes could be manipulated and the private >> "properties" or "parameters" describe the immutable "characteristics" >> or "constants" of an image. I find the "properties" name to generic but >> you are right the "parameters" name don't match. >>> >>> Why do you use an array of name value elements instead of an list of >> arbitrary attributes? >> >> Good question. Instead of: >> >> properties = ( >> { >> name = "variation"; >> value = "B"; >> }, >> { >> name = "othername"; >> value = "other value"; >> }, >> >> it seems more logical: >> >> properties = { >> variation = "B"; >> othername = "other value"; >> } >> >> However, I have not found in the libconfig API a way to get the name of >> the attribute. I can always get the value when I know the name of the >> attribute, but I have not found (without patching libconfig, that I do >> not want) a way to iterate in the list and retrieving both name and >> value. I will thank you if you find the way (with json-c is possible) >> and I will switch to the list then. > > The config_setting_length and config_setting_get_elem functions allow the iteration over a group. The returned config_setting_t type has a name attribute. In my tests, type->name was always NULL. Best regards, Stefano Babic
Hi Stefano, Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Freitag, 1. Dezember 2017 14:24 > An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: AW: AW: AW: [swupdate] [PATCH 6/6] parser: Add > variation support > > On 01/12/2017 14:13, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >> -----Ursprüngliche Nachricht----- > >> Von: Stefano Babic [mailto:sbabic@denx.de] > >> Gesendet: Freitag, 1. Dezember 2017 13:02 > >> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >> Betreff: Re: AW: AW: AW: [swupdate] [PATCH 6/6] parser: Add > variation > >> support > >> > >> On 01/12/2017 12:51, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >>> Hi Stefano, > >>> > >>>> -----Ursprüngliche Nachricht----- > >>>> Von: Stefano Babic [mailto:sbabic@denx.de] > >>>> Gesendet: Freitag, 1. Dezember 2017 10:49 > >>>> An: Herbrechtsmeier Dr.-Ing. , Stefan; sbabic@denx.de; > >>>> stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >>>> Betreff: Re: AW: AW: [swupdate] [PATCH 6/6] parser: Add variation > >>>> support > >>>> > >>>> I dropped most part and let just the open points: > >>>> > >>>> On 30/11/2017 10:02, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >>>> > >>>>>> On the other side, the single entries are called "attribute" in > >>>> whole > >>>>>> documentation. I get rid of correct names, I confess. No problem > >> to > >>>>>> switch to "parameters" if this is better understood. > >>>>> > >>>>> I would prefer "parameters" or "hook-parameters". > >>>> > >>>> They are not related to hook. They are already used for the > >>>> "swuforwarder" handler that I pushed 10 days ago. As this is a > very > >>>> new feature, I do not care about compatibility and we could also > >>>> change the name, but if we have not a real added value, I will let > >> it > >>>> as it is now ("properties"). > >>>> > >>>> We can say that this new set of attributes is specific for a later > >>>> entity, such as the embedded script or a handler. A split can be > >>>> thought that everything in "properties" or "parameters" (or > >>>> whatever we > >>>> choose) is not evaluated by the parser itself. This does not > >>>> require to change "struct img_type", fact that does not scale > well. > >>> > >>> The normal attributes could be manipulated and the private > >> "properties" or "parameters" describe the immutable > "characteristics" > >> or "constants" of an image. I find the "properties" name to generic > >> but you are right the "parameters" name don't match. > >>> > >>> Why do you use an array of name value elements instead of an list > of > >> arbitrary attributes? > >> > >> Good question. Instead of: > >> > >> properties = ( > >> { > >> name = "variation"; > >> value = "B"; > >> }, > >> { > >> name = "othername"; > >> value = "other value"; > >> }, > >> > >> it seems more logical: > >> > >> properties = { > >> variation = "B"; > >> othername = "other value"; > >> } > >> > >> However, I have not found in the libconfig API a way to get the name > >> of the attribute. I can always get the value when I know the name of > >> the attribute, but I have not found (without patching libconfig, > that > >> I do not want) a way to iterate in the list and retrieving both name > >> and value. I will thank you if you find the way (with json-c is > >> possible) and I will switch to the list then. > > > > The config_setting_length and config_setting_get_elem functions allow > the iteration over a group. The returned config_setting_t type has a > name attribute. > > In my tests, type->name was always NULL. You need to define a group in the configuration file. I have add the following lines in the example1.c and it works. config_setting_t *t = config_setting_get_elem(movie, 0); printf("%-30s\n", t->name); 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
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; }