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

8261949: fileStream::readln returns incorrect line string #2626

Closed
wants to merge 2 commits into from

Conversation

y1yang0
Copy link
Member

@y1yang0 y1yang0 commented Feb 18, 2021

When the last line does not contain a NEWLINE character, fileStream::readln would read
truncated line string:

$ cat file_content:
AA
BB
CC

fileStream::readln result:
"AA"
"BB"
"C"

This patch address this problem, it works for Posix and Windows since the last character
of these systems is always '\n'.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261949: fileStream::readln returns incorrect line string

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2626/head:pull/2626
$ git checkout pull/2626

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 18, 2021

👋 Welcome back kelthuzadx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 18, 2021
@openjdk
Copy link

openjdk bot commented Feb 18, 2021

@kelthuzadx The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 18, 2021
@y1yang0
Copy link
Member Author

y1yang0 commented Feb 18, 2021

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 18, 2021
@openjdk
Copy link

openjdk bot commented Feb 18, 2021

@kelthuzadx
The hotspot-runtime label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Feb 18, 2021

Webrevs

//Get rid of annoying \n char
data[::strlen(data)-1] = '\0';
// Get rid of annoying \n char only if it presents, it works for Posix
// and Windows since the last character of these systems is always '\n'
Copy link
Member

Choose a reason for hiding this comment

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

s/of these/on these/
Please end the comment with a period.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest simply:

// Get rid of \n char if it is present

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

Comment on lines 594 to 595
if (data[::strlen(data)-1] == '\n') {
data[::strlen(data)-1] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps:

    size_t last_char = ::strlen(data) - 1;
    if (last_char >= 0 && data[last_char] == '\n') {
      data[last_char] = '\0';

Copy link
Member

Choose a reason for hiding this comment

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

@dcubed-ojdk size_t is unsigned so by definition always >= 0. :) I suggest:
size_t len = ::strlen(data);
if (len > 0 && data[len - 1] == '\n') {
data[len - 1] = '\0';
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. Changed.

//Get rid of annoying \n char
data[::strlen(data)-1] = '\0';
// Get rid of annoying \n char only if it presents, it works for Posix
// and Windows since the last character of these systems is always '\n'
Copy link
Member

Choose a reason for hiding this comment

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

I suggest simply:

// Get rid of \n char if it is present

Comment on lines 594 to 595
if (data[::strlen(data)-1] == '\n') {
data[::strlen(data)-1] = '\0';
Copy link
Member

Choose a reason for hiding this comment

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

@dcubed-ojdk size_t is unsigned so by definition always >= 0. :) I suggest:
size_t len = ::strlen(data);
if (len > 0 && data[len - 1] == '\n') {
data[len - 1] = '\0';
}

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

I like David's suggestion better than mine.
Thumbs up.

@openjdk
Copy link

openjdk bot commented Feb 19, 2021

@kelthuzadx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8261949: fileStream::readln returns incorrect line string

Reviewed-by: dcubed, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 14 new commits pushed to the master branch:

  • 7e78c77: 8261905: Move implementation of OopStorage num_dead related functions
  • 78cde64: 8261860: Crash caused by lambda proxy class loaded in Shutdown hook
  • c158413: 8261098: Add clhsdb "findsym" command
  • 0c31d5b: 8261977: Fix comment for getPrefixed() in canonicalize_md.c
  • 9cf4f90: 8261473: Shenandoah: Add breakpoint support
  • c4664e6: 8261940: Fix references to IOException in BigDecimal javadoc
  • 0e9c5ae: 8075909: [TEST_BUG] The regression-swing case failed as it does not have the 'Open' button when select 'subdir' folder with NimbusLAF
  • e9f3aab: 8261912: Code IfNode::fold_compares_helper more defensively
  • fd098e7: 8261838: Shenandoah: reconsider heap region iterators memory ordering
  • f94a845: 8261600: NMT: Relax memory order for updating MemoryCounter and fix racy updating of peak values
  • ... and 4 more: https://git.openjdk.java.net/jdk/compare/b8fd61420c6aeda7bcd80be6fa5e341debbbb7c0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dcubed-ojdk, @dholmes-ora) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 19, 2021
@y1yang0
Copy link
Member Author

y1yang0 commented Feb 19, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 19, 2021
@openjdk
Copy link

openjdk bot commented Feb 19, 2021

@kelthuzadx
Your change (at version 2e816b2) is now ready to be sponsored by a Committer.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks,
David

@y1yang0
Copy link
Member Author

y1yang0 commented Feb 22, 2021

Thanks @dcubed-ojdk @dholmes-ora for reviews!

Would you be able to sponsor this patch? I'm not a committer so that I can not push this commit directly.

@DamonFool
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Feb 22, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 22, 2021
@openjdk
Copy link

openjdk bot commented Feb 22, 2021

@DamonFool @kelthuzadx Since your change was applied there have been 40 commits pushed to the master branch:

  • 539c80b: 8261702: ClhsdbFindPC can fail due to PointerFinder incorrectly thinking an address is in a .so
  • 564011c: 8261290: Improve error message for NumberFormatException on null input
  • 18188c2: 8261692: Bugs in clhsdb history support
  • 0825bc5: 8261929: ClhsdbFindPC fails with java.lang.RuntimeException: 'In java stack' missing from stdout/stderr
  • c2509ea: 8261857: serviceability/sa/ClhsdbPrintAll.java failed with "Test ERROR java.lang.RuntimeException: 'cannot be cast to' found in stdout"
  • 2b00367: 8261350: Create implementation for NSAccessibilityCheckBox protocol peer
  • 5a25cea: 8261998: Remove unused shared entry support from utilities/hashtable
  • 4755958: 8262041: javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java fails after JDK-8260858
  • b10376b: 8261938: ASN1Formatter.annotate should not return in the finally block
  • 977a21a: 8261225: TieredStopAtLevel should have no effect if TieredCompilation is disabled
  • ... and 30 more: https://git.openjdk.java.net/jdk/compare/b8fd61420c6aeda7bcd80be6fa5e341debbbb7c0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2b55501.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@y1yang0
Copy link
Member Author

y1yang0 commented Feb 22, 2021

@DamonFool I'm grateful for your sponsorship.

@y1yang0 y1yang0 deleted the bugfix_filestream_readln branch March 14, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants