[PATCH 0/5] Various cleanups
I've been playing with cocci lately and noticed it had troubles parsing some files. When I looked into them I had trouble parsing them as well. Michal Prívozník (5): node_device_conf: Bring variables into loops virpcivpd: Bring variables into loops virpcivpdtest: Declare variables at multiple lines lib: Use G_N_ELEMENTS instead of sizeof()/sizeof() scripts: Properly declare g_auto() stub for cocci scripts/cocci-macro-file.h | 2 +- src/conf/node_device_conf.c | 46 +++++++++++++--------------------- src/libxl/libxl_capabilities.c | 2 +- src/util/virpcivpd.c | 19 +++----------- tests/virpcimock.c | 2 +- tests/virpcivpdtest.c | 29 ++++++++++----------- 6 files changed, 39 insertions(+), 61 deletions(-) -- 2.32.0
I've noticed three functions inside node_device_conf.c, namely: - virNodeDeviceCapVPDParseCustomFields() - virNodeDeviceCapVPDParseReadOnlyFields() - virNodeDeviceCapVPDParseXML() that have strange attitude towards g_auto* variables. The first problem is that variables are declared at the top level despite being used inside a loop. The second problem is use of g_free() in combination with g_steal_pointer() even though we have VIR_FREE() which does exactly that. Bringing variable declarations into their respective loops allows us to make the code nicer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/node_device_conf.c | 46 ++++++++++++++----------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index e958367572..ca534dfbed 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -940,19 +940,20 @@ static int virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource *res, bool readOnly) { int nfields = -1; - g_autofree char *index = NULL, *value = NULL, *keyword = NULL; g_autofree xmlNodePtr *nodes = NULL; - xmlNodePtr orignode = NULL; size_t i = 0; - orignode = ctxt->node; if ((nfields = virXPathNodeSet("./vendor_field[@index]", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to evaluate <vendor_field> elements")); - ctxt->node = orignode; return -1; } for (i = 0; i < nfields; i++) { + g_autofree char *value = NULL; + g_autofree char *index = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autofree char *keyword = NULL; + ctxt->node = nodes[i]; if (!(index = virXPathStringLimit("string(./@index[1])", 2, ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -966,21 +967,21 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource } keyword = g_strdup_printf("V%c", index[0]); virPCIVPDResourceUpdateKeyword(res, readOnly, keyword, value); - g_free(g_steal_pointer(&index)); - g_free(g_steal_pointer(&keyword)); - g_free(g_steal_pointer(&value)); } - g_free(g_steal_pointer(&nodes)); - ctxt->node = orignode; + VIR_FREE(nodes); if (!readOnly) { if ((nfields = virXPathNodeSet("./system_field[@index]", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to evaluate <system_field> elements")); - ctxt->node = orignode; return -1; } for (i = 0; i < nfields; i++) { + g_autofree char *value = NULL; + g_autofree char *index = NULL; + g_autofree char *keyword = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt); + ctxt->node = nodes[i]; if (!(index = virXPathStringLimit("string(./@index[1])", 2, ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -994,11 +995,7 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource } keyword = g_strdup_printf("Y%c", index[0]); virPCIVPDResourceUpdateKeyword(res, readOnly, keyword, value); - g_free(g_steal_pointer(&index)); - g_free(g_steal_pointer(&keyword)); - g_free(g_steal_pointer(&value)); } - ctxt->node = orignode; } return 0; @@ -1009,8 +1006,6 @@ virNodeDeviceCapVPDParseReadOnlyFields(xmlXPathContextPtr ctxt, virPCIVPDResourc { const char *keywords[] = {"change_level", "manufacture_id", "serial_number", "part_number", NULL}; - g_autofree char *expression = NULL; - g_autofree char *result = NULL; size_t i = 0; if (res == NULL) @@ -1019,11 +1014,10 @@ virNodeDeviceCapVPDParseReadOnlyFields(xmlXPathContextPtr ctxt, virPCIVPDResourc res->ro = virPCIVPDResourceRONew(); while (keywords[i]) { - expression = g_strdup_printf("string(./%s)", keywords[i]); - result = virXPathString(expression, ctxt); + g_autofree char *expression = g_strdup_printf("string(./%s)", keywords[i]); + g_autofree char *result = virXPathString(expression, ctxt); + virPCIVPDResourceUpdateKeyword(res, true, keywords[i], result); - g_free(g_steal_pointer(&expression)); - g_free(g_steal_pointer(&result)); ++i; } if (virNodeDeviceCapVPDParseCustomFields(ctxt, res, true) < 0) @@ -1047,38 +1041,34 @@ virNodeDeviceCapVPDParseReadWriteFields(xmlXPathContextPtr ctxt, virPCIVPDResour static int virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) { - xmlNodePtr orignode = NULL; g_autofree xmlNodePtr *nodes = NULL; int nfields = -1; - g_autofree char *access = NULL; size_t i = 0; g_autoptr(virPCIVPDResource) newres = g_new0(virPCIVPDResource, 1); if (res == NULL) return -1; - orignode = ctxt->node; - if (!(newres->name = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Could not read a device name from the <name> element")); - ctxt->node = orignode; return -1; } if ((nfields = virXPathNodeSet("./fields[@access]", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("no VPD <fields> elements with an access type attribute found")); - ctxt->node = orignode; return -1; } for (i = 0; i < nfields; i++) { + g_autofree char *access = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt); + ctxt->node = nodes[i]; if (!(access = virXPathString("string(./@access[1])", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("VPD fields access type parsing has failed")); - ctxt->node = orignode; return -1; } @@ -1099,9 +1089,7 @@ virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) access); return -1; } - g_free(g_steal_pointer(&access)); } - ctxt->node = orignode; /* Replace the existing VPD representation if there is one already. */ if (*res != NULL) -- 2.32.0
I've noticed one function inside virpcivpd.c, namely virPCIVPDParseVPDLargeResourceFields() that declares some variables at the top level even though they are used only inside a loop in which they have to be freed explicitly. Bringing variable declarations into the loop allows us to make the code nicer. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virpcivpd.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index d8f2a43cde..9af0566d19 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -456,10 +456,6 @@ bool virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, bool readOnly, uint8_t *csum, virPCIVPDResource *res) { - g_autofree char *fieldKeyword = NULL; - g_autofree char *fieldValue = NULL; - virPCIVPDResourceFieldValueFormat fieldFormat = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; - /* A buffer of up to one resource record field size (plus a zero byte) is needed. */ g_autofree uint8_t *buf = g_malloc0(PCI_VPD_MAX_FIELD_SIZE + 1); uint16_t fieldDataLen = 0, bytesToRead = 0; @@ -473,6 +469,10 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re * just occupy 3 header bytes. In the in case of the RW field this may mean that * no more space is left in the section. */ while (fieldPos + 3 <= resPos + resDataLen) { + virPCIVPDResourceFieldValueFormat fieldFormat = VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST; + g_autofree char *fieldKeyword = NULL; + g_autofree char *fieldValue = NULL; + /* Keyword resources consist of keywords (2 ASCII bytes per the spec) and 1-byte length. */ if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) { /* Invalid field encountered which means the resource itself is invalid too. Report @@ -548,8 +548,6 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re /* Skip fields with invalid values - this is safe assuming field length is * correctly specified. */ VIR_DEBUG("A value for field %s contains invalid characters", fieldKeyword); - g_free(g_steal_pointer(&fieldKeyword)); - g_free(g_steal_pointer(&fieldValue)); continue; } } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { @@ -559,19 +557,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re return false; } hasChecksum = true; - g_free(g_steal_pointer(&fieldKeyword)); - g_free(g_steal_pointer(&fieldValue)); break; } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) { /* Skip the read-write space since it is used for indication only. */ hasRW = true; - g_free(g_steal_pointer(&fieldKeyword)); - g_free(g_steal_pointer(&fieldValue)); break; } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) { /* Skip unknown fields */ - g_free(g_steal_pointer(&fieldKeyword)); - g_free(g_steal_pointer(&fieldValue)); continue; } else { fieldValue = g_malloc(fieldDataLen); @@ -591,9 +583,6 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re _("Could not update the VPD resource keyword: %s"), fieldKeyword); return false; } - /* No longer need those since copies were made during the keyword update. */ - g_free(g_steal_pointer(&fieldKeyword)); - g_free(g_steal_pointer(&fieldValue)); } /* May have exited the loop prematurely in case RV or RW were encountered and -- 2.32.0
In testPCIVPDResourceCustomCompareIndex() there are two variables declared at one line. They are both g_autoptr() decorated which makes it worse, because coccinelle fails to parse that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virpcivpdtest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index a9405f9427..add1c74c04 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -178,7 +178,8 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) static int testPCIVPDResourceCustomCompareIndex(const void *data G_GNUC_UNUSED) { - g_autoptr(virPCIVPDResourceCustom) a = NULL, b = NULL; + g_autoptr(virPCIVPDResourceCustom) a = NULL; + g_autoptr(virPCIVPDResourceCustom) b = NULL; /* Both are NULL */ if (!virPCIVPDResourceCustomCompareIndex(a, b)) -- 2.32.0
For statically declared arrays one can use G_N_ELEMENTS() instead of explicit sizeof(array) / sizeof(item). I've noticed couple of places where the latter was used. I am not fixing every occurrence because we have some places which do not use glib (examples and NSS module). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_capabilities.c | 2 +- tests/virpcimock.c | 2 +- tests/virpcivpdtest.c | 26 +++++++++++++------------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index e03b6fd3c3..6263b5c8b5 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -380,7 +380,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCaps *caps) * we "own" the buffer. Parse out the features from each token. */ for (str = ver_info->capabilities, nr_guest_archs = 0; - nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0]) + nr_guest_archs < G_N_ELEMENTS(guest_archs) && (token = strtok_r(str, " ", &saveptr)) != NULL; str = NULL) { if (g_regex_match(regex, token, 0, &info)) { diff --git a/tests/virpcimock.c b/tests/virpcimock.c index f65ae7c0c5..77d46f0952 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -965,7 +965,7 @@ init_env(void) }; struct pciVPD exampleVPD = { .data = fullVPDExampleData, - .vpd_len = sizeof(fullVPDExampleData) / sizeof(fullVPDExampleData[0]), + .vpd_len = G_N_ELEMENTS(fullVPDExampleData), }; if (!(fakerootdir = getenv("LIBVIRT_FAKE_ROOT_DIR"))) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index add1c74c04..284350fe29 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -76,9 +76,9 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) {.keyword = "CP", .value = "42", .actual = NULL}, {.keyword = "EX", .value = "42", .actual = NULL}, }; - size_t numROCases = sizeof(readOnlyCases) / sizeof(TestPCIVPDKeywordValue); - size_t numRWCases = sizeof(readWriteCases) / sizeof(TestPCIVPDKeywordValue); - size_t numUnsupportedCases = sizeof(unsupportedFieldCases) / sizeof(TestPCIVPDKeywordValue); + size_t numROCases = G_N_ELEMENTS(readOnlyCases); + size_t numRWCases = G_N_ELEMENTS(readWriteCases); + size_t numUnsupportedCases = G_N_ELEMENTS(unsupportedFieldCases); g_autoptr(virPCIVPDResource) res = g_new0(virPCIVPDResource, 1); virPCIVPDResourceCustom *custom = NULL; @@ -328,7 +328,7 @@ testPCIVPDIsValidTextValue(const void *data G_GNUC_UNUSED) /* The first and last code points are outside ASCII (multi-byte in UTF-8). */ {"гbl🐧", false}, }; - for (i = 0; i < sizeof(textValueCases) / sizeof(textValueCases[0]); ++i) { + for (i = 0; i < G_N_ELEMENTS(textValueCases); ++i) { if (virPCIVPDResourceIsValidTextValue(textValueCases[i].keyword) != textValueCases[i].expected) return -1; @@ -385,7 +385,7 @@ testPCIVPDGetFieldValueFormat(const void *data G_GNUC_UNUSED) /* Many letters. */ {"EXAMPLE", VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST}, }; - for (i = 0; i < sizeof(valueFormatCases) / sizeof(valueFormatCases[0]); ++i) { + for (i = 0; i < G_N_ELEMENTS(valueFormatCases); ++i) { if (virPCIVPDResourceGetFieldValueFormat(valueFormatCases[i].keyword) != valueFormatCases[i].expected) return -1; @@ -442,7 +442,7 @@ testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, PCI_VPD_RESOURCE_END_VAL }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t) - 2; + dataLen = G_N_ELEMENTS(fullVPDExample) - 2; buf = g_malloc0(dataLen); fd = virCreateAnonymousFile(fullVPDExample, dataLen); @@ -480,7 +480,7 @@ testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) VPD_STRING_RESOURCE_EXAMPLE_DATA }; - dataLen = sizeof(stringResExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(stringResExample); fd = virCreateAnonymousFile(stringResExample, dataLen); result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum, res); VIR_FORCE_CLOSE(fd); @@ -550,7 +550,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) PCI_VPD_RESOURCE_END_VAL }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -618,7 +618,7 @@ testVirPCIVPDParseZeroLengthRW(const void *opaque G_GNUC_UNUSED) PCI_VPD_RESOURCE_END_VAL }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -668,7 +668,7 @@ testVirPCIVPDParseNoRW(const void *opaque G_GNUC_UNUSED) PCI_VPD_RESOURCE_END_VAL }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -721,7 +721,7 @@ testVirPCIVPDParseFullVPDSkipInvalidKeywords(const void *opaque G_GNUC_UNUSED) PCI_VPD_RESOURCE_END_VAL }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -774,7 +774,7 @@ testVirPCIVPDParseFullVPDSkipInvalidValues(const void *opaque G_GNUC_UNUSED) 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 'R', 'W', 0x00, 0x78, }; - dataLen = sizeof(fullVPDExample) / sizeof(uint8_t); + dataLen = G_N_ELEMENTS(fullVPDExample); fd = virCreateAnonymousFile(fullVPDExample, dataLen); res = virPCIVPDParse(fd); VIR_FORCE_CLOSE(fd); @@ -950,7 +950,7 @@ testVirPCIVPDParseFullVPDInvalid(const void *opaque G_GNUC_UNUSED) do { \ g_autoptr(virPCIVPDResource) res = NULL; \ const uint8_t testCase[] = { invalidVPD }; \ - dataLen = sizeof(testCase) / sizeof(uint8_t); \ + dataLen = G_N_ELEMENTS(testCase); \ fd = virCreateAnonymousFile(testCase, dataLen); \ if ((res = virPCIVPDParse(fd))) { \ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \ -- 2.32.0
While being great semantic patching tool, coccinelle fails to understand some of macros we use (including those provided by glib). What they have in common is use of __attribute__ under the hood. We store a list of such macros in a file. But in there, g_auto() macro is not defined properly. Indeed, g_auto(type) declares a local variable of given type, for instance from cocci's POV: g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; virBuffer buf = VIR_BUFFER_INITIALIZER; are both the same declaration. Fix declaration of g_auto() stub. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- scripts/cocci-macro-file.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/cocci-macro-file.h b/scripts/cocci-macro-file.h index a76ba533b4..4e6d218a97 100644 --- a/scripts/cocci-macro-file.h +++ b/scripts/cocci-macro-file.h @@ -34,6 +34,6 @@ #define g_autoptr(x) x##_autoptr #define g_autofree -#define g_auto +#define g_auto(x) x #define BAD_CAST -- 2.32.0
On Tue, 2021-11-02 at 18:10 +0100, Michal Privoznik wrote:
I've been playing with cocci lately and noticed it had troubles parsing some files. When I looked into them I had trouble parsing them as well.
Michal Prívozník (5): node_device_conf: Bring variables into loops virpcivpd: Bring variables into loops virpcivpdtest: Declare variables at multiple lines lib: Use G_N_ELEMENTS instead of sizeof()/sizeof() scripts: Properly declare g_auto() stub for cocci
scripts/cocci-macro-file.h | 2 +- src/conf/node_device_conf.c | 46 +++++++++++++------------------- -- src/libxl/libxl_capabilities.c | 2 +- src/util/virpcivpd.c | 19 +++----------- tests/virpcimock.c | 2 +- tests/virpcivpdtest.c | 29 ++++++++++----------- 6 files changed, 39 insertions(+), 61 deletions(-)
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
participants (2)
-
Michal Privoznik -
Tim Wiederhake