On 12/20/2012 02:20 PM, Simo Sorce wrote:
On Thu, 2012-12-20 at 12:29 +0100, Pavel Březina wrote:
On 12/20/2012 06:10 AM, Simo Sorce wrote:
-static errno_t sss_mc_get_record(struct sss_mc_ctx *mcc,
+static errno_t sss_mc_get_record(struct sss_mc_ctx **_mcc,
                                    size_t rec_len,
                                    struct sized_string *key,
                                    struct sss_mc_rec **_rec)
   {
+    struct sss_mc_ctx *mcc = *_mcc;
       struct sss_mc_rec *old_rec = NULL;
       struct sss_mc_rec *rec;
       int old_slots;
@@ -424,6 +425,16 @@ static errno_t sss_mc_get_record(struct sss_mc_ctx *mcc,
       /* we are going to use more space, find enough free slots */
       ret = sss_mc_find_free_slots(mcc, num_slots, &base_slot);
       if (ret != EOK) {
+        if (ret == EFAULT) {
+            TALLOC_CTX *parent;
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("Fatal internal mmap cache error, invalidating cache!\n"));
+            parent = talloc_parent(mcc);
+            if (parent == NULL) {
+                talloc_zfree(*_mcc);
+            }
+            (void)sss_mmap_cache_reinit(parent, -1, -1, _mcc);
+        }
           return ret;
       }

Hi,

Is if (parent == NULL) only precaution or can it actually happen?

Only a precaution, all the callers use an actual memory context.

If you talloc_zfree(*_mcc) then you will hit EINVAL in reinit() and the
cache won't be reinitialized. Is this intentional? If so, can you
comment it in the code?

Well it was not intentional I meant to return after the free.
However I am wondering if we should really care ...

If the caller had NULL as the parent something may have gone wrong, but
at worst we will leak a few bytes... maybe I shouldn't consider a NULL
parent as an error after all. Opinions ?

Simo.


Does the memory cache require a parent context for some reason (do you manipulate with the parent within the cache)?

If not I think you should not take care whether is has a parent or not. For what we know, it may be intentional by the caller. Otherwise you should check if mem_ctx != NULL in init().

Looking into the init() function I found out that you are using wrong context:

    mc_ctx->name = talloc_strdup(mem_ctx, name);
    if (!mc_ctx->name) {
        ret = ENOMEM;
        goto done;
    }

It should say mc_ctx not mem_ctx.

_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Reply via email to