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

libidset: internal cleanup #1521

Merged
merged 12 commits into from May 16, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 16, 2018

This was the beginning of a larger PR to migrate the remainder of libutil/nodeset functionality to a public API libidset. Recall this was started in #1498 but the bulk of the work deferred.

After a discussion of priorities wtih @grondo this morning, I decided to work on something else, but this groundwork actually can stand alone as cleanup, so I thought I'd go ahead and submit it as its own PR.

garlick added some commits May 16, 2018

libidset: add format attribute to internal printf
Problem: internal catprintf() has printf style args,
but is not tagged as such so the compiler can check
argument types.

Add printf attribute to catprintf().
libidset: [cleanup] rename slots to size
Problem: use of the term 'slots' to refer to the
veb tree size, which is the maximum id (plus one)
that can be a member of the idset, is confusing.

Rename parameters from 'slots' to 'size' and adjust
comments.
libidset: use size_t for internal functions
Problem: catprintf() represents string sizes as
int rather than size_t, but libc string functions
use size_t.  The use of int reduces the type's
range and is ambiguous about the possiblity of
a negative error return value.

Use size_t for string sizes and lengths internally.
libidset: check strtoul for overflow
Problem: idsets are decoded using strtoul() to
convert strings to ints but strtoul could return
up to ULONG_MAX, causing overflow.

Check that strtoul() result is < UINT_MAX.
libidset: use unsigned int for id's
Problem: id's are represented as signed integers,
reducing range and creating ambiguity about what
a negative id means.

Convert id's to unsigned int type.
libidset: handle reverse ranges
Problem: idset_decode() cannot handle a range
expressed in reverse order, e.g. 9-7 instead of 7-9.

Fix parse_range() to maintain invariant of lo <= hi.
libidset: encode large ranges
Problem: if an idset includes more than INT_MAX id's,
encoding fails.

Avoid wrapping the integer return value of internal
functions encode_ranged() and encode_simple().
Return INT_MAX in that case, which doesn't cause a
problem the way this count is used internally.
libidset/test: cover buffer growth
Problem: so called large idset 0-1023 doesn't
exceed DECODE_SIZE of 1024.

Bump size to 5000 so that idset_decode() will
have to grow the idset to 8192 while decoding.
libidset: fix off by one error in idset_grow()
Problem: idset_grow() grows the idset if the
requested size equals the current size.

Change <= to <.
libidset: define DECODE_SIZE
Problem: idset_decode() hardwires a value of 1024
as the initial idset size, hidden in the function
implementation.

Add a preprocessor macro so the hardwired value
is prominent at top of source module.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 16, 2018

Coverage Status

Coverage increased (+0.05%) to 78.91% when pulling dd4ed44 on garlick:idset_cleanup into 596d94e on flux-framework:master.

@garlick garlick changed the title libidset: internal cleanup preparing for full implementation libidset: internal cleanup May 16, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #1521 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1521      +/-   ##
==========================================
+ Coverage   78.56%   78.61%   +0.04%     
==========================================
  Files         165      165              
  Lines       30817    30823       +6     
==========================================
+ Hits        24212    24232      +20     
+ Misses       6605     6591      -14
Impacted Files Coverage Δ
src/common/libidset/idset.c 89.03% <100%> (+0.44%) ⬆️
src/common/libkvs/kvs_watch.c 89.69% <0%> (-0.43%) ⬇️
src/common/libflux/message.c 81.41% <0%> (+0.23%) ⬆️
src/cmd/flux-module.c 85.36% <0%> (+0.3%) ⬆️
src/common/libflux/future.c 89.42% <0%> (+0.44%) ⬆️
src/broker/content-cache.c 75.37% <0%> (+0.64%) ⬆️
src/broker/modservice.c 80.58% <0%> (+0.97%) ⬆️
src/modules/connector-local/local.c 74.38% <0%> (+1.43%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 16, 2018

LGTM!

@grondo grondo merged commit fdf0891 into flux-framework:master May 16, 2018

4 checks passed

codecov/patch 100% of diff hit (target 78.56%)
Details
codecov/project 78.61% (+0.04%) compared to 596d94e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 78.91%
Details

@garlick garlick deleted the garlick:idset_cleanup branch May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.