Review Request 98: Hardware: Added Processor Cache Memory Provider

[Stephen Gallagher]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/98/
-----------------------------------------------------------

Review request for OpenLMI Developers.


Repository: openlmi-providers


Description
-------

Hardware: Added Processor Cache Memory Provider

New Providers:
    * LMI_ProcessorCacheMemoryProvider
    * LMI_AssociatedProcessorCacheMemoryProvider

Other Changes:
    * Optimized usage of string constants
    * Fixed wrong usage of pointers in dmidecode.c
    * Filled unknown mandatory fields in providers with "Unknown" value
    * Replaced hard coded numbers with LMI constants
    * Minor optimization - don't gather data which won't be used

(Sorry for unrelated changes in one patch. I'll be more cautious next time!)


Diffs
-----

  mof/LMI_Hardware.mof e655c012f542509a313e496a68ea7b85957aae6d 
  src/hardware/CMakeLists.txt 352cebec53d9d6b877f81fd79a16219bb7e9c946 
  src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c PRE-CREATION 
  src/hardware/LMI_Hardware.h e93f9fe292e82d7761114425877efc7dfc38f3b9 
  src/hardware/LMI_ProcessorCacheMemoryProvider.c PRE-CREATION 
  src/hardware/LMI_ProcessorCapabilitiesProvider.c
c9f7e14675de5c4eab104d02230e56afc0a4b094 
  src/hardware/LMI_ProcessorElementCapabilitiesProvider.c
a477717789026c8522415f7114cc2070eaca22e3 
  src/hardware/LMI_ProcessorProvider.c c3b78e2974c9d2da3da8ff1a9595102c0c14d590 
  src/hardware/dmidecode.h 9a8c6c8c4cf745edc313f92150272cc9a3661007 
  src/hardware/dmidecode.c 14e4ac9fb21aa2467ac04c5830c9ba494c8cf80e 
  src/hardware/sysfs.h PRE-CREATION 
  src/hardware/sysfs.c PRE-CREATION 

Diff: http://reviewboard-openlmi.rhcloud.com/r/98/diff/


Testing
-------


Thanks,

Peter Schiffer

[Stephen Gallagher]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/98/#review97
-----------------------------------------------------------



src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c
<http://reviewboard-openlmi.rhcloud.com/r/98/#comment53>

    Nitpicking, no error found but: This whole if{} is being repeated several times in the
function. I think it would be better to:
    
    #define UNIT_STR_LEN 16
    char uint_str[UNIT_STR_LEN];
    ...
    for (...) {
      if (...) {
        snprintf(uint_str, UINT_STR_LEN, "%u", ...);
      }
      ...
    }
    
    It would save us the allocs, tests and frees in the loop and shorten the function a
bit...



src/hardware/LMI_ProcessorProvider.c
<http://reviewboard-openlmi.rhcloud.com/r/98/#comment54>

    Nitpicks again: Superflous if -- 'man 3 free' says "If ptr is NULL, no
operation is performed."
    



src/hardware/dmidecode.c
<http://reviewboard-openlmi.rhcloud.com/r/98/#comment55>

    I think using '&&' would make the code more readable than the if
nesting.



src/hardware/dmidecode.c
<http://reviewboard-openlmi.rhcloud.com/r/98/#comment56>

    Again: if before free... Many times actually.


In general -- there seems to be many blocks of very similar code.  I prefer using
functions or even macros in such cases.  It doesn't hinder the code readability but
saves some typing and reading. Also I understand the issues I found are not errors and in
fact some might disagree with my taste therefore I didn't open issues for them.

- Tomas Smetana


On April 4, 2013, 7:24 p.m., Peter Schiffer wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-openlmi.rhcloud.com/r/98/
>  -----------------------------------------------------------
>  
>  (Updated April 4, 2013, 7:24 p.m.)
>  
>  
>  Review request for OpenLMI Developers.
>  
>  
>  Repository: openlmi-providers
>  
>  
>  Description
>  -------
>  
>  Hardware: Added Processor Cache Memory Provider
>  
>  New Providers:
>      * LMI_ProcessorCacheMemoryProvider
>      * LMI_AssociatedProcessorCacheMemoryProvider
>  
>  Other Changes:
>      * Optimized usage of string constants
>      * Fixed wrong usage of pointers in dmidecode.c
>      * Filled unknown mandatory fields in providers with "Unknown" value
>      * Replaced hard coded numbers with LMI constants
>      * Minor optimization - don't gather data which won't be used
>  
>  (Sorry for unrelated changes in one patch. I'll be more cautious next time!)
>  
>  
>  Diffs
>  -----
>  
>    mof/LMI_Hardware.mof e655c012f542509a313e496a68ea7b85957aae6d 
>    src/hardware/CMakeLists.txt 352cebec53d9d6b877f81fd79a16219bb7e9c946 
>    src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_Hardware.h e93f9fe292e82d7761114425877efc7dfc38f3b9 
>    src/hardware/LMI_ProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_ProcessorCapabilitiesProvider.c
> c9f7e14675de5c4eab104d02230e56afc0a4b094 
>    src/hardware/LMI_ProcessorElementCapabilitiesProvider.c
> a477717789026c8522415f7114cc2070eaca22e3 
>    src/hardware/LMI_ProcessorProvider.c c3b78e2974c9d2da3da8ff1a9595102c0c14d590 
>    src/hardware/dmidecode.h 9a8c6c8c4cf745edc313f92150272cc9a3661007 
>    src/hardware/dmidecode.c 14e4ac9fb21aa2467ac04c5830c9ba494c8cf80e 
>    src/hardware/sysfs.h PRE-CREATION 
>    src/hardware/sysfs.c PRE-CREATION 
>  
>  Diff: http://reviewboard-openlmi.rhcloud.com/r/98/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Peter Schiffer
>  
>

[Stephen Gallagher]

>  On April 5, 2013, 1:29 p.m., Tomas Smetana wrote:
>  > src/hardware/dmidecode.c, lines 105-110
>  > <http://reviewboard-openlmi.rhcloud.com/r/98/diff/1/?file=508#file508line1...
>  >
>  >     I think using '&&' would make the code more readable than the if
> nesting. 
It wouldn't, 'goto done' is executed only if strdup fails, otherwise the
function continues with cpu->l1_cache_handle=="".


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/98/#review97
-----------------------------------------------------------


On April 4, 2013, 7:24 p.m., Peter Schiffer wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-openlmi.rhcloud.com/r/98/
>  -----------------------------------------------------------
>  
>  (Updated April 4, 2013, 7:24 p.m.)
>  
>  
>  Review request for OpenLMI Developers.
>  
>  
>  Repository: openlmi-providers
>  
>  
>  Description
>  -------
>  
>  Hardware: Added Processor Cache Memory Provider
>  
>  New Providers:
>      * LMI_ProcessorCacheMemoryProvider
>      * LMI_AssociatedProcessorCacheMemoryProvider
>  
>  Other Changes:
>      * Optimized usage of string constants
>      * Fixed wrong usage of pointers in dmidecode.c
>      * Filled unknown mandatory fields in providers with "Unknown" value
>      * Replaced hard coded numbers with LMI constants
>      * Minor optimization - don't gather data which won't be used
>  
>  (Sorry for unrelated changes in one patch. I'll be more cautious next time!)
>  
>  
>  Diffs
>  -----
>  
>    mof/LMI_Hardware.mof e655c012f542509a313e496a68ea7b85957aae6d 
>    src/hardware/CMakeLists.txt 352cebec53d9d6b877f81fd79a16219bb7e9c946 
>    src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_Hardware.h e93f9fe292e82d7761114425877efc7dfc38f3b9 
>    src/hardware/LMI_ProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_ProcessorCapabilitiesProvider.c
> c9f7e14675de5c4eab104d02230e56afc0a4b094 
>    src/hardware/LMI_ProcessorElementCapabilitiesProvider.c
> a477717789026c8522415f7114cc2070eaca22e3 
>    src/hardware/LMI_ProcessorProvider.c c3b78e2974c9d2da3da8ff1a9595102c0c14d590 
>    src/hardware/dmidecode.h 9a8c6c8c4cf745edc313f92150272cc9a3661007 
>    src/hardware/dmidecode.c 14e4ac9fb21aa2467ac04c5830c9ba494c8cf80e 
>    src/hardware/sysfs.h PRE-CREATION 
>    src/hardware/sysfs.c PRE-CREATION 
>  
>  Diff: http://reviewboard-openlmi.rhcloud.com/r/98/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Peter Schiffer
>  
>

[Stephen Gallagher]

>  On April 5, 2013, 1:29 p.m., Tomas Smetana wrote:
>  > src/hardware/dmidecode.c, lines 105-110
>  > <http://reviewboard-openlmi.rhcloud.com/r/98/diff/1/?file=508#file508line1...
>  >
>  >     I think using '&&' would make the code more readable than the if
> nesting.
>  
>  Jan Safranek wrote:
>      It wouldn't, 'goto done' is executed only if strdup fails, otherwise the
> function continues with cpu->l1_cache_handle=="". 
Ahh... right. Sorry. I missed that...


- Tomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/98/#review97
-----------------------------------------------------------


On April 4, 2013, 7:24 p.m., Peter Schiffer wrote:
>  
>  -----------------------------------------------------------
>  This is an automatically generated e-mail. To reply, visit:
>  http://reviewboard-openlmi.rhcloud.com/r/98/
>  -----------------------------------------------------------
>  
>  (Updated April 4, 2013, 7:24 p.m.)
>  
>  
>  Review request for OpenLMI Developers.
>  
>  
>  Repository: openlmi-providers
>  
>  
>  Description
>  -------
>  
>  Hardware: Added Processor Cache Memory Provider
>  
>  New Providers:
>      * LMI_ProcessorCacheMemoryProvider
>      * LMI_AssociatedProcessorCacheMemoryProvider
>  
>  Other Changes:
>      * Optimized usage of string constants
>      * Fixed wrong usage of pointers in dmidecode.c
>      * Filled unknown mandatory fields in providers with "Unknown" value
>      * Replaced hard coded numbers with LMI constants
>      * Minor optimization - don't gather data which won't be used
>  
>  (Sorry for unrelated changes in one patch. I'll be more cautious next time!)
>  
>  
>  Diffs
>  -----
>  
>    mof/LMI_Hardware.mof e655c012f542509a313e496a68ea7b85957aae6d 
>    src/hardware/CMakeLists.txt 352cebec53d9d6b877f81fd79a16219bb7e9c946 
>    src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_Hardware.h e93f9fe292e82d7761114425877efc7dfc38f3b9 
>    src/hardware/LMI_ProcessorCacheMemoryProvider.c PRE-CREATION 
>    src/hardware/LMI_ProcessorCapabilitiesProvider.c
> c9f7e14675de5c4eab104d02230e56afc0a4b094 
>    src/hardware/LMI_ProcessorElementCapabilitiesProvider.c
> a477717789026c8522415f7114cc2070eaca22e3 
>    src/hardware/LMI_ProcessorProvider.c c3b78e2974c9d2da3da8ff1a9595102c0c14d590 
>    src/hardware/dmidecode.h 9a8c6c8c4cf745edc313f92150272cc9a3661007 
>    src/hardware/dmidecode.c 14e4ac9fb21aa2467ac04c5830c9ba494c8cf80e 
>    src/hardware/sysfs.h PRE-CREATION 
>    src/hardware/sysfs.c PRE-CREATION 
>  
>  Diff: http://reviewboard-openlmi.rhcloud.com/r/98/diff/
>  
>  
>  Testing
>  -------
>  
>  
>  Thanks,
>  
>  Peter Schiffer
>  
>

[Stephen Gallagher]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/98/
-----------------------------------------------------------

(Updated April 8, 2013, 4:56 p.m.)


Status
------

This change has been marked as submitted.


Review request for OpenLMI Developers.


Repository: openlmi-providers


Description
-------

Hardware: Added Processor Cache Memory Provider

New Providers:
    * LMI_ProcessorCacheMemoryProvider
    * LMI_AssociatedProcessorCacheMemoryProvider

Other Changes:
    * Optimized usage of string constants
    * Fixed wrong usage of pointers in dmidecode.c
    * Filled unknown mandatory fields in providers with "Unknown" value
    * Replaced hard coded numbers with LMI constants
    * Minor optimization - don't gather data which won't be used

(Sorry for unrelated changes in one patch. I'll be more cautious next time!)


Diffs
-----

  mof/LMI_Hardware.mof e655c012f542509a313e496a68ea7b85957aae6d 
  src/hardware/CMakeLists.txt 352cebec53d9d6b877f81fd79a16219bb7e9c946 
  src/hardware/LMI_AssociatedProcessorCacheMemoryProvider.c PRE-CREATION 
  src/hardware/LMI_Hardware.h e93f9fe292e82d7761114425877efc7dfc38f3b9 
  src/hardware/LMI_ProcessorCacheMemoryProvider.c PRE-CREATION 
  src/hardware/LMI_ProcessorCapabilitiesProvider.c
c9f7e14675de5c4eab104d02230e56afc0a4b094 
  src/hardware/LMI_ProcessorElementCapabilitiesProvider.c
a477717789026c8522415f7114cc2070eaca22e3 
  src/hardware/LMI_ProcessorProvider.c c3b78e2974c9d2da3da8ff1a9595102c0c14d590 
  src/hardware/dmidecode.h 9a8c6c8c4cf745edc313f92150272cc9a3661007 
  src/hardware/dmidecode.c 14e4ac9fb21aa2467ac04c5830c9ba494c8cf80e 
  src/hardware/sysfs.h PRE-CREATION 
  src/hardware/sysfs.c PRE-CREATION 

Diff: http://reviewboard-openlmi.rhcloud.com/r/98/diff/


Testing
-------


Thanks,

Peter Schiffer