[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

Ticket URL: <http://dev.haiku-os.org/ticket/8434#comment:22>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.

Other related posts: