[Mesa-dev] [PATCH v3] HUD: Add support for block I/O, network I/O and lmsensor stats

[Brian Paul]

On 09/27/2016 05:17 PM, Steven Toth wrote:
> On Fri, Sep 23, 2016 at 12:19 PM, Brian Paul <brianp at vmware.com> wrote:
>> Hi Steven,
>>
>> I did a more thorough review per your request...
>
> Thank you Brian.
>
> All of your suggestions have been implemented, and new patches pushed to the ML.

Were you planning on squashing the two patches?  I think you should.

BTW, I see that some of the new changes could use const qualifiers.  For 
example:

@@ -97,10 +97,10 @@ find_dsi_by_name(char *n, int mode)
  }

  static int
-get_file_values(struct diskstat_info *dsi, struct stat_s *s)
+get_file_values(char *fn, struct stat_s *s)

That could be 'const char *fn'.  Same thing in other places.

Sorry to be nit picky about const qualifiers, but they're pretty helpful 
when reading code to help understand which arguments may or may not be 
modified by a function.


>
> ...with the exception of one, primarily because I wanted to comment.
>
>>> +#if HAVE_LIBSENSORS
>>> +      else if (sscanf(name, "sensors_temp_cu-%s", arg_name) == 1) {
>>> +         hud_sensors_temp_graph_install(pane, &name[16],
>>
>>
>> What's the significance of name[16]?  Should that be a #define ?
>
> Everything after the hyphen is essentially its unique sensor name,
> prior to the hyphen is a routing string that tells mesa HUD us to use
> the lmsensor HUD module, rather than say... the disk stats module.
>
> So 16, is the length of "sensors_temp_cu-" and we pass the remainder
> into the sensor specific initializer func - which is all it cares
> about.
>
> I'm happy to implement whatever the project recommends, so are you
> suggesting instead:
>
> #define SOMEPREFIX "sensors_temp_cu-"
> then
> hud_sensors_temp_graph_install(pane, &name[sizeof(SOMEPREFIX - 1)]
>
> Or, have I misunderstood your comment?

Thanks for explaining.  But why not do this?

       else if (sscanf(name, "sensors_temp_cu-%s", arg_name) == 1) {
          hud_sensors_temp_graph_install(pane, arg_name, ...

as you did in other places?

-Brian