diff mbox series

Added update description field support.

Message ID 1521737540-6888-1-git-send-email-afaustas@gmail.com
State Changes Requested
Headers show
Series Added update description field support. | expand

Commit Message

Faustas Azuolas Bagdonas March 22, 2018, 4:52 p.m. UTC 
These modifications adds a support of a description field in sw-description.
This field is optional. Example of sw-description file beginning:

software =
{
        version = "0.1.0";
	description = "This update adds a new feature and includes important bug fixes." # <-- This is a new field added by this modification.

        hardware-compatibility: [ "1.0", "1.2", "1.3"];

        <...>

Signed-off-by: Faustas Azuolas Bagdonas <afaustas@gmail.com>
---
 include/globals.h  | 1 +
 include/swupdate.h | 1 +
 parser/parser.c    | 9 +++++++++
 3 files changed, 11 insertions(+)

Comments

Stefano Babic March 23, 2018, 1:21 p.m. UTC | #1 
Hi Faustas,

I do not see big issues to merge this (after some changes), but this has
also raised some descrepancy in current code that this patch will
enhance. More later...

On 22/03/2018 17:52, Faustas Azuolas Bagdonas wrote:
> These modifications adds a support of a description field in sw-description.
> This field is optional. Example of sw-description file beginning:
> 
> software =
> {
>         version = "0.1.0";
> 	description = "This update adds a new feature and includes important bug fixes." # <-- This is a new field added by this modification.

The part starting with "# <--" is not useful and makes the commit
message unusually long.

I am expecting that any new command in the parsed is then described in
the documentation: doc/source/sw-description.rst

> 
>         hardware-compatibility: [ "1.0", "1.2", "1.3"];
> 
>         <...>
> 
> Signed-off-by: Faustas Azuolas Bagdonas <afaustas@gmail.com>
> ---
>  include/globals.h  | 1 +
>  include/swupdate.h | 1 +
>  parser/parser.c    | 9 +++++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/include/globals.h b/include/globals.h
> index 9e0c2b9..25d2a2e 100644
> --- a/include/globals.h
> +++ b/include/globals.h
> @@ -23,6 +23,7 @@
>  #define BANNER "Swupdate v" SWU_VER "\n"
>  
>  #define SWUPDATE_GENERAL_STRING_SIZE	256
> +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE	1024

Special reasons for such a long string ?

>  #define MAX_IMAGE_FNAME	SWUPDATE_GENERAL_STRING_SIZE
>  #define MAX_URL		SWUPDATE_GENERAL_STRING_SIZE
>  #define MAX_VOLNAME	SWUPDATE_GENERAL_STRING_SIZE
> diff --git a/include/swupdate.h b/include/swupdate.h
> index 09e81d5..a766951 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -123,6 +123,7 @@ struct swupdate_global_cfg {
>  
>  struct swupdate_cfg {
>  	char name[SWUPDATE_GENERAL_STRING_SIZE];
> +	char description[SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE];
>  	char version[SWUPDATE_GENERAL_STRING_SIZE];
>  	char software_set[SWUPDATE_GENERAL_STRING_SIZE];
>  	char running_mode[SWUPDATE_GENERAL_STRING_SIZE];
> diff --git a/parser/parser.c b/parser/parser.c
> index c8a51e2..d305e95 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -623,6 +623,15 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
>  		strncpy(swcfg->version, str, sizeof(swcfg->version));
>  		fprintf(stdout, "Version %s\n", swcfg->version);
>  	}
> +
> +	snprintf(node, sizeof(node), "%s.description",
> +			NODEROOT);
> +

We can also use GET_FIELD_STRING()

> +	if (config_lookup_string(&cfg, node, &str)) {
> +		strncpy(swcfg->description, str, sizeof(swcfg->description));
> +		fprintf(stdout, "Description %s\n", swcfg->description);
> +	}
> +

I have noted that this is not symmetric. It is just added to parse_cfg,
and it is not parsed at all in parse_json. Rather, I test json just for
compatibility and I have noted this now with your patch - thanks for that !

In fact, version is parsed just in case of libconfig. Your added
"description" is also parsed just by parse_cfg, but both should be
parsed independently if the parser is "libconfig2 or "json".


>  	snprintf(node, sizeof(node), "%s.embedded-script",
>  			NODEROOT);
>  	if (config_lookup_string(&cfg, node, &str)) {
> 

Best regards,
Stefano
Faustas Azuolas Bagdonas March 27, 2018, 2:44 p.m. UTC | #2 
2018 m. kovas 23 d., penktadienis 15:21:38 UTC+2, Stefano Babic rašė:
> Hi Faustas,
> 
> I do not see big issues to merge this (after some changes), but this has
> also raised some descrepancy in current code that this patch will
> enhance. More later...
> 
> On 22/03/2018 17:52, Faustas Azuolas Bagdonas wrote:
> > These modifications adds a support of a description field in sw-description.
> > This field is optional. Example of sw-description file beginning:
> > 
> > software =
> > {
> >         version = "0.1.0";
> > 	description = "This update adds a new feature and includes important bug fixes." # <-- This is a new field added by this modification.
> 
> The part starting with "# <--" is not useful and makes the commit
> message unusually long.
> 
> I am expecting that any new command in the parsed is then described in
> the documentation: doc/source/sw-description.rst
> 
> > 
> >         hardware-compatibility: [ "1.0", "1.2", "1.3"];
> > 
> >         <...>
> > 
> > Signed-off-by: Faustas Azuolas Bagdonas <afaustas@gmail.com>
> > ---
> >  include/globals.h  | 1 +
> >  include/swupdate.h | 1 +
> >  parser/parser.c    | 9 +++++++++
> >  3 files changed, 11 insertions(+)
> > 
> > diff --git a/include/globals.h b/include/globals.h
> > index 9e0c2b9..25d2a2e 100644
> > --- a/include/globals.h
> > +++ b/include/globals.h
> > @@ -23,6 +23,7 @@
> >  #define BANNER "Swupdate v" SWU_VER "\n"
> >  
> >  #define SWUPDATE_GENERAL_STRING_SIZE	256
> > +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE	1024
> 
> Special reasons for such a long string ?
> 
> >  #define MAX_IMAGE_FNAME	SWUPDATE_GENERAL_STRING_SIZE
> >  #define MAX_URL		SWUPDATE_GENERAL_STRING_SIZE
> >  #define MAX_VOLNAME	SWUPDATE_GENERAL_STRING_SIZE
> > diff --git a/include/swupdate.h b/include/swupdate.h
> > index 09e81d5..a766951 100644
> > --- a/include/swupdate.h
> > +++ b/include/swupdate.h
> > @@ -123,6 +123,7 @@ struct swupdate_global_cfg {
> >  
> >  struct swupdate_cfg {
> >  	char name[SWUPDATE_GENERAL_STRING_SIZE];
> > +	char description[SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE];
> >  	char version[SWUPDATE_GENERAL_STRING_SIZE];
> >  	char software_set[SWUPDATE_GENERAL_STRING_SIZE];
> >  	char running_mode[SWUPDATE_GENERAL_STRING_SIZE];
> > diff --git a/parser/parser.c b/parser/parser.c
> > index c8a51e2..d305e95 100644
> > --- a/parser/parser.c
> > +++ b/parser/parser.c
> > @@ -623,6 +623,15 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
> >  		strncpy(swcfg->version, str, sizeof(swcfg->version));
> >  		fprintf(stdout, "Version %s\n", swcfg->version);
> >  	}
> > +
> > +	snprintf(node, sizeof(node), "%s.description",
> > +			NODEROOT);
> > +
> 
> We can also use GET_FIELD_STRING()
> 
> > +	if (config_lookup_string(&cfg, node, &str)) {
> > +		strncpy(swcfg->description, str, sizeof(swcfg->description));
> > +		fprintf(stdout, "Description %s\n", swcfg->description);
> > +	}
> > +
> 
> I have noted that this is not symmetric. It is just added to parse_cfg,
> and it is not parsed at all in parse_json. Rather, I test json just for
> compatibility and I have noted this now with your patch - thanks for that !
> 
> In fact, version is parsed just in case of libconfig. Your added
> "description" is also parsed just by parse_cfg, but both should be
> parsed independently if the parser is "libconfig2 or "json".
> 
> 
> >  	snprintf(node, sizeof(node), "%s.embedded-script",
> >  			NODEROOT);
> >  	if (config_lookup_string(&cfg, node, &str)) {
> > 
> 
> Best regards,
> Stefano
> 
> -- 
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

Hi Stefano,

thank you for the review. I modified my patch according your recommendations.

> +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE      1024

This string is longer because I think that sometimes update descriptions can be
quite long. But I agree that I made it too long so I decreased its
length to 512.

Now I added version checking and description field support to parse_json() as
well.


Best regards,
Faustas
Faustas Azuolas Bagdonas April 10, 2018, 8:37 a.m. UTC | #3 
Hi Stefano,

could you review my updated patch which I sent on "Tue, 27 Mar 2018 17:40:15 +0300", please?


Best regards,
Faustas


2018 m. kovas 27 d., antradienis 17:44:45 UTC+3, afau...@gmail.com rašė:
> 2018 m. kovas 23 d., penktadienis 15:21:38 UTC+2, Stefano Babic rašė:
> > Hi Faustas,
> > 
> > I do not see big issues to merge this (after some changes), but this has
> > also raised some descrepancy in current code that this patch will
> > enhance. More later...
> > 
> > On 22/03/2018 17:52, Faustas Azuolas Bagdonas wrote:
> > > These modifications adds a support of a description field in sw-description.
> > > This field is optional. Example of sw-description file beginning:
> > > 
> > > software =
> > > {
> > >         version = "0.1.0";
> > > 	description = "This update adds a new feature and includes important bug fixes." # <-- This is a new field added by this modification.
> > 
> > The part starting with "# <--" is not useful and makes the commit
> > message unusually long.
> > 
> > I am expecting that any new command in the parsed is then described in
> > the documentation: doc/source/sw-description.rst
> > 
> > > 
> > >         hardware-compatibility: [ "1.0", "1.2", "1.3"];
> > > 
> > >         <...>
> > > 
> > > Signed-off-by: Faustas Azuolas Bagdonas <afaustas@gmail.com>
> > > ---
> > >  include/globals.h  | 1 +
> > >  include/swupdate.h | 1 +
> > >  parser/parser.c    | 9 +++++++++
> > >  3 files changed, 11 insertions(+)
> > > 
> > > diff --git a/include/globals.h b/include/globals.h
> > > index 9e0c2b9..25d2a2e 100644
> > > --- a/include/globals.h
> > > +++ b/include/globals.h
> > > @@ -23,6 +23,7 @@
> > >  #define BANNER "Swupdate v" SWU_VER "\n"
> > >  
> > >  #define SWUPDATE_GENERAL_STRING_SIZE	256
> > > +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE	1024
> > 
> > Special reasons for such a long string ?
> > 
> > >  #define MAX_IMAGE_FNAME	SWUPDATE_GENERAL_STRING_SIZE
> > >  #define MAX_URL		SWUPDATE_GENERAL_STRING_SIZE
> > >  #define MAX_VOLNAME	SWUPDATE_GENERAL_STRING_SIZE
> > > diff --git a/include/swupdate.h b/include/swupdate.h
> > > index 09e81d5..a766951 100644
> > > --- a/include/swupdate.h
> > > +++ b/include/swupdate.h
> > > @@ -123,6 +123,7 @@ struct swupdate_global_cfg {
> > >  
> > >  struct swupdate_cfg {
> > >  	char name[SWUPDATE_GENERAL_STRING_SIZE];
> > > +	char description[SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE];
> > >  	char version[SWUPDATE_GENERAL_STRING_SIZE];
> > >  	char software_set[SWUPDATE_GENERAL_STRING_SIZE];
> > >  	char running_mode[SWUPDATE_GENERAL_STRING_SIZE];
> > > diff --git a/parser/parser.c b/parser/parser.c
> > > index c8a51e2..d305e95 100644
> > > --- a/parser/parser.c
> > > +++ b/parser/parser.c
> > > @@ -623,6 +623,15 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
> > >  		strncpy(swcfg->version, str, sizeof(swcfg->version));
> > >  		fprintf(stdout, "Version %s\n", swcfg->version);
> > >  	}
> > > +
> > > +	snprintf(node, sizeof(node), "%s.description",
> > > +			NODEROOT);
> > > +
> > 
> > We can also use GET_FIELD_STRING()
> > 
> > > +	if (config_lookup_string(&cfg, node, &str)) {
> > > +		strncpy(swcfg->description, str, sizeof(swcfg->description));
> > > +		fprintf(stdout, "Description %s\n", swcfg->description);
> > > +	}
> > > +
> > 
> > I have noted that this is not symmetric. It is just added to parse_cfg,
> > and it is not parsed at all in parse_json. Rather, I test json just for
> > compatibility and I have noted this now with your patch - thanks for that !
> > 
> > In fact, version is parsed just in case of libconfig. Your added
> > "description" is also parsed just by parse_cfg, but both should be
> > parsed independently if the parser is "libconfig2 or "json".
> > 
> > 
> > >  	snprintf(node, sizeof(node), "%s.embedded-script",
> > >  			NODEROOT);
> > >  	if (config_lookup_string(&cfg, node, &str)) {
> > > 
> > 
> > Best regards,
> > Stefano
> > 
> > -- 
> > =====================================================================
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> > =====================================================================
> 
> Hi Stefano,
> 
> thank you for the review. I modified my patch according your recommendations.
> 
> > +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE      1024
> 
> This string is longer because I think that sometimes update descriptions can be
> quite long. But I agree that I made it too long so I decreased its
> length to 512.
> 
> Now I added version checking and description field support to parse_json() as
> well.
> 
> 
> Best regards,
> Faustas
Stefano Babic April 11, 2018, 7:57 a.m. UTC | #4 
On 10/04/2018 10:37, afaustas@gmail.com wrote:
> Hi Stefano,
> 
> could you review my updated patch which I sent on "Tue, 27 Mar 2018 17:40:15 +0300", please?
> 

It will be done - patches are not lost, you can check the status at any
time on patchworks:

http://patchwork.ozlabs.org/project/swupdate/list/

If a patch is not yet listed, it is accepted or it is requested to be
changed. Until it is listed, it remembers me that I should still take
care of it.

Best regards,
Stefano Babic

> 
> Best regards,
> Faustas
> 
> 
> 2018 m. kovas 27 d., antradienis 17:44:45 UTC+3, afau...@gmail.com rašė:
>> 2018 m. kovas 23 d., penktadienis 15:21:38 UTC+2, Stefano Babic rašė:
>>> Hi Faustas,
>>>
>>> I do not see big issues to merge this (after some changes), but this has
>>> also raised some descrepancy in current code that this patch will
>>> enhance. More later...
>>>
>>> On 22/03/2018 17:52, Faustas Azuolas Bagdonas wrote:
>>>> These modifications adds a support of a description field in sw-description.
>>>> This field is optional. Example of sw-description file beginning:
>>>>
>>>> software =
>>>> {
>>>>         version = "0.1.0";
>>>> 	description = "This update adds a new feature and includes important bug fixes." # <-- This is a new field added by this modification.
>>>
>>> The part starting with "# <--" is not useful and makes the commit
>>> message unusually long.
>>>
>>> I am expecting that any new command in the parsed is then described in
>>> the documentation: doc/source/sw-description.rst
>>>
>>>>
>>>>         hardware-compatibility: [ "1.0", "1.2", "1.3"];
>>>>
>>>>         <...>
>>>>
>>>> Signed-off-by: Faustas Azuolas Bagdonas <afaustas@gmail.com>
>>>> ---
>>>>  include/globals.h  | 1 +
>>>>  include/swupdate.h | 1 +
>>>>  parser/parser.c    | 9 +++++++++
>>>>  3 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/include/globals.h b/include/globals.h
>>>> index 9e0c2b9..25d2a2e 100644
>>>> --- a/include/globals.h
>>>> +++ b/include/globals.h
>>>> @@ -23,6 +23,7 @@
>>>>  #define BANNER "Swupdate v" SWU_VER "\n"
>>>>  
>>>>  #define SWUPDATE_GENERAL_STRING_SIZE	256
>>>> +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE	1024
>>>
>>> Special reasons for such a long string ?
>>>
>>>>  #define MAX_IMAGE_FNAME	SWUPDATE_GENERAL_STRING_SIZE
>>>>  #define MAX_URL		SWUPDATE_GENERAL_STRING_SIZE
>>>>  #define MAX_VOLNAME	SWUPDATE_GENERAL_STRING_SIZE
>>>> diff --git a/include/swupdate.h b/include/swupdate.h
>>>> index 09e81d5..a766951 100644
>>>> --- a/include/swupdate.h
>>>> +++ b/include/swupdate.h
>>>> @@ -123,6 +123,7 @@ struct swupdate_global_cfg {
>>>>  
>>>>  struct swupdate_cfg {
>>>>  	char name[SWUPDATE_GENERAL_STRING_SIZE];
>>>> +	char description[SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE];
>>>>  	char version[SWUPDATE_GENERAL_STRING_SIZE];
>>>>  	char software_set[SWUPDATE_GENERAL_STRING_SIZE];
>>>>  	char running_mode[SWUPDATE_GENERAL_STRING_SIZE];
>>>> diff --git a/parser/parser.c b/parser/parser.c
>>>> index c8a51e2..d305e95 100644
>>>> --- a/parser/parser.c
>>>> +++ b/parser/parser.c
>>>> @@ -623,6 +623,15 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
>>>>  		strncpy(swcfg->version, str, sizeof(swcfg->version));
>>>>  		fprintf(stdout, "Version %s\n", swcfg->version);
>>>>  	}
>>>> +
>>>> +	snprintf(node, sizeof(node), "%s.description",
>>>> +			NODEROOT);
>>>> +
>>>
>>> We can also use GET_FIELD_STRING()
>>>
>>>> +	if (config_lookup_string(&cfg, node, &str)) {
>>>> +		strncpy(swcfg->description, str, sizeof(swcfg->description));
>>>> +		fprintf(stdout, "Description %s\n", swcfg->description);
>>>> +	}
>>>> +
>>>
>>> I have noted that this is not symmetric. It is just added to parse_cfg,
>>> and it is not parsed at all in parse_json. Rather, I test json just for
>>> compatibility and I have noted this now with your patch - thanks for that !
>>>
>>> In fact, version is parsed just in case of libconfig. Your added
>>> "description" is also parsed just by parse_cfg, but both should be
>>> parsed independently if the parser is "libconfig2 or "json".
>>>
>>>
>>>>  	snprintf(node, sizeof(node), "%s.embedded-script",
>>>>  			NODEROOT);
>>>>  	if (config_lookup_string(&cfg, node, &str)) {
>>>>
>>>
>>> Best regards,
>>> Stefano
>>>
>>> -- 
>>> =====================================================================
>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>>> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
>>> =====================================================================
>>
>> Hi Stefano,
>>
>> thank you for the review. I modified my patch according your recommendations.
>>
>>> +#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE      1024
>>
>> This string is longer because I think that sometimes update descriptions can be
>> quite long. But I agree that I made it too long so I decreased its
>> length to 512.
>>
>> Now I added version checking and description field support to parse_json() as
>> well.
>>
>>
>> Best regards,
>> Faustas
>
diff mbox series

Patch

diff --git a/include/globals.h b/include/globals.h
index 9e0c2b9..25d2a2e 100644
--- a/include/globals.h
+++ b/include/globals.h
@@ -23,6 +23,7 @@ 
 #define BANNER "Swupdate v" SWU_VER "\n"
 
 #define SWUPDATE_GENERAL_STRING_SIZE	256
+#define SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE	1024
 #define MAX_IMAGE_FNAME	SWUPDATE_GENERAL_STRING_SIZE
 #define MAX_URL		SWUPDATE_GENERAL_STRING_SIZE
 #define MAX_VOLNAME	SWUPDATE_GENERAL_STRING_SIZE
diff --git a/include/swupdate.h b/include/swupdate.h
index 09e81d5..a766951 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -123,6 +123,7 @@  struct swupdate_global_cfg {
 
 struct swupdate_cfg {
 	char name[SWUPDATE_GENERAL_STRING_SIZE];
+	char description[SWUPDATE_UPDATE_DESCRIPTION_STRING_SIZE];
 	char version[SWUPDATE_GENERAL_STRING_SIZE];
 	char software_set[SWUPDATE_GENERAL_STRING_SIZE];
 	char running_mode[SWUPDATE_GENERAL_STRING_SIZE];
diff --git a/parser/parser.c b/parser/parser.c
index c8a51e2..d305e95 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -623,6 +623,15 @@  int parse_cfg (struct swupdate_cfg *swcfg, const char *filename)
 		strncpy(swcfg->version, str, sizeof(swcfg->version));
 		fprintf(stdout, "Version %s\n", swcfg->version);
 	}
+
+	snprintf(node, sizeof(node), "%s.description",
+			NODEROOT);
+
+	if (config_lookup_string(&cfg, node, &str)) {
+		strncpy(swcfg->description, str, sizeof(swcfg->description));
+		fprintf(stdout, "Description %s\n", swcfg->description);
+	}
+
 	snprintf(node, sizeof(node), "%s.embedded-script",
 			NODEROOT);
 	if (config_lookup_string(&cfg, node, &str)) {