From a297cbcbb07633a7d774ecdf7f8dde141ced3446 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Wed, 18 Apr 2018 13:12:25 -0500 Subject: Refactor group traversal code to eliminate sketchy casts and reduce the number of memcpys. --- src/H5VLrados.c | 179 ++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 136 insertions(+), 43 deletions(-) diff --git a/src/H5VLrados.c b/src/H5VLrados.c index 0b5d0d4..0ce9957 100644 --- a/src/H5VLrados.c +++ b/src/H5VLrados.c @@ -187,16 +187,21 @@ static herr_t H5VL_rados_attribute_close(void *_attr, hid_t dxpl_id, static herr_t H5VL_rados_file_close_helper(H5VL_rados_file_t *file, hid_t dxpl_id, void **req); -static herr_t H5VL_rados_link_read(H5VL_rados_group_t *grp, char *name, - size_t name_len, H5VL_rados_link_val_t *val); +static herr_t H5VL_rados_link_read(H5VL_rados_group_t *grp, const char *name, + H5VL_rados_link_val_t *val); static herr_t H5VL_rados_link_write(H5VL_rados_group_t *grp, const char *name, H5VL_rados_link_val_t *val); -static herr_t H5VL_rados_link_follow(H5VL_rados_group_t *grp, char *name, +static herr_t H5VL_rados_link_follow(H5VL_rados_group_t *grp, const char *name, + hid_t dxpl_id, void **req, uint64_t *oid); +static herr_t H5VL_rados_link_follow_comp(H5VL_rados_group_t *grp, char *name, size_t name_len, hid_t dxpl_id, void **req, uint64_t *oid); static H5VL_rados_group_t *H5VL_rados_group_traverse(H5VL_rados_item_t *item, - const char *path, hid_t dxpl_id, void **req, const char **obj_name, + char *path, hid_t dxpl_id, void **req, char **obj_name, void **gcpl_buf_out, uint64_t *gcpl_len_out); +static H5VL_rados_group_t *H5VL_rados_group_traverse_const( + H5VL_rados_item_t *item, const char *path, hid_t dxpl_id, void **req, + const char **obj_name, void **gcpl_buf_out, uint64_t *gcpl_len_out); static void *H5VL_rados_group_create_helper(H5VL_rados_file_t *file, hid_t gcpl_id, hid_t gapl_id, hid_t dxpl_id, void **req, H5VL_rados_group_t *parent_grp, const char *name, hbool_t collective); @@ -1127,7 +1132,7 @@ done: *------------------------------------------------------------------------- */ static herr_t -H5VL_rados_link_read(H5VL_rados_group_t *grp, char *name, size_t name_len, +H5VL_rados_link_read(H5VL_rados_group_t *grp, const char *name, H5VL_rados_link_val_t *val) { rados_read_op_t read_op; @@ -1137,7 +1142,6 @@ H5VL_rados_link_read(H5VL_rados_group_t *grp, char *name, size_t name_len, char *key; char *omap_val; size_t val_len; - char saved_end = name[name_len]; // Stop this, or copy path first RADOSINC uint8_t *p; int ret; int read_ret; @@ -1145,9 +1149,6 @@ H5VL_rados_link_read(H5VL_rados_group_t *grp, char *name, size_t name_len, FUNC_ENTER_NOAPI_NOINIT - /* Add null terminator to name (necessary for RADOS API) */ - name[name_len] = '\0'; - /* Create read op */ if(NULL == (read_op = rados_create_read_op())) HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, FAIL, "can't create read operation") @@ -1207,7 +1208,6 @@ done: rados_omap_get_end(iter); if(read_op_init) rados_release_read_op(read_op); - name[name_len] = saved_end; FUNC_LEAVE_NOAPI(ret_value) } /* end H5VL_rados_link_read() */ @@ -1315,7 +1315,8 @@ done: * Function: H5VL_rados_link_follow * * Purpose: Follows the link in grp identified with name, and returns - * in oid the oid of the target object. + * in oid the oid of the target object. name must be NULL + * terminated. * * Return: Success: SUCCEED * Failure: FAIL @@ -1326,8 +1327,8 @@ done: *------------------------------------------------------------------------- */ static herr_t -H5VL_rados_link_follow(H5VL_rados_group_t *grp, char *name, size_t name_len, - hid_t dxpl_id, void **req, uint64_t *oid) +H5VL_rados_link_follow(H5VL_rados_group_t *grp, const char *name, hid_t dxpl_id, + void **req, uint64_t *oid) { H5VL_rados_link_val_t link_val; hbool_t link_val_alloc = FALSE; @@ -1341,7 +1342,7 @@ H5VL_rados_link_follow(H5VL_rados_group_t *grp, char *name, size_t name_len, HDassert(oid); /* Read link to group */ - if(H5VL_rados_link_read(grp, name, name_len, &link_val) < 0) + if(H5VL_rados_link_read(grp, name, &link_val) < 0) HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, FAIL, "can't read link") switch(link_val.type) { @@ -1358,7 +1359,7 @@ H5VL_rados_link_follow(H5VL_rados_group_t *grp, char *name, size_t name_len, link_val_alloc = TRUE; /* Traverse the soft link path */ - if(NULL == (target_grp = H5VL_rados_group_traverse(&grp->obj.item, link_val.target.soft, dxpl_id, req, (const char **)&target_name, NULL, NULL))) + if(NULL == (target_grp = H5VL_rados_group_traverse(&grp->obj.item, link_val.target.soft, dxpl_id, req, &target_name, NULL, NULL))) HGOTO_ERROR(H5E_SYM, H5E_BADITER, FAIL, "can't traverse path") /* Check for no target_name, in this case just return @@ -1368,7 +1369,7 @@ H5VL_rados_link_follow(H5VL_rados_group_t *grp, char *name, size_t name_len, *oid = target_grp->obj.bin_oid; else /* Follow the last element in the path */ - if(H5VL_rados_link_follow(target_grp, target_name, HDstrlen(target_name), dxpl_id, req, oid) < 0) + if(H5VL_rados_link_follow(target_grp, target_name, dxpl_id, req, oid) < 0) HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, FAIL, "can't follow link") break; @@ -1397,6 +1398,50 @@ done: /*------------------------------------------------------------------------- + * Function: H5VL_rados_link_follow_comp + * + * Purpose: Follows the link in grp identified with name, and returns + * in oid the oid of the target object. name may be a + * component of a path, only the first name_len bytes of name + * are examined. + * + * Return: Success: SUCCEED + * Failure: FAIL + * + * Programmer: Neil Fortner + * April, 2018 + * + *------------------------------------------------------------------------- + */ +static herr_t +H5VL_rados_link_follow_comp(H5VL_rados_group_t *grp, char *name, + size_t name_len, hid_t dxpl_id, void **req, uint64_t *oid) +{ + char saved_end = name[name_len]; + herr_t ret_value = SUCCEED; + + FUNC_ENTER_NOAPI_NOINIT + + HDassert(grp); + HDassert(name); + HDassert(oid); + + /* Add null terminator to name so we can use the underlying routine */ + name[name_len] = '\0'; + + /* Follow the link now that name is NULL terminated */ + if(H5VL_rados_link_follow(grp, name, dxpl_id, req, oid) < 0) + HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, FAIL, "can't follow link to group") + +done: + /* Put name back the way it was */ + name[name_len] = saved_end; + + FUNC_LEAVE_NOAPI(ret_value) +} /* end H5VL_rados_link_follow_comp() */ + + +/*------------------------------------------------------------------------- * Function: H5VL_rados_group_traverse * * Purpose: Given a path name and base object, returns the final group @@ -1413,13 +1458,11 @@ done: *------------------------------------------------------------------------- */ static H5VL_rados_group_t * -H5VL_rados_group_traverse(H5VL_rados_item_t *item, const char *path, - hid_t dxpl_id, void **req, const char **obj_name, void **gcpl_buf_out, +H5VL_rados_group_traverse(H5VL_rados_item_t *item, char *path, + hid_t dxpl_id, void **req, char **obj_name, void **gcpl_buf_out, uint64_t *gcpl_len_out) { H5VL_rados_group_t *grp = NULL; - char *tmp_path = NULL; - char *tmp_obj_name; char *next_obj; uint64_t oid; H5VL_rados_group_t *ret_value = NULL; @@ -1430,21 +1473,13 @@ H5VL_rados_group_traverse(H5VL_rados_item_t *item, const char *path, HDassert(path); HDassert(obj_name); - /* Make a temporary copy of path so we do not write to the user's const - * buffer (since the RADOS API expects null terminated strings we must - * insert null terminators to pass path components to RADOS. We could - * alternatively copy each path name but this is simpler and shares more - * code with other VOL plugins) */ - if(NULL == (tmp_path = HDstrdup(path))) - HGOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, NULL, "can't duplicate path name") - /* Initialize obj_name */ - tmp_obj_name = tmp_path; + *obj_name = path; /* Open starting group */ - if((tmp_obj_name)[0] == '/') { + if((*obj_name)[0] == '/') { grp = item->file->root_grp; - tmp_obj_name++; + (*obj_name)++; } /* end if */ else { if(item->type == H5I_GROUP) @@ -1458,7 +1493,7 @@ H5VL_rados_group_traverse(H5VL_rados_item_t *item, const char *path, grp->obj.item.rc++; /* Search for '/' */ - next_obj = strchr(tmp_obj_name, '/'); + next_obj = strchr(*obj_name, '/'); /* Traverse path */ while(next_obj) { @@ -1467,8 +1502,8 @@ H5VL_rados_group_traverse(H5VL_rados_item_t *item, const char *path, *gcpl_buf_out = H5MM_xfree(*gcpl_buf_out); /* Follow link to next group in path */ - HDassert(next_obj > tmp_obj_name); - if(H5VL_rados_link_follow(grp, tmp_obj_name, (size_t)(next_obj - tmp_obj_name), dxpl_id, req, &oid) < 0) + HDassert(next_obj > *obj_name); + if(H5VL_rados_link_follow_comp(grp, *obj_name, (size_t)(next_obj - *obj_name), dxpl_id, req, &oid) < 0) HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, NULL, "can't follow link to group") /* Close previous group */ @@ -1481,12 +1516,70 @@ H5VL_rados_group_traverse(H5VL_rados_item_t *item, const char *path, HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, NULL, "can't open group") /* Advance to next path element */ - tmp_obj_name = next_obj + 1; - next_obj = strchr(tmp_obj_name, '/'); + *obj_name = next_obj + 1; + next_obj = strchr(*obj_name, '/'); } /* end while */ /* Set return values */ + ret_value = grp; + +done: + /* Cleanup on failure */ + if(NULL == ret_value) + /* Close group */ + if(grp && H5VL_rados_group_close(grp, dxpl_id, req) < 0) + HDONE_ERROR(H5E_FILE, H5E_CLOSEERROR, NULL, "can't close group") + + FUNC_LEAVE_NOAPI(ret_value) +} /* end H5VL_rados_group_traverse() */ + + +/*------------------------------------------------------------------------- + * Function: H5VL_rados_group_traverse_const + * + * Purpose: Wrapper for H5VL_rados_group_traverse for a const path. + * + * Return: Success: group object. + * Failure: NULL + * + * Programmer: Neil Fortner + * April, 2018 + * + *------------------------------------------------------------------------- + */ +static H5VL_rados_group_t * +H5VL_rados_group_traverse_const(H5VL_rados_item_t *item, const char *path, + hid_t dxpl_id, void **req, const char **obj_name, void **gcpl_buf_out, + uint64_t *gcpl_len_out) +{ + H5VL_rados_group_t *grp = NULL; + char *tmp_path = NULL; + char *tmp_obj_name; + H5VL_rados_group_t *ret_value = NULL; + + FUNC_ENTER_NOAPI_NOINIT + + HDassert(item); + HDassert(path); + HDassert(obj_name); + + /* Make a temporary copy of path so we do not write to the user's const + * buffer (since the RADOS API expects null terminated strings we must + * insert null terminators to pass path components to RADOS. We could + * alternatively copy each path name but this is simpler and shares more + * code with other VOL plugins) */ + if(NULL == (tmp_path = HDstrdup(path))) + HGOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, NULL, "can't duplicate path name") + + /* Forward the call to the non-const routine */ + if(NULL == (grp = H5VL_rados_group_traverse(item, tmp_path, dxpl_id, req, + &tmp_obj_name, gcpl_buf_out, gcpl_len_out))) + HGOTO_ERROR(H5E_SYM, H5E_BADITER, NULL, "can't traverse path") + + /* Set *obj_name in path to match tmp_obj_name in tmp_path */ *obj_name = path + (tmp_obj_name - tmp_path); + + /* Set return value */ ret_value = grp; done: @@ -1499,7 +1592,7 @@ done: H5MM_xfree(tmp_path); FUNC_LEAVE_NOAPI(ret_value) -} /* end H5VL_rados_group_traverse() */ +} /* end H5VL_rados_group_traverse_const() */ /*------------------------------------------------------------------------- @@ -1639,7 +1732,7 @@ H5VL_rados_group_create(void *_item, /* Traverse the path */ if(!collective || (item->file->my_rank == 0)) - if(NULL == (target_grp = H5VL_rados_group_traverse(item, name, dxpl_id, req, &target_name, NULL, NULL))) + if(NULL == (target_grp = H5VL_rados_group_traverse_const(item, name, dxpl_id, req, &target_name, NULL, NULL))) HGOTO_ERROR(H5E_SYM, H5E_BADITER, NULL, "can't traverse path") /* Create group and link to group */ @@ -1864,7 +1957,7 @@ H5VL_rados_group_open(void *_item, H5VL_loc_params_t loc_params, else { /* Open using name parameter */ /* Traverse the path */ - if(NULL == (target_grp = H5VL_rados_group_traverse(item, name, dxpl_id, req, &target_name, (collective && (item->file->num_procs > 1)) ? (void **)&gcpl_buf : NULL, &gcpl_len))) + if(NULL == (target_grp = H5VL_rados_group_traverse_const(item, name, dxpl_id, req, &target_name, (collective && (item->file->num_procs > 1)) ? (void **)&gcpl_buf : NULL, &gcpl_len))) HGOTO_ERROR(H5E_SYM, H5E_BADITER, NULL, "can't traverse path") /* Check for no target_name, in this case just return target_grp */ @@ -1890,7 +1983,7 @@ H5VL_rados_group_open(void *_item, H5VL_loc_params_t loc_params, gcpl_len = 0; /* Follow link to group */ - if(H5VL_rados_link_follow(target_grp, (char *)target_name, HDstrlen(target_name), dxpl_id, req, &oid) < 0) + if(H5VL_rados_link_follow(target_grp, target_name, dxpl_id, req, &oid) < 0) HGOTO_ERROR(H5E_SYM, H5E_CANTINIT, NULL, "can't follow link to group") /* Open group */ @@ -2385,7 +2478,7 @@ H5VL_rados_dataset_create(void *_item, size_t md_size = 0; /* Traverse the path */ - if(NULL == (target_grp = H5VL_rados_group_traverse(item, name, dxpl_id, req, &target_name, NULL, NULL))) + if(NULL == (target_grp = H5VL_rados_group_traverse_const(item, name, dxpl_id, req, &target_name, NULL, NULL))) HGOTO_ERROR(H5E_DATASET, H5E_BADITER, NULL, "can't traverse path") /* Create dataset */ @@ -2540,11 +2633,11 @@ H5VL_rados_dataset_open(void *_item, else { /* Open using name parameter */ /* Traverse the path */ - if(NULL == (target_grp = H5VL_rados_group_traverse(item, name, dxpl_id, req, &target_name, NULL, NULL))) + if(NULL == (target_grp = H5VL_rados_group_traverse_const(item, name, dxpl_id, req, &target_name, NULL, NULL))) HGOTO_ERROR(H5E_DATASET, H5E_BADITER, NULL, "can't traverse path") /* Follow link to dataset */ - if(H5VL_rados_link_follow(target_grp, (char *)target_name, HDstrlen(target_name), dxpl_id, req, &dset->obj.bin_oid) < 0) + if(H5VL_rados_link_follow(target_grp, target_name, dxpl_id, req, &dset->obj.bin_oid) < 0) HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, NULL, "can't follow link to dataset") /* Create string oid */ -- cgit v0.12