Hi Dominik,
On Fri, 03 May 2013 21:18:13 +0200, Jiri Svoboda <[email protected]>
"wrote:
> Hi Dominik,
>
> will you attend the next meeting (Saturday 11th)? Perhaps we can discuss
> this more efficiently in person.
I am not entirely sure I will be able to. And I won't know until sometime
next week. Do I have to announce my presence in advance?"
Of course not, I was merely asking.
"
Also, can you tell me what would you like to discuss specifically? So that
I can prepare."
It's not a president's duel. I simply wanted to give you my comments. Since
there are quite a few, I think it would be faster to do it in person, as
time I can spend on HelenOS is a precious commodity.
I will, however, try to give some feedback now. In general, completeness
(presence of a feature) is not an issue - it can be added later. But what is
there, a reasonable attempt should be made to make it correct, well-designed
and well-coded.
> 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.
In that case why don't we use our own partition format, if you don't plan to
interoperate with other OSes?
OK, let's clarify. The partition editor should be able to (i.e. an honest
attempt should be made to ) create a new label or edit existing labels (of
MBR and GPT format) created by (ideally) any contemporary OS/partitioning
tool (ideally also any legacy OS) supporting that label type.
If I created a label on my hard drive 10 years ago using WinXP fdisk, then
installed Gentoo and FreeBSD and OpenSolaris, then I run HelenOS and use the
editor to add (or delete) a HelenOS partition, I expect all my other OSes
to still boot and find all their file systems. Any permutation of the above
should yield the same (e.g. create HelenOS partition, install FreeBSD,
delete HelenOS partition using HelenOS partition editor).
>> Also primary partitions must not move (in the MBR) / change
>> numbers just because you deleted one of them!
>Hmmm. Why not?
As you should be aware, in many UNIX-like OSes (practically anything unless
you are using ZFS) will find filesystem by device name which contains the
primary partition index in the partition table (Solaris: something like /
dev/dsk/c1d0t0p1 - /dev/dsk/c1t0d0p4, Linux: something like /dev/sda1 - /
dev/sda4) and the logical partitions will have numbers 5,6,7... according to
their order).
If these numbers change the systems become unbootable or loose file systems.
Obviously for logical partitions deleting a partition is almost bound to
create trouble (anyone correct me if I'm wrong), FreeBSD has for a long time
refused to recognize logical partitions (they are Windows partitions, after
all), in general, (not Linux) UNIX derivatives only recognize primary
partitions and create their own partitioning/slicing within these (thus
avoiding this problem).
At the very least, primary partition numbers must not change, it would make
FreeBSD, Solaris, Linux, etc not boot!
Windows: traditionally, drive labels (C,D,E..) were assigned in the order: 1
st primary partition of 1s HDD, 1st p.p. of 2nd HDD, 2nd primary partition
of 1s HDD - here you theoretically might get away with it if you remove a
non-FAT partition (assuming they are not counted)
And the OS is not the only thing to consider - the various boot loaders also
may expect some things not to change.
Key takeaway: be as conservative as possible, don't change anything unless
necessary.
> I can't leave in there anything, can I? Have you considered the
> possibility that there might be garbage instead of actual MBR?
Trust me, people will get extremely angry if you corrupt their disk labels.
They will get much less angry if you *refuse* to edit their disk labels. So,
if you find anything you cannot handle, you must abort with an error. Still
you should make an effort to work somehow even in the presence of Solaris
partitions, etc.
> 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.
I doubt that libmbr.h under 'Read/Write/Set MBR partitions' would look the
way it does if your tab width was 8?
>
> Anyway, can you tell me what's so wrong about my coding style? Where
> exactly does it differ from HelenOS CS?
I am quite sure you can spot the differences yourself, but since you ask:
- lines should not, in general, exceed 80 characters width
- do not use // comments
- do not put comments on the same line as opening brace {, put it on the
next line instead (in general comments are preferably on a line of their
own)
- no space after asterisk - int *foo instead of "int * foo"
- ALL_CAPS should only be used for preprocessor symbols (macros/constants),
possibly for enum members in case of enums that replace preprocessor
constants - never for type identifiers
- static and header/extern function definitions should not contain argument
identifiers
- multi-line comments should start with a line consisting just of "/*" and
the comment itself should start on the next line "* Comment..." and the
liast line should be just "*/"
- in a header file using "/** doxy comment */" to describe a block of
functions does not make sense, Doxygen will apply it only to the first
function. Use /* */ multiline comment instead.
- there should be no tab indentation inside the line declaring a function
prototype (libmbr.h)
>>
>> I think you can write better structured code...
>
>Where? How?
First a nit: "disk label" is the generic term applied to the data structures
we are editing here, it usually consists of a partition table plus possibly
other information (volume name/ID, geometry, bootloader code, etc)
mbr_write_partitions() takes arguments parts, mbr, dev_handle. mbr_read_
partitions() returns two things: mbr_partitions_t and mbr_t (output
argument). Would it not make more sense to have one structure (e.g. mbr_
label_t) that holds both?
int 0/1 for yes/no should not be used. In general you can use stdbool.h, in
some cases special-purposed enum might be useful
functions such as mbr_add_partition() scream to be spilt into more functions
In libmbr.h you are mixing stuff related to the MBR "spec"/on-disk format
itself (which could be shared with mbr_part) with stuff specific to libmbr
hdisk: I would expect func_xxx to export a statically initialized table_t
structure, rather than doing that in fill_table_funcs()
Hope this helps.
Best regards,
Jiri
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
