Hi,
thank you for the review.
On 01/06/14 02:53, Daniel Shahaf wrote:
> Don't we prefer doing:
>
> svn_error_createf(SVN_ERR_BASE, NULL, _("%s: number %ld"),
> apr_psprintf(pool, "%" APR_UINT64_T_FMT,
> (apr_uint64_t)1),
> 1L)
>
> since it allows for compile-time type checking of varargs against the
> format string?
Yes, adjusted accordingly, and fixed another instance of same in
l2p_page_get_offset which was previously attempted by someone else.
>> /* copy the info */
>> - return l2p_page_info_copy(baton, header, page_table, page_table_index);
>> + return l2p_page_info_copy(baton, header, page_table, page_table_index,
>> result_pool);
>
> That should be scratch_pool, since you only use it to allocate an error
> message. (The svn_error_t->message member is constructed in the error's
> pool, which is the child of a global pool, not related to any of the
> pools in this scope.)
As discussed on IRC, a scratch_pool in not available. Changed to
local_pool where it is created in the function. Again review for the
updated patch would be appreciated, especially on the pool usage in the
context.
[[[
Fix xgettext warnings and incomplete format strings due to macros
* subversion/libsvn_fs_fs/cached_data.c
(svn_fs_fs__check_rep): make xgettext friendly by removing the
macro from the format string
* subversion/libsvn_fs_x/cached_data.c
(svn_fs_x__check_rep): same
* subversion/libsvn_fs_fs/index.c
(svn_fs_fs__l2p_index_create): same x2
(l2p_page_info_copy,l2p_page_get_entry): same, and grow
scratch_pool parameters to support it
(l2p_page_info_access_func,get_l2p_page_info): update calls to
l2p_page_info_copy to pass a pool
(l2p_entry_access_func,l2p_index_lookup): update calls to
l2p_page_get_entry to pass a pool
* subversion/libsvn_fs_x/index.c
(svn_fs_x__l2p_index_create): make xgettext friendly by removing
the macro from the format string
(l2p_header_copy): same, and grow scratch_pool parameters to
support it
(l2p_header_access_func,get_l2p_page_info): update calls to
l2p_header_copy to pass a pool
(l2p_page_get_offset): same pattern to allow compile-time
checking of arguments against format string
]]]
Andreas
Index: subversion/libsvn_fs_fs/cached_data.c
===================================================================
--- subversion/libsvn_fs_fs/cached_data.c (revision 1599006)
+++ subversion/libsvn_fs_fs/cached_data.c (working copy)
@@ -942,10 +942,11 @@ svn_fs_fs__check_rep(representation_t *rep,
|| entry->type > SVN_FS_FS__ITEM_TYPE_DIR_PROPS)
return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
_("No representation found at offset %s "
- "for item %" APR_UINT64_T_FMT
- " in revision %ld"),
+ "for item %s in revision %ld"),
apr_off_t_toa(pool, offset),
- rep->item_index, rep->revision);
+ apr_psprintf(pool, "%" APR_UINT64_T_FMT,
+ rep->item_index),
+ rep->revision);
SVN_ERR(svn_fs_fs__close_revision_file(&rev_file));
}
Index: subversion/libsvn_fs_fs/index.c
===================================================================
--- subversion/libsvn_fs_fs/index.c (revision 1599006)
+++ subversion/libsvn_fs_fs/index.c (working copy)
@@ -594,9 +594,10 @@ svn_fs_fs__l2p_index_create(svn_fs_t *fs,
* The current implementation is limited to 2G entries per page. */
if (ffd->l2p_page_size > APR_INT32_MAX)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("L2P index page size %" APR_UINT64_T_FMT
+ _("L2P index page size %s"
" exceeds current limit of 2G entries"),
- ffd->l2p_page_size);
+ apr_psprintf(local_pool, "%" APR_UINT64_T_FMT,
+ ffd->l2p_page_size));
/* start at the beginning of the source file */
SVN_ERR(svn_io_file_open(&proto_index, proto_file_name,
@@ -654,10 +655,10 @@ svn_fs_fs__l2p_index_create(svn_fs_t *fs,
/* store the mapping in our array */
if (proto_entry.item_index > APR_INT32_MAX)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("Item index %" APR_UINT64_T_FMT
- " too large in l2p proto index for"
- " revision %ld"),
- proto_entry.item_index,
+ _("Item index %s too large "
+ "in l2p proto index for revision %ld"),
+ apr_psprintf(local_pool, "%" APR_UINT64_T_FMT,
+ proto_entry.item_index),
revision + page_counts->nelts);
idx = (int)proto_entry.item_index;
@@ -900,7 +901,8 @@ static svn_error_t *
l2p_page_info_copy(l2p_page_info_baton_t *baton,
const l2p_header_t *header,
const l2p_page_table_entry_t *page_table,
- const apr_size_t *page_table_index)
+ const apr_size_t *page_table_index,
+ apr_pool_t *scratch_pool)
{
/* revision offset within the index file */
apr_size_t rel_revision = baton->revision - header->first_revision;
@@ -932,10 +934,12 @@ l2p_page_info_copy(l2p_page_info_baton_t *baton,
* (last_entry - first_entry);
if (baton->item_index >= max_item_index)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("Item index %" APR_UINT64_T_FMT
- " exceeds l2p limit of %" APR_UINT64_T_FMT
- " for revision %ld"),
- baton->item_index, max_item_index,
+ _("Item index %s exceeds l2p limit "
+ "of %s for revision %ld"),
+ apr_psprintf(scratch_pool, "%" APR_UINT64_T_FMT,
+ baton->item_index),
+ apr_psprintf(scratch_pool, "%" APR_UINT64_T_FMT,
+ max_item_index),
baton->revision);
/* all pages are of the same size and full, except for the last one */
@@ -970,7 +974,7 @@ l2p_page_info_access_func(void **out,
(const void *const *)&header->page_table_index);
/* copy the info */
- return l2p_page_info_copy(baton, header, page_table, page_table_index);
+ return l2p_page_info_copy(baton, header, page_table, page_table_index, result_pool);
}
/* Get the page info requested in *BATON from FS and set the output fields
@@ -1002,7 +1006,7 @@ get_l2p_page_info(l2p_page_info_baton_t *baton,
/* read from disk, cache and copy the result */
SVN_ERR(get_l2p_header_body(&result, rev_file, fs, baton->revision, pool));
SVN_ERR(l2p_page_info_copy(baton, result, result->page_table,
- result->page_table_index));
+ result->page_table_index, pool));
return SVN_NO_ERROR;
}
@@ -1241,14 +1245,17 @@ typedef struct l2p_entry_baton_t
static svn_error_t *
l2p_page_get_entry(l2p_entry_baton_t *baton,
const l2p_page_t *page,
- const apr_uint64_t *offsets)
+ const apr_uint64_t *offsets,
+ apr_pool_t *scratch_pool)
{
/* overflow check */
if (page->entry_count <= baton->page_offset)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("Item index %" APR_UINT64_T_FMT
+ _("Item index %s"
" too large in revision %ld"),
- baton->item_index, baton->revision);
+ apr_psprintf(scratch_pool, "%" APR_UINT64_T_FMT,
+ baton->item_index),
+ baton->revision);
/* return the result */
baton->offset = offsets[baton->page_offset];
@@ -1273,7 +1280,7 @@ l2p_entry_access_func(void **out,
= svn_temp_deserializer__ptr(page, (const void *const *)&page->offsets);
/* return the requested data */
- return l2p_page_get_entry(baton, page, offsets);
+ return l2p_page_get_entry(baton, page, offsets, result_pool);
}
/* Using the log-to-phys indexes in FS, find the absolute offset in the
@@ -1338,7 +1345,7 @@ l2p_index_lookup(apr_off_t *offset,
/* cache the page and extract the result we need */
SVN_ERR(svn_cache__set(ffd->l2p_page_cache, &key, page, pool));
- SVN_ERR(l2p_page_get_entry(&page_baton, page, page->offsets));
+ SVN_ERR(l2p_page_get_entry(&page_baton, page, page->offsets, pool));
/* prefetch pages from following and preceding revisions */
pages = apr_array_make(pool, 16, sizeof(l2p_page_table_entry_t));
Index: subversion/libsvn_fs_x/cached_data.c
===================================================================
--- subversion/libsvn_fs_x/cached_data.c (revision 1599006)
+++ subversion/libsvn_fs_x/cached_data.c (working copy)
@@ -750,10 +750,11 @@ svn_fs_x__check_rep(representation_t *rep,
&& entry->type != SVN_FS_X__ITEM_TYPE_REPS_CONT))
return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
_("No representation found at offset %s "
- "for item %" APR_UINT64_T_FMT
- " in revision %ld"),
+ "for item %s in revision %ld"),
apr_off_t_toa(pool, offset),
- rep->id.number, revision);
+ apr_psprintf(pool, "%" APR_UINT64_T_FMT,
+ rep->id.number),
+ revision);
return SVN_NO_ERROR;
}
Index: subversion/libsvn_fs_x/index.c
===================================================================
--- subversion/libsvn_fs_x/index.c (revision 1599006)
+++ subversion/libsvn_fs_x/index.c (working copy)
@@ -719,9 +719,10 @@ svn_fs_x__l2p_index_create(svn_fs_t *fs,
* The current implementation is limited to 2G entries per page. */
if (ffd->l2p_page_size > APR_INT32_MAX)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("L2P index page size %" APR_UINT64_T_FMT
+ _("L2P index page size %s"
" exceeds current limit of 2G entries"),
- ffd->l2p_page_size);
+ apr_psprintf(local_pool, "%" APR_UINT64_T_FMT,
+ ffd->l2p_page_size));
/* start at the beginning of the source file */
SVN_ERR(svn_io_file_open(&proto_index, proto_file_name,
@@ -781,10 +782,10 @@ svn_fs_x__l2p_index_create(svn_fs_t *fs,
if (proto_entry.item_index > APR_INT32_MAX)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("Item index %" APR_UINT64_T_FMT
- " too large in l2p proto index for"
- " revision %ld"),
- proto_entry.item_index,
+ _("Item index %s too large "
+ "in l2p proto index for revision %ld"),
+ apr_psprintf(local_pool, "%" APR_UINT64_T_FMT,
+ proto_entry.item_index),
revision + page_counts->nelts);
idx = (int)proto_entry.item_index;
@@ -907,7 +908,8 @@ static svn_error_t *
l2p_header_copy(l2p_page_info_baton_t *baton,
const l2p_header_t *header,
const l2p_page_table_entry_t *page_table,
- const apr_size_t *page_table_index)
+ const apr_size_t *page_table_index,
+ apr_pool_t *scratch_pool)
{
/* revision offset within the index file */
apr_size_t rel_revision = baton->revision - header->first_revision;
@@ -939,10 +941,12 @@ l2p_header_copy(l2p_page_info_baton_t *baton,
* (last_entry - first_entry);
if (baton->item_index >= max_item_index)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- _("Item index %" APR_UINT64_T_FMT
- " exceeds l2p limit of %" APR_UINT64_T_FMT
- " for revision %ld"),
- baton->item_index, max_item_index,
+ _("Item index %s exceeds l2p limit "
+ "of %s for revision %ld"),
+ apr_psprintf(scratch_pool, "%" APR_UINT64_T_FMT,
+ baton->item_index),
+ apr_psprintf(scratch_pool, "%" APR_UINT64_T_FMT,
+ max_item_index),
baton->revision);
/* all pages are of the same size and full, except for the last one */
@@ -977,7 +981,7 @@ l2p_header_access_func(void **out,
(const void *const *)&header->page_table_index);
/* copy the info */
- return l2p_header_copy(baton, header, page_table, page_table_index);
+ return l2p_header_copy(baton, header, page_table, page_table_index, result_pool);
}
/* Read COUNT run-length-encoded (see rle_array) uint64 from STREAM and
@@ -1139,7 +1143,7 @@ get_l2p_page_info(l2p_page_info_baton_t *baton,
/* read from disk, cache and copy the result */
SVN_ERR(get_l2p_header_body(&result, stream, fs, baton->revision, pool));
SVN_ERR(l2p_header_copy(baton, result, result->page_table,
- result->page_table_index));
+ result->page_table_index, pool));
return SVN_NO_ERROR;
}
@@ -1285,11 +1289,11 @@ l2p_page_get_offset(l2p_page_baton_t *baton,
/* overflow check */
if (page->entry_count <= baton->page_offset)
return svn_error_createf(SVN_ERR_FS_ITEM_INDEX_OVERFLOW , NULL,
- apr_psprintf(pool,
- _("Item index %%%s too large in"
- " revision %%ld"),
- APR_UINT64_T_FMT),
- baton->item_index, baton->revision);
+ _("Item index %s too large in"
+ " revision %ld"),
+ apr_psprintf(pool, "%" APR_UINT64_T_FMT,
+ baton->item_index),
+ baton->revision);
/* return the result */
baton->offset = offsets[baton->page_offset];
