0
\$\begingroup\$

This function formats raw buffer output in manner as Wireshark and many others does:

0x00: 68 65 6C 6C 6F 20 77 6F 72 6C 64 02 6B 68 67 61   |hello world.khga|
0x10: 76 73 64 20 0B 20 0A 05 58 61 73 6A 68 6C 61 73   |vsd . ..Xasjhlas|
0x20: 62 64 61 73 20 6A 61 6C 73 6A 64 6E 13 20 20 30   |bdas jalsjdn.  0|
0x30: 31 32 33 34 35 36 37 38 39                        |123456789       |

Please review and assess code quality.

std::string format_hex_payload(const char* payload, size_t payload_len)
{
    const char line_placeholder[] = "0x00:                                                   |                |";
    size_t number_of_lines = payload_len / 16 + (payload_len%16 > 0);
    size_t sizeof_output = sizeof line_placeholder * number_of_lines;
    char *output = (char*)alloca(sizeof_output);  //POSSIBLE REIMPL USE C++RVO std::string out; out.reserve(sizeof_output);

    char *pout = output;  //POSSIBLE REIMPL char *pout = out.data();
    const char *p = payload;
    const char *const end = payload + payload_len;
    size_t ascii_offset = strchr(line_placeholder,'|') - line_placeholder + 1;  //could be calculated at compile time

    unsigned short offset = 0;
    for(unsigned l=0; l < number_of_lines; l++, offset+=16)
    {
        char* pline_begin = pout;
        char* pline = pout;
        strcpy(pline,line_placeholder);
        pline += sprintf(pline, "0x%02X: ", offset);
        for(unsigned i=0; i<16 && p < end; ++i, ++p){
            pline += sprintf(pline, "%02X ", *p);
            *(pline_begin+ascii_offset+i) = isprint(*p) ? *p : '.';
        }
        *pline=' ';
        pout += sizeof line_placeholder;  // move pointer to next line
        pout[-1] = '\n';
    }
    pout[-1] = '\0';
    assert(pout == output + sizeof_output);  // sanity check

    return output;  //POSSIBLE REIMPL return out;
}


\$\endgroup\$
  • \$\begingroup\$ What happens when offset is >= 256 (more than 16 lines)? \$\endgroup\$ – 1201ProgramAlarm Feb 7 at 19:11
  • \$\begingroup\$ @1201ProgramAlarm, nothing criminal, but format will be shifted i.imgur.com/rlMNjc0.png \$\endgroup\$ – kyb Feb 11 at 11:23
  • 2
    \$\begingroup\$ @kyb: The policy on this site is not to add, remove, or edit code in a question after you've received an answer, because it invalidates existing answers. This is explained in What to do when someone answers. On the other hand, your original question was unclear/off-topic (tagged [C] but invalid C code), which is probably why you code 3 close votes up to now. \$\endgroup\$ – Martin R Feb 11 at 12:33
  • 3
    \$\begingroup\$ @kyb: (Cont.) Following feedback in chat, I have done a rollback the question to revision 3, i.e. the original code but with the [C++] tag, so that both Q and A make sense. I suggest to ask a new question about the C implementation. \$\endgroup\$ – Martin R Feb 11 at 12:33
  • \$\begingroup\$ Follow up question (Compiles to C rather than C++ now) \$\endgroup\$ – Peilonrayz Feb 11 at 15:39
5
\$\begingroup\$
  1. I think probably the first thing to do is decide whether you're really using C, or really using C++.

    • If you're really using C, then you need to get rid of the std::string, and allocate space for your return differently (e.g., using malloc).

    • If you're really using C++, then I'd at least consider using iostreams and manipulators to do most of the work. They are fairly verbose, but most people using C++ expect to deal with iostreams rather than raw buffers and C-style string manipulation.

  2. I think I'd break the code up into a few more functions. For example, I'd probably have format_hex, which would repeatedly call format_line, which might in turn call some format_hex_value (or something on that order) to write out each individual value.

  3. I'd probably avoid using the line placeholder, for a couple of reasons. The first (and probably most important) is that as you've done things right now, the last line is (as I see things) somewhat malformed. You've surrounded the ASCII part of the display with | as delimiters, but for that to make sense, you want the delimiter right next to the data, so that part of the last line should look like: |123456789|, so you immediately know there's no more data after the 9 (where your current display looks like there are space characters after the 9).

  4. I'd definitely expand the addresses on the left out to at least 4 characters, and maybe more. In theory, you should probably be thinking in terms of 16 characters (allowing 64-bit addresses), but in all honesty I've rarely seen much point in hex dump of multiple gigabytes of data (but I certainly have seen good uses for more than 256 bytes).

Putting those together, one possibility (going the C++ route) might come out something on this order:

#include <cctype>
#include <iostream>
#include <iomanip>
#include <sstream>

struct hex {
    unsigned value;
    int digits;
public:
    hex(unsigned value, int digits) : value(value), digits(digits) { }

    friend std::ostream &operator<<(std::ostream &os, hex const &h) {
        auto oldfill = os.fill('0');

        // Yeah, iostreams get really verbose. Sorry.
        os << std::hex 
           << std::setw(h.digits) 
           << std::uppercase 
           << std::setprecision(h.digits)
           << h.value;
        os.fill(oldfill);
        return os;
    }
};

void fmt_line(std::ostream &os, char const *data, size_t offset, size_t len) { 
    os << hex(offset, 8) << ":";

    for (size_t i=0; i<len; i++) {
        os << " " << hex(data[offset+i], 2);
    }

    os << std::setw((16-len)*3 +2) << "|";
    for (size_t i=0; i<len; i++) {
        char ch = data[offset+i];
        os << (std::isprint((unsigned char)ch) ? ch : '.');
    }
    os << "|\n";
}

std::string format_hex(char const *data, size_t len) { 
    unsigned lines = len/16;
    std::stringstream out;

    for (size_t line=0; line<lines; line++)
        fmt_line(out, data, line*16, 16);
    fmt_line(out, data, lines*16, len%16);
    return out.str();
}

int main() { 
    char input[] = "hello world\02khgavsd \xb \xa\x5Xasjhlasbdas jalsjdn\xd  0123456789";
    std::cout << format_hex(input, sizeof(input));
}

This could use some further improvement. For one thing, it's probably not immediately obvious what some things like: os << std::setw((16-len)*3 +2) << "|"; are really doing, so they could undoubtedly use at least a little more work.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.