[Meta Issue] Cleanup codebase

[MatthewTingum]

This began as a question in response to #7 but it went so far off the rails that I figured I'd open a meta-issue.

In regards to #7

Several lines that need whitespace fixes are commented out code. IMO, that's something that should rarely be seen in a master branch. Can we delete the unimportant lines of commented code (probably all) while we're at it? Even comments like this are useless:

//header->EntryPoint = (void *)((int) header->EntryPoint ^0xA8FC57AB);
// ...
header->EntryPoint ^= xorentry(1);

I can use ctags to figure out what xorentry does faster than I can overlook the casting, figure out what line of actual code this comment is referring to, and ask why myself why I care.

Sub-issues of codebase cleanup

Then there are commented out printf statements. Perhaps we still want these, but only in debug builds? Only with a verbose flag specified? My vote is for DEBUG, WARN, and ERROR print macros. With __LINE__ indicators!

Speaking of ERROR, I see a good many places where return values are ignored. The codebase should be audited to ensure return values are checked. There are more, but I'm looking at:

fwrite(xbe, 1, filesize, f);

All sorts of flags are not using definitions.

if (option_flag & 0x00020000)

What does this mean? Let's use the constants.

Formatting styles are mixed. xbestructure.h looks like someone pulled these structures from debug symbols or something. How about struct _XBE_HEADER -> struct xbe_header... and so on for all structs and members.

The comments in xbestructure.h destroy my eyes and make it impossible to see what the structure looks like. Lets inline the comments, and align the comments to the same x-coordinate (first 4 space tab alignment after the member with the longest name). Some of these might run over 80 chars but we can do multi-line, not care, or take suggestions.

The comments in xbestructure.h don't need offsets. I'll use offsetof if I care.

In xbestructure.h, member names should be based and commented on some reputable source. I remember using caustik's website in the past. There was one piece of errata, but I can't seem to find their site anymore :(
https://xboxdevwiki.net/Xbe instead?

Some of the comments in xbestructure.h are questions. Others are references to dead websites. Some of the time, it seems like they couldn't decide which data type to use.

• int32_t num_libraries
  • Seems like this would be unsigned to me....

• /*uint8_t*/
  uint32_t pc_exe_filename;

  • First off, let's not call this one pc_exe_...
  • Second off... what? Is this an off_t to a const char * maybe?
• There are seemingly lots of places where a pointer to a struct was commented in, but a uint32_t was ultimately used. I do seem to remember these being offsets. If they are, I vote that they are change to off_t data types.

I like to avoid typedefs and this is a small codebase. We can deal with writing struct a couple more times to make it blatantly obvious that we are working with structures we have defined ourselves.

• Just lots of not great comments here. They don't need to be acknowledged here. Fix, undergo review, repeat. Should handle most of the ugly.

I'd prefer #defines be at the top of headers, beneath the includes.

Some headers seem stretched to include things that don't belong there.

• (xbestructure.h)
  • void shax(...) -- Nope. This one is about the structure of XBEs
    • Move the extraneous and rename to xbe.h. structure seems redundant. When you're working with Linux elf files, you get elf.h. I like that. It's simple. xbe.h.

Dockerize the build? Document the build? Scuba da build?

The existing readme is cool and I am in full support of recognizing the original author. Maybe a better readme is introduced that acknowledges and links to original author / readme?

I have more to add, but I'd like a ping from a maintainer to know that this is even worth my time. I'll happily get some issues and PRs out once I know that this project is actively maintained!

Whipped in to shape, this could be not only a good utility, but a good library... which is the primary reason I'm eager to get some PRs in. A good XBE manipulation library is solid start to a permanent (or runtime) hook injection utility. With such hooks, I can inject start and stop points (Or create loops to eliminate snapshot reload time) for a kernel fuzzer I've been developing. They could also just be silly fun :D

I'm very excited to use the library I'm envisioning so hit me back at your earliest convenience!

[MatthewTingum]

A rough approximation of issues that exist or could be introduced from this issue.

• Whitespace issues #7 Whitespace Issues
• Debug prints
• Remove extraneous comments
  • Reword deserving comments
  • Inline / fix struct comments
• Check return values
• Define / use constants
• Standardize auto-doc (doc in headers!!!)
  • (CI Pipeline)
  • Also good if not CI...
• Testing? Even if we don't want to use paid services, locally test in docker container before merge to master.
  • Expect values for certain titles
• Dockerize the build
  • scuba preferred
• Move things that don't belong. Rename things.
• Use consistent c formatting

[MatthewTingum]

Comments above the license is janky too.

[mborgerson]

It's worth remembering this was written nearly ~20 years ago while the system was being researched. Such code is often hastily written, at the expense of good software engineering practice :)

It sounds like you already know how the code should look. If you want to fix this tool and make it the ultimate XBE parsing and manipulation library/tool, you are welcome to. Send PRs and I'll review them when I can; although I must warn you, review wait times can be long.

You are also free to just scrap the majority of it and start from scratch on a new thing. Frankly, this is what I would suggest, and have done before: https://github.com/mborgerson/pyxbe. I prefer to write my tools in Python, but there are certainly use cases for a native version.

If you do choose to continue working a native version, the headers are useful, the rest can be trivially re-written using existing code as reference. There is also XBE parsing/writing stuff in CXBE (perhaps the best native XBE tool, minus signing), Cxbx-Reloaded, XbSymbolDatabase etc. that you can work with. There is also a Ghidra extension ghidra-xbe that you can use for reversing.

I suggest joining the XboxDev Discord server where you can discuss your ideas with community/maintainers.

[MatthewTingum]

Cool beans. I'd like something in c for my projects so I'll forge ahead. That CXBX link for the xbe format is a lot nicer! I'll probably start with fixes for this codebase to get my bearings, then ultimately do a rewrite. I can send the patches your way as they come if you or the community care to have this utility cleaned up. If I end up doing a rewrite, maybe its not worth the time to even review patches for this utility...

I'll hop on the discord at some point to say hi!