[PATCH xserver 1/2] xkb: Introduce helper function to handle similar reallocations.
Peter Hutterer
peter.hutterer at who-t.net
Thu Mar 17 18:29:15 PDT 2011
On Thu, Mar 17, 2011 at 10:40:00AM +0200, Rami Ylimäki wrote:
> This is preparation for a memory leak fix and doesn't contain any
> functional changes.
>
> Reviewed-by: Erkki Seppälä <erkki.seppala at vincit.fi>
> Signed-off-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
> ---
> xkb/xkbUtils.c | 153 ++++++++++++++++++++++++++------------------------------
> 1 files changed, 71 insertions(+), 82 deletions(-)
>
> diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
> index 3a56bea..f4c8a6e 100644
> --- a/xkb/xkbUtils.c
> +++ b/xkb/xkbUtils.c
> @@ -1375,6 +1375,38 @@ _XkbCopyCompat(XkbDescPtr src, XkbDescPtr dst)
> return TRUE;
> }
>
> +/**
> + * Resize and clear an XKB geometry item array.
> + *
> + * @param items[in/out] array to reallocate and clear
I think this should be a comma symbol
http://www.stack.nl/~dimitri/doxygen/commands.html#cmdparam
> + * @param szItems[in] currently allocated item count for "items"
> + * @param nrItems[in] required item count for "items"
> + * @param clearBegin[in] first item to clear after allocation
> + * @param clearEnd[in] one past last item to clear after allocation
> + * @param itemSize[in] size of a single item in "items"
I wonder if we could make this more generic and memset everything past the
desired number of items (see my comment at the end of this email though). or
something more readable with an enum, so that we have something like
_XkbGeomRealloc(items, size, number, itemsize, GEOM_CLEAR_ALL)
with GEOM_CLEAR_NONE and GEOM_CLEAR_EXCESS as well
> + *
> + * @see _XkbGeomAlloc, which is very similar
> + *
> + * @return reallocated array
> + */
> +static void *
> +_XkbGeomRealloc(void *items, int szItems, int nrItems,
> + int clearBegin, int clearEnd, int itemSize)
> +{
> + /* Check if there is need to resize. */
> + if (nrItems != szItems)
> + {
> + assert((items && (szItems > 0)) || (!items && !szItems));
given geometry is not important at all, having asserts anywhere in the
geometry path seems unnecessary. just fail the geometry.
> + items = realloc(items, nrItems * itemSize);
> + }
> + /* Clear specified items to zero. */
> + clearEnd = (clearEnd < nrItems) ? clearEnd : nrItems;
> + if (items && (clearBegin < clearEnd))
> + memset((char *)items + (clearBegin * itemSize), 0, (clearEnd - clearBegin) * itemSize);
> +
> + return items;
> +}
> +
> static Bool
> _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
> {
> @@ -1398,42 +1430,28 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
>
> /* properties */
> if (src->geom->num_properties) {
> - if (src->geom->num_properties != dst->geom->sz_properties) {
> - /* If we've got more properties in the destination than
> - * the source, run through and free all the excess ones
> - * first. */
> - if (src->geom->num_properties < dst->geom->sz_properties) {
> - for (i = src->geom->num_properties,
> - dprop = dst->geom->properties + i;
> - i < dst->geom->num_properties;
> - i++, dprop++) {
> - free(dprop->name);
> - free(dprop->value);
> - }
> + /* If we've got more properties in the destination than
> + * the source, run through and free all the excess ones
> + * first. */
> + if (src->geom->num_properties < dst->geom->sz_properties) {
> + for (i = src->geom->num_properties, dprop = dst->geom->properties + i;
> + i < dst->geom->num_properties;
> + i++, dprop++) {
> + free(dprop->name);
> + free(dprop->value);
> }
> -
> - if (dst->geom->sz_properties)
> - tmp = realloc(dst->geom->properties,
> - src->geom->num_properties *
> - sizeof(XkbPropertyRec));
> - else
> - tmp = malloc(src->geom->num_properties *
> - sizeof(XkbPropertyRec));
> - if (!tmp)
> - return FALSE;
> - dst->geom->properties = tmp;
> }
>
> + /* Reallocate and clear all new items if the buffer grows. */
> + tmp = _XkbGeomRealloc(dst->geom->properties, dst->geom->sz_properties, src->geom->num_properties,
> + dst->geom->num_properties, src->geom->num_properties, sizeof(XkbPropertyRec));
> + if (!tmp)
> + return FALSE;
> + dst->geom->properties = tmp;
> /* We don't set num_properties as we need it to try and avoid
> * too much reallocing. */
> dst->geom->sz_properties = src->geom->num_properties;
>
> - if (dst->geom->sz_properties > dst->geom->num_properties) {
> - memset(dst->geom->properties + dst->geom->num_properties, 0,
> - (dst->geom->sz_properties - dst->geom->num_properties) *
> - sizeof(XkbPropertyRec));
> - }
> -
> for (i = 0,
> sprop = src->geom->properties,
> dprop = dst->geom->properties;
> @@ -1482,36 +1500,22 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
>
> /* colors */
> if (src->geom->num_colors) {
> - if (src->geom->num_colors != dst->geom->sz_colors) {
> - if (src->geom->num_colors < dst->geom->sz_colors) {
> - for (i = src->geom->num_colors,
> - dcolor = dst->geom->colors + i;
> - i < dst->geom->num_colors;
> - i++, dcolor++) {
> - free(dcolor->spec);
> - }
> + if (src->geom->num_colors < dst->geom->sz_colors) {
> + for (i = src->geom->num_colors, dcolor = dst->geom->colors + i;
> + i < dst->geom->num_colors;
> + i++, dcolor++) {
> + free(dcolor->spec);
> }
> -
> - if (dst->geom->sz_colors)
> - tmp = realloc(dst->geom->colors,
> - src->geom->num_colors *
> - sizeof(XkbColorRec));
> - else
> - tmp = malloc(src->geom->num_colors *
> - sizeof(XkbColorRec));
> - if (!tmp)
> - return FALSE;
> - dst->geom->colors = tmp;
> }
>
> + /* Reallocate and clear all new items if the buffer grows. */
> + tmp = _XkbGeomRealloc(dst->geom->colors, dst->geom->sz_colors, src->geom->num_colors,
> + dst->geom->num_colors, src->geom->num_colors, sizeof(XkbColorRec));
> + if (!tmp)
> + return FALSE;
> + dst->geom->colors = tmp;
> dst->geom->sz_colors = src->geom->num_colors;
>
> - if (dst->geom->sz_colors > dst->geom->num_colors) {
> - memset(dst->geom->colors + dst->geom->num_colors, 0,
> - (dst->geom->sz_colors - dst->geom->num_colors) *
> - sizeof(XkbColorRec));
> - }
> -
> for (i = 0,
> scolor = src->geom->colors,
> dcolor = dst->geom->colors;
> @@ -1697,15 +1701,11 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
> }
>
> if (src->geom->num_sections) {
> - if (dst->geom->sz_sections)
> - tmp = realloc(dst->geom->sections,
> - src->geom->num_sections *
> - sizeof(XkbSectionRec));
> - else
> - tmp = malloc(src->geom->num_sections * sizeof(XkbSectionRec));
> + /* Reallocate and clear all items. */
> + tmp = _XkbGeomRealloc(dst->geom->sections, dst->geom->sz_sections, src->geom->num_sections,
> + 0, src->geom->num_sections, sizeof(XkbSectionRec));
> if (!tmp)
> return FALSE;
> - memset(tmp, 0, src->geom->num_sections * sizeof(XkbSectionRec));
> dst->geom->sections = tmp;
> dst->geom->num_sections = src->geom->num_sections;
> dst->geom->sz_sections = src->geom->num_sections;
> @@ -1813,16 +1813,11 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
> }
>
> if (src->geom->num_doodads) {
> - if (dst->geom->sz_doodads)
> - tmp = realloc(dst->geom->doodads,
> - src->geom->num_doodads *
> - sizeof(XkbDoodadRec));
> - else
> - tmp = malloc(src->geom->num_doodads *
> - sizeof(XkbDoodadRec));
> + /* Reallocate and clear all items. */
> + tmp = _XkbGeomRealloc(dst->geom->doodads, dst->geom->sz_doodads, src->geom->num_doodads,
> + 0, src->geom->num_doodads, sizeof(XkbDoodadRec));
> if (!tmp)
> return FALSE;
> - memset(tmp, 0, src->geom->num_doodads * sizeof(XkbDoodadRec));
> dst->geom->doodads = tmp;
>
> dst->geom->sz_doodads = src->geom->num_doodads;
> @@ -1860,20 +1855,14 @@ _XkbCopyGeom(XkbDescPtr src, XkbDescPtr dst)
>
> /* key aliases */
> if (src->geom->num_key_aliases) {
> - if (src->geom->num_key_aliases != dst->geom->sz_key_aliases) {
> - if (dst->geom->sz_key_aliases)
> - tmp = realloc(dst->geom->key_aliases,
> - src->geom->num_key_aliases *
> - 2 * XkbKeyNameLength);
> - else
> - tmp = malloc(src->geom->num_key_aliases *
> - 2 * XkbKeyNameLength);
> - if (!tmp)
> - return FALSE;
> - dst->geom->key_aliases = tmp;
> + /* Reallocate but don't clear any items. */
again, just wondering: is this the intended behaviour or just a bug in the original code?
Cheers,
Peter
> + tmp = _XkbGeomRealloc(dst->geom->key_aliases, dst->geom->sz_key_aliases, src->geom->num_key_aliases,
> + src->geom->num_key_aliases, src->geom->num_key_aliases, 2 * XkbKeyNameLength);
> + if (!tmp)
> + return FALSE;
> + dst->geom->key_aliases = tmp;
>
> - dst->geom->sz_key_aliases = src->geom->num_key_aliases;
> - }
> + dst->geom->sz_key_aliases = src->geom->num_key_aliases;
>
> memcpy(dst->geom->key_aliases, src->geom->key_aliases,
> src->geom->num_key_aliases * 2 * XkbKeyNameLength);
> --
> 1.6.3.3
>
More information about the xorg-devel
mailing list