sed: extended discovery feature code printing

[gjoyce-ibm]

Added ability to discover and print details of all specified level 0 features.

[igaw]

There are handful of printf format complains from the arm compiler:

 ../plugins/sed/sedopal_cmd.c:590:55: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘__uint64_t’ {aka ‘long long unsigned int’} [-Werror=format=]
  590 |         printf("\tAlignment Granularity           : %lx\n",
      |                                                     ~~^
      |                                                       |
      |                                                       long unsigned int
      |                                                     %llx

I also noticed the introduction of __packed for the data structures, e.g.

struct geometry_reporting_desc {
	uint8_t         align;
	uint8_t         reserved[7];
	uint32_t        logical_block_size;
	uint64_t        alignment_granularity;
	uint64_t        lowest_aligned_lba;
} __packed;

Generally, __packed should be avoided if possible. It tells the compiler to ignore all alignment information. For example when accessing a not aligned member would cause the compiler to issue a warning. Certain architecture are not happy with ill aligned accesses. Another thing __packed removes is the outer alignment of the data struct, which can lead to another set of problems.

__packed is the right choice for wire format data structs, which these definitions are. Though if the spec has paid attention to the alignment, it might be possible to drop the __packed.

Data type endianess: I think it would be good to use __be64 for the definitions instead of uint64_t etc. I didn't noticed this before, sorry.

[gjoyce-ibm]

There are handful of printf format complains from the arm compiler:

 ../plugins/sed/sedopal_cmd.c:590:55: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘__uint64_t’ {aka ‘long long unsigned int’} [-Werror=format=]
  590 |         printf("\tAlignment Granularity           : %lx\n",
      |                                                     ~~^
      |                                                       |
      |                                                       long unsigned int
      |                                                     %llx

I also noticed the introduction of __packed for the data structures, e.g.

struct geometry_reporting_desc {
	uint8_t         align;
	uint8_t         reserved[7];
	uint32_t        logical_block_size;
	uint64_t        alignment_granularity;
	uint64_t        lowest_aligned_lba;
} __packed;

Generally, __packed should be avoided if possible. It tells the compiler to ignore all alignment information. For example when accessing a not aligned member would cause the compiler to issue a warning. Certain architecture are not happy with ill aligned accesses. Another thing __packed removes is the outer alignment of the data struct, which can lead to another set of problems.

__packed is the right choice for wire format data structs, which these definitions are. Though if the spec has paid attention to the alignment, it might be possible to drop the __packed.

Data type endianess: I think it would be good to use __be64 for the definitions instead of uint64_t etc. I didn't noticed this before, sorry.

I think that I fixed the arm compiler warning but I updated the commit before I saw your response so the __packed is still there. I believe that one of the structures had a potential alignment issue so I used __packed for that. And since as you noted these are all wire format, I did use packed for all of them since they do need to be packed one way or the other.

I'll update the commit to address your __be64 comment.

[gjoyce-ibm]

I think maybe we're good now. @igaw

[igaw]

Thanks!