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

[Vedvyas shanbhogue]

Thanks . Yes, I made the fix without the +1 in my local checkout . Sorry
about typo when sending the email.
On Mon, Apr 25, 2016 at 1:17 AM <furkan at theshell.co.jp> wrote:

> 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
>
> --
> sheepdog-users mailing lists
> sheepdog-users at lists.wpkg.org
> https://lists.wpkg.org/mailman/listinfo/sheepdog-users
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog-users/attachments/20160425/066c6932/attachment.html>