RE: [PATCH 03/12] gprs: core support for IPv6
Hi Denis,
> +struct pri_context;
> +
> struct ofono_gprs_context {
> struct ofono_gprs *gprs;
> + struct pri_context *pri;
> enum ofono_gprs_context_type type;
> - ofono_bool_t inuse;
I don't see how getting rid of this is not a good idea. As
soon as the
context driver is assigned to a context being activated we set it as
'inuse'. If you don't set this variable then it is possible
to pick the
same driver, even if that driver is busy but has not started
to set its
settings yet.
The pri pointer points to the owner of the context driver. It is assigned when the context
driver is allocated and cleared when the context driver is released. Therefore, the inuse
flag becomes redundant. It is enough to check if pri != NULL to know if the context is in
use.
> +static gboolean assign_context(struct pri_context *ctx)
..
> +static void release_context(struct pri_context *ctx)
..
For something like this I'd really like to see it in a separate
patch.
You're moving the function and also modifying the
implementation. It is
much easier to review a patch set if you move a function in one patch
and then modify its behavior in another. Same with
release_context. It
is probably a good idea to put the common code in here, but modify the
behavior as a part of a different patch.
Ok, I'll split it out. I decided to do this when in the middle of making my changes I
noticed that the release code is actually copy pasted many times over.
Ideally this patch should be at least 2:
- For moving assign_context and factoring out common code into
release_context
- The rest of this patch + modifications to the two above.
Will do.
> +static struct context_settings *get_settings(struct
ofono_gprs_context *gc)
> +{
> + if (gc == NULL || gc->pri == NULL)
> + return NULL;
> +
> + if (gc->pri->settings == NULL)
> + gc->pri->settings = g_try_new0(struct
context_settings, 1);
> +
I appreciate the lazy allocation, however you get into trouble if the
driver calls one of the _set functions but still returns a failure in
the callback. Overall this seems a more complicated approach than
returning values in the callback of activate_primary.
Oops. I thought I had the failure path covered but this bit changed from the RFC patches.
The lazy allocation is a remnant from having separate dicts for IPv4 and IPv6 settings.
I'll get rid of it.
> +void ofono_gprs_context_set_interface(struct
ofono_gprs_context *gc,
> + const char *interface)
> +{
> + struct context_settings *settings = get_settings(gc);
> +
> + if (settings == NULL || settings->interface != NULL)
> + return;
I'd let the driver call these functions as often as they want and just
take the last value.
Ok. Makes sense.
Regards,
MikaL