<div dir="ltr">Furkan, Vedvyas,<div><br></div><div>As Vedvyas agreed, Furukan's pointing seems to be valid. Vedvyas, could you create a PR on our github repository?</div><div><br></div><div>Thanks,</div><div>Hitoshi</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 25, 2016 at 4:53 PM, Vedvyas shanbhogue <span dir="ltr"><<a href="mailto:vedvyas13686@gmail.com" target="_blank">vedvyas13686@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks . Yes, I made the fix without the +1 in my local checkout . Sorry about typo when sending the email. <br><div class="gmail_quote"><div><div class="h5"><div dir="ltr">On Mon, Apr 25, 2016 at 1:17 AM <<a href="mailto:furkan@theshell.co.jp" target="_blank">furkan@theshell.co.jp</a>> wrote:<br></div></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">And one more reply (additional correction to my post),<br>
<br>
Now I understand the purpose of the proposed change a bit better.<br>
<br>
I believe it'd work OK like this;<br>
<br>
idx = entry->nr_participants;<br>
// ...<br>
entry->nr_participants++;<br>
<br>
The change here is to remove " + 1" from the first line of the proposed<br>
change.<br>
>> idx = entry->nr_participants + 1;<br>
<br>
Thanks,<br>
<br>
Furkan Mustafa<br>
<br>
On 2016-04-25 15:11, <a href="mailto:furkan@theshell.co.jp" target="_blank">furkan@theshell.co.jp</a> wrote:<br>
> Hello,<br>
><br>
> Sorry for butting in randomly, and sorry for top-posting.<br>
> I'd like to point out a possible problem in the proposed change;<br>
><br>
> Original Code:<br>
>> idx = entry->nr_participants++;<br>
>> ...<br>
><br>
> Proposed Change:<br>
>> idx = entry->nr_participants + 1;<br>
>> ...<br>
>> entry->nr_participants++;<br>
><br>
> I didn't read the source but I believe that the original code here is<br>
> making the assignment *before* increment on purpose.<br>
><br>
> Because current number of elements in an array would be same number<br>
> with<br>
> the index of the next added item. and *after that*, number of items<br>
> should<br>
> be increased.<br>
><br>
> Please correct me if I'm wrong.<br>
><br>
> Thanks,<br>
><br>
> Furkan Mustafa<br>
><br>
> On 2016-04-25 14:09, Hitoshi Mitake wrote:<br>
>> Hi Vedvyas, sorry for my late reply.<br>
>><br>
>> On Sun, Apr 17, 2016 at 12:20 PM, Vedvyas shanbhogue<br>
>> <<a href="mailto:vedvyas13686@gmail.com" target="_blank">vedvyas13686@gmail.com</a>> wrote:<br>
>><br>
>>> This code increments the nr_participants but the participant state<br>
>>> is not updated yet. The is_modified will now iterate over the<br>
>>> updated nr_participants and can see bad state in participant_state<br>
>>> array.<br>
>>><br>
>>> idx = entry->nr_participants++;<br>
>>> memcpy(&entry->participants[idx], owner, sizeof(*owner));<br>
>>> entry->participants_state[idx] =<br>
>>> is_modified(entry) ?<br>
>>> SHARED_LOCK_STATE_INVALIDATED :<br>
>>> SHARED_LOCK_STATE_SHARED;<br>
>>><br>
>>> A fix may be:<br>
>>><br>
>>> idx = entry->nr_participants+ 1;<br>
>>> memcpy(&entry->participants[idx], owner, sizeof(*owner));<br>
>>> entry->participants_state[idx] =<br>
>>> is_modified(entry) ?<br>
>>> SHARED_LOCK_STATE_INVALIDATED :<br>
>>> SHARED_LOCK_STATE_SHARED;<br>
>>> entry->nr_participants++;<br>
>>><br>
>>> Please tell me if this would be a good fix.<br>
>>><br>
>>> I was investigating a panic from is_modified where<br>
>>> is_valid_shared_state finds an invalid shared state - "invalid<br>
>>> shared state, two (or more) nodes are owning VDI..."<br>
>><br>
>> Good catch, thanks a lot! Could you create a PR on github for your<br>
>> change?<br>
>><br>
>> Thanks,<br>
>> Hitoshi<br>
<br></div></div><span class="">
--<br>
sheepdog-users mailing lists<br>
<a href="mailto:sheepdog-users@lists.wpkg.org" target="_blank">sheepdog-users@lists.wpkg.org</a><br>
<a href="https://lists.wpkg.org/mailman/listinfo/sheepdog-users" rel="noreferrer" target="_blank">https://lists.wpkg.org/mailman/listinfo/sheepdog-users</a><br>
</span></blockquote></div>
</blockquote></div><br></div>