Hopefully the final merge ofthe pre-factored code! #119

tacketar · 2016-09-01T22:41:16Z

No description provided.

The final log statement uses the segment ID which could be invalid because once the flushing_count is decremented the segment_destroy call could complete and destroy the actual segment.

…g down

…misc.c

This is not ideal there area couple of options 1) add a string helper function 2) just make the cap a printable string. I don't think there is much an issue doing this.

PerilousApricot · 2016-09-01T22:44:10Z

@tacketar In the future, please push your branches to your own fork then issue the PR from your fork to the main one. It's not a huge deal, but it kinda busts things up with Jenkins a bit to have the origin branch be from the same remote. The change in procedure is effectively push origin feature/partyhouse with push me feature/partyhouse.

I can tidy it up manually, but it's much preferred in the future.

tacketar · 2016-09-01T22:47:43Z

Will do. Sorry about that.

PerilousApricot · 2016-09-01T22:48:40Z

No biggie. You're just now seeing all the bits that I've been slamming on for a few months. Notice the nice "pending check"? :)

PerilousApricot · 2016-09-01T22:50:10Z

src/lio/bin/lio_mv.c

@@ -194,13 +197,15 @@ int main(int argc, char **argv)
        } else {
            info_printf(lio_ifd, 0, "ERROR: Multiple paths selected but the dest(%s) isn't a directory!\n", dtuple.path);
        }
+        n_errors = 1;


Is n_errors supposed the be the number of errors? Should this be += and not =?

Look at the line right after. It goes and kicks out. So +=1 or = 1 accomplish the same thing.

OK. But what is n_errors supposed to be? Up top, it's the counter of the errors that happened. At lines 289 and 300, you add up the error codes for any child GOPs, then you return it to the caller. Is it the number of errors? Is it "Sum of error codes" + "number of errors"? What does that number mean? Are there some sort of semantics to the exit code a user is supposed to interpret?

It's really more of a binary value. 0=Success and anything else means a
failure. You could change the final return to:
return((n_errors == 0) ? 0 : EIO)

That's probably best. If that change happens, then there's no need to even walk all the gop error codes, right?

Yup. But at that point in the code there aren't any GOP changes to walk. That's before doing any tasks.

PerilousApricot · 2016-09-02T03:26:00Z

src/lio/os/file.c

@@ -1128,7 +1128,7 @@ int va_timestamp_set_attr(lio_os_virtual_attr_t *va, lio_object_service_fn_t *os
    n = (int)(long)va->priv;  //** HACKERY ** to get the attribute prefix length
    key = &(fullkey[n+1]);

-    if (strlen(fullkey) < n) {  //** Nothing to do so return;
+    if ((int)strlen(fullkey) < n) {  //** Nothing to do so return;


This diff is a ton of signed->unsigned casts. What is the justification? Does that break C's UB guarantees?

I was under the impression we were trying to eliminate ALL warnings from the compile. All of these changes were warnings in my build.

That's true. But blindly casting away the error is not always correct,
hence my question. Signedness has strange spec-defined behavior that you
could be papering over.

It's dark in this basement.

Elaborate? What are you looking for? I did look at all of them each individually and didn't see any issues. Should we document each warning as well? IF so it should be in the guide.

The C behavior around signed and unsigned comparisons and implicit conversions is damned bonkers.

unsigned int x = 1; int y = -1; // Surprisingly false return (x > y);

On the other hand, this gets the conversions right

return (x - y > 0);

... go figure. In this case, though, it probably makes sense to have n be defined as unsigned int, right? I wasn't able to sort out what n = (int)(long)va->priv; actually resolved to, but if it's a length, changing the type is both more clear to the humans and sidesteps the compiler being truly surprising.

(I'm also curious about what the double conversion does at the end of the day. Does the compiler treat (int)(long) differently than (int)? Or does it optimize it all away?)

I really hate unsigned variables of any type. I've been burned by these conversion issues more than I can count. As a result I only rarely use unsigned variables. Making the change here is fine but I would require the name be changed from n ->n_unsigned or something similar. Otherwise a wonderfully generic name like "n" gets used to store negative numbers causing all sorts of chaos. The double conversion is because "n" is an "int" which isn't always the same as a "long" and leads to another warning. The whole purpose of the weirdness is to keep from having to do a malloc just to store an integer.

Another name that would work is count, I'd suppose

Nooooooooooooooooooooooo! "count" is also a perfectly wonderful generic name that could be negative.

Well, the name is probably the smallest thing. Tiem for the ole thesaurus!

To be clear, I also agree that unsigned variables are the worst. Unfortunately the comparison rules between mixed-signedness values is even worser, so we're kinda stuck. Especially since a lot of the static analysis/sanitization stuff will trip all over it.

Anyway, time to find dinner....

PerilousApricot · 2016-09-02T03:42:23Z

s/merge ofthe pre-factored/merge of the pre-factored/

PerilousApricot · 2016-09-02T03:44:15Z

src/lio/bin/lio_mv.c

@@ -76,7 +76,7 @@ gop_op_status_t mv_fn(void *arg, int id)
 //  tweak = (strcmp(mv->dest_tuple.path, "/") == 0) ? 1 : 0;  //** Tweak things for the root path
    while ((ftype = lio_next_object(mv->src_tuple.lc, it, &src_fname[slot], &prefix_len)) > 0) {
        snprintf(dname, OS_PATH_MAX, "%s/%s", mv->dest_tuple.path, &(src_fname[slot][prefix_len+1]));
-        gop = lio_move_op(mv->src_tuple.lc, mv->src_tuple.creds, src_fname[slot], dname);
+        gop = gop_lio_move_object(mv->src_tuple.lc, mv->src_tuple.creds, src_fname[slot], dname);


Do you really want gop_lio_*? That might not fit the naming scheme.

Again where are the style docs? I used the same naming convention of the exising routines in the same file. If we want to change it there needs to be a a guide we all agree to. Reading tea leaves doesn't work.

PerilousApricot · 2016-09-06T14:44:26Z

This build fails because it requires LevelDB, but LevelDB isn't shipped along with the docker builder images:

-- The following REQUIRED packages have not been found:

 * LevelDB

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
/tmp/lstore-package/src/lio/LEVELDB_INCLUDE_DIR

You'll need to add the appropriate statements to https://github.com/accre/lstore/blob/master/scripts/generate-docker-base.sh#L78, https://github.com/accre/lstore/blob/master/scripts/generate-docker-base.sh#L99 to instruct the build system about the new dependencies. Additionally, please update the documentation at https://github.com/accre/lstore/blob/master/README.md to tell humans of the new dep.

tacketar · 2016-09-06T15:06:31Z

Good catch. I completely forgot about the docker builder.

tacketar · 2016-09-30T17:21:26Z

Superseeded by #129

tacketar added 23 commits August 30, 2016 16:14

Only include UP and NO_SPACE drives in the usable tally

b763920

Properly pass the error code back to the calling program

d51e79b

Add support for getting the file list from STDIN

dfba712

Snag the segment ID before decrementing the flushing_count

f5a7975

The final log statement uses the segment ID which could be invalid because once the flushing_count is decremented the segment_destroy call could complete and destroy the actual segment.

Fix lio_inspect from segfaulting on SIGQUIT instead of cleanly windin…

0046768

…g down

Fix bug when renaming a file

88b7f6e

Remove all the cruft lioc_* routines and rename lio_core.c->lio_core_…

7d918bd

…misc.c

Fix missed variable initialization

6e05427

Added support for LIO_EXCL_MODE when opening files

f2484fd

Properly lock and roll-back if needed rename operations.

8afdead

Check and make sure we have a non-null pointer

8eecff8

Simplify the move object routine

44ef4fb

Fix the move object function and propagate the error code back to FUSE

6a3e3a3

Fix issue on overwriting a file and initialize a status

0b77238

Fix bug stopping depot updates from occuring.

e82c163

Fix signed/unsigned int warnings

8ca0841

Start the standardization of collecting paths from the command line

c04670d

Add using stock LevelDB to build

24e883a

Add support for LevelDB to lio_warm and add warmer_query for mining

bbdb8f4

Cast the cap to a string for printing

2c823fa

This is not ideal there area couple of options 1) add a string helper function 2) just make the cap a printable string. I don't think there is much an issue doing this.

Switch to using the standard path iterator

5677c1f

Forgot to destroy the iterator

bda817a

Need to strdup the rid_key otherwise you get a double free on cleanup

6191711

PerilousApricot reviewed Sep 1, 2016
View reviewed changes

PerilousApricot reviewed Sep 2, 2016
View reviewed changes

tacketar closed this Sep 30, 2016

PerilousApricot deleted the alan branch October 4, 2016 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hopefully the final merge ofthe pre-factored code! #119

Hopefully the final merge ofthe pre-factored code! #119

tacketar commented Sep 1, 2016

PerilousApricot commented Sep 1, 2016

tacketar commented Sep 1, 2016

PerilousApricot commented Sep 1, 2016

PerilousApricot Sep 1, 2016

tacketar Sep 1, 2016 via email

PerilousApricot Sep 1, 2016

tacketar Sep 1, 2016

PerilousApricot Sep 2, 2016

tacketar Sep 2, 2016 via email

PerilousApricot Sep 2, 2016

tacketar Sep 2, 2016 via email

PerilousApricot Sep 2, 2016

tacketar Sep 2, 2016 via email

PerilousApricot Sep 6, 2016

tacketar Sep 6, 2016 via email

PerilousApricot Sep 6, 2016

tacketar Sep 6, 2016 via email

PerilousApricot Sep 6, 2016

PerilousApricot commented Sep 2, 2016

PerilousApricot Sep 2, 2016

tacketar Sep 2, 2016 via email

PerilousApricot commented Sep 6, 2016

tacketar commented Sep 6, 2016 via email

tacketar commented Sep 30, 2016

Hopefully the final merge ofthe pre-factored code! #119

Hopefully the final merge ofthe pre-factored code! #119

Conversation

tacketar commented Sep 1, 2016

PerilousApricot commented Sep 1, 2016

tacketar commented Sep 1, 2016

PerilousApricot commented Sep 1, 2016

Choose a reason for hiding this comment

tacketar Sep 1, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacketar Sep 2, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacketar Sep 2, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacketar Sep 2, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacketar Sep 6, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacketar Sep 6, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PerilousApricot commented Sep 2, 2016

Choose a reason for hiding this comment

tacketar Sep 2, 2016 via email

Choose a reason for hiding this comment

PerilousApricot commented Sep 6, 2016

tacketar commented Sep 6, 2016 via email

tacketar commented Sep 30, 2016