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

Add a C API test for appending #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noahgoldman
Copy link
Contributor

This adds a test for appending to a file by writing, closing the file, opening it again and seeking to the end, then writing again. I added a clarifying comment before the test describing why this method is used as opposed to just opening the file with "O_APPEND".

@kstinsonqc @mckurt @mikeov

//
// Using O_APPEND itself does not produce the same result. Opening a file with
// O_APPEND and writing to it will create hole of the appropriate size to make
// the previous chunk reach the chunksize. This can be useful as an atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This can be useful as an atomic append, but is not the behavior you'd expect from a normal filesystem."

I don't think this statement is needed here.


// Generate the string consisting of the testdata twice
char expected_str[expected_len];
strcpy(expected_str, testdata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use memcpy instead of strcpy followed by strcat?

@mckurt
Copy link
Contributor

mckurt commented Nov 22, 2016

@noahgoldman Can you squeeze this into a single commit? One that reads something like "Add an append test by writing, closing, then seeking and writing ...".

strcpy(expected_str, testdata);
strcat(expected_str, testdata);

char buf[expected_len];
Copy link
Contributor

@mckurt mckurt Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if variable sized array declaration is a good idea.

@noahgoldman noahgoldman force-pushed the topic/qfsc-append-test branch from c35348d to 41c45e0 Compare November 23, 2016 15:26
@codecov-io
Copy link

Current coverage is 64.89% (diff: 100%)

Merging #198 into master will not change coverage

@@             master       #198   diff @@
==========================================
  Files             8          8          
  Lines           245        245          
  Methods          34         34          
  Messages          0          0          
  Branches         31         31          
==========================================
  Hits            159        159          
  Misses           69         69          
  Partials         17         17          

Powered by Codecov. Last update 03f40df...c35348d

This adds a test for appending to a file by writing, closing the file,
opening it again and seeking to the end, then writing again. I added a
clarifying comment before the test describing why this method is used as
opposed to just opening the file with "O_APPEND".
@noahgoldman
Copy link
Contributor Author

@mckurt Updated this PR from your comments:

  • Rebased into a single commit and changed the message
  • Removed the ""This can be useful as an atomic append..." comment
  • Replaced strcpy and strcat with two calls of memcpy
  • Changed the qfs_read buffer to initialize with size 4096

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

Successfully merging this pull request may close these issues.

3 participants