[sheepdog-users] Possible bug when adding participants to vdi

[furkan at theshell.co.jp]

Hello,

Sorry for butting in randomly, and sorry for top-posting.
I'd like to point out a possible problem in the proposed change;

Original Code:
> idx = entry->nr_participants++;
> ...

Proposed Change:
> idx = entry->nr_participants + 1;
> ...
> entry->nr_participants++;

I didn't read the source but I believe that the original code here is
making the assignment *before* increment on purpose.

Because current number of elements in an array would be same number with
the index of the next added item. and *after that*, number of items 
should
be increased.

Please correct me if I'm wrong.

Thanks,

Furkan Mustafa

On 2016-04-25 14:09, Hitoshi Mitake wrote:
> Hi Vedvyas, sorry for my late reply.
> 
> On Sun, Apr 17, 2016 at 12:20 PM, Vedvyas shanbhogue
> <vedvyas13686 at gmail.com> wrote:
> 
>> This code increments the nr_participants but the participant state
>> is not updated yet. The is_modified will now iterate over the
>> updated nr_participants and can see bad state in participant_state
>> array.
>> 
>> idx = entry->nr_participants++;
>> memcpy(&entry->participants[idx], owner, sizeof(*owner));
>> entry->participants_state[idx] =
>> is_modified(entry) ?
>> SHARED_LOCK_STATE_INVALIDATED :
>> SHARED_LOCK_STATE_SHARED;
>> 
>> A fix may be:
>> 
>> idx = entry->nr_participants+ 1;
>> memcpy(&entry->participants[idx], owner, sizeof(*owner));
>> entry->participants_state[idx] =
>> is_modified(entry) ?
>> SHARED_LOCK_STATE_INVALIDATED :
>> SHARED_LOCK_STATE_SHARED;
>> entry->nr_participants++;
>> 
>> Please tell me if this would be a good fix.
>> 
>> I was investigating a panic from is_modified where
>> is_valid_shared_state finds an invalid shared state - "invalid
>> shared state, two (or more) nodes are owning VDI..."
> 
> Good catch, thanks a lot! Could you create a PR on github for your
> change?
> 
> Thanks,
> Hitoshi