[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v2] hwclock: add get/set parameters option
From:       Andrej Picej <andrej.picej () norik ! com>
Date:       2023-07-18 9:10:26
Message-ID: 0eeb0d49-4f40-078c-00be-5cf7addcec32 () norik ! com
[Download RAW message or body]

Hi Denys,

On 12. 07. 23 16:28, Denys Vlasenko wrote:
> On Tue, Jul 11, 2023 at 10:43 AM Andrej Picej <andrej.picej@norik.com> wrote:
>> --- a/include/rtc_.h
>> +++ b/include/rtc_.h
>> @@ -22,6 +22,11 @@ int rtc_xopen(const char **default_rtc, int flags) FAST_FUNC;
>>   void rtc_read_tm(struct tm *ptm, int fd) FAST_FUNC;
>>   time_t rtc_tm2time(struct tm *ptm, int utc) FAST_FUNC;
>>
>> +struct hwclock_param {
>> +       int id;
>> +       const char *name;
>> +       const char *help;
>> +};
> 
> Can be in hwclock.c, its only user
> 
>>   /*
>>    * Everything below this point has been copied from linux/rtc.h
>> @@ -46,6 +51,17 @@ struct linux_rtc_wkalrm {
>>          struct linux_rtc_time time;  /* time the alarm is set to */
>>   };
>>
>> +struct rtc_param {
>> +       __u64 param;
>> +       union {
>> +               __u64 uvalue;
>> +               __s64 svalue;
>> +               __u64 ptr;
>> +       };
>> +       __u32 index;
>> +       __u32 __pad;
>> +};
> 
> Should use uint64_t and such, noit kernel internal typedefs.
> 
>> +static const struct hwclock_param hwclock_params[] =
>> +{
>> +       { RTC_PARAM_FEATURES,  "features", "supported features" },
>> +       { RTC_PARAM_CORRECTION, "correction", "time correction" },
>> +       { RTC_PARAM_BACKUP_SWITCH_MODE, "bsm", "backup switch mode" },
>> +       { }
>> +};
> 
> help strings are unused.
> 
>> +const struct hwclock_param *get_hwclock_params(void)
>> +{
>> +       return hwclock_params;
>> +}
> 
> util-linux/hwclock.c:65:29: error: no previous prototype for
> ‘get_hwclock_params'
> 
> - unused function
> 
>> +static void set_rtc_param(const char **pp_rtcname, const char *rtc_param)
>> +{
>> +       int rtc;
>> +       struct rtc_param param = { .param = 0 };
> 
> Why zeroing?
> 
>> +       char *tok, *opt = xstrdup(rtc_param);
> 
> Why strdup? Using the string in-place is more efficient
> 
>> +
>> +       /* handle param name */
>> +       tok = strtok(opt, "=");
>> +       if (resolve_rtc_param_alias(tok, &param.param) != 0) {
>> +               param.param = bb_strtoull(tok, NULL, 0);
>> +               if (errno)
>> +                       bb_error_msg_and_die("could not convert parameter name: '%s' to number",
>> +                                            tok);
> 
> Duplicated code. Too verbose message. Can just use xstrtoull():
> 
> static uint64_t resolve_rtc_param_alias(const char *alias)
> {
>          int n;
> 
>          BUILD_BUG_ON(RTC_PARAM_FEATURES != 0
>                  || RTC_PARAM_CORRECTION != 1
>                  || RTC_PARAM_BACKUP_SWITCH_MODE != 2
>          );
>          n = index_in_strings(
>                  "features"   "\0"
>                  "correction" "\0"
>                  "bsm"        "\0"
>                  , alias);
>          if (n >= 0)
>                  return n;
>          return xstrtoull(alias, 0);
> }
> 
> 
>> +       }
>> +
> ) pulls in > +       /* handle param value */
>> +       tok = strtok(NULL, "=");
> 
> strtok() pulls in a global variable, increasing bss.
> 
>> const char *param = NULL;
> 
> Why zeroing?
> 
> 
> After addressing these, the size is twice as small:
> 
> function                                             old     new   delta
> hwclock_main                                         298     756    +458
> .rodata                                           105231  105526    +295
> strtok                                                 -     148    +148
> hwclock_params                                         -      48     +48
> packed_usage                                       34541   34576     +35
> static.hwclock_longopts                               60      84     +24
> static.p                                               -       4      +4
> ------------------------------------------------------------------------------
> (add/remove: 4/0 grow/shrink: 4/0 up/down: 1012/0)           Total: 1012 bytes
> 
> 
> function                                             old     new   delta
> hwclock_main                                         298     584    +286
> .rodata                                           105231  105400    +169
> packed_usage                                       34541   34576     +35
> static.hwclock_longopts                               60      84     +24
> ------------------------------------------------------------------------------
> (add/remove: 0/0 grow/shrink: 4/0 up/down: 514/0)             Total: 514 bytes

First of all sorry for late answer, I was quite busy the last week. Just 
as I wanted to analyse your comments and prepare a v2 I saw that you 
already fixed what was necessary and applied the patch. Thanks for that.

Next time I'll check the size and optimize the code.

Best regards,
Andrej
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]