Am 10.10.2013 um 14:54 hat Max Reitz geschrieben:
If the "currently" implied that this will turn out bad if there is a
new error condition between a successful call to
qcow2_alloc_cluster_link_l2 and the removal of the L2Meta request
from the list: Yes, that's true, of course.
Yes, that's the scenario. It seems easy to miss the error handling path
when reviewing a change to this code.
However, as you've said,
currently, there is no such condition; and I don't see why it should
be introduced. The sole purpose of the list seems to be (to me) to
execute qcow2_alloc_cluster_link_l2 on every of its elements. Thus,
as soon as qcow2_alloc_cluster_link_l2 is successful, the
corresponding request should be removed from the list.
If anything isn't complex enough in qcow2, think about how things will
turn out with Delayed COW and chances are that it does become complex.
For example, you can then have other requests running in parallel which
use the newly allocated cluster. You may decrease the refcount only
after the last of them has completed. This is just the first case that
comes to mind, I'm sure there's more fun to be had.
So, in case you do agree that it currently works fine, I would not
consider it risky; if this patch is applied and some time in the
future anything introduces a "goto fail" between
qcow2_alloc_cluster_link_l2 and l2_meta = next, this patch would
simply have to make sure that qcow2_free_clusters isn't called in
this case. In the probably very unlikely case all my previous
assumptions and conclusions were true, I'd just add a comment in the
qcow2_alloc_cluster_link_l2 loop informing about this case (“If you
add a goto fail here, make sure to pay attention” or something along
these lines).
Adding a comment there sounds like a fair compromise.
Also, shouldn't it be QCOW2_DISCARD_OTHER?
I'm always unsure about the discard flags. ;-)
I try to follow the rule of “use the specific type (or ‘other’) for
freeing ‘out of the blue’, but use ‘always’ if it's just a very
recent allocation that is being undone again”. I'd gladly accept
better recommendations. ;-)
To be honest, I'm not sure if there are any legitimate use cases for
'always'... Discard is a slow operation, so unless there's a specific
reason anyway, I'd default to 'other' (or a specific type, of course).