Hi Dominik,
Dominik Taborsky wrote:
> On a side note, would it be a good idea to merge libmbr's error codes into
> errno.h?
Short answer: no.
Long answer: Obviously the errno.h approach does not scale. Every function
has its own set of ways it can fail and so it could have its own set of
error codes - that would be one extreme case. The other extreme case would
be to put all these error codes into a single header/enum, with none or some
unification.
Since in case of most functions we don't really feel the need to distinguish
betweem a whole lot of failure reasons, we usually use the lazy approach
where we try to map the one or two failure reasons to the existing errno
error codes. In the cases where an interface/library needs a larger and
meaningful set of error codes, it is perfectly fine to define your own set.
Note: Your error codes have names like ERR_xxx, I would suggest prefixing
them with the name of the library, eg. MBR_xxx, MBR_Exxxx or so, to prevent
name clashes.
libgpt now checks partition boundaries, as promised. It is already on
launchpad (lp:hdisk), feel free to test. Otherwise, it is ready for
merge.
I didn't have a chance to re-test your code so far. I suggest merging with
the mainline, btw, due to big recent changes like list_foreach().
I haven't had a chance to try running your code again. I've looked at
briefly and I still find some deviations from the official Cstyle, such as:
(I am only including one exaple of each type of Cstyle error):
* Copyright (c) 2012, 2013 Dominik Taborsky
we only put the last year of modification in, such as:
* Copyright (c) 2013 Dominik Taborsky
I have to admit, though, that I am not sure whether the current way is a
good idea, we might discuss about changing it
hdisk.c:
int interact(service_id_t);
void print_help(void);
void select_label_format(tinput_t *);
void construct_label(layouts_t);
void free_label(void);
int try_read(service_id_t);
int try_read_mbr(service_id_t);
int try_read_gpt(service_id_t);
void set_alignment(tinput_t *);
I suggest that module-private functions should be declared static.
input.h:
extern int get_input_line(tinput_t * in, char ** str);
extern uint8_t get_input_uint8(tinput_t * in);
extern uint32_t get_input_uint32(tinput_t * in);
extern uint64_t get_input_uint64(tinput_t * in);
extern size_t get_input_size_t(tinput_t * in);
func_gpt.h:
extern int construct_gpt_label(label_t *);
extern int add_gpt_part (label_t *, tinput_t *);
extern int delete_gpt_part (label_t *, tinput_t *);
extern int destroy_gpt_label(label_t *);
extern int new_gpt_label (label_t *);
extern int print_gpt_parts (label_t *);
extern int read_gpt_parts (label_t *, service_id_t);
extern int write_gpt_parts (label_t *, service_id_t);
extern int extra_gpt_funcs (label_t *, tinput_t *, service_id_t);
Here's a block of function declarations that use tab for aligning - in one
case you are aligning function names, in the other you are aligning the
opening brackets?!. You should not align either, use a single space between
type and name (same for lists of variable declarations) and no space between
name and opening bracket.
input.c:
int get_input_line(tinput_t * in, char ** str)
There should be no space after asterisk.
func_mbr.c:133:
printf("\t%10u %10u %10u %7u\n", it->start_addr, it->start_addr + it
->length, it->length, it->type);
This line ends at column 117 - lines should not exceed 80 characters.
func_mbr.c:
#include <stdio.h>
#include <errno.h>
#include <str_error.h>
#include <sys/types.h>
#include "func_mbr.h"
#include "input.h"
This is a clear violation of IWIU (include what you use). You are using
common.h to include libmbr.h and libgpt.h, but really func_mbr.c should
directly include libmbr.h.
Speaking of which I don't think the headers should be named libmbr.h and
libgpt.h - after all the library names are 'mbr' and 'gpt'. libmbr and
libgpt are just part of the scheme of file names where the libraries are
stored.
func_gpt.c:
printf("%3zu %10" PRIu64 " %10" PRIu64 " %10" PRIu64 " %3zu %s\
n",
i-1, gpt_get_start_lba(iter), gpt_get_end_lba(iter),
gpt_get_end_lba(iter) - gpt_get_start_lba(iter),
gpt_get_part_type(iter), gpt_get_part_name(iter));
libgpt.c:
label->gpt->header->pe_array_crc32 = host2uint32_t_le(compute_crc32(
(uint8_t *) label->parts->part_array,
fillries * e_size));
etc.
Continuation lines should be indented by 4 spaces.
(again I don't think you should call the main source module of gpt library
libgpt.c)
Blank line placement is also a bit erratic sometimes. For example:
int gpt_add_partition(gpt_label_t *label, gpt_part_t *partition)
{
gpt_part_t *p;
/* Find the first empty entry */
do {
if (label->parts->fill == label->parts->arr_size) {
if (extend_part_array(label->parts) == -1)
return ENOMEM;
}
p = label->parts->part_array + label->parts->fill++;
} while (gpt_get_part_type(p) != GPT_PTE_UNUSED);
memcpy(p, partition, sizeof(gpt_entry_t));
return EOK;
}
should really look something like:
int gpt_add_partition(gpt_label_t *label, gpt_part_t *partition)
{
gpt_part_t *p;
/* Find the first empty entry */
do {
if (label->parts->fill == label->parts->arr_size) {
if (extend_part_array(label->parts) == -1)
return ENOMEM;
}
p = label->parts->part_array + label->parts->fill++;
} while (gpt_get_part_type(p) != GPT_PTE_UNUSED);
memcpy(p, partition, sizeof(gpt_entry_t));
return EOK;
}
Note that blank lines are placed:
- after the closing line of a block {} statement
- between variable declarations and other statements
- no more than one blank line is used
libgpt.c:
/** Set partition type
* @param p partition to be set
* @param type partition type to set
* - see our fine selection at gpt_ptypes to choose from
*/
The spacing in the above block is:
3 tabs between 'p' and 'partition to be set'
2 tabs between 'type' and 'partition type to be set'
1 space and 5 tabs between '*' and '- see our fine selection'
that puts the text you were trying to align at columns 33, 25 and 41,
respectively, if tab width is 8 characters.
libmbr.c:248:
link_t *l = label->parts->list.head.next;
/* Encoding primary partitions */
for (i = 0; i < N_PRIMARY; i++) {
p = list_get_instance(l, mbr_part_t, link);
encode_part(p, &(label->mbr->raw_data.pte[i]), 0, false);
l = l->next;
}
Use either list_foreach() or list_first() and list_next(). Also, what
happens if there are less than N_PRIMARY entries in the list?
libmbr.c:256:
&(label->mbr->raw_data)
the parentheses are unnecessary, it should be just &label->mbr->raw_data
/* Encoding and writing first logical partition */
if (l != &(label->parts->list.head)) {
had you used list_first()/list_next() you could check for l != NULL.
/** Remove partition
* Removes partition by index, indexed from zero. When removing extended
* partition, all logical partitions get removed as well.
*
It should be indented/spaced like:
/** Remove partition
*
* Remove partition by index, indexed from zero. When removing extended
* partition, all logical partitions are removed aswell.
*
typedef enum {
/** Other flags unknown - saving previous state */
/** Bootability */
ST_BOOT = 7,
/** Logical partition, 0 = primary, 1 = logical*/
ST_LOGIC = 8
} MBR_FLAGS;
typedef enum {
/** No error */
ERR_OK = 0,
/** All primary partitions already present */
ERR_PRIMARY_FULL,
/** Extended partition already present */
ERR_EXTENDED_PRESENT,
/** No extended partition present */
ERR_NO_EXTENDED,
/** Partition overlapping */
ERR_OVERLAP,
/** Partition out of bounds */
ERR_OUT_BOUNDS,
/** No space left for EBR */
ERR_NO_EBR,
/** Out of memory */
ERR_NOMEM,
/** Libblock error */
ERR_LIBBLOCK,
} mbr_err_val;
Typedefs should have a lowercase_t name, e.g. mbr_flags_t, mbr_err_t.
/** Check whether two partitions overlap
*
* @return true/false
*/
does not really say any more useful information than
/** Check whether two partitions overlap. */
If you want to convey more information, how about:
/** Check whether two partitions overlap
*
* @param p1 First partition
* @param p2 Second partition
*
* @return @c true if @a p1 and @a p2 overlap, @c false otherwise.
*/
Functionality wise:
- it appears to me that mbr checks for overlap when adding a partition,
but gpt checks it only when writing the table?
- maybe you should check the consistency (e.g. overlaps) when you read the
label
- I really really think the interface should allow you to specify just the
size of the partition
- you should be able to specify the size/capacity not only in number of
blocks, but also in number of {T|G|M|k}Bytes or percent
Terminology wise:
You are using the term 'sector' to mean a unit of data on the block
device, but that is really called a 'block'. Sector is the coordinate in the
cylinder-head-sector coordinate system, i.e. a 'sector' s contains the
blocks Forall x,y: (C,H,S)=(x,y,s). In the LBA coordinate system there are
no sectors at all, just linear block addresses.
When I get some time to test your code, I may have further comments.
Best regards,
Jiri
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/listinfo/helenos-devel
