On 4/6/23 11:20, Andrea Bolognani wrote:
> On Thu, Apr 06, 2023 at 10:20:46AM +0200, Michal Prívozník wrote:
>> On 4/5/23 19:21, Andrea Bolognani wrote:
>>> On Wed, Apr 05, 2023 at 01:30:17PM +0200, Michal Privoznik wrote:
>>>> + if (nodeHeader->len < sizeof(*nodeHeader)) {
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("IORT table node type %1$s has invalid length: %2$" PRIu16),
>>>
>>> I'm not sure this plays well with the recently introduced changes to
>>> translatable strings. Have you checked with Jirka?
>>
>> I haven't. But, gcc complains if only a part of string contains
>> positioned arguments. That is:
>>
>> "argument value %1$s and another argument %s"
>> "argument value %s and another argument %2$s"
>>
>> are invalid format string and compiler throws an error. Now, using no
>> positions works:
>>
>> "argument value %s and another argument %s"
>>
>> but then, Jirka's syntax-check rule throws an error. But it doesn't for
>> the case I'm using:
>>
>> "integer value %" PRIu16
>
> Yeah, either your usage is fine or the syntax-check rule should be
> improve to catch it. Jirka?
>
>> Although, one could argue that if the translate tool doesn't allow fixed
>> size integer types modifiers, then the tool is broken. Surely, libvirt's
>> not the only one using fixed size integers. But okay, for error messages
>> I can typecast arguments. IOW:
>>
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("IORT table node type %1$s has invalid length: %2$u"),
>> NULLSTR(typeStr), (unsigned int)nodeHeader->len);
>
> This looks fine.
>
> Perhaps a bit more useful:
>
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("IORT table node type %1$s has invalid length: got
> %2$u, expected at least %3$lu"),
> NULLSTR(typeStr), (unsigned int)nodeHeader->len,
> sizeof(*nodeHeader));
>
>>>> +typedef enum {
>>>> + VIR_IORT_NODE_TYPE_UNKNOWN = -1,
>>>
>>> Do we need this? virIORTNodeHeader.type is defined as unsigned.
>
> You didn't answer this one :)
That's because I removed it in v2 ;-)
>
>>>> +typedef struct virIORTNodeHeader virIORTNodeHeader;
>>>> +struct virIORTNodeHeader {
>>>> + uint8_t type; /* One of virIORTNodeType */
>>>> + uint16_t len;
>>>> + uint8_t revision;
>>>> + uint32_t identifier;
>>>> + uint32_t nmappings;
>>>> + uint32_t reference_id;
>>>
>>> Since we only care about type and length, we could simply not declare
>>> the other four fields at all, right?
>>
>> Yes, except these fields are defined by the standard. We may need them
>> in the future. Anyway - these are thrown right away anyway, if it's
>> added memory consumption you're worried about.
>> By the same token, we don't care about other types (root comples, PMCG,
>> ITS group, ...) and yet we have them in the enum.
>
> It's fine to keep them, I'm not too concerned about the memory usage,
> I was just wondering how much we should push towards minimalism ;)
>
> What about renaming @reference_id to @mappings_offset though, to
> match the corresponding fields for nodes in virIORTHeader?
>
I can do that, but then again - it's named 'Reference to ID Array' in
the spec. And since I renamed SMMU_V1_2 to match whatever was in the
spec I figured staying close to the standard is key. Or how do we decide
when to stay true to the standard an when not?
If anything, it should be named reference_to_id_array. But I believe
that anybody who will extend this implementation will do the same as I
did - open the code and the spec next to each other and try to find a
match between tables in spec and structs in our code. At that point,
whether this field is named 'reference_id', 'mappings_offset' or
'reference_to_id_array' doesn't really matter.
Michal
