Hi Jiri,
thanks for comments. Replies are inline. Sorry about removing the text you
replied to - your email format wasn't plaintext nor html and it messed up
the reply blocks.
On Fri, 03 May 2013 12:32:58 +0200, Jiri Svoboda <[email protected]>
wrote:
What does 'consistent list of partitions' mean? Does it check for
partition
overlap? Does it check device bounds? Does it check alignment?
You can see from the code that it checks overlapping, adding too many
primary partitions, having only one extended partition and that extended
partition encapsulates the logical ones. It does not check device boundary
as of yet, but that's simple to add.
'consistent list of partitions' means that it is correct in every way and
can be written at any time. That it has no overlapping partitions or any
of the bad stuff.
Do you support only LBA or also CHS addressing? Do you deal with
alignment
constraints? (Some OSes depend on partitions being aligned in a specific
way, such as at the beginning of a cylinder).
It is LBA only, since that's in use nowadays. CHS is deprecated and I
don't think it is necessary. Your code in bd/part also considers LBA only.
Alignment is left for the frontend. But libmbr already offers a function
that finds nearest aligned address.
It would be nice if you could simply ask it to create a partition of
given
size (in MB/GB/..) at the beginning or end of a free space segment. That
would be user-friendly.
Yes, I know.
Adhere to HelenOS coding style (indentation, comment style, identifiers,
max
line length,..)
I think you have incorrectly configured text editor. You have tabs in
function declarations in libmbr.h between return type and identifier, for
example.
OK, I will fix this.
It think if you read a label with libmbr, then make no changes and write
it,
you should not get any changes on your disk. I don't think this is the
case.
As an example, you will loose any partition flags besides 0x80. I think
you
need to be really careful when messing with the label, which could be
created by other OSes and you don't have to understand all their features
and constraints.
OK, will fix that.
Also primary partitions must not move (in the MBR) / change
numbers just because you deleted one of them!
Hmmm. Why not?
libmbr automatically sorts the list (for better handling purposes). I made
this decision so that not all partitions have to be checked for
overlapping, EBR allocation and alike. I don't think it's a big deal. Do
you have anything specific on mind that would require this feature?
What is the purpose of gpt_memcmp()? Why not use bcmp() from mem.h? (or
introduce memcmp() to mem.h)
The purpose is to compare memory blocks. I couldn't find memcmp(), so I
wrote it myself. I didn't look at bcmp since the name was confusing. Why
is it called bcmp and not memcmp? Isn't that a conflict with HelenOS
Coding Standard?
"I'd like to have an argument, please." is neither helpful nor funny. How
about printing a normal error message (and opt. a synopsis)?
Sorry about that old Monty Python reference. It's been there since the
beginning and I was focused on features, extensibility and correctness.
That's how our license works, isn't it? If you base your work on some
file /
copy parts of that file, copy the copyright lines as well.
But you don't follow our conventions for copyright attribution - why do
you
think you need to specify which parts of the file fall under a specific
copyright line, we don't do that.
OK. I just wasn't sure.
I think your approach is not very good in that it does not try to
preserve
the original label data (even data it does not fully understand) as much
as
possible. I think a robust MBR implementation needs to take care about
LBA,
CHS, alignment issues, preserving primary partition numbers, etc. I
think we
need to be at least as user friendly as Fdisk in MS-DOS (at least on a
conceptual level, e.g "create partition of size x MB/% of space at
beginning/end/%/block of hole xyz).
I can't leave in there anything, can I? Have you considered the
possibility that there might be garbage instead of actual MBR?
I think you may have misunderstood the point of this library. This library
is aimed at HelenOS, not to replace standard tools. It is for the case
when you have blank harddrive so that you can format it and install
HelenOS. It's not designed to fit all needs. If it is really necessary
(for inter-OS compatibility), you can use better tool instead, since you
are going to be using a different OS anyway.
Reimplementing complete MBR manipulation library, and thus reinventing the
wheel once again, without any real specification, just guessing what other
tools might put there and supporting everything even though it's already
deprecated (CHS), does it really look worth it?
I think you can follow HelenOS coding style much better (start by
properly
configuring your text editor).
My text editor is configured correctly. I'm just used to different style,
I guess.
Anyway, can you tell me what's so wrong about my coding style? Where
exactly does it differ from HelenOS CS?
I think you can write better structured code...
Where? How?
I think you can self-review your code before submitting it and notice
many
of the issues yourself.
I did self-review the code. Please be more specific on your issues.
Anyway, thanks for comments.
Dominik
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
