Re: [Lost] [Patch] fdisk

[Kevin Wolf <mail@xxxxxxxxxxxxx>]

Ich gehe jetzt mal in einem ersten Lauf vor allem inhaltlich drüber. Die 
Kommentare zur Form überlasse ich erstmal den anderen, es dürfte davon 
aber genug geben, denn der Code weicht vom üblichen LOST-Codestil in 
einigen Punkten ab.

Den einzigen Punkt, den ich dazu anmerken möchte, ist, daß der Code in 
LOST komplett auf deutsch kommentiert ist. LOST soll ja nicht zuletzt 
auch als Beispiel-OS für Lowlevel dienen, und was Lowlevel so besonders 
macht, ist, daß es eine _deutschsprachige_ Community zum Thema ist. Wir 
sollten deswegen auch die Erklärungen in den Kommentaren auf deutsch 
liefern.

> +/* This is a single entry in the partition table. Its size should
> +   be PARTITION_ENTRY_SIZE */
> +struct partition_entry_t

Wieso hast du dann ein PARTITION_ENTRY_SIZE konstant definiert und 
arbeitest nicht einfach mit sizeof(partition_entry_t)?

> +/* Contains all information about a single partition that is needed
> +   by fdisk */
> +struct partition_t
> +{
> +	char exists;				/* 1 if the partition exists, 0 if not */
> +	unsigned int number;			/* Number of the partition, starting with 1 */
> +	char type;				/* 0 is primary, 1 extended, 2 logical */

Ich würde eher unsigned char (oder gleich int) schreiben, bei char denke 
ich immer an ein ASCII-Zeichen.

> +/* Delete a partition */
> +void deletePartition(struct partition_t *p);
> +/* Create a new partition */
> +void createPartition(struct partition_t *p, unsigned int number, unsigned int start_cyl, unsigned int start_head, unsigned int start_sec, unsigned int start_lba,
> +			unsigned int end_cyl, unsigned int end_head, unsigned int end_sec, unsigned int end_lba);

Wow. Mir hat man damals beigebracht, daß ich was falsch mache, wenn ich 
mehr als fünf Parameter brauche.

> +
> +#define PROG_VERSION "0.1"
> +

Und du meinst, das wird jemals angepaßt werden? Nimm besser die 
SVN-Revision als Versionsnummer.

> +/* This function takes to characters and converts them into a 8bit integer
> +   For example: c1 = F and c2 = F => return value = 255 */
> +unsigned char hex2uint8(char c1, char c2)

Wir haben ein sscanf.

> +void shell()
> +{
> +	printf("Type h for help\n");
> +	
> +	char command_buffer[COMMAND_BUFFER_SIZE];
> +	memset(command_buffer, 0, COMMAND_BUFFER_SIZE);
> +	while(1)
> +	{
> +		/* Get input */
> +		char *input = readline("> ");
> +		if(input)
> +		{
> +			strncpy(command_buffer, input, COMMAND_BUFFER_SIZE);
> +			free(input);

Wozu der extrapuffer?

Mehr dann, wenn ich wirklich Zeit habe.