2011/11/8 Sachin Prabhu <sprabhu@xxxxxxxxxx>:
> Pavel,
>
> I may have found a bug in this patch. I've commented inline.
>
>> + rc = cifs_lock_add_if(cinode, lock, wait_flag);
>> if (rc < 0)
>> - return rc;
>> - else if (!rc)
>> + kfree(lock);
>> + if (rc <= 0)
>> goto out;
>
> The return values for cifs_lock_add_if could be
> 0 - If no locks exist and we can cache byte range locks
> 1 - If lock doesn't exist and we do not cache byte range locks.
> < 0 in case of an error
>
> In case of an error, ie. return value < 0, shouldn't we just return rc
> and skip the call to posix_lock_file_wait below?
>
> This should be
> if (rc < 0) {
> kfree(lock);
> return rc;
> if (!rc)
> goto out;
>
You may be right - I was thinking about it and decided not to bring
any new behavior with my lock patchset. So, this
patch revert this change (that was previously made by my lock patchset).
>
>>
>> rc = CIFSSMBLock(xid, tcon, netfid, current->tgid, length,
>> flock->fl_start, 0, 1, type, wait_flag, 0);
>> - if (rc == 0) {
>> - /* For Windows locks we must store them. */
>> - rc = cifs_lock_add(cinode, length, flock->fl_start,
>> - type, netfid);
>> + if (rc) {
>> + kfree(lock);
>> + goto out;
>> }
I do the similar thing here. I agree with you that this may be not what we want.
>> +
>> + cifs_lock_add(cinode, lock);
>> } else if (unlock)
>> rc = cifs_unlock_range(cfile, flock, xid);
>>
>
>
>
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
