Proposal: List files in Xen originated from external sources

Proposal: List files in Xen originated from external sources

[Luca Fancellu]

Hi all,

I’m starting a proposal for the external files that needs to be removed from the MISRA scan,
the work was originally started by Michal here:
https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.orzel@amd.com/
so I started by that thread, the aim of this work is to have an initial format to start as soon as possible to
exclude the external files from the MISRA scan (at least initially from cppcheck).

I think we can use the JSON format because it’s easy to integrate it with python and it’s easy to add/remove
fields from the structure without interfering with the other elements during time, so we can define a structure
now but if in the future we see the needs for additional field, we can just add them without changes to the
analysis script.

In my opinion many of these fields can be left empty in a first push, to let the script exclude the files and during
time with the contributions of the community we can add the missing informations.
I think it’s easier for the community to pick an entry, do some research to fill the gaps and push, instead to wait
until having all the informations before adding the entry.


This is my first though for the fields, let me know yours:

docs/misra/external-files.json:
{
 "version": "1.0”,
 "content": [
   {
     "path": "relative/path/from/xen/“,
     "backport": True/False,
     "origin_project": "URL to origin project",
     "origin_path": "relative path in the original project",
     "origin_revision": "revision in original project”
   }
 ]
}


Maybe, documentation for this file and the fields can reside in docs/misra/external-files.rst.

Here follows the explanation for the fields:

path: is a relative path from the xen base folder, it can refer to a file or it can be a path to an entire folder
         (Taking as example libfdt)

backport: it’s a boolean flag that says if the file is subject to backport, so every file where this field is true
                won’t be included in the analysis. A file is subject to backport when it’s external and it doesn’t
                accept direct changes (changes needs to be made in the original project and then backported
                to Xen)

origin_project: url of the repository where this file was originated

origin_path: relative path in the original project, mostly linked to the original_revision field

origin_revision: revision of the changes in the repository when this file was taken.


Now, I’m not entirely sure about the field “backport”, because if a file is not subject to backport, then why
we should not make direct changes? Shall we maybe change its codestyle and convert it to Xen codestyle?
And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
the external file list so we don’t lose the history.
So we would end up excluding just all the file listed in external-file.json by the analysis.

What are your thoughts about that? Thank you


Cheers,
Luca

Re: Proposal: List files in Xen originated from external sources

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4448 bytes --]

On Fri, 3 Feb 2023, Luca Fancellu wrote:
> Hi all,
> 
> I’m starting a proposal for the external files that needs to be removed from the MISRA scan,
> the work was originally started by Michal here:
> https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.orzel@amd.com/
> so I started by that thread, the aim of this work is to have an initial format to start as soon as possible to
> exclude the external files from the MISRA scan (at least initially from cppcheck).
> 
> I think we can use the JSON format because it’s easy to integrate it with python and it’s easy to add/remove
> fields from the structure without interfering with the other elements during time, so we can define a structure
> now but if in the future we see the needs for additional field, we can just add them without changes to the
> analysis script.
> 
> In my opinion many of these fields can be left empty in a first push, to let the script exclude the files and during
> time with the contributions of the community we can add the missing informations.
> I think it’s easier for the community to pick an entry, do some research to fill the gaps and push, instead to wait
> until having all the informations before adding the entry.
> 
> 
> This is my first though for the fields, let me know yours:
> 
> docs/misra/external-files.json:
> {
>  "version": "1.0”,
>  "content": [
>    {
>      "path": "relative/path/from/xen/“,
>      "backport": True/False,
>      "origin_project": "URL to origin project",
>      "origin_path": "relative path in the original project",
>      "origin_revision": "revision in original project”
>    }
>  ]
> }
> 
> 
> Maybe, documentation for this file and the fields can reside in docs/misra/external-files.rst.
> 
> Here follows the explanation for the fields:
> 
> path: is a relative path from the xen base folder, it can refer to a file or it can be a path to an entire folder
>          (Taking as example libfdt)

I think "path" should be the only mandatory field. path alone should be
sufficient for a MISRA C scanner to know if a file or directory should
be excluded.


> backport: it’s a boolean flag that says if the file is subject to backport, so every file where this field is true
>                 won’t be included in the analysis. A file is subject to backport when it’s external and it doesn’t
>                 accept direct changes (changes needs to be made in the original project and then backported
>                 to Xen)
> 
> origin_project: url of the repository where this file was originated
> 
> origin_path: relative path in the original project, mostly linked to the original_revision field
> 
> origin_revision: revision of the changes in the repository when this file was taken.
 
These other fields (backport, origin_project, origin_path,
origin_revision) are nice to have but not required. They don't provide
information necessary to exclude something from MISRA C scans, however
they provide information that are useful to the Xen maintainers.

This is to say that we should introduce these fields only if they are
useful to us human maintainers, not automated tools.

That said, I think they are all useful and I would keep them as you did.


> Now, I’m not entirely sure about the field “backport”, because if a file is not subject to backport, then why
> we should not make direct changes? 

"backport" is meant to say that *only* backports are allowed, no direct
changes to the source file. E.g. only backports from Linux commits, so
first you need to get your patch in Linux, then we'll take it from
there.


> Shall we maybe change its codestyle and convert it to Xen codestyle?

Typically if you only accept backports to a file, it doesn't make any
sense to convert it to Xen codestyle. But it is not a hard rule and I
don't think we should say anything in that regard here.


> And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
> in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
> the external file list so we don’t lose the history.

Yes, I agree. If a file is coverted to Xen coding style and follows
MISRA rules, then there is no need to list the file in
docs/misra/external-files.json.


> So we would end up excluding just all the file listed in external-file.json by the analysis.

Right, I think so too

Re: Proposal: List files in Xen originated from external sources

[Jan Beulich]

On 03.02.2023 20:55, Stefano Stabellini wrote:
> On Fri, 3 Feb 2023, Luca Fancellu wrote:
>> And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
>> in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
>> the external file list so we don’t lose the history.
> 
> Yes, I agree. If a file is coverted to Xen coding style and follows
> MISRA rules, then there is no need to list the file in
> docs/misra/external-files.json.
> 
> 
>> So we would end up excluding just all the file listed in external-file.json by the analysis.
> 
> Right, I think so too

Personally I think this is too focused on Misra. There are other uses of
such data, like automatically identifying changes to the origin file(s)
which we ought to be pulling in (recall that we've been doing a pretty
bad job in this regard).

Jan

Re: Proposal: List files in Xen originated from external sources

[Jan Beulich]

On 03.02.2023 10:30, Luca Fancellu wrote:
> I’m starting a proposal for the external files that needs to be removed from the MISRA scan,
> the work was originally started by Michal here:
> https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.orzel@amd.com/
> so I started by that thread, the aim of this work is to have an initial format to start as soon as possible to
> exclude the external files from the MISRA scan (at least initially from cppcheck).
> 
> I think we can use the JSON format because it’s easy to integrate it with python and it’s easy to add/remove
> fields from the structure without interfering with the other elements during time, so we can define a structure
> now but if in the future we see the needs for additional field, we can just add them without changes to the
> analysis script.
> 
> In my opinion many of these fields can be left empty in a first push, to let the script exclude the files and during
> time with the contributions of the community we can add the missing informations.

I'm having trouble making a connection between a file having an entry here
and the need/desire to include it in scanning. Can you clarify please why
you see an implication from one to the other here?

> I think it’s easier for the community to pick an entry, do some research to fill the gaps and push, instead to wait
> until having all the informations before adding the entry.

Maybe it's easier, but it's then also less useful. For example I view
it as quite relevant what the origin of a file is. That may, for
example, have an implication on whether "backport" is sensible to set
to "true" (origin projects could, after all, be largely unmaintained,
and hence it may be difficult to get any changes into there).

> This is my first though for the fields, let me know yours:
> 
> docs/misra/external-files.json:
> {
>  "version": "1.0”,
>  "content": [
>    {
>      "path": "relative/path/from/xen/“,
>      "backport": True/False,
>      "origin_project": "URL to origin project",
>      "origin_path": "relative path in the original project",
>      "origin_revision": "revision in original project”
>    }
>  ]
> }
> 
> 
> Maybe, documentation for this file and the fields can reside in docs/misra/external-files.rst.

Couldn't documentation of the fields live at the top of the file itself?

> Here follows the explanation for the fields:
> 
> path: is a relative path from the xen base folder, it can refer to a file or it can be a path to an entire folder
>          (Taking as example libfdt)
> 
> backport: it’s a boolean flag that says if the file is subject to backport, so every file where this field is true
>                 won’t be included in the analysis. A file is subject to backport when it’s external and it doesn’t
>                 accept direct changes (changes needs to be made in the original project and then backported
>                 to Xen)
> 
> origin_project: url of the repository where this file was originated

Personally I'd drop "url of" - the specification of the origin needs to be
precise, but I'm not convinced it absolutely needs to come in the form of
a URL. What would imo be more important to mandate is that the reference
be to the "canonical" copy of the project, not e.g. some github clone.

> origin_path: relative path in the original project, mostly linked to the original_revision field
> 
> origin_revision: revision of the changes in the repository when this file was taken.

I guess this needs clarifying: What revision is meant here? The one originally
imported, the one last updated from, or yet something else? Keep in mind that
we may also be selectively importing changes, in which case any particular
"revision" can easily end up misleading. The format likely also wants
specifying, e.g. (like we do for Fixes: tags) the first 12 digits of a commit
hash. For said selective updating this might then allow for something like
<base-hash>+<cherry-pick>+...

Jan

Re: Proposal: List files in Xen originated from external sources

[Luca Fancellu]

Hi Jan,

Thank you for leaving your feedbacks, really appreciated.

> On 6 Feb 2023, at 10:01, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.02.2023 10:30, Luca Fancellu wrote:
>> I’m starting a proposal for the external files that needs to be removed from the MISRA scan,
>> the work was originally started by Michal here:
>> https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.orzel@amd.com/
>> so I started by that thread, the aim of this work is to have an initial format to start as soon as possible to
>> exclude the external files from the MISRA scan (at least initially from cppcheck).
>> 
>> I think we can use the JSON format because it’s easy to integrate it with python and it’s easy to add/remove
>> fields from the structure without interfering with the other elements during time, so we can define a structure
>> now but if in the future we see the needs for additional field, we can just add them without changes to the
>> analysis script.
>> 
>> In my opinion many of these fields can be left empty in a first push, to let the script exclude the files and during
>> time with the contributions of the community we can add the missing informations.
> 
> I'm having trouble making a connection between a file having an entry here
> and the need/desire to include it in scanning. Can you clarify please why
> you see an implication from one to the other here?

My understanding is that an external file should not be included in the report, because we can’t do direct changes
so there might be situations where the fix can’t be included in the original project and back ported afterwards because
the original project doesn’t apply the same coding standard, or code that needs to be justified simply can’t be justified
because the tag we use doesn’t make sense in the original project.
So we decided (?) to leave this burden to the final user that might just consult the list of external files to fix/justify what
is not compliant with the coding standard that needs to be followed.

> 
>> I think it’s easier for the community to pick an entry, do some research to fill the gaps and push, instead to wait
>> until having all the informations before adding the entry.
> 
> Maybe it's easier, but it's then also less useful. For example I view
> it as quite relevant what the origin of a file is. That may, for
> example, have an implication on whether "backport" is sensible to set
> to "true" (origin projects could, after all, be largely unmaintained,
> and hence it may be difficult to get any changes into there).

It’s a first step to unblock the code scanner to do properly its job, that is produce a report of findings we need to fix/justify
without the problem of having external files in the report (that can’t be touched).

So I get your point that a list of files without other data is not really useful, but it enables the community member to share
the burden of finding the history of it, currently we know which files are external (most of the times by experience or just by looking
into its codestyle), we list them and from time to time community members can pick one entry and do some archeology on it
to reconstruct its origin.
We might end up eventually to recognise that for a particular file we could just make direct modifications
because it can’t be anymore subject to back ports (origin diverged too much maybe, but I’m open to suggestions about this case).

My personal opinion is that we can’t give the burden of doing that from A to Z for every entry to the contributors, it will results on
not having anything at all, the community should spread the burden in these situations, it’s not like a feature that introduces issues
in Xen and the person should make it complete from the beginning, it’s documentation that can be produced during time and contributions
are welcome.

> 
>> This is my first though for the fields, let me know yours:
>> 
>> docs/misra/external-files.json:
>> {
>> "version": "1.0”,
>> "content": [
>>   {
>>     "path": "relative/path/from/xen/“,
>>     "backport": True/False,
>>     "origin_project": "URL to origin project",
>>     "origin_path": "relative path in the original project",
>>     "origin_revision": "revision in original project”
>>   }
>> ]
>> }
>> 
>> 
>> Maybe, documentation for this file and the fields can reside in docs/misra/external-files.rst.
> 
> Couldn't documentation of the fields live at the top of the file itself?

Unfortunately not, JSON is a bit strict about that.

> 
>> Here follows the explanation for the fields:
>> 
>> path: is a relative path from the xen base folder, it can refer to a file or it can be a path to an entire folder
>>         (Taking as example libfdt)
>> 
>> backport: it’s a boolean flag that says if the file is subject to backport, so every file where this field is true
>>                won’t be included in the analysis. A file is subject to backport when it’s external and it doesn’t
>>                accept direct changes (changes needs to be made in the original project and then backported
>>                to Xen)
>> 
>> origin_project: url of the repository where this file was originated
> 
> Personally I'd drop "url of" - the specification of the origin needs to be
> precise, but I'm not convinced it absolutely needs to come in the form of
> a URL. What would imo be more important to mandate is that the reference
> be to the "canonical" copy of the project, not e.g. some github clone.

Sure, could you explain it a bit more, maybe with an example?

> 
>> origin_path: relative path in the original project, mostly linked to the original_revision field
>> 
>> origin_revision: revision of the changes in the repository when this file was taken.
> 
> I guess this needs clarifying: What revision is meant here? The one originally
> imported, the one last updated from, or yet something else? Keep in mind that
> we may also be selectively importing changes, in which case any particular
> "revision" can easily end up misleading. The format likely also wants
> specifying, e.g. (like we do for Fixes: tags) the first 12 digits of a commit
> hash. For said selective updating this might then allow for something like
> <base-hash>+<cherry-pick>+...

This is a good point, what would it be the best format in your opinion?
Maybe we should have the field as an array of revisions?

"origin_revision": [
   "Revision of latest backport”,
   [...]
   "originally imported revision”,
]

What should be the best to capture the history of the file?


> 
> Jan

> On 6 Feb 2023, at 09:49, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.02.2023 20:55, Stefano Stabellini wrote:
>> On Fri, 3 Feb 2023, Luca Fancellu wrote:
>>> And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
>>> in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
>>> the external file list so we don’t lose the history.
>> 
>> Yes, I agree. If a file is coverted to Xen coding style and follows
>> MISRA rules, then there is no need to list the file in
>> docs/misra/external-files.json.
>> 
>> 
>>> So we would end up excluding just all the file listed in external-file.json by the analysis.
>> 
>> Right, I think so too
> 
> Personally I think this is too focused on Misra. There are other uses of
> such data, like automatically identifying changes to the origin file(s)
> which we ought to be pulling in (recall that we've been doing a pretty
> bad job in this regard).

Yes, we could find a place outside the /misra/ folder

> 
> Jan

Re: Proposal: List files in Xen originated from external sources

[Jan Beulich]

On 06.02.2023 13:21, Luca Fancellu wrote:
>> On 6 Feb 2023, at 10:01, Jan Beulich <jbeulich@suse.com> wrote:
>> On 03.02.2023 10:30, Luca Fancellu wrote:
>>> origin_path: relative path in the original project, mostly linked to the original_revision field
>>>
>>> origin_revision: revision of the changes in the repository when this file was taken.
>>
>> I guess this needs clarifying: What revision is meant here? The one originally
>> imported, the one last updated from, or yet something else? Keep in mind that
>> we may also be selectively importing changes, in which case any particular
>> "revision" can easily end up misleading. The format likely also wants
>> specifying, e.g. (like we do for Fixes: tags) the first 12 digits of a commit
>> hash. For said selective updating this might then allow for something like
>> <base-hash>+<cherry-pick>+...
> 
> This is a good point, what would it be the best format in your opinion?
> Maybe we should have the field as an array of revisions?
> 
> "origin_revision": [
>    "Revision of latest backport”,
>    [...]
>    "originally imported revision”,
> ]
> 
> What should be the best to capture the history of the file?

That would be taking the 2nd step before the 1st one. For now we need to
determine _what_ information we want to record. Then we can determine how
to best represent it.

Jan

Re: Proposal: List files in Xen originated from external sources

[Luca Fancellu]

+ adding Bertrand in the CC, apologies for forgetting you in the first mail

> On 6 Feb 2023, at 12:38, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 06.02.2023 13:21, Luca Fancellu wrote:
>>> On 6 Feb 2023, at 10:01, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 03.02.2023 10:30, Luca Fancellu wrote:
>>>> origin_path: relative path in the original project, mostly linked to the original_revision field
>>>> 
>>>> origin_revision: revision of the changes in the repository when this file was taken.
>>> 
>>> I guess this needs clarifying: What revision is meant here? The one originally
>>> imported, the one last updated from, or yet something else? Keep in mind that
>>> we may also be selectively importing changes, in which case any particular
>>> "revision" can easily end up misleading. The format likely also wants
>>> specifying, e.g. (like we do for Fixes: tags) the first 12 digits of a commit
>>> hash. For said selective updating this might then allow for something like
>>> <base-hash>+<cherry-pick>+...
>> 
>> This is a good point, what would it be the best format in your opinion?
>> Maybe we should have the field as an array of revisions?
>> 
>> "origin_revision": [
>>   "Revision of latest backport”,
>>   [...]
>>   "originally imported revision”,
>> ]
>> 
>> What should be the best to capture the history of the file?
> 
> That would be taking the 2nd step before the 1st one. For now we need to
> determine _what_ information we want to record. Then we can determine how
> to best represent it.

Yes you are right, what informations makes sense to track for a file, my first thought
is clearly the revision where the file was taken, at first I thought that updating the revision
to the latest one back ported would have been enough, but from your comment I see that
it's not so simple.



> 
> Jan

Re: Proposal: List files in Xen originated from external sources

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On Mon, 6 Feb 2023, Jan Beulich wrote:
> On 03.02.2023 20:55, Stefano Stabellini wrote:
> > On Fri, 3 Feb 2023, Luca Fancellu wrote:
> >> And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
> >> in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
> >> the external file list so we don’t lose the history.
> > 
> > Yes, I agree. If a file is coverted to Xen coding style and follows
> > MISRA rules, then there is no need to list the file in
> > docs/misra/external-files.json.
> > 
> > 
> >> So we would end up excluding just all the file listed in external-file.json by the analysis.
> > 
> > Right, I think so too
> 
> Personally I think this is too focused on Misra.

We are trying to do two things at once:
1) list of external files with their management info (backports, origin, etc.)
2) list of files and paths to exclude from MISRA checking

From this discussion it became clear that 1) and 2) are very different,
have different requirements, and different info attached. In fact, the
two lists of files and paths don't even match: not all external files
exclude MISRA checking and some non-external files also exclude MISRA
checking.

I suggest we go forward with a MISRA-only exclude list with files and
paths to exclude from automatic checking, and without any implication
about external files. The list could be a json file called
"misra-exclude.json" to avoid any doubts.

Re: Proposal: List files in Xen originated from external sources

[Jan Beulich]

On 06.02.2023 22:23, Stefano Stabellini wrote:
> On Mon, 6 Feb 2023, Jan Beulich wrote:
>> On 03.02.2023 20:55, Stefano Stabellini wrote:
>>> On Fri, 3 Feb 2023, Luca Fancellu wrote:
>>>> And then, if the file is not subject to backport and now we “own” the file, there is no more the need to list it
>>>> in the external file, a commit search can reveal when it was converted to Xen codestyle and removed from
>>>> the external file list so we don’t lose the history.
>>>
>>> Yes, I agree. If a file is coverted to Xen coding style and follows
>>> MISRA rules, then there is no need to list the file in
>>> docs/misra/external-files.json.
>>>
>>>
>>>> So we would end up excluding just all the file listed in external-file.json by the analysis.
>>>
>>> Right, I think so too
>>
>> Personally I think this is too focused on Misra.
> 
> We are trying to do two things at once:
> 1) list of external files with their management info (backports, origin, etc.)
> 2) list of files and paths to exclude from MISRA checking
> 
> From this discussion it became clear that 1) and 2) are very different,
> have different requirements, and different info attached. In fact, the
> two lists of files and paths don't even match: not all external files
> exclude MISRA checking and some non-external files also exclude MISRA
> checking.
> 
> I suggest we go forward with a MISRA-only exclude list with files and
> paths to exclude from automatic checking, and without any implication
> about external files. The list could be a json file called
> "misra-exclude.json" to avoid any doubts.

That would be fine with me. Such a file could then include a justification
for each exclusion right away.

Jan