Improve ACPI device path formatting

This factors a bunch of the duplication out to another function, which
also does a better job of it.

Signed-off-by: Peter Jones <pjones@redhat.com>

diff --git a/src/dp-acpi.c b/src/dp-acpi.c
index 70162f3..a49ef38 100644 (file)
--- a/src/dp-acpi.c
+++ b/src/dp-acpi.c
@@ -44,6 +44,59 @@ _format_acpi_adr(char *buf, size_t size,
 #define format_acpi_adr(buf, size, off, dp)                            \
        format_helper(_format_acpi_adr, buf, size, off, "AcpiAdr", dp)
 
+static ssize_t
+_format_acpi_hid_ex(char *buf, size_t size, const char *dp_type UNUSED,
+                   const_efidp dp,
+                   const char *hidstr, const char *cidstr, const char *uidstr)
+{
+       ssize_t off = 0;
+
+       debug(DEBUG, "hid:0x%08x hidstr:\"%s\"", dp->acpi_hid_ex.hid, hidstr);
+       debug(DEBUG, "cid:0x%08x cidstr:\"%s\"", dp->acpi_hid_ex.cid, cidstr);
+       debug(DEBUG, "uid:0x%08x uidstr:\"%s\"", dp->acpi_hid_ex.uid, uidstr);
+
+       if (!hidstr && !cidstr && (uidstr || dp->acpi_hid_ex.uid)) {
+               format(buf, size, off, "AcpiExp",
+                      "AcpiExp(0x%"PRIx32",0x%"PRIx32",",
+                      dp->acpi_hid_ex.hid, dp->acpi_hid_ex.cid);
+               if (uidstr) {
+                       format(buf, size, off, "AcpiExp", "%s)", uidstr);
+               } else {
+                       format(buf, size, off, "AcpiExp", "0x%"PRIx32")",
+                              dp->acpi_hid_ex.uid);
+               }
+               return off;
+       }
+
+       format(buf, size, off, "AcpiEx", "AcpiEx(");
+       if (hidstr) {
+               format(buf, size, off, "AcpiEx", "%s,", hidstr);
+       } else {
+               format(buf, size, off, "AcpiEx", "0x%"PRIx32",",
+                      dp->acpi_hid_ex.hid);
+       }
+
+       if (cidstr) {
+               format(buf, size, off, "AcpiEx", "%s,", cidstr);
+       } else {
+               format(buf, size, off, "AcpiEx", "0x%"PRIx32",",
+                      dp->acpi_hid_ex.cid);
+       }
+
+       if (uidstr) {
+               format(buf, size, off, "AcpiEx", "%s)", uidstr);
+       } else {
+               format(buf, size, off, "AcpiEx", "0x%"PRIx32")",
+                      dp->acpi_hid_ex.uid);
+       }
+
+       return off;
+}
+
+#define format_acpi_hid_ex(buf, size, off, dp, hidstr, cidstr, uidstr)  \
+       format_helper(_format_acpi_hid_ex, buf, size, off, "AcpiEx", dp,\
+                     hidstr, cidstr, uidstr)
+
 ssize_t
 _format_acpi_dn(char *buf, size_t size, const_efidp dp)
 {
@@ -53,13 +106,15 @@ _format_acpi_dn(char *buf, size_t size, const_efidp dp)
        const char *uidstr = NULL;
        size_t uidlen = 0;
        const char *cidstr = NULL;
-       size_t cidlen = 0;
+       // size_t cidlen = 0;
 
        if (dp->subtype == EFIDP_ACPI_ADR) {
+               debug(DEBUG, "formatting ACPI _ADR");
                format_acpi_adr(buf, size, off, dp);
                return off;
        } else if (dp->subtype != EFIDP_ACPI_HID_EX &&
                   dp->subtype != EFIDP_ACPI_HID) {
+               debug(DEBUG, "DP subtype %d, formatting as ACPI Path", dp->subtype);
                format(buf, size, off, "AcpiPath", "AcpiPath(%d,", dp->subtype);
                format_hex(buf, size, off, "AcpiPath", (uint8_t *)dp+4,
                           (efidp_node_size(dp)-4) / 2);
@@ -69,6 +124,7 @@ _format_acpi_dn(char *buf, size_t size, const_efidp dp)
                ssize_t limit = efidp_node_size(dp)
                                - offsetof(efidp_acpi_hid_ex, hidstr);
 
+               debug(DEBUG, "formatting ACPI HID EX");
                hidstr = dp->acpi_hid_ex.hidstr;
                hidlen = strnlen(hidstr, limit);
                limit -= hidlen + 1;
@@ -81,7 +137,7 @@ _format_acpi_dn(char *buf, size_t size, const_efidp dp)
 
                if (limit) {
                        cidstr = uidstr + uidlen + 1;
-                       cidlen = strnlen(cidstr, limit);
+                       // cidlen = strnlen(cidstr, limit);
                        // limit -= cidlen + 1;
                }
 
@@ -96,143 +152,102 @@ _format_acpi_dn(char *buf, size_t size, const_efidp dp)
                                       "PcieRoot(%s)", uidstr);
                                return off;
                        default:
-                               format(buf, size, off, "AcpiEx", "AcpiEx(");
-                               if (hidlen)
-                                       format(buf, size, off, "AcpiEx", "%s",
-                                                       hidstr);
-                               else
-                                       format(buf, size, off, "AcpiEx", "0x%"PRIx32,
-                                                       dp->acpi_hid_ex.hid);
-                               if (cidlen)
-                                       format(buf, size, off, "AcpiEx", ",%s",
-                                                       cidstr);
-                               else
-                                       format(buf, size, off, "AcpiEx", ",0x%"PRIx32,
-                                                       dp->acpi_hid_ex.cid);
-                               if (uidlen)
-                                       format(buf, size, off, "AcpiEx", ",%s",
-                                                       uidstr);
-                               else
-                                       format(buf, size, off, "AcpiEx", ",0x%"PRIx32,
-                                                       dp->acpi_hid_ex.uid);
-                               format(buf, size, off, "AcpiEx", ")");
-                               break;
+                               format_acpi_hid_ex(buf, size, off, dp,
+                                                  hidstr, cidstr, uidstr);
+                               return off;
                        }
                }
-       }
-
-       switch (dp->acpi_hid.hid) {
-       case EFIDP_ACPI_PCI_ROOT_HID:
-               format(buf, size, off, "PciRoot", "PciRoot(0x%"PRIx32")",
-                      dp->acpi_hid.uid);
-               break;
-       case EFIDP_ACPI_PCIE_ROOT_HID:
-               format(buf, size, off, "PcieRoot", "PcieRoot(0x%"PRIx32")",
-                      dp->acpi_hid.uid);
-               break;
-       case EFIDP_ACPI_FLOPPY_HID:
-               format(buf, size, off, "Floppy", "Floppy(0x%"PRIx32")",
-                      dp->acpi_hid.uid);
-               break;
-       case EFIDP_ACPI_KEYBOARD_HID:
-               format(buf, size, off, "Keyboard", "Keyboard(0x%"PRIx32")",
-                      dp->acpi_hid.uid);
-               break;
-       case EFIDP_ACPI_SERIAL_HID:
-               format(buf, size, off, "Keyboard", "Serial(0x%"PRIx32")",
-                      dp->acpi_hid.uid);
-               break;
-       case EFIDP_ACPI_NVDIMM_HID: {
-               int rc;
-               const_efidp next = NULL;
-               efidp_acpi_adr *adrdp;
-               int end;
-
-               format(buf, size, off, "NvRoot()", "NvRoot()");
-
-               rc = efidp_next_node(dp, &next);
-               if (rc < 0 || !next) {
-                       efi_error("could not format DP");
-                       return rc;
-               }
+       } else if (dp->subtype == EFIDP_ACPI_HID_EX) {
+               switch (dp->acpi_hid.hid) {
+               case EFIDP_ACPI_PCI_ROOT_HID:
+                       format(buf, size, off, "PciRoot",
+                              "PciRoot(0x%"PRIx32")",
+                              dp->acpi_hid.uid);
+                       break;
+               case EFIDP_ACPI_PCIE_ROOT_HID:
+                       format(buf, size, off, "PcieRoot",
+                              "PcieRoot(0x%"PRIx32")",
+                              dp->acpi_hid.uid);
+                       break;
+               case EFIDP_ACPI_FLOPPY_HID:
+                       format(buf, size, off, "Floppy",
+                              "Floppy(0x%"PRIx32")",
+                              dp->acpi_hid.uid);
+                       break;
+               case EFIDP_ACPI_KEYBOARD_HID:
+                       format(buf, size, off, "Keyboard",
+                              "Keyboard(0x%"PRIx32")",
+                              dp->acpi_hid.uid);
+                       break;
+               case EFIDP_ACPI_SERIAL_HID:
+                       format(buf, size, off, "Serial",
+                              "Serial(0x%"PRIx32")",
+                              dp->acpi_hid.uid);
+                       break;
+               case EFIDP_ACPI_NVDIMM_HID: {
+                       int rc;
+                       const_efidp next = NULL;
+                       efidp_acpi_adr *adrdp;
+                       int end;
 
-               if (efidp_type(next) != EFIDP_ACPI_TYPE ||
-                   efidp_subtype(next) != EFIDP_ACPI_ADR) {
-                       efi_error("Invalid child node type (0x%02x,0x%02x)",
-                                 efidp_type(next), efidp_subtype(next));
-                       return -EINVAL;
-               }
-               adrdp = (efidp_acpi_adr *)next;
+                       format(buf, size, off, "NvRoot()", "NvRoot()");
 
-               end = efidp_size_after(adrdp, header)
-                       / sizeof(adrdp->adr[0]);
+                       rc = efidp_next_node(dp, &next);
+                       if (rc < 0 || !next) {
+                               efi_error("could not format DP");
+                               return rc;
+                       }
 
-               for (int i = 0; i < end; i++) {
-                       uint32_t node_controller, socket, memory_controller;
-                       uint32_t memory_channel, dimm;
-                       uint32_t adr = adrdp->adr[i];
+                       if (efidp_type(next) != EFIDP_ACPI_TYPE ||
+                           efidp_subtype(next) != EFIDP_ACPI_ADR) {
+                               efi_error("Invalid child node type (0x%02x,0x%02x)",
+                                         efidp_type(next), efidp_subtype(next));
+                               return -EINVAL;
+                       }
+                       adrdp = (efidp_acpi_adr *)next;
 
-                       efidp_decode_acpi_nvdimm_adr(adr, &node_controller,
-                                                    &socket,
-                                                    &memory_controller,
-                                                    &memory_channel, &dimm);
+                       end = efidp_size_after(adrdp, header)
+                               / sizeof(adrdp->adr[0]);
 
-                       if (i != 0)
-                               format(buf, size, off, "NvDimm", ",");
+                       for (int i = 0; i < end; i++) {
+                               uint32_t node_controller, socket, memory_controller;
+                               uint32_t memory_channel, dimm;
+                               uint32_t adr = adrdp->adr[i];
 
-                       format(buf, size, off, "NvDimm",
-                              "NvDimm(0x%03x,0x%01x,0x%01x,0x%01x,0x%01x)",
-                              node_controller, socket, memory_controller,
-                              memory_channel, dimm);
-               }
-               break;
-                                   }
-       default:
-               switch (dp->subtype) {
-               case EFIDP_ACPI_HID_EX:
-                       if (!hidstr && !cidstr &&
-                                       (uidstr || dp->acpi_hid_ex.uid)){
-                               format(buf, size, off, "AcpiExp",
-                                      "AcpiExp(0x%"PRIx32",0x%"PRIx32",",
-                                      dp->acpi_hid_ex.hid,
-                                      dp->acpi_hid_ex.cid);
-                               if (uidstr) {
-                                       format(buf, size, off, "AcpiExp",
-                                              "%s)", uidstr);
-                               } else {
-                                       format(buf, size, off, "AcpiExp",
-                                              "0x%"PRIx32")",
-                                              dp->acpi_hid.uid);
-                               }
-                               break;
-                       }
-                       format(buf, size, off, "AcpiEx", "AcpiEx(");
-                       if (hidstr) {
-                               format(buf, size, off, "AcpiEx", "%s,", hidstr);
-                       } else {
-                               format(buf, size, off, "AcpiEx", "0x%"PRIx32",",
-                                             dp->acpi_hid.hid);
-                       }
+                               efidp_decode_acpi_nvdimm_adr(adr,
+                                       &node_controller, &socket,
+                                       &memory_controller, &memory_channel,
+                                       &dimm);
 
-                       if (cidstr) {
-                               format(buf, size, off, "AcpiEx", "%s,", cidstr);
-                       } else {
-                               format(buf, size, off, "AcpiEx", "0x%"PRIx32",",
-                                      dp->acpi_hid_ex.cid);
-                       }
+                               if (i != 0)
+                                       format(buf, size, off, "NvDimm", ",");
 
-                       if (uidstr) {
-                               format(buf, size, off, "AcpiEx", "%s)", uidstr);
-                       } else {
-                               format(buf, size, off, "AcpiEx", "0x%"PRIx32")",
-                                      dp->acpi_hid.uid);
+                               format(buf, size, off, "NvDimm",
+                                      "NvDimm(0x%03x,0x%01x,0x%01x,0x%01x,0x%01x)",
+                                      node_controller, socket, memory_controller,
+                                      memory_channel, dimm);
                        }
                        break;
-               case EFIDP_ACPI_HID:
-                       format(buf, size, off, "Acpi",
-                              "Acpi(0x%"PRIx32",0x%"PRIx32")",
-                              dp->acpi_hid.hid, dp->acpi_hid.uid);
-                       break;
+                                           }
+               default:
+                       debug(DEBUG, "Decoding non-well-known HID");
+                       switch (dp->subtype) {
+                       case EFIDP_ACPI_HID_EX:
+                               format_acpi_hid_ex(buf, size, off, dp,
+                                                  hidstr, cidstr, uidstr);
+                               break;
+                       case EFIDP_ACPI_HID:
+                               debug(DEBUG, "Decoding ACPI HID");
+                               format(buf, size, off, "Acpi",
+                                      "Acpi(0x%08x,0x%"PRIx32")",
+                                      dp->acpi_hid.hid, dp->acpi_hid.uid);
+                               break;
+                       default:
+                               debug(DEBUG, "ACPI subtype %d???",
+                                     dp->subtype);
+                               errno = EINVAL;
+                               return -1;
+                       }
                }
        }
 
@@ -259,7 +274,7 @@ efidp_make_acpi_hid(uint8_t *buf, ssize_t size, uint32_t hid, uint32_t uid)
        return sz;
 }
 
-ssize_t PUBLIC NONNULL(6, 7, 8)
+ssize_t PUBLIC
 efidp_make_acpi_hid_ex(uint8_t *buf, ssize_t size,
                       uint32_t hid, uint32_t uid, uint32_t cid,
                       const char *hidstr, const char *uidstr,
@@ -268,21 +283,26 @@ efidp_make_acpi_hid_ex(uint8_t *buf, ssize_t size,
        efidp_acpi_hid_ex *acpi_hid = (efidp_acpi_hid_ex *)buf;
        ssize_t req;
        ssize_t sz;
+       size_t hidlen = hidstr ? strlen(hidstr) : 0;
+       size_t uidlen = uidstr ? strlen(uidstr) : 0;
+       size_t cidlen = cidstr ? strlen(cidstr) : 0;
 
-       req = sizeof (*acpi_hid) + 3 +
-               strlen(hidstr) + strlen(uidstr) + strlen(cidstr);
+       req = sizeof (*acpi_hid) + 3 + hidlen + uidlen + cidlen;
        sz = efidp_make_generic(buf, size, EFIDP_ACPI_TYPE, EFIDP_ACPI_HID_EX,
                                req);
        if (size && sz == req) {
-               acpi_hid->uid = uid;
-               acpi_hid->hid = hid;
-               acpi_hid->cid = cid;
+               acpi_hid->hid = hidlen ? 0 : hid;
+               acpi_hid->uid = uidlen ? 0 : uid;
+               acpi_hid->cid = cidlen ? 0 : cid;
                char *next = (char *)buf+offsetof(efidp_acpi_hid_ex, hidstr);
-               strcpy(next, hidstr);
-               next += strlen(hidstr) + 1;
-               strcpy(next, uidstr);
-               next += strlen(uidstr) + 1;
-               strcpy(next, cidstr);
+               if (hidlen)
+                       strcpy(next, hidstr);
+               next += hidlen + 1;
+               if (uidlen)
+                       strcpy(next, uidstr);
+               next += uidlen + 1;
+               if (cidlen)
+                       strcpy(next, cidstr);
        }
 
        if (sz < 0)

diff --git a/src/include/efivar/efivar-dp.h b/src/include/efivar/efivar-dp.h
index 106d364..1b05775 100644 (file)
--- a/src/include/efivar/efivar-dp.h
+++ b/src/include/efivar/efivar-dp.h
@@ -133,7 +133,7 @@ typedef struct {
        /* three ascii string fields follow */
        char            hidstr[];
 } EFIVAR_PACKED efidp_acpi_hid_ex;
-extern ssize_t __attribute__((__nonnull__ (6,7,8)))
+extern ssize_t
 efidp_make_acpi_hid_ex(uint8_t *buf, ssize_t size,
                        uint32_t hid, uint32_t uid, uint32_t cid,
                        const char *hidstr, const char *uidstr,