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

[Cole Robinson]

On 10/17/2009 08:58 AM, Matthias Bolte wrote:
>>
>> 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.
> 

Good catch. I agree a follow up patch is the way to go.

Thanks,
Cole