[PATCH 1/3] util: Introduce virAcpi module

[Andrea Bolognani]

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

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

-- 
Andrea Bolognani / Red Hat / Virtualization