Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should instance IDs (pcp::instance_id_type) been signed or unsigned? #11

Closed
pcolby opened this issue Dec 8, 2013 · 4 comments
Closed
Assignees
Labels
Milestone

Comments

@pcolby
Copy link
Owner

pcolby commented Dec 8, 2013

Currently pcp::instance_id_type is int.

In typical PMDA flow, instance IDs are first assigned to the PCP's pmdaInstid::i_inst (via the pmdaIndom table) as int.

However, PCP's pmdaFetchCallBack signature uses unsigned int to supply those same instance IDs back to the PMDA.

So it appears that there's no technically-right signedness to use for instance IDs. Instead, we should choose a signedness that provides the PMDA++ users the most intuitive behaviour.

Of course, testing the high-bit cases would be the right way to start.

@ghost ghost assigned pcolby Dec 21, 2013
@pcolby
Copy link
Owner Author

pcolby commented Dec 21, 2013

Pretty much as expected... all positive i_inst survive the trip through to pmdaFetchCallback, however negatives wrap, and become greater than or equal to 1<<31 (two's compliment).

In the end, there's no "right" or "wrong" approach here, since PCP's API is unconsistent, not PMDA++. But, I think it's pretty clear that unsigned int provides the most intuitive behaviour, because:

  1. PMDA++ hides the pmdaInstid::i_inst type anyway, so if we cast that internally to int for PCP's sake, the PMDA++ user does not need to know about it.
  2. If we stick with int, then the negative values (if someone chose to use them) would have the unintuitive behaviour of -2, -3, -4 ... etc all being valid / fine to use, but -1 being invalid / resulting in unexpected behavior as -1 is the same as PM_INDOM_NULL on many (most?) systems.

So, unsigned int it is (or soon will be).

@pcolby
Copy link
Owner Author

pcolby commented Dec 21, 2013

But here's a kicker... the pminfo program reports the instance IDs as signed :(

👎 for PCP consistency...

$ pminfo simple.color -f

simple.color
    inst [0 or "red"] value 123
    inst [2147483640 or "green"] value 123
    inst [-2 or "blue"] value 123
$

pcolby added a commit that referenced this issue Dec 21, 2013
…ot perfect) API consistency; unfortunately, PCP itself is inconsistent.
pcolby added a commit that referenced this issue Dec 21, 2013
@pcolby pcolby closed this as completed Dec 21, 2013
@pcolby
Copy link
Owner Author

pcolby commented Dec 21, 2013

Additionally, this value also sometimes get encoded into __pmInDom_int::serial, which is a 22-bit unsigned int.

@pcolby
Copy link
Owner Author

pcolby commented Feb 8, 2014

Note also, the pmdaCache* functions enforce the range 2^31 -1 (see man pmdaCache(3)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant