[haiku-bugs] Re: [Haiku] #8434: Building to GPT disks may write the wrong partition offset under Linux
- From: "bonefish" <trac@xxxxxxxxxxxx>
- Date: Tue, 01 May 2012 18:59:51 -0000
#8434: Building to GPT disks may write the wrong partition offset under Linux
----------------------------+----------------------------
Reporter: tqh | Owner: bonefish
Type: bug | Status: new
Priority: normal | Milestone: R1
Component: Build System | Version: R1/Development
Resolution: | Keywords:
Blocked By: | Blocking:
Has a Patch: 1 | Platform: All
----------------------------+----------------------------
Comment (by bonefish):
Here you go:
- The indentation isn't correct. Should be one tab per indentation level
(set to 4 characters width -- relevant for the line length limit).
- If you only use kGPTSignatureSize in this one method, there's no reason
to expose it in the header (unless it's intended to be used elsewhere in
the future). Also, why make a constant for the size, but inline the
signature string itself? I would define both as constants either directly
in the method before their use or at the beginning of the source file.
- There should be a space after `//`.
- When you look at the rest of the source file, you'll notice that the
source is formatted more loosely, using single blank lines to separate
code blocks from each other. There should be at least one blank line to
separate the new code from the preceding block. I would also add another
one before the second (top level) `if` block.
- Generally the "early return on error" style is preferred. The existing
code was written before that style was embraced, but new code should
follow it. E.g. in this case it would be preferable to rewrite the
previous block to do that too, so your code isn't executed unnecessarily.
Also, the way the `read_pos()` result is processed is not correct. If it's
just a short read (i.e. no error) `errno` will not have been set and the
assumption that it is `B_OK` is incorrect. Under the assumption that
`toRead` doesn't overflow `ssize_t`, that should better be done like:
{{{
ssize_t bytesRead = read_pos(...);
if (bytesRead != (ssize_t)(toRead))
return bytesRead < 0 ? errno : B_IO_ERROR;
}}}
- The `_BOOT_MODE` exception is no longer necessary. The boot loader's
`read_pos()` has been fixed to set `errno` correctly.
- Using "early return on error" style also makes it unnecessary to
initialize `gptSignature`.
- The first `if` line is too long. The limit is 80 columns.
- Using `strcmp()` is incorrect. `gptSignature` is only 8 bytes wide, but
`strcmp()` will compare 9 bytes, including the terminating null character.
If the null character is part of the signature, `kGPTSignatureSize` needs
to be adjusted. In either case I would prefer `memcpy()` for the
comparison.
--
Ticket URL: <http://dev.haiku-os.org/ticket/8434#comment:22>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
