Change in vdsm[master]: Use a copy of the domainsToUpgrade

Saggi Mizrahi has uploaded a new change for review.

Change subject: Use a copy of the domainsToUpgrade
......................................................................

Use a copy of the domainsToUpgrade

The original code just uses another ref that is changed while iterated
which has unexpected results. This was not apparent as the remove might
happened after the iteration because it might run in another thread.

To illustrate:

In [1]: a = [1,2,3,4,5,6,7,8,9,10]

In [2]: for i in a:
...:     a.remove(i)
...:

In [3]: a
Out[3]: [2, 4, 6, 8, 10]

In [4]: for i in a:
...:     a.remove(i)
...:

In [5]: a
Out[5]: [4, 8]

In [6]: for i in a[:]:
...:     a.remove(i)
...:

In [7]: a
Out[7]: []

Fun!

Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
---
M vdsm/storage/sp.py
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/900/1
--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#771329 Use a copy of the domainsToUpgrade

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#771329 Use a copy of the domainsToUpgrade
......................................................................


BZ#771329 Use a copy of the domainsToUpgrade

The original code just uses another ref that is changed while iterated
which has unexpected results. This was not apparent as the remove might
happened after the iteration because it might run in another thread.

To illustrate:

In [1]: a = [1,2,3,4,5,6,7,8,9,10]

In [2]: for i in a:
...:     a.remove(i)
...:

In [3]: a
Out[3]: [2, 4, 6, 8, 10]

In [4]: for i in a:
...:     a.remove(i)
...:

In [5]: a
Out[5]: [4, 8]

In [6]: for i in a[:]:
...:     a.remove(i)
...:

In [7]: a
Out[7]: []

Fun!

Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
---
M vdsm/storage/sp.py
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Use a copy of the domainsToUpgrade
......................................................................


Patch Set 1: Looks good to me, approved

(1 inline comment)

but please mention 771329

....................................................
File vdsm/storage/sp.py
Line 391:             domains = self._domainsToUpgrade[:]
I do not share your opinion about readability, but mine is not very strong.

--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: Use a copy of the domainsToUpgrade
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 391:             domains = self._domainsToUpgrade[:]
Because I think it's more readable. And I like it that I take the copy right at the
start of the method.

--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Use a copy of the domainsToUpgrade
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

please mention the rhel-6.3.0 bug in the commit message. it would later have to be
backported into 6.2.z.

....................................................
File vdsm/storage/sp.py
Line 391:             domains = self._domainsToUpgrade[:]
why do you even need this variable? just put the list copy operation in line 406

--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

Change in vdsm[master]: BZ#771329 Use a copy of the domainsToUpgrade

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#771329 Use a copy of the domainsToUpgrade
......................................................................


Patch Set 2: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/900
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3de0646af2fe9a9fcde708837cfcd83d10385026
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;