Re: [Linuxptp-devel] [PATCH RESEND v1 5/5] phc_ctl: Handle errors returned by various clockadj helpers

[Erez <ere...@gm...>]

On Tue, 5 Sept 2023 at 21:55, Rahul Rameshbabu via Linuxptp-devel <
lin...@li...> wrote:

> Do not print success notices if clockadj operation fails using return
> values provided by the clockadj helpers.
>

Good idea.


>
> Signed-off-by: Rahul Rameshbabu <rra...@nv...>
> ---
>  phc_ctl.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/phc_ctl.c b/phc_ctl.c
> index 34d9e0c..e174189 100644
> --- a/phc_ctl.c
> +++ b/phc_ctl.c
> @@ -232,9 +232,8 @@ static int do_adj(clockid_t clkid, int cmdc, char
> *cmdv[])
>         nsecs = (int64_t)(NSEC_PER_SEC * time_arg);
>
>         clockadj_init(clkid);
> -       clockadj_step(clkid, nsecs);
> -
> -       pr_notice("adjusted clock by %lf seconds", time_arg);
> +       if (!clockadj_step(clkid, nsecs))
>

I think a (clockadj_step(clkid, nsecs) == 0) is better.


> +               pr_notice("adjusted clock by %lf seconds", time_arg);
>
>
You can add else and return -2, clockadj_step() already prints an error
message.



>         /* adjustment always consumes one argument */
>         return 1;
> @@ -271,8 +270,8 @@ static int do_freq(clockid_t clkid, int cmdc, char
> *cmdv[])
>                 return -2;
>         }
>
> -       clockadj_set_freq(clkid, ppb);
> -       pr_notice("adjusted clock frequency offset to %lfppb", ppb);
> +       if (!clockadj_set_freq(clkid, ppb))
> +               pr_notice("adjusted clock frequency offset to %lfppb",
> ppb);
>

Same here.


>
>         /* consumed one argument to determine the frequency adjustment
> value */
>         return 1;
> @@ -308,10 +307,9 @@ static int do_phase(clockid_t clkid, int cmdc, char
> *cmdv[])
>         nsecs = (long)(NSEC_PER_SEC * offset_arg);
>
>         clockadj_init(clkid);
> -       clockadj_set_phase(clkid, nsecs);
> -
> -       pr_notice("offset of %lf seconds provided to PHC phase control
> keyword",
> -                 offset_arg);
> +       if (!clockadj_set_phase(clkid, nsecs))
> +               pr_notice("offset of %lf seconds provided to PHC phase
> control keyword",
> +                         offset_arg);
>

And here

Erez


>
>         /* phase offset always consumes one argument */
>         return 1;
> --
> 2.40.1
>
>
>
> _______________________________________________
> Linuxptp-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>