On 04/22/2013 04:57 PM, Josh Durgin wrote:
> A couple small things below. With those fixed:
>
> Reviewed-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
>
> On 04/21/2013 02:17 PM, Alex Elder wrote:
>> Callers of rbd_obj_method_sync() don't know how many bytes of data
>> got returned by the class method call. As a result, they have been
>> assuming enough got returned to decode whatever was expected.
>>
>> This isn't safe. We know how many bytes got transferred, so have
>> rbd_obj_method_sync() return that amount (rather than just 0) if
>> the call is successful.
. . .
>> @@ -3709,9 +3714,9 @@ static int rbd_dev_v2_parent_info(struct
>> rbd_device *rbd_dev)
>> if (ret < 0)
>> goto out_err;
>>
>> - ret = -ERANGE;
>> p = reply_buf;
>> end = reply_buf + size;
You didn't mention this, but now that I look at this I
managed to screw up the point of this patch in this spot.
The above line should (and will) be:
end = reply_buf + ret;
(That was why the "ret = -ERANGE" had to be moved to begin
with.) I did it right in one similar spot elsewhere in
the patch.
>> + ret = -ERANGE;
>
> maybe check for the full parent_spec here? even if there's no parent,
> a complete parent_spec should be returned.
I'm not sure I understand this comment.
As it stands, we allocate a local parent_spec structure, and
that gets filled based on what comes back from the "get_parent"
method call. If anything goes wrong, we discard the parent
spec and return an error. Only when all goes well do we assign
rbd_dev->parent_spec = parent_spec;
(and the same goes for the overlap). ceph_extract_encoded_string()
will return an error if it ends up short, and the other things
that extract values also check for length.
Is that what you were referring to--return either a complete
one or none? Or was there something else?
>> ceph_decode_64_safe(&p, end, parent_spec->pool_id, out_err);
>> if (parent_spec->pool_id == CEPH_NOPOOL)
>> goto out; /* No parent? No problem. */
. . .
>> @@ -4605,7 +4612,7 @@ static int rbd_dev_v1_probe(struct rbd_device
>> *rbd_dev)
>> /* Version 1 images have no parent (no layering) */
>>
>> rbd_dev->parent_spec = NULL;
>> - rbd_dev->parent_overlap = 0;
>> + rbd_dev->parent_overlap =40;
>
> extraneous 4
Yes I spotted this and already fixed it in my own branch but
thought it was too small and obvious to warrant a re-post.
>>
>> rbd_dev->image_format = 1;
. . .
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
