Adding ByteBuffer.hexDump in xxd and hexdump -C formats #2475

natikgadzhi · 2023-07-14T07:56:39Z

This PR adds a bunch of hexDump functions to ByteBuffer.

Motivation:

@weissi says he ends up writing hex dump helper functions in most projects with NIO, so they would be useful to have straight out of the box.

Modifications:

Adds a String(byte: UInt, padding: Int) initializer to format bytes.
Changes the output format of ByteBuffer.Storage.dumpBytes and re-uses it.
Adds hexDump implementations for different formats.
Tests for hexDump functions that this PR adds.

TODO

Rework reading data from the buffer in long-format dump and avoid using get*.
Add test cases for situations where reader and writer indices are in non-default states.
Add a test case for a single line output in long-format dump.
Adds ByteBuffer.HexDumpFormat enum and a public function to get a hex dump of a ByteBuffer providing the desired format.
Documentation for public hexDump functions.
Add hexDumpLong(maxLength:) implementation for long-format clipped hex dumps
Cleanup the code and wire up the clipped length dump in the public hexDump() function
Perhaps more tests?

Result:

We'll have one new public function on ByteBuffer that accepts desired HexDumpFormat and returns the formatted hex dump string.

Sources/NIOCore/ByteBuffer-core.swift

natikgadzhi

I've cleaned up the short hexdump helpers for now, but will work on the long-format more. Not yet ready for a thorough review.

Sources/NIOCore/ByteBuffer-core.swift

glbrntt

Great start @natikgadzhi! Sorry for the delay, I've been out of office for a while. I added a little to weissi's feedback.

Sources/NIOCore/ByteBuffer-core.swift

Sources/NIOCore/ByteBuffer-conversions.swift

Sources/NIOCore/ByteBuffer-core.swift

Tests/NIOCoreTests/ByteBufferTest.swift

natikgadzhi · 2023-07-24T17:10:02Z

@glbrntt thank you for the review! I was slow last week, but will get back at it today — should have the next stage to review on Wednesday.

natikgadzhi · 2023-08-01T05:48:39Z

@glbrntt, I cleaned up what I already had — but I still have to write the clipped version of the hexdump, and then implement the format enum and the switcher method. Time to get some sleep, but I hope to get it to work tomorrow.

UPD: added the public API that switches formats. Take a look at the implementations. If they make sense overall, I'll bang my head against a wall and figure out a way to clip the long output.

…n the buffer

…not just readableBytes

glbrntt

This is coming along nicely!

Sources/NIOCore/ByteBuffer-core.swift

Sources/NIOCore/ByteBuffer-conversions.swift

Sources/NIOCore/ByteBuffer-hexdump.swift

natikgadzhi · 2023-08-02T04:43:22Z

I've worked through most of your feedback, but left the clipped hexdump format implementation for tomorrow. I hope to find time and work on it in the morning, though.

One tricky part (to me) is to figure out what to use for the placeholder when clipping, if the clipping happens within a line.

One option would be to always clip on 16-byte increments (but that won't be precise). Another option would be to use "." or space to show that that byte was omitted. And always insert one empty line with "..." in the middle to show that this line signifies the clipped part.

So it would be something like this:

        let buf = ByteBuffer(string: "Goodbye, world! It was nice knowing you.\n")
        let expected = """
        00000000  47 6f 6f 64 62 79 65 2c  20 77 6f 72 6c           |Goodbye, worl   |

                                         ...

        00000010                                       6b 6e 6f 77  |            know|
        00000020  69 6e 67 20 79 6f 75 2e  0a                       |ing you..|
        00000029
        """

But that looks weird to me. Admittedly, I also didn't use hex dumps very often. What format should I implement, @glbrntt?

1proprogrammerchant · 2023-08-02T08:11:58Z

I like this

natikgadzhi · 2023-08-09T18:31:30Z

@glbrntt, okay, done with a little update!

I think I combed through your feedback. I left the safe to unwrap comments there, but I think I acted on the rest.
I've reworked how .detailed format prints out the dump to accommodate the format. It's a little bit tricky, and I had to write some repetitive code to get it to work. I'm not entirely happy with it, and tried to document it thoroughly. I think there's a trade-off between the dump readability (format) and code complexity to get it dumped in said format.
Caught a bug in a case when the dump with maxBytes is long enough for the clipped pieces to take multiple lines and fixed it.

I think this is ready for a CI run and a review.

natikgadzhi · 2023-08-09T18:34:19Z

@weissi, @glbrntt, would you like me to remove Storage.dumpBytes and change the spots that used it to use the new hexDump instead, maybe in another PR to keep this moving and unblock others?

glbrntt · 2023-08-10T07:04:26Z

@swift-server-bot test this please

glbrntt

This is looking really good. I mostly left doc/typo fixes but also left a little feedback on hexDumpLine. Thanks for sticking with this!

Sources/NIOCore/ByteBuffer-hexdump.swift

glbrntt · 2023-08-10T07:35:22Z

Sources/NIOCore/ByteBuffer-hexdump.swift

+        result += String(repeating: " ", count: paddingBefore * 3)
+        // If the padding is 8 or more bytes, pad with extra space to match hexdump -C format.
+        if paddingBefore >= 8 {
+            result += " "
+        }
+
+        // Iterate over the bytes in the line, and dump them one by one, insert a separator if needed.
+        for (byteIndex, byte) in self.readableBytesView.enumerated() {
+            result += String(byte, radix: 16, padding: 2)
+            result += " "
+            if byteIndex + paddingBefore == 7 {
+                result += " "
+            }
+        }


This part is a little hard to follow because of the extra spaces being added conditionally. Can I suggest something like this?

result += String(repeating: " ", count: paddingBefore * 3) // Add the left side of the central column if paddingBefore < 8 { for byte in self.readableBytesView.prefix(8 - paddingBefore) { result += String(byte, radix: 16, padding: 2) result += " " } } // Add an extra space for the centre column. result += " " // Add the right side of the central column. let bytesToSkip = paddingBefore < 8 ? 8 - paddingBefore : 0 for byte in self.readableBytesView.dropFirst(bytesToSkip) { result += String(byte, radix: 16, padding: 2) result += " " }

Reworked it based on your feedback: d057109

That way, all if statements can go away, seems more readable?

glbrntt · 2023-08-10T07:38:57Z

Tests/NIOCoreTests/ByteBufferTest.swift

+    func testHexDumpDetailedWithMultilineFrontAndBack() {
+        let buf = ByteBuffer(string: """
+        Goodbye, world! It was nice knowing you.
+        I will miss this pull request with all of it's 94+ comments.


We can keep it going if you like? 😅 (Also, thank you again, this is a really awesome addition!)

such good changes

Co-authored-by: George Barnett <gbarnett@apple.com>

natikgadzhi

Ready for the next round ;)

natikgadzhi · 2023-08-13T22:18:21Z

Sources/NIOCore/ByteBuffer-hexdump.swift

+        result += String(repeating: " ", count: paddingBefore * 3)
+        // If the padding is 8 or more bytes, pad with extra space to match hexdump -C format.
+        if paddingBefore >= 8 {
+            result += " "
+        }
+
+        // Iterate over the bytes in the line, and dump them one by one, insert a separator if needed.
+        for (byteIndex, byte) in self.readableBytesView.enumerated() {
+            result += String(byte, radix: 16, padding: 2)
+            result += " "
+            if byteIndex + paddingBefore == 7 {
+                result += " "
+            }
+        }


Reworked it based on your feedback: d057109

That way, all if statements can go away, seems more readable?

glbrntt · 2023-08-14T09:41:41Z

@swift-server-bot test this please

glbrntt

This looks good to me now, great job on this and thanks for sticking with it 🙂

Lukasa · 2023-08-14T09:44:49Z

Sources/NIOCore/ByteBuffer-conversions.swift

+    ///     - radix: radix base to use for conversion.
+    ///     - padding: the desired lenght of the resulting string.
+    @inlinable
+    internal init<T>(_ value: T, radix: Int, padding: Int) where T: BinaryInteger {


Can we give this a better generic type parameter name? Maybe Value?

Good call. Done in 96c077b.

nicely done

yim-lee · 2023-08-14T21:33:32Z

@swift-server-bot add to allowlist

yim-lee · 2023-08-14T21:43:07Z

@swift-server-bot test this please

natikgadzhi · 2023-08-14T23:49:05Z

Huh, let me do a clean build and fix those.

…is happy

natikgadzhi · 2023-08-15T00:12:50Z

Fixed a test failure of storage.dumpBytes. I dropped [ and ] earlier, but then reverted it, and forgot to revert the test. That was the only test failure on Swift 5.7 -> nightly.
Fixed a Swift 5.6 compile error in dc65676 — the code is a tad cleaner that way, too.

It should be good now. @glbrntt can you poke the swift CI gods, please?

yim-lee · 2023-08-15T01:06:34Z

@natikgadzhi I added you to the allow list, so your pushes to the PR should automatically trigger CI from now on. (I just fixed the soundness check job permissions)

@swift-server-bot test this please

natikgadzhi · 2023-08-15T02:46:10Z

@yim-lee, thank you <3

1proprogrammerchant · 2023-08-15T04:22:55Z

@natikgadzhi I added you to the allow list, so your pushes to the PR should automatically trigger CI from now on. (I just fixed the soundness check job permissions)

@swift-server-bot test this please

cool person

glbrntt

Awesome, thanks @natikgadzhi!

natikgadzhi mentioned this pull request Jul 14, 2023

ByteBuffer docs issue: Does not dissuade users from using get* #2469

Closed

weissi reviewed Jul 14, 2023

View reviewed changes

Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved

weissi reviewed Jul 14, 2023

View reviewed changes

Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved

natikgadzhi force-pushed the feature/byte-buffer-hexdump branch from 206173f to 4975ffa Compare July 15, 2023 05:30

natikgadzhi commented Jul 15, 2023

View reviewed changes

Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved

weissi reviewed Jul 15, 2023

View reviewed changes

Sources/NIOCore/ByteBuffer-core.swift Outdated Show resolved Hide resolved

glbrntt reviewed Jul 24, 2023

View reviewed changes

weissi mentioned this pull request Jul 29, 2023

Logging contains newlines (because it uses ByteBuffer.debugDescription which IMHO it shouldn't) swift-server/async-http-client#690

Open

natikgadzhi added 12 commits July 31, 2023 23:07

Initial implementation of hexdump for ByteBuffer

3de39bb

Put that iff back there

fb8d173

Reword hexDumpShort(): naming, tests, offset and full length support

066d7e0

Use writerIndex to figure out the convinience end of useful content i…

e89714b

…n the buffer

hexDumpLong(offset:) — supports offset, and reads full buffer slice, …

5e5dede

…not just readableBytes

hexDumpLong() extra line failing test

64e3089

Using ByteBuffer(string:) instead of the allocator

4134644

Move hexDumps code into ByteBuffer-hexdump.swift

b640602

Optimized String(byte: poadding:)

006bf1d

Avoid using storage.dumpBytes in hexDumpShort()

3e0573f

Implemented a cleaner long hexdump

a31717e

Implemented the public hexdump API

f188f67

natikgadzhi force-pushed the feature/byte-buffer-hexdump branch from 3c96a83 to f188f67 Compare August 1, 2023 06:07

natikgadzhi requested a review from glbrntt August 1, 2023 06:07

glbrntt reviewed Aug 1, 2023

View reviewed changes

natikgadzhi added 2 commits August 1, 2023 21:17

Revert ByteBuffer-core.swift to main

6716354

Addressed review feedback!

5edc450

hexdump long format with clipped length

2f0d956

natikgadzhi mentioned this pull request Aug 9, 2023

Docs: explain the basic safe usage of ByteBuffer #2499

Merged

glbrntt reviewed Aug 10, 2023

View reviewed changes

natikgadzhi and others added 3 commits August 13, 2023 15:08

Apply suggestions from code review

75a82b4

Co-authored-by: George Barnett <gbarnett@apple.com>

Cleaner way to dump a line in .detailed format

d057109

Merge branch 'main' into feature/byte-buffer-hexdump

56abf58

natikgadzhi requested a review from glbrntt August 13, 2023 22:18

natikgadzhi commented Aug 13, 2023

View reviewed changes

glbrntt approved these changes Aug 14, 2023

View reviewed changes

Lukasa reviewed Aug 14, 2023

View reviewed changes

Cleaner generic argument type for String(_, radix:, padding:)

96c077b

natikgadzhi added 2 commits August 14, 2023 16:59

Revert _storage.dumpBytes test to match previous format

47bebae

Simplify printable bytes closure to a single expression so Swift 5.6 …

dc65676

…is happy

glbrntt approved these changes Aug 15, 2023

View reviewed changes

Lukasa approved these changes Aug 15, 2023

View reviewed changes

Merge branch 'main' into feature/byte-buffer-hexdump

c26275b

Lukasa enabled auto-merge (squash) August 15, 2023 09:15

Lukasa merged commit cdfbb20 into apple:main Aug 15, 2023
6 of 8 checks passed

weissi mentioned this pull request Jun 21, 2024

ByteBuffer should have hex bytes initialiser / write method #2748

Open

weissi mentioned this pull request Aug 5, 2024

ByteBuffer should have a .hexdump(format: .compact) #2825

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ByteBuffer.hexDump in xxd and hexdump -C formats #2475

Adding ByteBuffer.hexDump in xxd and hexdump -C formats #2475

natikgadzhi commented Jul 14, 2023 •

edited

Loading

natikgadzhi left a comment

glbrntt left a comment

natikgadzhi commented Jul 24, 2023

natikgadzhi commented Aug 1, 2023 •

edited

Loading

glbrntt left a comment

natikgadzhi commented Aug 2, 2023

1proprogrammerchant commented Aug 2, 2023

natikgadzhi commented Aug 9, 2023

natikgadzhi commented Aug 9, 2023

glbrntt commented Aug 10, 2023

glbrntt left a comment

glbrntt Aug 10, 2023

natikgadzhi Aug 13, 2023

glbrntt Aug 10, 2023

1proprogrammerchant Aug 10, 2023

natikgadzhi left a comment

natikgadzhi Aug 13, 2023

glbrntt commented Aug 14, 2023

glbrntt left a comment

Lukasa Aug 14, 2023

natikgadzhi Aug 14, 2023

1proprogrammerchant Aug 15, 2023

yim-lee commented Aug 14, 2023

yim-lee commented Aug 14, 2023

natikgadzhi commented Aug 14, 2023

natikgadzhi commented Aug 15, 2023

yim-lee commented Aug 15, 2023

natikgadzhi commented Aug 15, 2023

1proprogrammerchant commented Aug 15, 2023

glbrntt left a comment

Adding ByteBuffer.hexDump in xxd and hexdump -C formats #2475

Adding ByteBuffer.hexDump in xxd and hexdump -C formats #2475

Conversation

natikgadzhi commented Jul 14, 2023 • edited Loading

Motivation:

Modifications:

TODO

Result:

natikgadzhi left a comment

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment

natikgadzhi commented Jul 24, 2023

natikgadzhi commented Aug 1, 2023 • edited Loading

glbrntt left a comment

Choose a reason for hiding this comment

natikgadzhi commented Aug 2, 2023

1proprogrammerchant commented Aug 2, 2023

natikgadzhi commented Aug 9, 2023

natikgadzhi commented Aug 9, 2023

glbrntt commented Aug 10, 2023

glbrntt left a comment

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

Choose a reason for hiding this comment

natikgadzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt commented Aug 14, 2023

glbrntt left a comment

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

yim-lee commented Aug 14, 2023

yim-lee commented Aug 14, 2023

natikgadzhi commented Aug 14, 2023

natikgadzhi commented Aug 15, 2023

yim-lee commented Aug 15, 2023

natikgadzhi commented Aug 15, 2023

1proprogrammerchant commented Aug 15, 2023

glbrntt left a comment

Choose a reason for hiding this comment

natikgadzhi commented Jul 14, 2023 •

edited

Loading

natikgadzhi commented Aug 1, 2023 •

edited

Loading