Re: [libvirt] [PATCH 2/3] node device: Break out get_wwns and get_parent_node helpers

> > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index f09f814..77f7be3 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -1215,6 +1215,95 @@ virNodeDeviceDefParseFile(virConnectPtr conn, > Â Â return virNodeDeviceDefParse(conn, NULL, filename, create); > Â } > > +/* > + * Return fc_host dev's WWNN and WWPN > + */ > +int > +virNodeDeviceGetWWNs(virConnectPtr conn, > + Â Â Â Â Â Â Â Â Â Â virNodeDeviceDefPtr def, > + Â Â Â Â Â Â Â Â Â Â char **wwnn, > + Â Â Â Â Â Â Â Â Â Â char **wwpn) > +{ > + Â Â virNodeDevCapsDefPtr cap = NULL; > + Â Â int ret = 0; > + > + Â Â cap = def->caps; > + Â Â while (cap != NULL) { > + Â Â Â Â if (cap->type == VIR_NODE_DEV_CAP_SCSI_HOST && > + Â Â Â Â Â Â cap->data.scsi_host.flags & VIR_NODE_DEV_CAP_FLAG_HBA_FC_HOST) { > + Â Â Â Â Â Â *wwnn = strdup(cap->data.scsi_host.wwnn); > + Â Â Â Â Â Â *wwpn = strdup(cap->data.scsi_host.wwpn); > + Â Â Â Â Â Â break; > + Â Â Â Â } > + > + Â Â Â Â cap = cap->next; > + Â Â } > + > + Â Â if (cap == NULL) { > + Â Â Â Â virNodeDeviceReportError(conn, VIR_ERR_NO_SUPPORT, > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â "%s", _("Device is not a fibre channel HBA")); > + Â Â Â Â ret = -1; > + Â Â } > + > + Â Â if (*wwnn == NULL || *wwpn == NULL) { I know you have just moved this function to another file, but here seems to be a logical error that may result in a false-positive OOM error. If the while loop is left without finding the wanted device (cap == NULL), then *wwnn and *wwpn have not been set inside this function and point to their initial value (probably NULL). Because they have not been set inside this function (in the cap == NULL case), they should not be tested for NULL to detect an OOM condition. To fix this if (*wwnn == NULL || *wwpn == NULL) { could be changed to else if (*wwnn == NULL || *wwpn == NULL) { so the OOM check is only done if the wanted device has been found (cap != NULL). > + Â Â Â Â /* Free the other one, if allocated... */ > + Â Â Â Â VIR_FREE(wwnn); > + Â Â Â Â VIR_FREE(wwpn); > + Â Â Â Â ret = -1; > + Â Â Â Â virReportOOMError(conn); > + Â Â } > + > + Â Â return ret; > +} > + The OOM detection should be fixed by an additional patch, so ACK to this one.