On 12/14/2012 11:17 PM, Sage Weil wrote:
> Most of the code uses int64_t/__s64 for the pool id, although in a few
> cases we screwed up and limited it to 32 bits. In reality, that's way
> overkill anyway; we could have left it at 32 bits to begin with.
The differing types representing the same abstraction are precisely
the point of making this change. What really needs to happen is we
need to fix *that*; that is, decide whether a pool id is 32 or 64
bits, signed or not, and then make sure it's that and only that
throughout the code.
In the mean time, this change is defensive, making sure there's
no uncertainty in what we're dealing with within rbd. The code
will guarantee some future change won't inadvertently let a
wrong-sized pool id attempt to sneak through an interface
unnoticed. It may seem like overkill but this kind of bug is
really hard to track down, and it's better to simply preclude
it from happening.
> My first instinct would be to change the return type to long long or s64
> and avoid the use magic #defines...
I absolutely like using base types (like long long). But where
those types are used to represent a true abstraction (like a
snapshot id, or a pool id), it is the one place I think the use
of typedefs and "magic #defines" is actually a real help because
it makes explicit you're working with something more than an (e.g.)
an int. A typedef makes obviously to the reader that it's restricted
a bit (so, for example, it isn't meaningful to do math on it).
And symbolic constants make it a lot easier to search through
code for special situations like this.
This stuff is all sort of philosophical rather than technical.
The code before works, and the code as I've changed it works.
Anybody else have thoughts?
-Alex
>
>
> On Thu, 13 Dec 2012, Alex Elder wrote:
>
>> Currently ceph_pg_poolid_by_name() returns an int, which is used to
>> encode a ceph pool id. This could be a problem because a pool id
>> (at least in some cases) is a 64-bit value. We have a defined pool
>> id value that represents "no pool," and that's a very sensible
>> return value here.
>>
>> This patch changes ceph_pg_poolid_by_name() to return a 64-bit
>> pool id value, or CEPH_NOPOOL if the named pool is not found.
>>
>> The patch also gratuitously renames the function, separating "pool"
>> from "id" in the name by an underscore.
>>
>> Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
>> ---
>> drivers/block/rbd.c | 6 +++---
>> include/linux/ceph/osdmap.h | 2 +-
>> net/ceph/osdmap.c | 14 ++++++++------
>> 3 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index 4daa400..706824b 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -3642,11 +3642,11 @@ static ssize_t rbd_add(struct bus_type *bus,
>> ceph_opts = NULL; /* rbd_dev client now owns this */
>>
>> /* pick the pool */
>> + rc = -ENOENT;
>> osdc = &rbdc->client->osdc;
>> - rc = ceph_pg_poolid_by_name(osdc->osdmap, spec->pool_name);
>> - if (rc < 0)
>> + spec->pool_id = ceph_pg_pool_id_by_name(osdc->osdmap, spec->pool_name);
>> + if (spec->pool_id == CEPH_NOPOOL)
>> goto err_out_client;
>> - spec->pool_id = (u64) rc;
>>
>> rbd_dev = rbd_dev_create(rbdc, spec);
>> if (!rbd_dev)
>> diff --git a/include/linux/ceph/osdmap.h b/include/linux/ceph/osdmap.h
>> index 5ea57ba..c841396 100644
>> --- a/include/linux/ceph/osdmap.h
>> +++ b/include/linux/ceph/osdmap.h
>> @@ -124,6 +124,6 @@ extern int ceph_calc_pg_primary(struct ceph_osdmap
>> *osdmap,
>> struct ceph_pg pgid);
>>
>> extern const char *ceph_pg_pool_name_by_id(struct ceph_osdmap *map, u64
>> id);
>> -extern int ceph_pg_poolid_by_name(struct ceph_osdmap *map, const char
>> *name);
>> +extern __u64 ceph_pg_pool_id_by_name(struct ceph_osdmap *map, const
>> char *name);
>>
>> #endif
>> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
>> index de73214..27e904e 100644
>> --- a/net/ceph/osdmap.c
>> +++ b/net/ceph/osdmap.c
>> @@ -485,19 +485,21 @@ const char *ceph_pg_pool_name_by_id(struct
>> ceph_osdmap *map, u64 id)
>> }
>> EXPORT_SYMBOL(ceph_pg_pool_name_by_id);
>>
>> -int ceph_pg_poolid_by_name(struct ceph_osdmap *map, const char *name)
>> +__u64 ceph_pg_pool_id_by_name(struct ceph_osdmap *map, const char *name)
>> {
>> struct rb_node *rbp;
>>
>> for (rbp = rb_first(&map->pg_pools); rbp; rbp = rb_next(rbp)) {
>> - struct ceph_pg_pool_info *pi =
>> - rb_entry(rbp, struct ceph_pg_pool_info, node);
>> + struct ceph_pg_pool_info *pi;
>> +
>> + pi = rb_entry(rbp, struct ceph_pg_pool_info, node);
>> if (pi->name && strcmp(pi->name, name) == 0)
>> - return pi->id;
>> + return (__u64) pi->id;
>> }
>> - return -ENOENT;
>> +
>> + return CEPH_NOPOOL;
>> }
>> -EXPORT_SYMBOL(ceph_pg_poolid_by_name);
>> +EXPORT_SYMBOL(ceph_pg_pool_id_by_name);
>>
>> static void __remove_pg_pool(struct rb_root *root, struct
>> ceph_pg_pool_info *pi)
>> {
>> --
>> 1.7.9.5
>>
>> --
>> 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
>>
>>
--
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
