| Message ID | 20210112132054.49756-10-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add SIE library and simple tests | expand |
On 12/01/2021 14.20, Janosch Frank wrote: > Let's make it clear that there is something at the end of the > struct. The exact offset is reported by the cpu_offset member. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > lib/s390x/sclp.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h > index dccbaa8..395895f 100644 > --- a/lib/s390x/sclp.h > +++ b/lib/s390x/sclp.h > @@ -134,7 +134,10 @@ typedef struct ReadInfo { > uint8_t reserved7[134 - 128]; > uint8_t byte_134_diag318 : 1; > uint8_t : 7; > - struct CPUEntry entries[0]; > + /* > + * The cpu entries follow, they start at the offset specified > + * in offset_cpu. > + */ > } __attribute__((packed)) ReadInfo; > > typedef struct ReadCpuInfo { > Reviewed-by: Thomas Huth <thuth@redhat.com>
On 12.01.21 14:20, Janosch Frank wrote: > Let's make it clear that there is something at the end of the > struct. The exact offset is reported by the cpu_offset member. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > lib/s390x/sclp.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h > index dccbaa8..395895f 100644 > --- a/lib/s390x/sclp.h > +++ b/lib/s390x/sclp.h > @@ -134,7 +134,10 @@ typedef struct ReadInfo { > uint8_t reserved7[134 - 128]; > uint8_t byte_134_diag318 : 1; > uint8_t : 7; > - struct CPUEntry entries[0]; > + /* > + * The cpu entries follow, they start at the offset specified > + * in offset_cpu. > + */ I mean, that's just best practice. At least when I spot "[0];" and the end of a struct, I know what's happening. No strong opinion about the comment, I wouldn't need it to understand it. > } __attribute__((packed)) ReadInfo; > > typedef struct ReadCpuInfo { >
On 1/13/21 11:25 AM, David Hildenbrand wrote: > On 12.01.21 14:20, Janosch Frank wrote: >> Let's make it clear that there is something at the end of the >> struct. The exact offset is reported by the cpu_offset member. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> lib/s390x/sclp.h | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h >> index dccbaa8..395895f 100644 >> --- a/lib/s390x/sclp.h >> +++ b/lib/s390x/sclp.h >> @@ -134,7 +134,10 @@ typedef struct ReadInfo { >> uint8_t reserved7[134 - 128]; >> uint8_t byte_134_diag318 : 1; >> uint8_t : 7; >> - struct CPUEntry entries[0]; >> + /* >> + * The cpu entries follow, they start at the offset specified >> + * in offset_cpu. >> + */ > > I mean, that's just best practice. At least when I spot "[0];" and the > end of a struct, I know what's happening. > > No strong opinion about the comment, I wouldn't need it to understand it. The comment was a wish by Thomas. Anyway, it doesn't hurt :) > >> } __attribute__((packed)) ReadInfo; >> >> typedef struct ReadCpuInfo { >> > >
On Wed, 13 Jan 2021 11:25:45 +0100 David Hildenbrand <david@redhat.com> wrote: > On 12.01.21 14:20, Janosch Frank wrote: > > Let's make it clear that there is something at the end of the > > struct. The exact offset is reported by the cpu_offset member. > > > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > --- > > lib/s390x/sclp.h | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h > > index dccbaa8..395895f 100644 > > --- a/lib/s390x/sclp.h > > +++ b/lib/s390x/sclp.h > > @@ -134,7 +134,10 @@ typedef struct ReadInfo { > > uint8_t reserved7[134 - 128]; > > uint8_t byte_134_diag318 : 1; > > uint8_t : 7; > > - struct CPUEntry entries[0]; > > + /* > > + * The cpu entries follow, they start at the offset specified > > + * in offset_cpu. > > + */ > > I mean, that's just best practice. At least when I spot "[0];" and the > end of a struct, I know what's happening. Agreed. > > No strong opinion about the comment, I wouldn't need it to understand it. I'd keep it as-is; maybe add a comment where offset_cpu points to? > > > } __attribute__((packed)) ReadInfo; > > > > typedef struct ReadCpuInfo { > > > >
On 13/01/2021 11.25, David Hildenbrand wrote: > On 12.01.21 14:20, Janosch Frank wrote: >> Let's make it clear that there is something at the end of the >> struct. The exact offset is reported by the cpu_offset member. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> lib/s390x/sclp.h | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h >> index dccbaa8..395895f 100644 >> --- a/lib/s390x/sclp.h >> +++ b/lib/s390x/sclp.h >> @@ -134,7 +134,10 @@ typedef struct ReadInfo { >> uint8_t reserved7[134 - 128]; >> uint8_t byte_134_diag318 : 1; >> uint8_t : 7; >> - struct CPUEntry entries[0]; >> + /* >> + * The cpu entries follow, they start at the offset specified >> + * in offset_cpu. >> + */ > > I mean, that's just best practice. At least when I spot "[0];" and the > end of a struct, I know what's happening. > > No strong opinion about the comment, I wouldn't need it to understand it. The problem is that the "CPUEntry" entries either might be at this location, or later, so the entries[0] can be misleading. You always have to go via the offset_cpu field to find the right location. Thomas
On Tue, 12 Jan 2021 08:20:54 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Let's make it clear that there is something at the end of the > struct. The exact offset is reported by the cpu_offset member. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > lib/s390x/sclp.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h > index dccbaa8..395895f 100644 > --- a/lib/s390x/sclp.h > +++ b/lib/s390x/sclp.h > @@ -134,7 +134,10 @@ typedef struct ReadInfo { > uint8_t reserved7[134 - 128]; > uint8_t byte_134_diag318 : 1; > uint8_t : 7; > - struct CPUEntry entries[0]; > + /* > + * The cpu entries follow, they start at the offset specified > + * in offset_cpu. > + */ > } __attribute__((packed)) ReadInfo; > > typedef struct ReadCpuInfo { Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h index dccbaa8..395895f 100644 --- a/lib/s390x/sclp.h +++ b/lib/s390x/sclp.h @@ -134,7 +134,10 @@ typedef struct ReadInfo { uint8_t reserved7[134 - 128]; uint8_t byte_134_diag318 : 1; uint8_t : 7; - struct CPUEntry entries[0]; + /* + * The cpu entries follow, they start at the offset specified + * in offset_cpu. + */ } __attribute__((packed)) ReadInfo; typedef struct ReadCpuInfo {
Let's make it clear that there is something at the end of the struct. The exact offset is reported by the cpu_offset member. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- lib/s390x/sclp.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)