Re: [PATCH 1/3] util: Introduce virAcpi module

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

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);

>> +typedef enum { >> + VIR_IORT_NODE_TYPE_UNKNOWN = -1, > > Do we need this? virIORTNodeHeader.type is defined as unsigned.

>> +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.