Re: [PATCH 4/4] rbd: use a single value of snap_name to mean no snap

[Alex Elder <elder@dreamhost.com>]

On 02/28/2012 08:53 PM, Yehuda Sadeh Weinraub wrote:
> On Tue, Feb 28, 2012 at 7:35 PM, Alex Elder<elder@dreamhost.com>  wrote:
>>  From Josh Durgin<josh.durgin@dreamhost.com>
>>
>> There's already a constant for this anyway.
>>
>> (I changed Josh's code to use memcmp() and memcpy() instead. -Alex)
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>
>> ---
>>   drivers/block/rbd.c |    8 +++-----
>>   1 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 3d0f8cf..25ed3c0 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -563,10 +563,7 @@ static int rbd_header_set_snap(struct rbd_device *dev,
>>
>>         down_write(&header->snap_rwsem);
>>
>> -       if (!snap_name ||
>> -           !*snap_name ||
>> -           strcmp(snap_name, "-") == 0 ||
>> -           strcmp(snap_name, RBD_SNAP_HEAD_NAME) == 0) {
>> +       if (!memcmp(snap_name, RBD_SNAP_HEAD_NAME, sizeof
>
> The original code checked for snap_name, we don't do it here. Also, do
> we know that snap_name is pointing to at least
> sizeof(RBD_SNAP_HEAD_NAME)? if not (or in any case) we should use
> strcmp instead of memcmp. Also, sizeof(x) instead of sizeof x.

This function is only called in one place, within this file.
That place passes rbd_dev->snap_name as the "snap_name" argument.
rbd_dev->snap_name is an array.  So if rbd_dev is a valid structure,
rbd_dev->snap_name will not be a null pointer.

The size of that string is RBD_MAX_SNAP_NAME_LEN.  It would be
an error for sizeof(RBD_SNAP_HEAD_NAME) to exceed that length,
but I could add a check for that.  (Checking at build time would
be best but sizeof() is not available then.)

I'll put in a BUG_ON() or something, or will see if I can put in
some other kind of check.

					-Alex

>
>> RBD_SNAP_HEAD_NAME)) {
>>                 if (header->total_snaps)
>>                         snapc->seq = header->snap_seq;
>>                 else
>> @@ -2213,7 +2210,8 @@ static ssize_t rbd_add(struct bus_type *bus,
>>         }
>>
>>         if (rbd_dev->snap_name[0] == 0)
>> -               rbd_dev->snap_name[0] = '-';
>> +               memcpy(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME,
>> +                       sizeof RBD_SNAP_HEAD_NAME);
>>
>>         rbd_dev->obj_len = strlen(rbd_dev->obj);
>>         snprintf(rbd_dev->obj_md_name, sizeof(rbd_dev->obj_md_name), "%s%s",
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html