Re: [chrony-dev] [PATCH v4 1/3] sys_linux: report if CAP_SYS_TIME is not present

[Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>]

On Wed, Mar 14, 2018 at 3:32 PM, Miroslav Lichvar <mlichvar@xxxxxxxxxx> wrote:

On Wed, Mar 14, 2018 at 03:05:29PM +0100, Christian Ehrhardt wrote:
> Instead of having adjtimex just fail with a permission issue
> improve the error messaging by warning for the lack of
> CAP_SYS_TIME on SYS_Linux_Initialise.
>
> Message will look like (instead of only the latter message):
>  CAP_SYS_TIME not present
>  adjtimex(0x8001) failed : Operation not permitted

Looks good.

>  /* ================================================== */
> +
> +static void SYS_Linux_ReportTimeAdjustBlockers(void)

Following the coding style, "static void" should be on its own line.
Also, as a static function it should be named report_time_adjust_blockers.

> +{
> +  if (CAP_IS_SUPPORTED(CAP_SYS_TIME) && cap_get_bound(CAP_SYS_TIME))
> +    return;
> +  LOG(LOGS_WARN, "CAP_SYS_TIME not present");

This code needs to be wrapped in #ifdef FEAT_PRIVDROP, otherwise it
will cause the compilation to fail without libcap even if no other
code is using the function.

> +}
> +
> +/* ================================================== */
>  /* Initialisation code for this module */
>
>  void
> @@ -388,6 +397,10 @@ SYS_Linux_Initialise(void)
>  {
>    get_version_specific_details();
>
> +#if defined (FEAT_PRIVDROP)
> +  SYS_Linux_ReportTimeAdjustBlockers();
> +#endif

The #if here is unnecessary.

I can fix these and push it if it's ok with you.

It is ok please go ahead, and vice versa if you want me to reroll for that let me know. 

 

Thanks,

--
Miroslav Lichvar

--
To unsubscribe email chrony-dev-request@chrony.tuxfamily.org with "unsubscribe" in the subject.
For help email chrony-dev-request@chrony.tuxfamily.org with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxx.org.

--

Christian Ehrhardt

Software Engineer, Ubuntu Server

Canonical Ltd