[libvirt] [PATCH] nwfilter: cleanup return codes in nwfilter subsystem
This patch cleans up return codes in the nwfilter subsystem. Some functions in nwfilter_conf.c (validators and formatters) are keeping their bool return for now and I am converting their return code to true/false. All other functions now return -1 on failure and 0 on success. [I searched for all occurences of ' 1;' and checked all 'if ' and adapted where needed. After that I did a grep for 'NWFilter' in the source tree.] --- src/conf/nwfilter_conf.c | 127 ++++++------- src/conf/nwfilter_params.c | 22 +- src/nwfilter/nwfilter_driver.c | 2 src/nwfilter/nwfilter_ebiptables_driver.c | 286 +++++++++++++++--------------- src/nwfilter/nwfilter_gentech_driver.c | 78 ++++---- src/nwfilter/nwfilter_learnipaddr.c | 47 ++-- src/qemu/qemu_command.c | 3 src/qemu/qemu_process.c | 2 src/uml/uml_conf.c | 2 9 files changed, 290 insertions(+), 279 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -214,23 +214,24 @@ static const char state_str[] = " * @attr: The attribute to look up * @res: Pointer to string pointer for result * - * Returns 1 if value was found with result returned, 0 otherwise. + * Returns 0 if value was found with result returned, -1 otherwise. * * lookup a map entry given the integer. */ -static bool +static int intMapGetByInt(const struct int_map *intmap, int32_t attr, const char **res) { int i = 0; - bool found = 0; + int found = false; + while (intmap[i].val && !found) { if (intmap[i].attr == attr) { *res = intmap[i].val; - found = 1; + found = true; } i++; } - return found; + return (found) ? 0 : -1; } @@ -241,26 +242,27 @@ intMapGetByInt(const struct int_map *int * @casecmp : Whether to ignore case when doing string matching * @result: Pointer to int for result * - * Returns 0 if no entry was found, 1 otherwise. + * Returns 0 if entry was found, -1 otherwise. * * Do a lookup in the map trying to find an integer key using the string - * value. Returns 1 if entry was found with result returned, 0 otherwise. + * value. Returns 0 if entry was found with result returned, -1 otherwise. */ -static bool +static int intMapGetByString(const struct int_map *intmap, const char *str, int casecmp, int32_t *result) { int i = 0; - bool found = 0; + bool found = false; + while (intmap[i].val && !found) { if ( (casecmp && STRCASEEQ(intmap[i].val, str)) || STREQ (intmap[i].val, str) ) { *result = intmap[i].attr; - found = 1; + found = true; } i++; } - return found; + return (found) ? 0 : -1; } @@ -367,14 +369,14 @@ virNWFilterRuleDefAddVar(virNWFilterRule if (VIR_REALLOC_N(nwf->vars, nwf->nvars+1) < 0) { virReportOOMError(); - return 1; + return -1; } nwf->vars[nwf->nvars] = strdup(var); if (!nwf->vars[nwf->nvars]) { virReportOOMError(); - return 1; + return -1; } item->var = nwf->vars[nwf->nvars++]; @@ -479,7 +481,7 @@ checkMacProtocolID(enum attrDatatype dat int32_t res = -1; if (datatype == DATATYPE_STRING) { - if (intMapGetByString(macProtoMap, value->c, 1, &res) == 0) + if (intMapGetByString(macProtoMap, value->c, 1, &res) < 0) res = -1; datatype = DATATYPE_UINT16; } else if (datatype == DATATYPE_UINT16 || @@ -492,10 +494,10 @@ checkMacProtocolID(enum attrDatatype dat if (res != -1) { nwf->p.ethHdrFilter.dataProtocolID.u.u16 = res; nwf->p.ethHdrFilter.dataProtocolID.datatype = datatype; - return 1; + return true; } - return 0; + return false; } @@ -509,7 +511,7 @@ macProtocolIDFormatter(virBufferPtr buf, if (intMapGetByInt(macProtoMap, nwf->p.ethHdrFilter.dataProtocolID.u.u16, - &str)) { + &str) == 0) { virBufferAdd(buf, str, -1); } else { if (nwf->p.ethHdrFilter.dataProtocolID.datatype == DATATYPE_UINT16) @@ -517,7 +519,7 @@ macProtocolIDFormatter(virBufferPtr buf, virBufferAsprintf(buf, asHex ? "0x%x" : "%d", nwf->p.ethHdrFilter.dataProtocolID.u.u16); } - return 1; + return true; } @@ -550,7 +552,7 @@ checkVlanProtocolID(enum attrDatatype da int32_t res = -1; if (datatype == DATATYPE_STRING) { - if (intMapGetByString(macProtoMap, value->c, 1, &res) == 0) + if (intMapGetByString(macProtoMap, value->c, 1, &res) < 0) res = -1; datatype = DATATYPE_UINT16; } else if (datatype == DATATYPE_UINT16 || @@ -579,7 +581,7 @@ vlanProtocolIDFormatter(virBufferPtr buf if (intMapGetByInt(macProtoMap, nwf->p.vlanHdrFilter.dataVlanEncap.u.u16, - &str)) { + &str) == 0) { virBufferAdd(buf, str, -1); } else { if (nwf->p.vlanHdrFilter.dataVlanEncap.datatype == DATATYPE_UINT16) @@ -607,7 +609,7 @@ checkValidMask(unsigned char *data, int checkones = 0; } else { if ((data[idx>>3] & mask)) - return 0; + return false; } idx++; @@ -615,7 +617,7 @@ checkValidMask(unsigned char *data, int if (!mask) mask = 0x80; } - return 1; + return true; } @@ -655,7 +657,7 @@ arpOpcodeValidator(enum attrDatatype dat int32_t res = -1; if (datatype == DATATYPE_STRING) { - if (intMapGetByString(arpOpcodeMap, value->c, 1, &res) == 0) + if (intMapGetByString(arpOpcodeMap, value->c, 1, &res) < 0) res = -1; datatype = DATATYPE_UINT16; } else if (datatype == DATATYPE_UINT16 || @@ -666,9 +668,9 @@ arpOpcodeValidator(enum attrDatatype dat if (res != -1) { nwf->p.arpHdrFilter.dataOpcode.u.u16 = res; nwf->p.arpHdrFilter.dataOpcode.datatype = datatype; - return 1; + return true; } - return 0; + return false; } @@ -681,12 +683,12 @@ arpOpcodeFormatter(virBufferPtr buf, if (intMapGetByInt(arpOpcodeMap, nwf->p.arpHdrFilter.dataOpcode.u.u16, - &str)) { + &str) == 0) { virBufferAdd(buf, str, -1); } else { virBufferAsprintf(buf, "%d", nwf->p.arpHdrFilter.dataOpcode.u.u16); } - return 1; + return true; } @@ -708,15 +710,16 @@ static const struct int_map ipProtoMap[] }; -static bool checkIPProtocolID(enum attrDatatype datatype, - union data *value, - virNWFilterRuleDefPtr nwf, - nwItemDesc *item ATTRIBUTE_UNUSED) +static bool +checkIPProtocolID(enum attrDatatype datatype, + union data *value, + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) { int32_t res = -1; if (datatype == DATATYPE_STRING) { - if (intMapGetByString(ipProtoMap, value->c, 1, &res) == 0) + if (intMapGetByString(ipProtoMap, value->c, 1, &res) < 0) res = -1; datatype = DATATYPE_UINT8_HEX; } else if (datatype == DATATYPE_UINT8 || @@ -727,9 +730,9 @@ static bool checkIPProtocolID(enum attrD if (res != -1) { nwf->p.ipHdrFilter.ipHdr.dataProtocolID.u.u8 = res; nwf->p.ipHdrFilter.ipHdr.dataProtocolID.datatype = datatype; - return 1; + return true; } - return 0; + return false; } @@ -743,7 +746,7 @@ formatIPProtocolID(virBufferPtr buf, if (intMapGetByInt(ipProtoMap, nwf->p.ipHdrFilter.ipHdr.dataProtocolID.u.u8, - &str)) { + &str) == 0) { virBufferAdd(buf, str, -1); } else { if (nwf->p.ipHdrFilter.ipHdr.dataProtocolID.datatype == DATATYPE_UINT8) @@ -751,7 +754,7 @@ formatIPProtocolID(virBufferPtr buf, virBufferAsprintf(buf, asHex ? "0x%x" : "%d", nwf->p.ipHdrFilter.ipHdr.dataProtocolID.u.u8); } - return 1; + return true; } @@ -762,11 +765,11 @@ dscpValidator(enum attrDatatype datatype { uint8_t dscp = val->ui; if (dscp > 63) - return 0; + return false; nwf->p.ipHdrFilter.ipHdr.dataDSCP.datatype = datatype; - return 1; + return true; } @@ -805,7 +808,7 @@ parseStringItems(const struct int_map *i } } if (!found) { - rc = 1; + rc = -1; break; } } @@ -874,15 +877,15 @@ stateValidator(enum attrDatatype datatyp char *input = val->c; int32_t flags = 0; - if (parseStateMatch(input, &flags)) - return 0; + if (parseStateMatch(input, &flags) < 0) + return false; item->u.u16 = flags; nwf->flags |= flags; item->datatype = DATATYPE_UINT16; - return 1; + return true; } @@ -1663,13 +1666,11 @@ static const virAttributes virAttr[] = { }; -static bool +static int virNWMACAddressParser(const char *input, nwMACAddressPtr output) { - if (virParseMacAddr(input, &output->addr[0]) == 0) - return 1; - return 0; + return virParseMacAddr(input, &output->addr[0]); } @@ -1714,7 +1715,7 @@ virNWFilterRuleDetailsParse(xmlNodePtr n flags_set |= NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR; if (virNWFilterRuleDefAddVar(nwf, item, - &prop[1])) + &prop[1]) < 0) rc = -1; found = 1; } @@ -1805,8 +1806,8 @@ virNWFilterRuleDetailsParse(xmlNodePtr n break; case DATATYPE_MACADDR: - if (!virNWMACAddressParser(prop, - &item->u.macaddr)) { + if (virNWMACAddressParser(prop, + &item->u.macaddr) < 0) { rc = -1; } found = 1; @@ -1814,8 +1815,8 @@ virNWFilterRuleDetailsParse(xmlNodePtr n case DATATYPE_MACMASK: validator = checkMACMask; - if (!virNWMACAddressParser(prop, - &item->u.macaddr)) { + if (virNWMACAddressParser(prop, + &item->u.macaddr) < 0) { rc = -1; } data.v = &item->u.macaddr; @@ -2418,8 +2419,8 @@ virNWFilterDefParseXML(xmlXPathContextPt } else { /* assign default priority if none can be found via lookup */ if (!name_prefix || - !intMapGetByString(chain_priorities, name_prefix, 0, - &ret->chainPriority)) { + intMapGetByString(chain_priorities, name_prefix, 0, + &ret->chainPriority) < 0) { /* assign default chain priority */ ret->chainPriority = (NWFILTER_MAX_FILTER_PRIORITY + NWFILTER_MIN_FILTER_PRIORITY) / 2; @@ -2620,7 +2621,7 @@ int virNWFilterSaveConfig(const char *co if (!(xml = virNWFilterDefFormat(def))) goto cleanup; - if (virNWFilterSaveXML(configDir, def, xml)) + if (virNWFilterSaveXML(configDir, def, xml) < 0) goto cleanup; ret = 0; @@ -2649,7 +2650,7 @@ _virNWFilterDefLoopDetect(virConnectPtr if (entry->include) { if (STREQ(filtername, entry->include->filterref)) { - rc = 1; + rc = -1; break; } @@ -2660,8 +2661,8 @@ _virNWFilterDefLoopDetect(virConnectPtr obj->def, filtername); virNWFilterObjUnlock(obj); - if (rc) - break; + if (rc < 0) + break; } } } @@ -2679,7 +2680,7 @@ _virNWFilterDefLoopDetect(virConnectPtr * Detect a loop introduced through the filters being able to * reference each other. * - * Returns 0 in case no loop was detected, 1 otherwise. + * Returns 0 in case no loop was detected, -1 otherwise. */ static int virNWFilterDefLoopDetect(virConnectPtr conn, @@ -2736,7 +2737,7 @@ virNWFilterTriggerVMFilterRebuild(virCon }; if (!cb.skipInterfaces) - return 1; + return -1; for (i = 0; i < nCallbackDriver; i++) { callbackDrvArray[i]->vmFilterRebuild(conn, @@ -2778,7 +2779,7 @@ virNWFilterTestUnassignDef(virConnectPtr nwfilter->wantRemoved = 1; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) - rc = 1; + rc = -1; nwfilter->wantRemoved = 0; @@ -2807,7 +2808,7 @@ virNWFilterObjAssignDef(virConnectPtr co virNWFilterObjUnlock(nwfilter); } - if (virNWFilterDefLoopDetect(conn, nwfilters, def)) { + if (virNWFilterDefLoopDetect(conn, nwfilters, def) < 0) { virNWFilterReportError(VIR_ERR_OPERATION_FAILED, "%s", _("filter would introduce a loop")); return NULL; @@ -3297,8 +3298,8 @@ int virNWFilterConfLayerInit(virHashIter initialized = true; - if (virMutexInitRecursive(&updateMutex)) - return 1; + if (virMutexInitRecursive(&updateMutex) < 0) + return -1; return 0; } Index: libvirt-acl/src/nwfilter/nwfilter_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_driver.c @@ -384,7 +384,7 @@ nwfilterUndefine(virNWFilterPtr obj) { goto cleanup; } - if (virNWFilterTestUnassignDef(obj->conn, nwfilter)) { + if (virNWFilterTestUnassignDef(obj->conn, nwfilter) < 0) { virNWFilterReportError(VIR_ERR_OPERATION_INVALID, "%s", _("nwfilter is in use")); Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -82,7 +82,7 @@ virNWFilterVarValueCopy(const virNWFilte } break; case NWFILTER_VALUE_TYPE_ARRAY: - if (VIR_ALLOC_N(res->u.array.values, val->u.array.nValues)) + if (VIR_ALLOC_N(res->u.array.values, val->u.array.nValues) < 0) goto err_exit; res->u.array.nValues = val->u.array.nValues; for (i = 0; i < val->u.array.nValues; i++) { @@ -490,7 +490,7 @@ hashDataFree(void *payload, const void * * @val: The value associated with the key * @freeName: Whether the name must be freed on table destruction * - * Returns 0 on success, 1 on failure. + * Returns 0 on success, -1 on failure. * * Put an entry into the hashmap replacing and freeing an existing entry * if one existed. @@ -505,25 +505,25 @@ virNWFilterHashTablePut(virNWFilterHashT if (copyName) { name = strdup(name); if (!name) - return 1; + return -1; if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) { VIR_FREE(name); - return 1; + return -1; } table->names[table->nNames++] = (char *)name; } - if (virHashAddEntry(table->hashTable, name, val) != 0) { + if (virHashAddEntry(table->hashTable, name, val) < 0) { if (copyName) { VIR_FREE(name); table->nNames--; } - return 1; + return -1; } } else { - if (virHashUpdateEntry(table->hashTable, name, val) != 0) { - return 1; + if (virHashUpdateEntry(table->hashTable, name, val) < 0) { + return -1; } } return 0; @@ -614,7 +614,7 @@ addToTable(void *payload, const void *na return; } - if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) != 0) { + if (virNWFilterHashTablePut(atts->target, (const char *)name, val, 1) < 0){ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Could not put variable '%s' into hashmap"), (const char *)name); @@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHa return 0; err_exit: - return 1; + return -1; } @@ -700,7 +700,7 @@ virNWFilterParseParamAttributes(xmlNodeP value = virNWFilterParseVarValue(val); if (!value) goto skip_entry; - if (virNWFilterHashTablePut(table, nam, value, 1)) + if (virNWFilterHashTablePut(table, nam, value, 1) < 0) goto err_exit; } value = NULL; Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -106,7 +106,7 @@ virNWFilterRuleInstAddData(virNWFilterRu { if (VIR_REALLOC_N(res->data, res->ndata+1) < 0) { virReportOOMError(); - return 1; + return -1; } res->data[res->ndata++] = data; return 0; @@ -151,28 +151,28 @@ virNWFilterVarHashmapAddStdValues(virNWF if (macaddr) { val = virNWFilterVarValueCreateSimple(macaddr); if (!val) - return 1; + return -1; if (virHashAddEntry(table->hashTable, NWFILTER_STD_VAR_MAC, val) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'MAC' to hashmap")); - return 1; + return -1; } } if (ipaddr) { val = virNWFilterVarValueCopy(ipaddr); if (!val) - return 1; + return -1; if (virHashAddEntry(table->hashTable, NWFILTER_STD_VAR_IP, val) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'IP' to hashmap")); - return 1; + return -1; } } @@ -200,7 +200,7 @@ virNWFilterCreateVarHashmap(char *macadd return NULL; } - if (virNWFilterVarHashmapAddStdValues(table, macaddr, ipaddr)) { + if (virNWFilterVarHashmapAddStdValues(table, macaddr, ipaddr) < 0) { virNWFilterHashTableFree(table); return NULL; } @@ -339,10 +339,10 @@ virNWFilterCreateVarsFrom(virNWFilterHas return NULL; } - if (virNWFilterHashTablePutAll(vars1, res)) + if (virNWFilterHashTablePutAll(vars1, res) < 0) goto err_exit; - if (virNWFilterHashTablePutAll(vars2, res)) + if (virNWFilterHashTablePutAll(vars2, res) < 0) goto err_exit; return res; @@ -404,13 +404,13 @@ _virNWFilterInstantiateRec(virNWFilterTe ifname, vars); if (!inst) { - rc = 1; + rc = -1; break; } if (VIR_REALLOC_N(*insts, (*nEntries)+1) < 0) { virReportOOMError(); - rc = 1; + rc = -1; break; } @@ -425,7 +425,7 @@ _virNWFilterInstantiateRec(virNWFilterTe virNWFilterReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), inc->filterref); - rc = 1; + rc = -1; virNWFilterObjUnlock(obj); break; } @@ -436,7 +436,7 @@ _virNWFilterInstantiateRec(virNWFilterTe vars); if (!tmpvars) { virReportOOMError(); - rc = 1; + rc = -1; virNWFilterObjUnlock(obj); break; } @@ -467,13 +467,13 @@ _virNWFilterInstantiateRec(virNWFilterTe virNWFilterHashTableFree(tmpvars); virNWFilterObjUnlock(obj); - if (rc) + if (rc < 0) break; } else { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), inc->filterref); - rc = 1; + rc = -1; break; } } @@ -504,7 +504,7 @@ virNWFilterDetermineMissingVarsRec(virNW if (!virHashLookup(vars->hashTable, rule->vars[j])) { val = virNWFilterVarValueCreateSimpleCopyValue("1"); if (!val) { - rc = 1; + rc = -1; break; } virNWFilterHashTablePut(missing_vars, rule->vars[j], @@ -522,7 +522,7 @@ virNWFilterDetermineMissingVarsRec(virNW virNWFilterReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), inc->filterref); - rc = 1; + rc = -1; virNWFilterObjUnlock(obj); break; } @@ -533,7 +533,7 @@ virNWFilterDetermineMissingVarsRec(virNW vars); if (!tmpvars) { virReportOOMError(); - rc = 1; + rc = -1; virNWFilterObjUnlock(obj); break; } @@ -559,13 +559,13 @@ virNWFilterDetermineMissingVarsRec(virNW virNWFilterHashTableFree(tmpvars); virNWFilterObjUnlock(obj); - if (rc) + if (rc < 0) break; } else { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("referenced filter '%s' is missing"), inc->filterref); - rc = 1; + rc = -1; break; } } @@ -592,7 +592,7 @@ virNWFilterRuleInstancesToArray(int nEnt if (VIR_ALLOC_N((*ptrs), (*nptrs)) < 0) { virReportOOMError(); - return 1; + return -1; } (*nptrs) = 0; @@ -649,7 +649,7 @@ virNWFilterInstantiate(virNWFilterTechDr virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { virReportOOMError(); - rc = 1; + rc = -1; goto err_exit; } @@ -658,7 +658,7 @@ virNWFilterInstantiate(virNWFilterTechDr missing_vars, useNewFilter, driver); - if (rc) + if (rc < 0) goto err_exit; if (virHashSize(missing_vars->hashTable) == 1) { @@ -693,7 +693,7 @@ virNWFilterInstantiate(virNWFilterTechDr useNewFilter, foundNewFilter, driver); - if (rc) + if (rc < 0) goto err_exit; switch (useNewFilter) { @@ -709,10 +709,10 @@ virNWFilterInstantiate(virNWFilterTechDr rc = virNWFilterRuleInstancesToArray(nEntries, insts, &ptrs, &nptrs); - if (rc) + if (rc < 0) goto err_exit; - if (virNWFilterLockIface(ifname)) + if (virNWFilterLockIface(ifname) < 0) goto err_exit; rc = techdriver->applyNewRules(ifname, nptrs, ptrs); @@ -724,7 +724,7 @@ virNWFilterInstantiate(virNWFilterTechDr virResetLastError(); /* interface changed/disppeared */ techdriver->allTeardown(ifname); - rc = 1; + rc = -1; } virNWFilterUnlockIface(ifname); @@ -752,7 +752,7 @@ err_unresolvable_vars: VIR_FREE(buf); } - rc = 1; + rc = -1; goto err_exit; } @@ -792,7 +792,7 @@ __virNWFilterInstantiateFilter(bool tear _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; } VIR_DEBUG("filter name: %s", filtername); @@ -802,14 +802,14 @@ __virNWFilterInstantiateFilter(bool tear virNWFilterReportError(VIR_ERR_NO_NWFILTER, _("Could not find filter '%s'"), filtername); - return 1; + return -1; } if (obj->wantRemoved) { virNWFilterReportError(VIR_ERR_NO_NWFILTER, _("Filter '%s' is in use."), filtername); - rc = 1; + rc = -1; goto err_exit; } @@ -817,7 +817,7 @@ __virNWFilterInstantiateFilter(bool tear str_macaddr = strdup(vmmacaddr); if (!str_macaddr) { virReportOOMError(); - rc = 1; + rc = -1; goto err_exit; } @@ -825,7 +825,7 @@ __virNWFilterInstantiateFilter(bool tear vars1 = virNWFilterCreateVarHashmap(str_macaddr, ipaddr); if (!vars1) { - rc = 1; + rc = -1; goto err_exit; } @@ -835,7 +835,7 @@ __virNWFilterInstantiateFilter(bool tear vars = virNWFilterCreateVarsFrom(vars1, filterparams); if (!vars) { - rc = 1; + rc = -1; goto err_exit_vars1; } @@ -955,7 +955,7 @@ virNWFilterInstantiateFilterLate(const c driver, true, &foundNewFilter); - if (rc) { + if (rc < 0) { /* something went wrong... 'DOWN' the interface */ if ((virNetDevValidateConfig(ifname, NULL, ifindex) <= 0) || (virNetDevSetOnline(ifname, false) < 0)) { @@ -1012,7 +1012,7 @@ int virNWFilterRollbackUpdateFilter(cons _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; } /* don't tear anything while the address is being learned */ @@ -1038,7 +1038,7 @@ virNWFilterTearOldFilter(virDomainNetDef _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; } /* don't tear anything while the address is being learned */ @@ -1063,13 +1063,13 @@ _virNWFilterTeardownFilter(const char *i _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; } virNWFilterTerminateLearnReq(ifname); - if (virNWFilterLockIface(ifname)) - return 1; + if (virNWFilterLockIface(ifname) < 0) + return -1; techdriver->allTeardown(ifname); Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -149,7 +149,7 @@ virNWFilterLockIface(const char *ifname) goto err_exit; } - if (virMutexInitRecursive(&ifaceLock->lock)) { + if (virMutexInitRecursive(&ifaceLock->lock) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("mutex initialization failed")); VIR_FREE(ifaceLock); @@ -184,7 +184,7 @@ virNWFilterLockIface(const char *ifname) err_exit: virMutexUnlock(&ifaceMapLock); - return 1; + return -1; } @@ -248,7 +248,7 @@ virNWFilterRegisterLearnReq(virNWFilterI int virNWFilterTerminateLearnReq(const char *ifname) { - int rc = 1; + int rc = -1; int ifindex; virNWFilterIPAddrLearnReqPtr req; @@ -336,9 +336,6 @@ virNWFilterAddIpAddrForIfname(const char goto cleanup; } ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); - /* FIXME: fix when return code of virNWFilterHashTablePut changes */ - if (ret) - ret = -1; goto cleanup; } else { if (virNWFilterVarValueAddValue(val, addr) < 0) @@ -494,7 +491,7 @@ learnIPAddressThread(void *arg) enum howDetect howDetected = 0; virNWFilterTechDriverPtr techdriver = req->techdriver; - if (virNWFilterLockIface(req->ifname)) + if (virNWFilterLockIface(req->ifname) < 0) goto err_no_lock; req->status = 0; @@ -520,7 +517,7 @@ learnIPAddressThread(void *arg) case DETECT_DHCP: if (techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, - NULL, false)) { + NULL, false) < 0) { req->status = EINVAL; goto done; } @@ -530,7 +527,7 @@ learnIPAddressThread(void *arg) break; default: if (techdriver->applyBasicRules(req->ifname, - req->macaddr)) { + req->macaddr) < 0) { req->status = EINVAL; goto done; } @@ -701,7 +698,7 @@ learnIPAddressThread(void *arg) sa.data.inet4.sin_addr.s_addr = vmaddr; char *inetaddr; - if ((inetaddr = virSocketAddrFormat(&sa))!= NULL) { + if ((inetaddr = virSocketAddrFormat(&sa)) != NULL) { if (virNWFilterAddIpAddrForIfname(req->ifname, inetaddr) < 0) { VIR_ERROR(_("Failed to add IP address %s to IP address " "cache for interface %s"), inetaddr, req->ifname); @@ -781,14 +778,14 @@ virNWFilterLearnIPAddress(virNWFilterTec virNWFilterHashTablePtr ht = NULL; if (howDetect == 0) - return 1; + return -1; if ( !techdriver->canApplyBasicRules()) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IP parameter must be provided since " "snooping the IP address does not work " "possibly due to missing tools")); - return 1; + return -1; } if (VIR_ALLOC(req) < 0) { @@ -802,7 +799,7 @@ virNWFilterLearnIPAddress(virNWFilterTec goto err_free_req; } - if (virNWFilterHashTablePutAll(filterparams, ht)) + if (virNWFilterHashTablePutAll(filterparams, ht) < 0) goto err_free_ht; req->filtername = strdup(filtername); @@ -838,7 +835,7 @@ virNWFilterLearnIPAddress(virNWFilterTec rc = virNWFilterRegisterLearnReq(req); - if (rc) + if (rc < 0) goto err_free_req; if (pthread_create(&req->thread, @@ -856,7 +853,7 @@ err_free_ht: err_free_req: virNWFilterIPAddrLearnReqFree(req); err_no_req: - return 1; + return -1; } #else @@ -876,7 +873,7 @@ virNWFilterLearnIPAddress(virNWFilterTec _("IP parameter must be given since libvirt " "was not compiled with IP address learning " "support")); - return 1; + return -1; } #endif /* HAVE_LIBPCAP */ @@ -895,35 +892,35 @@ virNWFilterLearnInit(void) { pendingLearnReq = virHashCreate(0, freeLearnReqEntry); if (!pendingLearnReq) { - return 1; + return -1; } - if (virMutexInit(&pendingLearnReqLock)) { + if (virMutexInit(&pendingLearnReqLock) < 0) { virNWFilterLearnShutdown(); - return 1; + return -1; } ipAddressMap = virNWFilterHashTableCreate(0); if (!ipAddressMap) { virReportOOMError(); virNWFilterLearnShutdown(); - return 1; + return -1; } - if (virMutexInit(&ipAddressMapLock)) { + if (virMutexInit(&ipAddressMapLock) < 0) { virNWFilterLearnShutdown(); - return 1; + return -1; } ifaceLockMap = virHashCreate(0, freeIfaceLock); if (!ifaceLockMap) { virNWFilterLearnShutdown(); - return 1; + return -1; } - if (virMutexInit(&ifaceMapLock)) { + if (virMutexInit(&ifaceMapLock) < 0) { virNWFilterLearnShutdown(); - return 1; + return -1; } return 0; Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -233,15 +233,15 @@ printVar(virNWFilterVarCombIterPtr vars, val = virNWFilterVarCombIterGetVarValue(vars, item->var); if (!val) { /* error has been reported */ - return 1; + return -1; } if (!virStrcpy(buf, val, bufsize)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - _("Buffer to small to print MAC address " + _("Buffer too small to print MAC address " "'%s' into"), item->var); - return 1; + return -1; } *done = 1; @@ -259,8 +259,8 @@ _printDataType(virNWFilterVarCombIterPtr int done; char *data; - if (printVar(vars, buf, bufsize, item, &done)) - return 1; + if (printVar(vars, buf, bufsize, item, &done) < 0) + return -1; if (done) return 0; @@ -269,12 +269,12 @@ _printDataType(virNWFilterVarCombIterPtr case DATATYPE_IPADDR: data = virSocketAddrFormat(&item->u.ipaddr); if (!data) - return 1; + return -1; if (snprintf(buf, bufsize, "%s", data) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer too small for IP address")); VIR_FREE(data); - return 1; + return -1; } VIR_FREE(data); break; @@ -282,13 +282,13 @@ _printDataType(virNWFilterVarCombIterPtr case DATATYPE_IPV6ADDR: data = virSocketAddrFormat(&item->u.ipaddr); if (!data) - return 1; + return -1; if (snprintf(buf, bufsize, "%s", data) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer too small for IPv6 address")); VIR_FREE(data); - return 1; + return -1; } VIR_FREE(data); break; @@ -298,7 +298,7 @@ _printDataType(virNWFilterVarCombIterPtr if (bufsize < VIR_MAC_STRING_BUFLEN) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for MAC address")); - return 1; + return -1; } virFormatMacAddr(item->u.macaddr.addr, buf); @@ -310,7 +310,7 @@ _printDataType(virNWFilterVarCombIterPtr item->u.u8) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for uint8 type")); - return 1; + return -1; } break; @@ -320,7 +320,7 @@ _printDataType(virNWFilterVarCombIterPtr item->u.u32) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for uint32 type")); - return 1; + return -1; } break; @@ -330,7 +330,7 @@ _printDataType(virNWFilterVarCombIterPtr item->u.u16) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for uint16 type")); - return 1; + return -1; } break; @@ -340,14 +340,14 @@ _printDataType(virNWFilterVarCombIterPtr item->u.u8) >= bufsize) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Buffer too small for uint8 type")); - return 1; + return -1; } break; default: virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Unhandled datatype %x"), item->datatype); - return 1; + return -1; break; } @@ -417,7 +417,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns if (VIR_ALLOC(inst) < 0) { virReportOOMError(); - return 1; + return -1; } inst->commandTemplate = commandTemplate; @@ -442,7 +442,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, if (HAS_ENTRY_ITEM(ðHdr->dataSrcMACAddr)) { if (printDataType(vars, macaddr, sizeof(macaddr), - ðHdr->dataSrcMACAddr)) + ðHdr->dataSrcMACAddr) < 0) goto err_exit; virBufferAsprintf(buf, @@ -454,7 +454,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, if (HAS_ENTRY_ITEM(ðHdr->dataSrcMACMask)) { if (printDataType(vars, macaddr, sizeof(macaddr), - ðHdr->dataSrcMACMask)) + ðHdr->dataSrcMACMask) < 0) goto err_exit; virBufferAsprintf(buf, @@ -466,7 +466,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, if (HAS_ENTRY_ITEM(ðHdr->dataDstMACAddr)) { if (printDataType(vars, macaddr, sizeof(macaddr), - ðHdr->dataDstMACAddr)) + ðHdr->dataDstMACAddr) < 0) goto err_exit; virBufferAsprintf(buf, @@ -478,7 +478,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, if (HAS_ENTRY_ITEM(ðHdr->dataDstMACMask)) { if (printDataType(vars, macaddr, sizeof(macaddr), - ðHdr->dataDstMACMask)) + ðHdr->dataDstMACMask) < 0) goto err_exit; virBufferAsprintf(buf, @@ -492,7 +492,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, err_exit: virBufferFreeAndReset(buf); - return 1; + return -1; } @@ -895,7 +895,7 @@ iptablesHandleSrcMacAddr(virBufferPtr bu if (printDataType(vars, macaddr, sizeof(macaddr), - srcMacAddr)) + srcMacAddr) < 0) goto err_exit; virBufferAsprintf(buf, @@ -909,7 +909,7 @@ iptablesHandleSrcMacAddr(virBufferPtr bu err_exit: virBufferFreeAndReset(buf); - return 1; + return -1; } @@ -940,7 +940,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataSrcIPAddr)) + &ipHdr->dataSrcIPAddr) < 0) goto err_exit; virBufferAsprintf(buf, @@ -953,7 +953,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, number, sizeof(number), - &ipHdr->dataSrcIPMask)) + &ipHdr->dataSrcIPMask) < 0) goto err_exit; virBufferAsprintf(buf, @@ -964,7 +964,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataSrcIPFrom)) + &ipHdr->dataSrcIPFrom) < 0) goto err_exit; virBufferAsprintf(buf, @@ -977,7 +977,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataSrcIPTo)) + &ipHdr->dataSrcIPTo) < 0) goto err_exit; virBufferAsprintf(buf, @@ -990,7 +990,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataDstIPAddr)) + &ipHdr->dataDstIPAddr) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1003,7 +1003,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, number, sizeof(number), - &ipHdr->dataDstIPMask)) + &ipHdr->dataDstIPMask) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1015,7 +1015,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataDstIPFrom)) + &ipHdr->dataDstIPFrom) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1028,7 +1028,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, ipaddr, sizeof(ipaddr), - &ipHdr->dataDstIPTo)) + &ipHdr->dataDstIPTo) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1041,7 +1041,7 @@ iptablesHandleIpHdr(virBufferPtr buf, if (printDataType(vars, number, sizeof(number), - &ipHdr->dataDSCP)) + &ipHdr->dataDSCP) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1057,7 +1057,7 @@ iptablesHandleIpHdr(virBufferPtr buf, } else { if (printDataType(vars, number, sizeof(number), - &ipHdr->dataConnlimitAbove)) + &ipHdr->dataConnlimitAbove) < 0) goto err_exit; /* place connlimit after potential -m state --state ... @@ -1085,7 +1085,7 @@ err_exit: virBufferFreeAndReset(buf); virBufferFreeAndReset(afterStateMatch); - return 1; + return -1; } @@ -1106,7 +1106,7 @@ iptablesHandlePortData(virBufferPtr buf, if (HAS_ENTRY_ITEM(&portData->dataSrcPortStart)) { if (printDataType(vars, portstr, sizeof(portstr), - &portData->dataSrcPortStart)) + &portData->dataSrcPortStart) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1118,7 +1118,7 @@ iptablesHandlePortData(virBufferPtr buf, if (HAS_ENTRY_ITEM(&portData->dataSrcPortEnd)) { if (printDataType(vars, portstr, sizeof(portstr), - &portData->dataSrcPortEnd)) + &portData->dataSrcPortEnd) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1130,7 +1130,7 @@ iptablesHandlePortData(virBufferPtr buf, if (HAS_ENTRY_ITEM(&portData->dataDstPortStart)) { if (printDataType(vars, portstr, sizeof(portstr), - &portData->dataDstPortStart)) + &portData->dataDstPortStart) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1142,7 +1142,7 @@ iptablesHandlePortData(virBufferPtr buf, if (HAS_ENTRY_ITEM(&portData->dataDstPortEnd)) { if (printDataType(vars, portstr, sizeof(portstr), - &portData->dataDstPortEnd)) + &portData->dataDstPortEnd) < 0) goto err_exit; virBufferAsprintf(buf, @@ -1154,7 +1154,7 @@ iptablesHandlePortData(virBufferPtr buf, return 0; err_exit: - return 1; + return -1; } @@ -1244,7 +1244,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.tcpHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1253,7 +1253,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.tcpHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; if (HAS_ENTRY_ITEM(&rule->p.tcpHdrFilter.dataTCPFlags)) { @@ -1268,13 +1268,13 @@ _iptablesCreateRuleInstance(int directio if (iptablesHandlePortData(&buf, vars, &rule->p.tcpHdrFilter.portData, - directionIn)) + directionIn) < 0) goto err_exit; if (HAS_ENTRY_ITEM(&rule->p.tcpHdrFilter.dataTCPOption)) { if (printDataType(vars, number, sizeof(number), - &rule->p.tcpHdrFilter.dataTCPOption)) + &rule->p.tcpHdrFilter.dataTCPOption) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -1299,7 +1299,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.udpHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1308,13 +1308,13 @@ _iptablesCreateRuleInstance(int directio &rule->p.udpHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; if (iptablesHandlePortData(&buf, vars, &rule->p.udpHdrFilter.portData, - directionIn)) + directionIn) < 0) goto err_exit; break; @@ -1332,7 +1332,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.udpliteHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1341,7 +1341,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.udpliteHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; break; @@ -1360,7 +1360,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.espHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1369,7 +1369,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.espHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; break; @@ -1388,7 +1388,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.ahHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1397,7 +1397,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.ahHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; break; @@ -1416,7 +1416,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.sctpHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1425,13 +1425,13 @@ _iptablesCreateRuleInstance(int directio &rule->p.sctpHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; if (iptablesHandlePortData(&buf, vars, &rule->p.sctpHdrFilter.portData, - directionIn)) + directionIn) < 0) goto err_exit; break; @@ -1452,7 +1452,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.icmpHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1461,7 +1461,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.icmpHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; if (HAS_ENTRY_ITEM(&rule->p.icmpHdrFilter.dataICMPType)) { @@ -1479,7 +1479,7 @@ _iptablesCreateRuleInstance(int directio if (printDataType(vars, number, sizeof(number), - &rule->p.icmpHdrFilter.dataICMPType)) + &rule->p.icmpHdrFilter.dataICMPType) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -1491,7 +1491,7 @@ _iptablesCreateRuleInstance(int directio if (HAS_ENTRY_ITEM(&rule->p.icmpHdrFilter.dataICMPCode)) { if (printDataType(vars, number, sizeof(number), - &rule->p.icmpHdrFilter.dataICMPCode)) + &rule->p.icmpHdrFilter.dataICMPCode) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -1514,7 +1514,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.igmpHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1523,7 +1523,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.igmpHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; break; @@ -1542,7 +1542,7 @@ _iptablesCreateRuleInstance(int directio vars, &rule->p.allHdrFilter.dataSrcMACAddr, directionIn, - &srcMacSkipped)) + &srcMacSkipped) < 0) goto err_exit; if (iptablesHandleIpHdr(&buf, @@ -1551,7 +1551,7 @@ _iptablesCreateRuleInstance(int directio &rule->p.allHdrFilter.ipHdr, directionIn, &skipRule, &skipMatch, - &prefix)) + &prefix) < 0) goto err_exit; break; @@ -1664,7 +1664,7 @@ printStateMatchFlags(int32_t flags, char if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return 1; + return -1; } *bufptr = virBufferContentAndReset(&buf); return 0; @@ -1704,8 +1704,8 @@ iptablesCreateRuleInstanceStateCtrl(virN } if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState)) - return 1; + if (printStateMatchFlags(rule->flags, &matchState) < 0) + return -1; } chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; @@ -1723,7 +1723,7 @@ iptablesCreateRuleInstanceStateCtrl(virN maySkipICMP); VIR_FREE(matchState); - if (rc) + if (rc < 0) return rc; } @@ -1736,8 +1736,8 @@ iptablesCreateRuleInstanceStateCtrl(virN } if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState)) - return 1; + if (printStateMatchFlags(rule->flags, &matchState) < 0) + return -1; } chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; @@ -1756,7 +1756,7 @@ iptablesCreateRuleInstanceStateCtrl(virN VIR_FREE(matchState); - if (rc) + if (rc < 0) return rc; } @@ -1769,8 +1769,8 @@ iptablesCreateRuleInstanceStateCtrl(virN create = false; } else { if ((rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState)) - return 1; + if (printStateMatchFlags(rule->flags, &matchState) < 0) + return -1; } } @@ -1852,7 +1852,7 @@ iptablesCreateRuleInstance(virNWFilterDe "RETURN", isIPv6, maySkipICMP); - if (rc) + if (rc < 0) return rc; @@ -1874,7 +1874,7 @@ iptablesCreateRuleInstance(virNWFilterDe "ACCEPT", isIPv6, maySkipICMP); - if (rc) + if (rc < 0) return rc; maySkipICMP = directionIn; @@ -1963,13 +1963,13 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.ethHdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; if (HAS_ENTRY_ITEM(&rule->p.ethHdrFilter.dataProtocolID)) { if (printDataTypeAsHex(vars, number, sizeof(number), - &rule->p.ethHdrFilter.dataProtocolID)) + &rule->p.ethHdrFilter.dataProtocolID) < 0) goto err_exit; virBufferAsprintf(&buf, " -p %s %s", @@ -1988,7 +1988,7 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.vlanHdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; virBufferAddLit(&buf, @@ -1998,7 +1998,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM)) { \ if (printDataType(vars, \ field, sizeof(field), \ - &rule->p.STRUCT.ITEM)) \ + &rule->p.STRUCT.ITEM) < 0) \ goto err_exit; \ virBufferAsprintf(&buf, \ " " CLI " %s %s", \ @@ -2010,7 +2010,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM)) { \ if (printDataType(vars, \ field, sizeof(field), \ - &rule->p.STRUCT.ITEM)) \ + &rule->p.STRUCT.ITEM) < 0) \ goto err_exit; \ virBufferAsprintf(&buf, \ " " CLI " %s %s", \ @@ -2019,7 +2019,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM_HI)) { \ if (printDataType(vars, \ field, sizeof(field), \ - &rule->p.STRUCT.ITEM_HI)) \ + &rule->p.STRUCT.ITEM_HI) < 0) \ goto err_exit; \ virBufferAsprintf(&buf, SEP "%s", field); \ } \ @@ -2055,7 +2055,7 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.stpHdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; virBufferAddLit(&buf, " -d " NWFILTER_MAC_BGA); @@ -2092,7 +2092,7 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.arpHdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; virBufferAsprintf(&buf, " -p 0x%x", @@ -2103,7 +2103,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataHWType)) { if (printDataType(vars, number, sizeof(number), - &rule->p.arpHdrFilter.dataHWType)) + &rule->p.arpHdrFilter.dataHWType) < 0) goto err_exit; virBufferAsprintf(&buf, " --arp-htype %s %s", @@ -2114,7 +2114,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataOpcode)) { if (printDataType(vars, number, sizeof(number), - &rule->p.arpHdrFilter.dataOpcode)) + &rule->p.arpHdrFilter.dataOpcode) < 0) goto err_exit; virBufferAsprintf(&buf, " --arp-opcode %s %s", @@ -2125,7 +2125,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataProtocolType)) { if (printDataTypeAsHex(vars, number, sizeof(number), - &rule->p.arpHdrFilter.dataProtocolType)) + &rule->p.arpHdrFilter.dataProtocolType) < 0) goto err_exit; virBufferAsprintf(&buf, " --arp-ptype %s %s", @@ -2136,7 +2136,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPSrcIPAddr)) { if (printDataType(vars, ipaddr, sizeof(ipaddr), - &rule->p.arpHdrFilter.dataARPSrcIPAddr)) + &rule->p.arpHdrFilter.dataARPSrcIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2149,7 +2149,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPDstIPAddr)) { if (printDataType(vars, ipaddr, sizeof(ipaddr), - &rule->p.arpHdrFilter.dataARPDstIPAddr)) + &rule->p.arpHdrFilter.dataARPDstIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2162,7 +2162,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPSrcMACAddr)) { if (printDataType(vars, macaddr, sizeof(macaddr), - &rule->p.arpHdrFilter.dataARPSrcMACAddr)) + &rule->p.arpHdrFilter.dataARPSrcMACAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2175,7 +2175,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.arpHdrFilter.dataARPDstMACAddr)) { if (printDataType(vars, macaddr, sizeof(macaddr), - &rule->p.arpHdrFilter.dataARPDstMACAddr)) + &rule->p.arpHdrFilter.dataARPDstMACAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2201,7 +2201,7 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.ipHdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; virBufferAddLit(&buf, @@ -2210,7 +2210,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr)) { if (printDataType(vars, ipaddr, sizeof(ipaddr), - &rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr)) + &rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2222,7 +2222,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.ipHdr.dataSrcIPMask)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.ipHdr.dataSrcIPMask)) + &rule->p.ipHdrFilter.ipHdr.dataSrcIPMask) + < 0) goto err_exit; virBufferAsprintf(&buf, "/%s", @@ -2234,7 +2235,7 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, ipaddr, sizeof(ipaddr), - &rule->p.ipHdrFilter.ipHdr.dataDstIPAddr)) + &rule->p.ipHdrFilter.ipHdr.dataDstIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2246,7 +2247,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.ipHdr.dataDstIPMask)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.ipHdr.dataDstIPMask)) + &rule->p.ipHdrFilter.ipHdr.dataDstIPMask) + < 0) goto err_exit; virBufferAsprintf(&buf, "/%s", @@ -2257,7 +2259,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.ipHdr.dataProtocolID)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.ipHdr.dataProtocolID)) + &rule->p.ipHdrFilter.ipHdr.dataProtocolID) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2270,7 +2272,8 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.portData.dataSrcPortStart)) + &rule->p.ipHdrFilter.portData.dataSrcPortStart) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2282,7 +2285,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.portData.dataSrcPortEnd)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.portData.dataSrcPortEnd)) + &rule->p.ipHdrFilter.portData.dataSrcPortEnd) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2295,7 +2299,8 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.portData.dataDstPortStart)) + &rule->p.ipHdrFilter.portData.dataDstPortStart) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2307,7 +2312,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.portData.dataDstPortEnd)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipHdrFilter.portData.dataDstPortEnd)) + &rule->p.ipHdrFilter.portData.dataDstPortEnd) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2319,7 +2325,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipHdrFilter.ipHdr.dataDSCP)) { if (printDataTypeAsHex(vars, number, sizeof(number), - &rule->p.ipHdrFilter.ipHdr.dataDSCP)) + &rule->p.ipHdrFilter.ipHdr.dataDSCP) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2337,7 +2343,7 @@ ebtablesCreateRuleInstance(char chainPre if (ebtablesHandleEthHdr(&buf, vars, &rule->p.ipv6HdrFilter.ethHdr, - reverse)) + reverse) < 0) goto err_exit; virBufferAddLit(&buf, @@ -2346,7 +2352,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr)) { if (printDataType(vars, ipv6addr, sizeof(ipv6addr), - &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr)) + &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2358,7 +2364,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask)) + &rule->p.ipv6HdrFilter.ipHdr.dataSrcIPMask) + < 0) goto err_exit; virBufferAsprintf(&buf, "/%s", @@ -2370,7 +2377,7 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, ipv6addr, sizeof(ipv6addr), - &rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr)) + &rule->p.ipv6HdrFilter.ipHdr.dataDstIPAddr) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2382,7 +2389,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask)) + &rule->p.ipv6HdrFilter.ipHdr.dataDstIPMask) + < 0) goto err_exit; virBufferAsprintf(&buf, "/%s", @@ -2393,7 +2401,7 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.ipHdr.dataProtocolID)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.ipHdr.dataProtocolID)) + &rule->p.ipv6HdrFilter.ipHdr.dataProtocolID) < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2406,7 +2414,8 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.portData.dataSrcPortStart)) + &rule->p.ipv6HdrFilter.portData.dataSrcPortStart) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2418,7 +2427,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.portData.dataSrcPortEnd)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.portData.dataSrcPortEnd)) + &rule->p.ipv6HdrFilter.portData.dataSrcPortEnd) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2431,7 +2441,8 @@ ebtablesCreateRuleInstance(char chainPre if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.portData.dataDstPortStart)) + &rule->p.ipv6HdrFilter.portData.dataDstPortStart) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2443,7 +2454,8 @@ ebtablesCreateRuleInstance(char chainPre if (HAS_ENTRY_ITEM(&rule->p.ipv6HdrFilter.portData.dataDstPortEnd)) { if (printDataType(vars, number, sizeof(number), - &rule->p.ipv6HdrFilter.portData.dataDstPortEnd)) + &rule->p.ipv6HdrFilter.portData.dataDstPortEnd) + < 0) goto err_exit; virBufferAsprintf(&buf, @@ -2510,7 +2522,7 @@ err_exit: * Convert a single rule into its representation for later instantiation * * Returns 0 in case of success with the result stored in the data structure - * pointed to by res, != 0 otherwise. + * pointed to by res, -1 otherwise */ static int ebiptablesCreateRuleInstance(enum virDomainNetType nettype ATTRIBUTE_UNUSED, @@ -2542,7 +2554,7 @@ ebiptablesCreateRuleInstance(enum virDom vars, res, rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT); - if (rc) + if (rc < 0) return rc; } @@ -2596,7 +2608,7 @@ ebiptablesCreateRuleInstance(enum virDom case VIR_NWFILTER_RULE_PROTOCOL_LAST: virNWFilterReportError(VIR_ERR_OPERATION_FAILED, "%s", _("illegal protocol type")); - rc = 1; + rc = -1; break; } @@ -2621,7 +2633,7 @@ ebiptablesCreateRuleInstanceIterate( */ vciter = virNWFilterVarCombIterCreate(vars, rule->vars, rule->nvars); if (!vciter) - return 1; + return -1; do { rc = ebiptablesCreateRuleInstance(nettype, @@ -2630,7 +2642,7 @@ ebiptablesCreateRuleInstanceIterate( ifname, vciter, res); - if (rc) + if (rc < 0) break; vciter = virNWFilterVarCombIterNext(vciter); } while (vciter != NULL); @@ -3111,7 +3123,7 @@ ebtablesApplyBasicRules(const char *ifna virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1; } virFormatMacAddr(macaddr, macaddr_str); @@ -3170,7 +3182,7 @@ tear_down_tmpebchains: "%s", _("Some rules could not be created.")); - return 1; + return -1; } @@ -3186,7 +3198,7 @@ tear_down_tmpebchains: * names (true) or also perform the renaming to their final names as * part of this call (false) * - * Returns 0 on success, 1 on failure with the rules removed + * Returns 0 on success, -1 on failure with the rules removed * * Apply filtering rules so that the VM can only send and receive * DHCP traffic and nothing else. @@ -3207,13 +3219,15 @@ ebtablesApplyDHCPOnlyRules(const char *i virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1; } if (dhcpserver) { virBufferAsprintf(&buf, " --ip-src %s", dhcpserver); - if (virBufferError(&buf)) - return 1; + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } srcIPParam = virBufferContentAndReset(&buf); } @@ -3298,7 +3312,7 @@ tear_down_tmpebchains: VIR_FREE(srcIPParam); - return 1; + return -1; } @@ -3307,7 +3321,7 @@ tear_down_tmpebchains: * * @ifname: name of the backend-interface to which to apply the rules * - * Returns 0 on success, 1 on failure with the rules removed + * Returns 0 on success, -1 on failure with the rules removed * * Apply filtering rules so that the VM cannot receive or send traffic. */ @@ -3322,7 +3336,7 @@ ebtablesApplyDropAllRules(const char *if virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1; } ebiptablesAllTeardown(ifname); @@ -3368,7 +3382,7 @@ tear_down_tmpebchains: "%s", _("Some rules could not be created.")); - return 1; + return -1; } @@ -3575,13 +3589,13 @@ ebiptablesApplyNewRules(const char *ifna const char *name = inst[i]->neededProtocolChain; if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) { if (virHashUpdateEntry(chains_in_set, name, - &inst[i]->chainPriority)) { + &inst[i]->chainPriority) < 0) { virReportOOMError(); goto exit_free_sets; } } else { if (virHashUpdateEntry(chains_out_set, name, - &inst[i]->chainPriority)) { + &inst[i]->chainPriority) < 0) { virReportOOMError(); goto exit_free_sets; } @@ -3606,9 +3620,9 @@ ebiptablesApplyNewRules(const char *ifna /* create needed chains */ if (ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_in_set , 1, - &ebtChains, &nEbtChains) || + &ebtChains, &nEbtChains) < 0 || ebtablesCreateTmpRootAndSubChains(&buf, ifname, chains_out_set, 0, - &ebtChains, &nEbtChains)) { + &ebtChains, &nEbtChains) < 0) { goto tear_down_tmpebchains; } @@ -3809,7 +3823,7 @@ exit_free_sets: VIR_FREE(errmsg); - return 1; + return -1; } @@ -3905,7 +3919,7 @@ ebiptablesTearOldRules(const char *ifnam * * Remove all rules one after the other * - * Return 0 on success, 1 if execution of one or more cleanup + * Return 0 on success, -1 if execution of one or more cleanup * commands failed. */ static int @@ -3927,14 +3941,14 @@ ebiptablesRemoveRules(const char *ifname 'D', -1, 0); - if (ebiptablesExecCLI(&buf, &cli_status, NULL)) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) < 0) goto err_exit; if (cli_status) { virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, "%s", _("error while executing CLI commands")); - rc = 1; + rc = -1; } err_exit: @@ -4022,8 +4036,8 @@ ebiptablesDriverInit(bool privileged) if (!privileged) return 0; - if (virMutexInit(&execCLIMutex)) - return EINVAL; + if (virMutexInit(&execCLIMutex) < 0) + return -EINVAL; gawk_cmd_path = virFindFileInPath("gawk"); grep_cmd_path = virFindFileInPath("grep"); @@ -4086,7 +4100,7 @@ ebiptablesDriverInit(bool privileged) _("firewall tools were not found or " "cannot be used")); ebiptablesDriverShutdown(); - return ENOTSUP; + return -ENOTSUP; } ebiptables_driver.flags = TECHDRV_FLAG_INITIALIZED; Index: libvirt-acl/src/uml/uml_conf.c =================================================================== --- libvirt-acl.orig/src/uml/uml_conf.c +++ libvirt-acl/src/uml/uml_conf.c @@ -143,7 +143,7 @@ umlConnectTapDevice(virConnectPtr conn, } if (net->filter) { - if (virDomainConfNWFilterInstantiate(conn, net)) { + if (virDomainConfNWFilterInstantiate(conn, net) < 0) { if (template_ifname) VIR_FREE(net->ifname); goto error; Index: libvirt-acl/src/qemu/qemu_process.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_process.c +++ libvirt-acl/src/qemu/qemu_process.c @@ -2321,7 +2321,7 @@ qemuProcessFiltersInstantiate(virConnect for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; if ((net->filter) && (net->ifname)) { - if (virDomainConfNWFilterInstantiate(conn, net)) { + if (virDomainConfNWFilterInstantiate(conn, net) < 0) { err = 1; break; } Index: libvirt-acl/src/qemu/qemu_command.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_command.c +++ libvirt-acl/src/qemu/qemu_command.c @@ -275,8 +275,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr if (tapfd >= 0) { if ((net->filter) && (net->ifname)) { - err = virDomainConfNWFilterInstantiate(conn, net); - if (err) + if (virDomainConfNWFilterInstantiate(conn, net) < 0); VIR_FORCE_CLOSE(tapfd); } }
On 11/23/2011 02:19 PM, Stefan Berger wrote:
This patch cleans up return codes in the nwfilter subsystem.
Some functions in nwfilter_conf.c (validators and formatters) are keeping their bool return for now and I am converting their return code to true/false.
All other functions now return -1 on failure and 0 on success.
[I searched for all occurences of ' 1;' and checked all 'if ' and adapted where needed. After that I did a grep for 'NWFilter' in the source tree.]
---
I compile tested this, and assume you also ran it through libvirt-TCK. My compile run complained: qemu/qemu_command.c: In function 'qemuNetworkIfaceConnect': qemu/qemu_command.c:278:65: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body] [more on this below]
-static bool +static int intMapGetByInt(const struct int_map *intmap, int32_t attr, const char **res) { int i = 0; - bool found = 0; + int found = false;
This should still be 'bool found', given how you assign bool values to it.
-static bool +static int intMapGetByString(const struct int_map *intmap, const char *str, int casecmp, int32_t *result)
Conversion of intMapGetBy{Int,String} is correct, including all callers. (I'm not sure if this would have been easier broken up into multiple patches, a couple functions per patch, but don't bother splitting it now).
@@ -367,14 +369,14 @@ virNWFilterRuleDefAddVar(virNWFilterRule
if (VIR_REALLOC_N(nwf->vars, nwf->nvars+1) < 0) { virReportOOMError(); - return 1; + return -1;
Conversion of virNWFilterRuleDefAddVar is correct, including all callers.
@@ -805,7 +808,7 @@ parseStringItems(const struct int_map *i } } if (!found) { - rc = 1; + rc = -1;
You still have two callers to parseStringItems that merely checked for non-zero, rather than < 0 (in tcpFlagsValidator), but no logic regression introduced.
@@ -1663,13 +1666,11 @@ static const virAttributes virAttr[] = { };
-static bool +static int virNWMACAddressParser(const char *input, nwMACAddressPtr output) { - if (virParseMacAddr(input, &output->addr[0]) == 0) - return 1; - return 0; + return virParseMacAddr(input, &output->addr[0]);
Conversion of virNWMACAddressParser is correct.
@@ -2649,7 +2650,7 @@ _virNWFilterDefLoopDetect(virConnectPtr if (entry->include) {
if (STREQ(filtername, entry->include->filterref)) { - rc = 1; + rc = -1;
conversion of [_]virNWFilterDefLoopDetect and callers looks correct.
@@ -2736,7 +2737,7 @@ virNWFilterTriggerVMFilterRebuild(virCon };
if (!cb.skipInterfaces) - return 1; + return -1;
for (i = 0; i < nCallbackDriver; i++) { callbackDrvArray[i]->vmFilterRebuild(conn,
This one's tricky - you have err = cb.err, but I didn't check whether all callers to virNWFilterConfLayerInit() pass a function that sets cb.err to -1, or if you still need more cleanup.
@@ -2778,7 +2779,7 @@ virNWFilterTestUnassignDef(virConnectPtr nwfilter->wantRemoved = 1; /* trigger the update on VMs referencing the filter */ if (virNWFilterTriggerVMFilterRebuild(conn)) - rc = 1; + rc = -1;
Conversion of virNWFilterTestUnassignDef and callers looks correct.
@@ -3297,8 +3298,8 @@ int virNWFilterConfLayerInit(virHashIter
initialized = true;
- if (virMutexInitRecursive(&updateMutex)) - return 1; + if (virMutexInitRecursive(&updateMutex) < 0) + return -1;
Conversion of virNWFilterConfLayerInit() and callers looks correct.
return 0; } Index: libvirt-acl/src/nwfilter/nwfilter_driver.c
This is as far as I got today. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/23/2011 03:49 PM, Eric Blake wrote:
On 11/23/2011 02:19 PM, Stefan Berger wrote:
This patch cleans up return codes in the nwfilter subsystem.
Some functions in nwfilter_conf.c (validators and formatters) are keeping their bool return for now and I am converting their return code to true/false.
All other functions now return -1 on failure and 0 on success.
I compile tested this, and assume you also ran it through libvirt-TCK. My compile run complained:
qemu/qemu_command.c: In function 'qemuNetworkIfaceConnect': qemu/qemu_command.c:278:65: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
The quickie fix: diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 4310a62..0220ac3 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -275,7 +275,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (tapfd >= 0) { if ((net->filter) && (net->ifname)) { - if (virDomainConfNWFilterInstantiate(conn, net) < 0); + if (virDomainConfNWFilterInstantiate(conn, net) < 0) VIR_FORCE_CLOSE(tapfd); } }
Index: libvirt-acl/src/nwfilter/nwfilter_driver.c
This is as far as I got today.
Alas, I don't have time to resume this today; it will probably be Monday before I can audit the rest of the patch for sanity. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/23/2011 02:19 PM, Stefan Berger wrote:
This patch cleans up return codes in the nwfilter subsystem.
Some functions in nwfilter_conf.c (validators and formatters) are keeping their bool return for now and I am converting their return code to true/false.
All other functions now return -1 on failure and 0 on success.
[I searched for all occurences of ' 1;' and checked all 'if ' and adapted where needed. After that I did a grep for 'NWFilter' in the source tree.]
Resuming where I left off last time...
Index: libvirt-acl/src/conf/nwfilter_params.c
@@ -505,25 +505,25 @@ virNWFilterHashTablePut(virNWFilterHashT if (copyName) { name = strdup(name); if (!name) - return 1; + return -1;
virNWFilterDetermineMissingVarsRec() has an unchecked call to this function, meaning that an OOM error can go undetected in that call site. All other calls to this function look sanely converted.
@@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHa return 0;
err_exit: - return 1; + return -1;
All callers look sanely converted.
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -106,7 +106,7 @@ virNWFilterRuleInstAddData(virNWFilterRu
Comment prior to the function needs an update.
{ if (VIR_REALLOC_N(res->data, res->ndata+1) < 0) { virReportOOMError(); - return 1; + return -1;
All callers look sanely converted.
@@ -151,28 +151,28 @@ virNWFilterVarHashmapAddStdValues(virNWF if (macaddr) { val = virNWFilterVarValueCreateSimple(macaddr); if (!val) - return 1; + return -1;
Comment prior to the function needs an update. All callers correctly converted.
@@ -404,13 +404,13 @@ _virNWFilterInstantiateRec(virNWFilterTe ifname, vars); if (!inst) { - rc = 1; + rc = -1;
Looks sane.
@@ -504,7 +504,7 @@ virNWFilterDetermineMissingVarsRec(virNW if (!virHashLookup(vars->hashTable, rule->vars[j])) { val = virNWFilterVarValueCreateSimpleCopyValue("1"); if (!val) { - rc = 1; + rc = -1;
Looks sane.
@@ -592,7 +592,7 @@ virNWFilterRuleInstancesToArray(int nEnt
if (VIR_ALLOC_N((*ptrs), (*nptrs)) < 0) { virReportOOMError(); - return 1; + return -1;
Looks sane.
@@ -649,7 +649,7 @@ virNWFilterInstantiate(virNWFilterTechDr virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { virReportOOMError(); - rc = 1; + rc = -1;
This one calls through a callback that I did not audit: rc = techdriver->applyNewRules(ifname, nptrs, ptrs); but assuming the callback is okay (or that you change things to ensure rc is -1 even if the callback returns positive), then clients of this look okay.
@@ -792,7 +792,7 @@ __virNWFilterInstantiateFilter(bool tear _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1;
Ultimately called via virNWFilterInstantiateFilterLate, in turn called by nwfilter_learnipaddr.c:learnIPAddressThread, which collects the return value and prints it in a VIR_DEBUG but does nothing if the value was negative. Is that a problem? Also called by the .instantiateFilter callback installed by nwfilter_driver.c, which feeds virDomainConfNWFilterInstantiate, and all callers look clean.
@@ -1012,7 +1012,7 @@ int virNWFilterRollbackUpdateFilter(cons _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1;
Should this function be static? No one outside of gentech_driver calls it. At any rate, callers inside the file are sane.
@@ -1038,7 +1038,7 @@ virNWFilterTearOldFilter(virDomainNetDef _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1;
Same comments about static, and sane usage.
@@ -1063,13 +1063,13 @@ _virNWFilterTeardownFilter(const char *i _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1;
Doesn't strictly matter (the ultimate caller nwfilterTeardownFilter returns void), but might as well be consistent.
Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c
@@ -184,7 +184,7 @@ virNWFilterLockIface(const char *ifname) err_exit: virMutexUnlock(&ifaceMapLock);
- return 1; + return -1;
Looks sane.
@@ -248,7 +248,7 @@ virNWFilterRegisterLearnReq(virNWFilterI
int virNWFilterTerminateLearnReq(const char *ifname) { - int rc = 1; + int rc = -1;
The only caller (in gentech_driver) doesn't pay attention to the return value; but that's probably okay. Looks sane.
@@ -336,9 +336,6 @@ virNWFilterAddIpAddrForIfname(const char goto cleanup; } ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); - /* FIXME: fix when return code of virNWFilterHashTablePut changes */ - if (ret) - ret = -1;
Always fun to get rid of a FIXME.
@@ -530,7 +527,7 @@ learnIPAddressThread(void *arg) break; default: if (techdriver->applyBasicRules(req->ifname, - req->macaddr)) { + req->macaddr) < 0) {
I didn't audit where this callback came from, but assume it was okay.
@@ -781,14 +778,14 @@ virNWFilterLearnIPAddress(virNWFilterTec virNWFilterHashTablePtr ht = NULL;
if (howDetect == 0) - return 1; + return -1;
Looks sane.
@@ -895,35 +892,35 @@ virNWFilterLearnInit(void) {
pendingLearnReq = virHashCreate(0, freeLearnReqEntry); if (!pendingLearnReq) { - return 1; + return -1;
Looks sane.
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -233,15 +233,15 @@ printVar(virNWFilterVarCombIterPtr vars, val = virNWFilterVarCombIterGetVarValue(vars, item->var); if (!val) { /* error has been reported */ - return 1; + return -1;
Looks sane.
@@ -259,8 +259,8 @@ _printDataType(virNWFilterVarCombIterPtr int done; char *data;
- if (printVar(vars, buf, bufsize, item, &done)) - return 1; + if (printVar(vars, buf, bufsize, item, &done) < 0) + return -1;
Looks sane.
@@ -417,7 +417,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns
if (VIR_ALLOC(inst) < 0) { virReportOOMError(); - return 1; + return -1;
Looks sane.
@@ -492,7 +492,7 @@ ebtablesHandleEthHdr(virBufferPtr buf, err_exit: virBufferFreeAndReset(buf);
- return 1; + return -1;
Looks sane.
@@ -909,7 +909,7 @@ iptablesHandleSrcMacAddr(virBufferPtr bu err_exit: virBufferFreeAndReset(buf);
- return 1; + return -1;
Looks sane.
@@ -1057,7 +1057,7 @@ iptablesHandleIpHdr(virBufferPtr buf, @@ -1085,7 +1085,7 @@ err_exit: virBufferFreeAndReset(buf); virBufferFreeAndReset(afterStateMatch);
- return 1; + return -1;
Looks sane.
@@ -1154,7 +1154,7 @@ iptablesHandlePortData(virBufferPtr buf, return 0;
err_exit: - return 1; + return -1;
Looks sane.
@@ -1664,7 +1664,7 @@ printStateMatchFlags(int32_t flags, char if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return 1; + return -1;
Looks sane.
@@ -1704,8 +1704,8 @@ iptablesCreateRuleInstanceStateCtrl(virN }
if (create && (rule->flags & IPTABLES_STATE_FLAGS)) { - if (printStateMatchFlags(rule->flags, &matchState)) - return 1; + if (printStateMatchFlags(rule->flags, &matchState) < 0) + return -1;
Looks sane.
@@ -2542,7 +2554,7 @@ ebiptablesCreateRuleInstance(enum virDom vars, res, rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT); - if (rc) + if (rc < 0) return rc;
Looks sane.
@@ -3111,7 +3123,7 @@ ebtablesApplyBasicRules(const char *ifna virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1;
Comment before function needs touch up. Otherwise looks sane.
@@ -3207,13 +3219,15 @@ ebtablesApplyDHCPOnlyRules(const char *i virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1;
Looks sane, although I didn't closely audit if all uses of the ebiptables_driver tech callbacks were adjusted.
@@ -3322,7 +3336,7 @@ ebtablesApplyDropAllRules(const char *if virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1;
Looks sane.
@@ -4086,7 +4100,7 @@ ebiptablesDriverInit(bool privileged) _("firewall tools were not found or " "cannot be used")); ebiptablesDriverShutdown(); - return ENOTSUP; + return -ENOTSUP;
Looks sane. Overall, I found one or two nits between my two-stage review, but they seemed easy to fix. ACK. Up to you if you want to post a delta to this patch for your touchups, or just go ahead and commit, since I know you have other patches backed up behind this one. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/08/2011 06:19 PM, Eric Blake wrote:
On 11/23/2011 02:19 PM, Stefan Berger wrote:
This patch cleans up return codes in the nwfilter subsystem. [...] Resuming where I left off last time...
Index: libvirt-acl/src/conf/nwfilter_params.c @@ -505,25 +505,25 @@ virNWFilterHashTablePut(virNWFilterHashT if (copyName) { name = strdup(name); if (!name) - return 1; + return -1; virNWFilterDetermineMissingVarsRec() has an unchecked call to this function, meaning that an OOM error can go undetected in that call site.
putting a virReportOOMError() into this one...
All other calls to this function look sanely converted.
@@ -640,7 +640,7 @@ virNWFilterHashTablePutAll(virNWFilterHa return 0;
err_exit: - return 1; + return -1; All callers look sanely converted.
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -106,7 +106,7 @@ virNWFilterRuleInstAddData(virNWFilterRu Comment prior to the function needs an update.
Thanks. Fixed.
{ if (VIR_REALLOC_N(res->data, res->ndata+1)< 0) { virReportOOMError(); - return 1; + return -1; All callers look sanely converted.
@@ -151,28 +151,28 @@ virNWFilterVarHashmapAddStdValues(virNWF if (macaddr) { val = virNWFilterVarValueCreateSimple(macaddr); if (!val) - return 1; + return -1; Comment prior to the function needs an update. All callers correctly converted.
Fixed.
@@ -649,7 +649,7 @@ virNWFilterInstantiate(virNWFilterTechDr virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { virReportOOMError(); - rc = 1; + rc = -1; This one calls through a callback that I did not audit: rc = techdriver->applyNewRules(ifname, nptrs, ptrs); ebiptablesApplyNewRules is being called. The whole ebiptables driver has only -1 as failure codes now. So should be ok.
but assuming the callback is okay (or that you change things to ensure rc is -1 even if the callback returns positive), then clients of this look okay.
@@ -792,7 +792,7 @@ __virNWFilterInstantiateFilter(bool tear _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; Ultimately called via virNWFilterInstantiateFilterLate, in turn called by nwfilter_learnipaddr.c:learnIPAddressThread, which collects the return value and prints it in a VIR_DEBUG but does nothing if the value was negative. Is that a problem? Well, if the instantiation for any reason fails, then the code currently 'downs' the interface. I didn't know how else it should react since this
path is invoked in a thread and tearing down the VM would be too severe.
Also called by the .instantiateFilter callback installed by nwfilter_driver.c, which feeds virDomainConfNWFilterInstantiate, and all callers look clean.
@@ -1012,7 +1012,7 @@ int virNWFilterRollbackUpdateFilter(cons _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; Should this function be static? No one outside of gentech_driver calls it. At any rate, callers inside the file are sane.
Yes, indeed. I changed it now to be static.
@@ -1038,7 +1038,7 @@ virNWFilterTearOldFilter(virDomainNetDef _("Could not get access to ACL tech " "driver '%s'"), drvname); - return 1; + return -1; Same comments about static, and sane usage.
Also changed this one.@@ -530,7 +527,7 @@ learnIPAddressThread(void *arg)
break; default: if (techdriver->applyBasicRules(req->ifname, - req->macaddr)) { + req->macaddr)< 0) {
I didn't audit where this callback came from, but assume it was okay.
ebtablesApplyBasicRules now returns -1 in case of failure, so it's ok.
@@ -3111,7 +3123,7 @@ ebtablesApplyBasicRules(const char *ifna virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create rules since ebtables tool is " "missing.")); - return 1; + return -1; Comment before function needs touch up. Otherwise looks sane.
Fixed. Thanks.
@@ -4086,7 +4100,7 @@ ebiptablesDriverInit(bool privileged) _("firewall tools were not found or " "cannot be used")); ebiptablesDriverShutdown(); - return ENOTSUP; + return -ENOTSUP; Looks sane.
Overall, I found one or two nits between my two-stage review, but they seemed easy to fix.
ACK. Up to you if you want to post a delta to this patch for your touchups, or just go ahead and commit, since I know you have other patches backed up behind this one.
I went ahead and pushed the patch. Thanks for this thorough review with all the cross-checking and looking through the function descriptions. Cheers! Stefan
participants (2)
-
Eric Blake -
Stefan Berger