The subject says "potential" leak, but in fact it looks like we leaked
every single time we succeeded.
On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> The convention in all other parts of the code that call run_pmc() is to
> free the management PTP message if an error code was returned, or pass
> the message to the caller otherwise.
>
> run_pmc_events() wasn't doing that, and was leaking a reference to the
> message, while also discarding the return code from run_pmc(). Fix that.
>
> Signed-off-by: Vladimir Oltean <olte...@gmail.com>
> ---
> phc2sys.c | 8 +++++++-
> pmc_common.c | 9 +++++++--
> pmc_common.h | 2 +-
> 3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/phc2sys.c b/phc2sys.c
> index c4d72bd7d17a..6c048b887d0b 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -702,6 +702,7 @@ static int do_loop(struct phc2sys_private *priv, int
> subscriptions)
> struct clock *clock;
> uint64_t ts;
> int64_t offset, delay;
> + int err;
>
> interval.tv_sec = priv->phc_interval;
> interval.tv_nsec = (priv->phc_interval - interval.tv_sec) * 1e9;
> @@ -712,7 +713,12 @@ static int do_loop(struct phc2sys_private *priv, int
> subscriptions)
> continue;
>
> if (subscriptions) {
> - run_pmc_events(&priv->node);
> + err = run_pmc_events(&priv->node);
> + if (err) {
> + pr_err("run_pmc_events returned %d", err);
> + return err;
> + }
> +
> if (priv->state_changed) {
> /* force getting offset, as it may have
> * changed after the port state change */
> diff --git a/pmc_common.c b/pmc_common.c
> index 89d7f40b84fe..cdc1eb4616ae 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -939,11 +939,16 @@ int run_pmc_subscribe(struct pmc_node *node, int
> timeout)
> return 1;
> }
>
> -void run_pmc_events(struct pmc_node *node)
> +int run_pmc_events(struct pmc_node *node)
> {
> struct ptp_message *msg;
> + int res;
>
> - run_pmc(node, 0, -1, &msg);
> + res = run_pmc(node, 0, -1, &msg);
> + if (res <= 0)
> + return res;
> + msg_put(msg);
Ok, so this is a bit confusing, and I think it is right but the commit
message is wrong.
run_pmc appears to call msg_put whenever it fails, so we do want to
avoid calling msg_put on a failure.
But either (a) we were always leaking the msg because we weren't
cleaning it up before or (b) on success run_pmc_events shouldn't cleanup
the message since it would be used by the caller.. but that doesn't make
sense since the msg pointer is a local variable.
Based on the other callers, I think this is right, but the commit
message is wrong.
> + return 1;
> }
>
> int run_pmc_port_properties(struct pmc_node *node, int timeout,
> diff --git a/pmc_common.h b/pmc_common.h
> index a28bab767e9c..a3c7e956cba6 100644
> --- a/pmc_common.h
> +++ b/pmc_common.h
> @@ -76,7 +76,7 @@ int run_pmc_subscribe(struct pmc_node *node, int timeout);
> int run_pmc_clock_identity(struct pmc_node *node, int timeout);
> int run_pmc_wait_sync(struct pmc_node *node, int timeout);
> int run_pmc_get_number_ports(struct pmc_node *node, int timeout);
> -void run_pmc_events(struct pmc_node *node);
> +int run_pmc_events(struct pmc_node *node);
> int run_pmc_port_properties(struct pmc_node *node, int timeout,
> unsigned int port, int *state,
> int *tstamping, char *iface);
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
