On Tue, 17 Sep 2013 11:22:30 +0200, Jiri Svoboda <[email protected]>
wrote:
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.
In that case, may I ask why there's USB error codes in errno.h?
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.
Fine. Thanks.
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.
OK.
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
This is even more unimportant if you yourself don't know how it should be
done. So, am I expected to fix this to later find out the rule has changed?
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.
OK.
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.
I am sorry about using tabs for alignment, I thought I fixed that
everywhere.
But I don't agree with you on your second point - not aligning argument
lists. I find that very handy and helpful. Looking up the function is
fast, but finding the opening bracket slows me down. Aligning them makes
it easy to spot differences.
input.c:
int get_input_line(tinput_t * in, char ** str)
There should be no space after asterisk.
Sorry.
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.
As I said before, I make some exceptions to this rule. I know keeping
short lines allow you to open several source files at once and show them
side-by-side. But this is just a printf. It doesn't do anything important.
You don't need to care what's in there...
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.
So the violation is that I don't include "libmbr.h"? Which is included by
"func_mbr.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.
The trouble is, if you name it "mbr.h", you might think it describes MBR
label only - not the library API as well. I think "libmbr.h" makes it
clear what it defines. Any other opinions?
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, as I said before, the first indentation is for readability
purposes. I'm sorry about the second one, must have slipped through...
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
I don't know where you got this from, but it's not from the official
CStyle guide.
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.
Sorry about those tabs. And my tab is 4 characters. BTW, I don't see how
"80 chars per line" and "8-char tab" can get together. Seems contradicting
to me...
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?
I can't use list_foreach() and I am surprised there's something like the
other two. I missed those, probably.
And also, as I said before, there cannot be 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
That's not in the CStyle guide. And I prefer my style.
/* 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.
Explained before.
/** 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.
*
CStyle guide reference?
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.
OK.
/** 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.
*/
I don't think it needs to convey more information. It's a private
function, not included in the API.
Functionality wise:
- it appears to me that mbr checks for overlap when adding a partition,
but gpt checks it only when writing the table?
Yes. I think I explained that. That's implementation-specific.
- maybe you should check the consistency (e.g. overlaps) when you read
the
label
Within libmbr, you mean? Can do, but that's still a detail. Not very
useful, IMO.
- I really really think the interface should allow you to specify just
the
size of the partition
And I really really think HelenOS should support joysticks. I mean, it
takes some progress. We're getting there.
- 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
The same point as above?
When I get some time to test your code, I may have further comments.
Thanks.
Best regards,
Dominik
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/listinfo/helenos-devel
