On Wed, Jul 23, 2014 at 03:29:04AM -0400, Yassir Elley wrote:
> General question here -- what happens if two users whose GPO
policies
> reference the same files log in simultaneously? Do we clobber the files?
With the current patch, the answer is "yes" (the files are over-written).
However, in the next patch, I am including a gpo_cache_expire_timeout option, in which
case the answer will be "no". Specifically, if the policy files related to a
gpo-guid have been cached at a certain time, and if the same gpo-guid is needed again
within the expire_timeout time, whether by the same user or by a different user, then the
gpo code will re-use the existing files in the GPO_CACHE dir (and will not interact with
the SMB server at all). Does that make sense?
As long as there is a plan, I'm fine. Thank you for the explanation.
>
> > >+ if (ret != EOK) {
> > >+ DEBUG(SSSDBG_CRIT_FAILURE,
> > >+ "gpo_cache_store_file failed [%d][%s]\n",
> > >+ ret, strerror(ret));
> > >+ goto done;
> > >+ }
> > >+
> > >+ done:
> > >+ talloc_free(tmp_ctx);
> > >+ return ret;
> > >+}
> > >+
> > >+
> > >+/*
> > >+ * Using its smb_uri components and cached_gpt_version inputs, this
> > >function
> > >+ * does several things:
> > >+ * - it downloads the GPT_INI file to GPO_CACHE
> > >+ * - it parses the sysvol_gpt_version field from the GPT_INI file
> > >+ * - if the sysvol_gpt_version is greater than the cached_gpt_version
> > >+ * - it downloads the policy file to GPO_CACHE
> > >+ * - else
> > >+ * - it doesn't retrieve the policy file
> > >+ * - in this case, the backend will use the existing policy file in
> > >GPO_CACHE
> > >+ * - it returns the sysvol_gpt_version in the _sysvol_gpt_version output
> > >param
> > >+ *
> > >+ * Note that if the cached_gpt_version sent by the backend is -1 (to
> > >indicate
> > >+ * that no gpt_version has been set in the cache for the corresponding
> > >gpo_guid),
> > >+ * then the parsed sysvol_gpt_version (which must be at least 0) will be
> > >greater
> > >+ * than the cached_gpt_version, thereby triggering a fresh download.
> > >+ *
> > >+ * Note that the backend will later do the following:
> > >+ * - backend will save the the sysvol_gpt_version to sysdb cache
> > >+ * - backend will read the policy file from the GPO_CACHE
> > >+ */
> > >+static errno_t
> > >+perform_smb_operations(int cached_gpt_version,
> > >+ const char *smb_server,
> > >+ const char *smb_share,
> > >+ const char *smb_path,
> > >+ const char *smb_cse_suffix,
> > >+ int *_sysvol_gpt_version)
> > >+{
> > >+ SMBCCTX *smbc_ctx;
> > >+ int ret = 0;
> > I don't think we need to initialise ret to zero. From defensive
point
> > of
> > view it would be better to init with EINVAL. I think zero can be
> > removed.
>
> I think it's better to set ret to zero (or EOK) prior to entering the
> done label from the main control flow, like this:
>
> static int fn(ctx)
> {
> ret = do_stuff(ctx);
> if (ret != EOK) {
> goto done;
> }
>
> ret = EOK;
> done:
> return ret;
> }
Why? In your example, if do_stuff() returns EOK, won't ret already be set to EOK (by
the time it reaches the "ret = EOK;" statement)?
I might have confused you, I was answering to Lukas mostly. But in
general, the point I was trying to make was that I feel it's safer that
the the functions generally return EOK if the code flows all the way to
the done label w/o any jumps. (of course shortcuts are a special case
when there's nothing to do for the function for example.)
[...]
>
> I personally prefer a) here.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Revised patch attached, which addresses the last three code review emails for this patch
(Jakub-1, Lukas, Jakub-2).
Thanks,
Yassir.
The code now looks OK to me. I will just do some testing -- if you have
more patches that depend on this one, feel free to send them right away
so that you're not blocked on review.
Thanks for the fast turnaround fixing the issues we spotted.