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

[furkan at theshell.co.jp]

And one more reply (additional correction to my post),

Now I understand the purpose of the proposed change a bit better.

I believe it'd work OK like this;

idx = entry->nr_participants;
// ...
entry->nr_participants++;

The change here is to remove " + 1" from the first line of the proposed 
change.
>> idx = entry->nr_participants + 1;

Thanks,

Furkan Mustafa

On 2016-04-25 15:11, furkan at theshell.co.jp wrote:
> 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