Hi Dominik,
please see my comments inline.
---------- Původní zpráva ----------
Od: Dominik Taborsky <[email protected]>
Datum: 3. 5. 2013
Předmět: [HelenOS-devel] libmbr & hdisk update
"Dear fellow HelenOS developers,
I have just uploaded latest version of my libmbr (library for MBR
manipulation) and hdisk (HelenOS's partition editor). The branch is here:
https://launchpad.net/hdisk.";
I've downloaded your branch, glanced through the code, ran hdisk and tried a
few commands. Did not try using the created labels with anything.
"
libmbr seems fully functional, reading/writing primary and logical
partitions. No implicit limitations imposed. Libmbr checks inputs, keeps
consistent list of partitions and checks for return values (so it's "
What does 'consistent list of partitions' mean? Does it check for partition
overlap? Does it check device bounds? Does it check alignment?
"nothing like concept or heavy-development stuff). It shouldn't change very
much from now on."
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).
""
"hdisk is quite simple yet powerful. Although it's not very user-friendly
so far, you can do pretty much anything with it."
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.
"It is quite easily
extensible, so it can support other partition table formats (GPT on the
way). It surely has bugs and space for improvement.""
libgpt is still under development, but it is already included. So far you
can check the code, but don't expect it work.
Any constructive criticism/comments regarding anything are very welcome.
Feel free to give advice, comment, ask or point out a bug."
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.
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. Also primary partitions must not move (in the MBR) / change
numbers just because you deleted one of them!
What is the purpose of gpt_memcmp()? Why not use bcmp() from mem.h? (or
introduce memcmp() to mem.h)
"I'd like to have an argument, please." is neither helpful nor funny. How
about printing a normal error message (and opt. a synopsis)?
"
In the meantime, I have some questions for you:
1) Before merging into mainline, shall I break it into parts? libmbr,
libgpt, hdisk?"
I don't think that is necessary.
"
2) When submitting libmbr and libgpt, should the code in
uspace/srv/bd/part/ be replaced? There are read-only implementations of
MBR and GPT. Is that code staying as it is for the near future?"
I can imagine that the label drivers could share/use some headers that
define the physical structures. I don't think they should be built on top of
a label-editing library, though.
""
"3) Also, as you can see, I have copied and extended definitions from those
implementations. I have credited Jiri Svoboda for the code in my files. Is
that OK?"
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.
"
Looking forward to your comments."
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 think you can follow HelenOS coding style much better (start by properly
configuring your text editor).
I think you can write better structured code...
I think you can self-review your code before submitting it and notice many
of the issues yourself.
Cheers
-Jiri
"
Dominik
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel";
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
