linux-kernel - Re: [PATCH 2/2] xen/scsiback: avoid warnings when adding multiple LUNs to a domain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 8 Feb 2016 13:28:52 +0100
From:	Juergen Gross <jgross@...e.com>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	linux-kernel@...r.kernel.org, xen-devel@...ts.xen.org,
	konrad.wilk@...cle.com, david.vrabel@...rix.com
Cc:	stable@...r.kernel.org
Subject: Re: [PATCH 2/2] xen/scsiback: avoid warnings when adding multiple
 LUNs to a domain

On 05/02/16 18:24, Boris Ostrovsky wrote:
> 
> 
> On 02/05/2016 11:59 AM, Juergen Gross wrote:
>> On 05/02/16 16:50, Boris Ostrovsky wrote:
>>>
>>> On 02/05/2016 08:21 AM, Juergen Gross wrote:
>>>> When adding more than one LUN to a frontend a warning for a failed
>>>> assignment is issued in dom0 for each already existing LUN. Avoid this
>>>> warning.
>>> Aren't you just factoring out the check? The warning is still printed
>>> for each scsiback_add_translation_entry() invocation, no?
>> I don't call scsiback_add_translation_entry() in the critical case.
> 
> Which is scsiback_do_add_lun()? If yes then perhaps you could mention it
> in the commit message since there are few changes that this patch
> provides and it's not clear which is the one that prevents the warning.
> 
>>
>> @@ -962,33 +973,31 @@ static int scsiback_del_translation_entry(struct
>> vscsibk_info *info,
>>                          struct ids_tuple *v)
>>    {
>>        struct v2p_entry *entry;
>> -    struct list_head *head = &(info->v2p_entry_lists);
>>        unsigned long flags;
>>          spin_lock_irqsave(&info->v2p_lock, flags);
>>        /* Find out the translation entry specified */
>> -    list_for_each_entry(entry, head, l) {
>> -        if ((entry->v.chn == v->chn) &&
>> -            (entry->v.tgt == v->tgt) &&
>> -            (entry->v.lun == v->lun)) {
>> -            goto found;
>> -        }
>> -    }
>> -
>> -    spin_unlock_irqrestore(&info->v2p_lock, flags);
>> -    return 1;
>> -
>> -found:
>> -    /* Delete the translation entry specfied */
>> -    __scsiback_del_translation_entry(entry);
>> +    entry = scsiback_chk_translation_entry(info, v);
>> +    if (entry)
>> +        __scsiback_del_translation_entry(entry);
>>          spin_unlock_irqrestore(&info->v2p_lock, flags);
>> -    return 0;
>> +    return entry == NULL;
>>> Might be better to return -ENOENT instead of 1 above and -EEXISTS if
>>> entry!=NULL, given that this returns an int.
>> I just didn't want to change more than necessary. In case it is
>> okay to do some cleanup as well I'd rather change the return type
>> to "bool".
> 
> I don't think using error code will require changing anything except the
> last line above (which is already a change anyway)

And returning -ENOENT or 0 will be even better, I guess.


Juergen