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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list