linux-core.c: ensure that all CPUs have model information of some kind #663

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

tfeb commented Dec 21, 2012

This patch makes sure that none of the model fields can be null: it either infers them from earlier fields or if no information was got it fills them with a default value ("unknown" in this fix).

This is not a problem on x86 but it can be on some other platforms where /proc/cpuinfo has a different format.

This fix makes no change to what is returned on platforms which were already getting all the model information (ie x86).

If the fields are left null then node calls like os.cpus() will cause bad things to happen (node will die).

src/unix/linux/linux-core.c
+ */
+ if (model_idx == 0) {
+ /* no models at all: fake up the first one */
+ ci[0].model = strndup(bogus_model, strlen(bogus_model));
@bnoordhuis

bnoordhuis Dec 28, 2012

Contributor

Using strndup() here is superfluous if you're calling strlen() anyway.

@bnoordhuis

bnoordhuis Dec 28, 2012

Contributor

Though you could use sizeof(bogus_model) - 1 here.

@tfeb

tfeb Dec 29, 2012

Contributor

On 28 Dec 2012, at 13:26, Ben Noordhuis wrote:

Using strndup() here is superfluous if you're calling strlen() anyway.

I think so too, but this was copying what other code close to that did on the grounds of being stylistically-compatible with it. Would be happy to change it either way.

@bnoordhuis

bnoordhuis Dec 31, 2012

Contributor

In case of doubt, pick the most efficient approach. sizeof(bogus_model) - 1 is guaranteed to compile down to a compile-time constant.

@tfeb

tfeb Dec 31, 2012

Contributor

On 31 Dec 2012, at 17:18, Ben Noordhuis wrote:

In case of doubt, pick the most efficient approach. sizeof(bogus_model) - 1 is guaranteed to compile down to a compile-time constant.

OK, I've pushed a commit which does that for bogus_model.

src/unix/linux/linux-core.c
+ * the remaining ones have been guessed.
+ */
+ char* inferred_model = ci[model_idx -1].model;
+ unsigned int i;
@bnoordhuis

bnoordhuis Dec 28, 2012

Contributor

Declarations go at the top.

@tfeb

tfeb Dec 29, 2012

Contributor

Do you want me to fix these (and the other changes) and send another set of pull requests?

Thanks

On 28 Dec 2012, at 13:28, Ben Noordhuis wrote:

In src/unix/linux/linux-core.c:

  • /* Now we want to make sure that all the models contain something:
  • * it's not safe to leave them as null.
  • */
  • if (model_idx == 0) {
  • /* no models at all: fake up the first one */
  • ci[0].model = strndup(bogus_model, strlen(bogus_model));
  • model_idx = 1;
  • }
  • if (model_idx < numcpus - 1) {
  • /* not enough models, but we do have at least one. So we'll just
  • \* copy the rest down: it might be better to indicate somehow that
    
  • \* the remaining ones have been guessed.
    
  • */
    
  • char* inferred_model = ci[model_idx -1].model;
  • unsigned int i;
    Declarations go at the top.


Reply to this email directly or view it on GitHub.

Contributor

bnoordhuis commented Jan 8, 2013

Thanks Tim, landed in bc0c61c.

@bnoordhuis bnoordhuis closed this Jan 8, 2013

Contributor

bnoordhuis commented Jan 8, 2013

Oh, I simplified the patch a little; I removed a borderline superfluous if check. The less indentation, the easier code is to read.

Contributor

tfeb commented Jan 8, 2013

On 8 Jan 2013, at 04:32, Ben Noordhuis wrote:

Oh, I simplified the patch a little; I removed a borderline superfluous if check. The less indentation, the easier code is to read.

Yes, that's better than my version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment