Changeset 6453e306 in mainline for uspace/lib/mbr/libmbr.c

Timestamp:

2013-12-25T16:09:43Z (11 years ago)

Author:

Martin Decky <martin@…>

Branches:

lfn, master, serial, ticket/834-toolchain-update, topic/msim-upgrade, topic/simplify-dev-export

Children:

ac36aed

Parents:

d51beba3

Message:

basic code review and coding style cleanup

File:

• uspace/lib/mbr/libmbr.c (modified) (18 diffs)

uspace/lib/mbr/libmbr.c

@@ -1 +1 @@
 /*
- * Copyright (c) 2011, 2012, 2013 Dominik Taborsky
+ * Copyright (c) 2011-2013 Dominik Taborsky
  * All rights reserved.
  *
@@ -27 +27 @@
  */
 
- /** @addtogroup libmbr
+/** @addtogroup libmbr
  * @{
  */
@@ -43 +43 @@
 #include <stdlib.h>
 #include <str_error.h>
-
+#include <align.h>
 #include "libmbr.h"
 
-static br_block_t * alloc_br(void);
+static br_block_t *alloc_br(void);
 static int decode_part(pt_entry_t *, mbr_part_t *, uint32_t);
 static int decode_logical(mbr_label_t *, mbr_part_t *);
@@ -57 +57 @@
 
 /** Allocate and initialize mbr_label_t structure */
-mbr_label_t * mbr_alloc_label(void)
+mbr_label_t *mbr_alloc_label(void)
 {
         mbr_label_t *label = malloc(sizeof(mbr_label_t));
@@ -88 +88 @@
 
 /** Allocate memory for mbr_t */
-mbr_t * mbr_alloc_mbr(void)
+mbr_t *mbr_alloc_mbr(void)
 {
         return malloc(sizeof(mbr_t));
@@ -94 +94 @@
 
 /** Read MBR from specific device
- * @param   label       label to write data to
- * @param   dev_handle  device to read MBR from
- *
- * @return                              EOK on success, error code on error
+ *
+ * @param label      Label to be read.
+ * @param dev_handle Device to read MBR from.
+ *
+ * @return EOK on success, error code on error.
+ *
  */
 int mbr_read_mbr(mbr_label_t *label, service_id_t dev_handle)
 {       
-        int rc;
-        
         if (label->mbr == NULL) {
                 label->mbr = mbr_alloc_mbr();
-                if (label->mbr == NULL) {
+                if (label->mbr == NULL)
                         return ENOMEM;
-                }
-        }
-
-        rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
+        }
+        
+        int rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
         if (rc != EOK)
                 return rc;
-
-        rc = block_read_direct(dev_handle, 0, 1, &(label->mbr->raw_data));
+        
+        rc = block_read_direct(dev_handle, 0, 1, &label->mbr->raw_data);
         block_fini(dev_handle);
         if (rc != EOK)
                 return rc;
-
+        
         label->device = dev_handle;
-
+        
         return EOK;
 }
 
-/** Write mbr to disk
- * @param label                 MBR to be written
- * @param dev_handle    device handle to write MBR to (may be different
- *                                                      from the device in 'mbr')
- *
- * @return                              0 on success, otherwise libblock error code
+/** Write MBR to specific device
+ *
+ * @param label      Label to be written.
+ * @param dev_handle Device to write MBR to.
+ *
+ * @return EOK on success, error code on error.
+ *
  */
 int mbr_write_mbr(mbr_label_t *label, service_id_t dev_handle)
 {
-        int rc;
-
-        rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
-        if (rc != EOK) {
+        int rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
+        if (rc != EOK)
                 return rc;
-        }
-
-        rc = block_write_direct(dev_handle, 0, 1, &(label->mbr->raw_data));
+        
+        rc = block_write_direct(dev_handle, 0, 1, &label->mbr->raw_data);
         block_fini(dev_handle);
-        if (rc != EOK) {
-                return rc;
-        }
-
-        return EOK;
-}
-
-/** Decide whether this is an actual MBR or a Protective MBR from GPT
- *
- * @param mbr           the actual MBR to decide upon
- *
- * @return                      1 if MBR, 0 if GPT
+        
+        return rc;
+}
+
+/** Decide whether this is an actual MBR or a Protective MBR for GPT
+ *
+ * @param label Label to decide upon.
+ *
+ * @return True if MBR.
+ * @return False if Protective MBR for GPT.
+ *
  */
 int mbr_is_mbr(mbr_label_t *label)
 {
-        return (label->mbr->raw_data.pte[0].ptype != PT_GPT) ? 1 : 0;
-}
-
-/** Parse partitions from MBR, freeing previous partitions if any
- * NOTE: it is assumed mbr_read_mbr(label) was called before.
- * @param label  MBR to be parsed
- *
- * @return       linked list of partitions or NULL on error
+        return (label->mbr->raw_data.pte[0].ptype != PT_GPT);
+}
+
+/** Parse partitions from MBR (freeing previous partitions if any)
+ *
+ * It is assumed that mbr_read_mbr() was called before.
+ *
+ * @param label Label to be parsed.
+ *
+ * @return EOK on success, error code on error.
+ *
  */
 int mbr_read_partitions(mbr_label_t *label)
 {
-        if (label == NULL || label->mbr == NULL)
+        if ((label == NULL) || (label->mbr == NULL))
                 return EINVAL;
-        
-        int rc, rc_ext;
-        unsigned int i;
-        mbr_part_t *p;
-        mbr_part_t *ext = NULL;
         
         if (label->parts != NULL)
@@ -180 +174 @@
         
         label->parts = mbr_alloc_partitions();
-        if (label->parts == NULL) {
+        if (label->parts == NULL)
                 return ENOMEM;
-        }
+        
+        mbr_part_t *extended = NULL;
         
         /* Generate the primary partitions */
-        for (i = 0; i < N_PRIMARY; ++i) {
+        for (unsigned int i = 0; i < N_PRIMARY; i++) {
                 if (label->mbr->raw_data.pte[i].ptype == PT_UNUSED)
                         continue;
                 
-                p = mbr_alloc_partition();
-                if (p == NULL) {
-                        printf(LIBMBR_NAME ": Error on memory allocation.\n");
+                mbr_part_t *partition = mbr_alloc_partition();
+                if (partition == NULL) {
                         mbr_free_partitions(label->parts);
                         return ENOMEM;
                 }
                 
-                rc_ext = decode_part(&(label->mbr->raw_data.pte[i]), p, 0);
-                mbr_set_flag(p, ST_LOGIC, false);
-                rc = mbr_add_partition(label, p);
+                int is_extended =
+                    decode_part(&label->mbr->raw_data.pte[i], partition, 0);
+                
+                mbr_set_flag(partition, ST_LOGIC, false);
+                
+                int rc = mbr_add_partition(label, partition);
                 if (rc != ERR_OK) {
-                        printf(LIBMBR_NAME ": Error occured during decoding the MBR. (%d)\n" \
-                               LIBMBR_NAME ": MBR is invalid.\n", rc);
                         mbr_free_partitions(label->parts);
                         return EINVAL;
                 }
                 
-                if (rc_ext) {
-                        ext = p;
-                        label->parts->l_extended = &p->link;
-                }
-        }
-        
-        /* Fill in the primary partitions and generate logical ones, if any */
-        rc = decode_logical(label, ext);
-        if (rc != EOK) {
-                printf(LIBMBR_NAME ": Error during decoding logical partitions: %d - %s.\n" \
-                           LIBMBR_NAME ": Partition list may be incomplete.\n", rc, str_error(rc));
-                return rc;
-        }
-        
-        return EOK;
+                if (is_extended) {
+                        extended = partition;
+                        label->parts->l_extended = &partition->link;
+                }
+        }
+        
+        /* Fill in the primary partitions and generate logical ones (if any) */
+        return decode_logical(label, extended);
 }
 
 /** Write MBR and partitions to device
- * @param label        label to write
- * @param dev_handle   device to write the data to
- *
- * @return             returns EOK on succes, specific error code otherwise
+ *
+ * @param label      Label to write.
+ * @param dev_handle Device to write the data to.
+ *
+ * @return EOK on success, specific error code otherwise.
+ *
  */
 int mbr_write_partitions(mbr_label_t *label, service_id_t dev_handle)
@@ -237 +227 @@
                 label->mbr = mbr_alloc_mbr();
         
-        int i = 0;
-        int rc;
-        mbr_part_t *p;
-        mbr_part_t *ext = (label->parts->l_extended == NULL) ? NULL
-                                        : list_get_instance(label->parts->l_extended, mbr_part_t, link);
-        
-        rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
-        if (rc != EOK) {
-                printf(LIBMBR_NAME ": Error while initializing libblock: %d - %s.\n", rc, str_error(rc));
+        int rc = block_init(EXCHANGE_ATOMIC, dev_handle, 512);
+        if (rc != EOK)
                 return rc;
-        }
-        
-        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;
-        }
-        
-        label->mbr->raw_data.signature = host2uint16_t_le(BR_SIGNATURE);
-        
-        /* Writing MBR */
-        rc = block_write_direct(dev_handle, 0, 1, &(label->mbr->raw_data));
-        if (rc != EOK) {
-                printf(LIBMBR_NAME ": Error while writing MBR : %d - %s.\n", rc, str_error(rc));
+        
+        mbr_part_t *partition = NULL;
+        mbr_part_t *extended = NULL;
+        
+        if (label->parts->l_extended != NULL)
+                extended = list_get_instance(label->parts->l_extended,
+                    mbr_part_t, link);
+        
+        link_t *link = label->parts->list.head.next;
+        
+        /* Encode primary partitions */
+        for (unsigned int i = 0; i < N_PRIMARY; i++) {
+                partition = list_get_instance(link, mbr_part_t, link);
+                
+                encode_part(partition, &label->mbr->raw_data.pte[i], 0, false);
+                link = link->next;
+        }
+        
+        /* Write MBR */
+        rc = block_write_direct(dev_handle, 0, 1, &label->mbr->raw_data);
+        if ((rc != EOK) || (extended == NULL))
                 goto end;
-        }
-        
-        if (ext == NULL) {
-                rc = EOK;
+        
+        uint32_t base = extended->start_addr;
+        mbr_part_t *prev_partition;
+        
+        /* Encode and write first logical partition */
+        if (link != &label->parts->list.head) {
+                partition = list_get_instance(link, mbr_part_t, link);
+                
+                partition->ebr_addr = base;
+                encode_part(partition, &partition->ebr->pte[0], base, false);
+                link = link->next;
+        } else {
+                /*
+                 * If there was an extended partition but no logical partitions,
+                 * we should overwrite the space where the first logical
+                 * partitions's EBR would have been. There might be some
+                 * garbage from the past.
+                 */
+                
+                br_block_t *br = alloc_br();
+                rc = block_write_direct(dev_handle, base, 1, br);
+                if (rc != EOK)
+                        goto end;
+                
+                free(br);
                 goto end;
         }
         
-        uint32_t base = ext->start_addr;
-        mbr_part_t *prev_p;
-        
-        /* Note for future changes: Some thought has been put into design
-         * and implementation. If you don't have to change it, don't. Other
-         * designs have been tried, this came out as the least horror with
-         * as much power over it as you can get. */
-        
-        /* Encoding and writing first logical partition */
-        if (l != &(label->parts->list.head)) {
-                p = list_get_instance(l, mbr_part_t, link);
-                p->ebr_addr = base;
-                encode_part(p, &(p->ebr->pte[0]), base, false);
-                l = l->next;
-        } else {
-                /* If there was an extended but no logical, we should overwrite
-                 * the space where the first logical's EBR would have been. There
-                 * might be some garbage from the past. */
-                br_block_t * tmp = alloc_br();
-                rc = block_write_direct(dev_handle, base, 1, tmp);
-                if (rc != EOK) {
-                        printf(LIBMBR_NAME ": Error while writing EBR: %d - %s.\n", rc, str_error(rc));
-                        goto end;
-                }
-                free(tmp);
-                rc = EOK;
-                goto end;
-        }
-        
-        prev_p = p;
-        
-        /* Check EBR addresses
-         * This piece of code saves previous EBR placements from other
-         * software. But if our user modifies the logical partition chain,
-         * we have to fix those placements if needed.*/
-        link_t *l_ebr = l;
-        link_t *l_iter;
-        mbr_part_t *tmp = mbr_alloc_partition();
-        tmp->length = 1;
-        while (l_ebr != &(label->parts->list.head)) {
-                p = list_get_instance(l_ebr, mbr_part_t, link);
-                tmp->start_addr = p->ebr_addr;
-                
-                l_iter = l;
-                while (l_iter != &(label->parts->list.head)) {
-                        /* Checking whether EBR address makes sense. If not, we take a guess.
-                         * So far this is simple, we just take the first preceeding sector.
-                         * Fdisk always reserves at least 2048 sectors (1MiB), so it can have 
-                         * the EBR aligned as well as the partition itself. Parted reserves
-                         * minimum one sector, like we do.
-                         * 
-                         * Note that we know there is at least one sector free from previous checks.
-                         * Also note that the user can set ebr_addr to their liking (if it's valid). */          
-                        if (p->ebr_addr < base || p->ebr_addr >= base + ext->length ||
-                          check_overlap(tmp, list_get_instance(l_iter, mbr_part_t, link))) {
-                                p->ebr_addr = p->start_addr - 1;
+        prev_partition = partition;
+        
+        /*
+         * Check EBR addresses: The code saves previous EBR
+         * placements from other software. But if our user
+         * modifies the logical partition chain, we have to
+         * fix those placements if needed.
+         */
+        
+        link_t *link_ebr = link;
+        link_t *link_iter;
+        
+        mbr_part_t tmp_partition;
+        tmp_partition.length = 1;
+        
+        while (link_ebr != &label->parts->list.head) {
+                partition = list_get_instance(link_ebr, mbr_part_t, link);
+                
+                tmp_partition.start_addr = partition->ebr_addr;
+                
+                link_iter = link;
+                while (link_iter != &label->parts->list.head) {
+                        /*
+                         * Check whether EBR address makes sense. If not, we take
+                         * a guess.  So far this is simple, we just take the first
+                         * preceeding sector. FDisk always reserves at least 2048
+                         * sectors (1 MiB), so it can have the EBR aligned as well
+                         * as the partition itself. Parted reserves minimum one
+                         * sector, like we do.
+                         *
+                         * Note that we know there is at least one sector free from
+                         * previous checks. Also note that the user can set ebr_addr
+                         * to their liking (if it is valid).
+                         */
+                        
+                        if ((partition->ebr_addr < base) ||
+                            (partition->ebr_addr >= base + extended->length) ||
+                            (check_overlap(&tmp_partition,
+                            list_get_instance(link_iter, mbr_part_t, link)))) {
+                                partition->ebr_addr = partition->start_addr - 1;
                                 break;
                         }
                         
-                        l_iter = l_iter->next;
-                }
-                
-                l_ebr = l_ebr->next;
-        }
-        mbr_free_partition(tmp);
-        
-        /* Encoding and writing logical partitions */
-        while (l != &(label->parts->list.head)) {
-                p = list_get_instance(l, mbr_part_t, link);
-                
-                
-                encode_part(p, &(p->ebr->pte[0]), p->ebr_addr, false);
-                encode_part(p, &(prev_p->ebr->pte[1]), base, true);
-                
-                rc = block_write_direct(dev_handle, prev_p->ebr_addr, 1, prev_p->ebr);
-                if (rc != EOK) {
-                        printf(LIBMBR_NAME ": Error while writing EBR: %d - %s.\n", rc, str_error(rc));
+                        link_iter = link_iter->next;
+                }
+                
+                link_ebr = link_ebr->next;
+        }
+        
+        /* Encode and write logical partitions */
+        while (link != &label->parts->list.head) {
+                partition = list_get_instance(link, mbr_part_t, link);
+                
+                encode_part(partition, &partition->ebr->pte[0],
+                    partition->ebr_addr, false);
+                encode_part(partition, &prev_partition->ebr->pte[1],
+                    base, true);
+                
+                rc = block_write_direct(dev_handle, prev_partition->ebr_addr, 1,
+                    prev_partition->ebr);
+                if (rc != EOK)
                         goto end;
-                }
-                
-                prev_p = p;
-                l = l->next;
-        }
-        
-        /* write the last EBR */
-        encode_part(NULL, &(prev_p->ebr->pte[1]), 0, false);
-        rc = block_write_direct(dev_handle, prev_p->ebr_addr, 1, prev_p->ebr);
-        if (rc != EOK) {
-                printf(LIBMBR_NAME ": Error while writing EBR: %d - %s.\n", rc, str_error(rc));
-                goto end;
-        }
-        
-        rc = EOK;
+                
+                prev_partition = partition;
+                link = link->next;
+        }
+        
+        /* Write the last EBR */
+        encode_part(NULL, &prev_partition->ebr->pte[1], 0, false);
+        rc = block_write_direct(dev_handle, prev_partition->ebr_addr,
+            1, prev_partition->ebr);
         
 end:
         block_fini(dev_handle);
-        
         return rc;
 }
 
-/** mbr_part_t constructor */
-mbr_part_t * mbr_alloc_partition(void)
-{
-        mbr_part_t *p = malloc(sizeof(mbr_part_t));
-        if (p == NULL) {
+/** Partition constructor */
+mbr_part_t *mbr_alloc_partition(void)
+{
+        mbr_part_t *partition = malloc(sizeof(mbr_part_t));
+        if (partition == NULL)
                 return NULL;
-        }
-        
-        link_initialize(&(p->link));
-        p->ebr = NULL;
-        p->type = PT_UNUSED;
-        p->status = 0;
-        p->start_addr = 0;
-        p->length = 0;
-        p->ebr_addr = 0;
-        
-        return p;
-}
-
-/** mbr_partitions_t constructor */
-mbr_partitions_t * mbr_alloc_partitions(void)
+        
+        link_initialize(&partition->link);
+        partition->ebr = NULL;
+        partition->type = PT_UNUSED;
+        partition->status = 0;
+        partition->start_addr = 0;
+        partition->length = 0;
+        partition->ebr_addr = 0;
+        
+        return partition;
+}
+
+/** Partitions constructor */
+mbr_partitions_t *mbr_alloc_partitions(void)
 {
         mbr_partitions_t *parts = malloc(sizeof(mbr_partitions_t));
-        if (parts == NULL) {
+        if (parts == NULL)
                 return NULL;
-        }
-        
-        list_initialize(&(parts->list));
+        
+        list_initialize(&parts->list);
         parts->n_primary = 0;
         parts->n_logical = 0;
         parts->l_extended = NULL;
         
-        /* add blank primary partitions */
-        int i;
-        mbr_part_t *p;
-        for (i = 0; i < N_PRIMARY; ++i) {
-                p = mbr_alloc_partition();
-                if (p == NULL) {
+        /* Add blank primary partitions */
+        for (unsigned int i = 0; i < N_PRIMARY; ++i) {
+                mbr_part_t *part = mbr_alloc_partition();
+                if (part == NULL) {
                         mbr_free_partitions(parts);
                         return NULL;
                 }
-                list_append(&(p->link), &(parts->list));
-        }
-        
-
+                
+                list_append(&part->link, &parts->list);
+        }
+        
         return parts;
 }
 
 /** Add partition
- *      Performs checks, sorts the list.
- * 
- * @param label                 label to add to
- * @param p                             partition to add
- * 
- * @return                              ERR_OK (0) on success, other MBR_ERR_VAL otherwise
- */
-mbr_err_val mbr_add_partition(mbr_label_t *label, mbr_part_t *p)
-{
-        int rc1, rc2;
+ *
+ * Perform checks, sort the list.
+ *
+ * @param label Label to add to.
+ * @param part  Partition to add.
+ *
+ * @return ERR_OK on success, other MBR_ERR_VAL otherwise
+ *
+ */
+mbr_err_val mbr_add_partition(mbr_label_t *label, mbr_part_t *part)
+{
+        int rc = block_init(EXCHANGE_ATOMIC, label->device, 512);
+        if ((rc != EOK) && (rc != EEXIST))
+                return ERR_LIBBLOCK;
+        
         aoff64_t nblocks;
-        
-        rc1 = block_init(EXCHANGE_ATOMIC, label->device, 512);
-        if (rc1 != EOK && rc1 != EEXIST) {
-                printf(LIBMBR_NAME ": Error during libblock init: %d - %s.\n", rc1, str_error(rc1));
+        int ret = block_get_nblocks(label->device, &nblocks);
+        
+        if (rc != EEXIST)
+                block_fini(label->device);
+        
+        if (ret != EOK)
                 return ERR_LIBBLOCK;
-        }
-        
-        rc2 = block_get_nblocks(label->device, &nblocks);
-        
-        if (rc1 != EEXIST)
-                block_fini(label->device);
-        
-        if (rc2 != EOK) {
-                printf(LIBMBR_NAME ": Error while getting number of blocks: %d - %s.\n", rc2, str_error(rc2));
-                return ERR_LIBBLOCK;
-        }
-        
-        if ((aoff64_t) p->start_addr + p->length > nblocks)
+        
+        if ((aoff64_t) part->start_addr + part->length > nblocks)
                 return ERR_OUT_BOUNDS;
         
@@ -455 +432 @@
                 label->parts = mbr_alloc_partitions();
                 if (label->parts == NULL)
-                        return ENOMEM; //FIXME! merge mbr_err_val into errno.h
-        }
-        
-        if (mbr_get_flag(p, ST_LOGIC))
-                /* adding logical partition */
-                return mbr_add_logical(label, p);
+                        // FIXME! merge mbr_err_val into errno.h
+                        return ENOMEM;
+        }
+        
+        if (mbr_get_flag(part, ST_LOGIC))
+                return mbr_add_logical(label, part);
         else
-                /* adding primary */
-                return mbr_add_primary(label, p);
+                return mbr_add_primary(label, part);
 }
 
 /** Remove partition
- *      Removes partition by index, indexed from zero. When removing extended
- *  partition, all logical partitions get removed as well.
- * 
- * @param label                 label to remove from
- * @param idx                   index of the partition to remove
- * 
- * @return                              EOK on success, EINVAL if idx invalid
+ *
+ * Remove partition (indexed from zero). When removing the extended
+ * partition, all logical partitions get removed as well.
+ *
+ * @param label Label to remove from.
+ * @param idx   Index of the partition to remove.
+ *
+ * @return EOK on success.
+ * @return EINVAL if the index is invalid.
+ *
  */
 int mbr_remove_partition(mbr_label_t *label, size_t idx)
 {
-        link_t *l = list_nth(&(label->parts->list), idx);
-        if (l == NULL)
+        link_t *link = list_nth(&label->parts->list, idx);
+        if (link == NULL)
                 return EINVAL;
         
-        mbr_part_t *p;
-        
-        /* If we're removing an extended partition, remove all logical as well */
-        if (l == label->parts->l_extended) {
+        /*
+         * If removing the extended partition, remove all
+         * logical partitions as well.
+         */
+        if (link == label->parts->l_extended) {
                 label->parts->l_extended = NULL;
                 
-                link_t *it = l->next;
-                link_t *next_it;
-                while (it != &(label->parts->list.head)) {
-                        next_it = it->next;
+                link_t *iterator = link->next;
+                link_t *next;
+                
+                while (iterator != &label->parts->list.head) {
+                        next = iterator->next;
+                        mbr_part_t *partition =
+                            list_get_instance(iterator, mbr_part_t, link);
                         
-                        p = list_get_instance(it, mbr_part_t, link);
-                        if (mbr_get_flag(p, ST_LOGIC)) {
-                                list_remove(it);
-                                label->parts->n_logical -= 1;
-                                mbr_free_partition(p);
+                        if (mbr_get_flag(partition, ST_LOGIC)) {
+                                list_remove(iterator);
+                                label->parts->n_logical--;
+                                mbr_free_partition(partition);
                         }
                         
-                        it = next_it;
-                }
-                
+                        iterator = next;
+                }
         }
         
         /* Remove the partition itself */
-        p = list_get_instance(l, mbr_part_t, link);
-        if (mbr_get_flag(p, ST_LOGIC)) {
-                label->parts->n_logical -= 1;
-                list_remove(l);
-                mbr_free_partition(p);
+        mbr_part_t *partition =
+            list_get_instance(link, mbr_part_t, link);
+        
+        if (mbr_get_flag(partition, ST_LOGIC)) {
+                label->parts->n_logical--;
+                list_remove(link);
+                mbr_free_partition(partition);
         } else {
-                /* Cannot remove primary - it would break ordering, just zero it */
-                label->parts->n_primary -= 1;
-                p->type = 0;
-                p->status = 0;
-                p->start_addr = 0;
-                p->length = 0;
-                p->ebr_addr = 0;
+                /*
+                 * Cannot remove a primary partition without
+                 * breaking the ordering. Just zero it.
+                 */
+                label->parts->n_primary--;
+                partition->type = 0;
+                partition->status = 0;
+                partition->start_addr = 0;
+                partition->length = 0;
+                partition->ebr_addr = 0;
         }
         
@@ -523 +509 @@
 }
 
-/** mbr_part_t destructor */
-void mbr_free_partition(mbr_part_t *p)
-{
-        if (p->ebr != NULL)
-                free(p->ebr);
-        free(p);
-}
-
-/** Get flag bool value */
-int mbr_get_flag(mbr_part_t *p, MBR_FLAGS flag)
-{
-        return (p->status & (1 << flag)) ? 1 : 0;
-}
-
-/** Set a specifig status flag to a value */
-void mbr_set_flag(mbr_part_t *p, MBR_FLAGS flag, bool value)
-{
-        uint16_t status = p->status;
-
-        if (value)
-                status = status | (1 << flag);
+/** Partition destructor */
+void mbr_free_partition(mbr_part_t *partition)
+{
+        if (partition->ebr != NULL)
+                free(partition->ebr);
+        
+        free(partition);
+}
+
+/** Check for flag */
+int mbr_get_flag(mbr_part_t *partition, mbr_flags_t flag)
+{
+        return (partition->status & (1 << flag));
+}
+
+/** Set a specific status flag */
+void mbr_set_flag(mbr_part_t *partition, mbr_flags_t flag, bool set)
+{
+        if (set)
+                partition->status |= 1 << flag;
         else
-                status = status ^ (status & (1 << flag));
-
-        p->status = status;
+                partition->status &= ~((uint16_t) (1 << flag));
 }
 
@@ -553 +536 @@
 uint32_t mbr_get_next_aligned(uint32_t addr, unsigned int alignment)
 {
-        uint32_t div = addr / alignment;
-        return (div + 1) * alignment;
-}
-
-list_t * mbr_get_list(mbr_label_t *label)
+        return ALIGN_UP(addr + 1, alignment);
+}
+
+list_t *mbr_get_list(mbr_label_t *label)
 {
         if (label->parts != NULL)
-                return &(label->parts->list);
+                return &label->parts->list;
         else
                 return NULL;
 }
 
-mbr_part_t * mbr_get_first_partition(mbr_label_t *label)
+mbr_part_t *mbr_get_first_partition(mbr_label_t *label)
 {
         list_t *list = mbr_get_list(label);
-        if (list != NULL && !list_empty(list))
+        if ((list != NULL) && (!list_empty(list)))
                 return list_get_instance(list->head.next, mbr_part_t, link);
         else
@@ -574 +556 @@
 }
 
-mbr_part_t * mbr_get_next_partition(mbr_label_t *label, mbr_part_t *p)
+mbr_part_t *mbr_get_next_partition(mbr_label_t *label, mbr_part_t *partition)
 {
         list_t *list = mbr_get_list(label);
-        if (list != NULL && &(p->link) != list_last(list))
-                return list_get_instance(p->link.next, mbr_part_t, link);
+        if ((list != NULL) && (&partition->link != list_last(list)))
+                return list_get_instance(partition->link.next, mbr_part_t, link);
         else
                 return NULL;
 }
 
-/** Just a wrapper for free() */
 void mbr_free_mbr(mbr_t *mbr)
 {
@@ -591 +572 @@
 /** Free partition list
  *
- * @param parts         partition list to be freed
+ * @param parts Partition list to be freed
+ *
  */
 void mbr_free_partitions(mbr_partitions_t *parts)
 {
         list_foreach_safe(parts->list, cur_link, next) {
-                mbr_part_t *p = list_get_instance(cur_link, mbr_part_t, link);
+                mbr_part_t *partition = list_get_instance(cur_link, mbr_part_t, link);
                 list_remove(cur_link);
-                mbr_free_partition(p);
-        }
-
+                mbr_free_partition(partition);
+        }
+        
         free(parts);
 }
 
-/* Internal functions follow */
-
-static br_block_t *alloc_br()
+static br_block_t *alloc_br(void)
 {
         br_block_t *br = malloc(sizeof(br_block_t));
@@ -618 +598 @@
 }
 
-/** Parse partition entry to mbr_part_t
- * @return              returns 1, if extended partition, 0 otherwise
- * */
-static int decode_part(pt_entry_t *src, mbr_part_t *trgt, uint32_t base)
-{
-        trgt->type = src->ptype;
-
-        trgt->status = (trgt->status & 0xFF00) | (uint16_t) src->status;
-
-        trgt->start_addr = uint32_t_le2host(src->first_lba) + base;
-        trgt->length = uint32_t_le2host(src->length);
-
-        return (src->ptype == PT_EXTENDED) ? 1 : 0;
-}
-
-/** Parse MBR contents to mbr_part_t list */
-static int decode_logical(mbr_label_t *label, mbr_part_t * ext)
-{
-        int rc;
-        mbr_part_t *p;
-
-        if (ext == NULL)
+/** Decode partition entry */
+static int decode_part(pt_entry_t *src, mbr_part_t *partition, uint32_t base)
+{
+        partition->type = src->ptype;
+        partition->status = (partition->status & 0xff00) | (uint16_t) src->status;
+        partition->start_addr = uint32_t_le2host(src->first_lba) + base;
+        partition->length = uint32_t_le2host(src->length);
+        
+        return (src->ptype == PT_EXTENDED);
+}
+
+/** Parse logical partitions */
+static int decode_logical(mbr_label_t *label, mbr_part_t *extended)
+{
+        if (extended == NULL)
                 return EOK;
-
-        uint32_t base = ext->start_addr;
+        
+        br_block_t *ebr = alloc_br();
+        if (ebr == NULL)
+                return ENOMEM;
+        
+        uint32_t base = extended->start_addr;
         uint32_t addr = base;
-        br_block_t *ebr;
-        
-        rc = block_init(EXCHANGE_ATOMIC, label->device, 512);
+        
+        int rc = block_init(EXCHANGE_ATOMIC, label->device, 512);
         if (rc != EOK)
-                return rc;
-        
-        ebr = alloc_br();
-        if (ebr == NULL) {
+                goto end;
+        
+        rc = block_read_direct(label->device, addr, 1, ebr);
+        if (rc != EOK)
+                goto end;
+        
+        if (uint16_t_le2host(ebr->signature) != BR_SIGNATURE) {
+                rc = EINVAL;
+                goto end;
+        }
+        
+        if (ebr->pte[0].ptype == PT_UNUSED) {
+                rc = EOK;
+                goto end;
+        }
+        
+        mbr_part_t *partition = mbr_alloc_partition();
+        if (partition == NULL) {
                 rc = ENOMEM;
                 goto end;
         }
         
-        rc = block_read_direct(label->device, addr, 1, ebr);
-        if (rc != EOK) {
-                goto free_ebr_end;
-        }
-        
-        if (uint16_t_le2host(ebr->signature) != BR_SIGNATURE) {
-                rc = EINVAL;
-                goto free_ebr_end;
-        }
-        
-        if (ebr->pte[0].ptype == PT_UNUSED) {
-                rc = EOK;
-                goto free_ebr_end;
-        }
-        
-        p = mbr_alloc_partition();
-        if (p == NULL) {
-                rc = ENOMEM;
-                goto free_ebr_end;
-        }
-        
-        decode_part(&(ebr->pte[0]), p, base);
-        mbr_set_flag(p, ST_LOGIC, true);
-        p->ebr = ebr;
-        p->ebr_addr = addr;
-        rc = mbr_add_partition(label, p);
-        if (rc != ERR_OK) 
-                return EINVAL;
+        decode_part(&ebr->pte[0], partition, base);
+        mbr_set_flag(partition, ST_LOGIC, true);
+        partition->ebr = ebr;
+        partition->ebr_addr = addr;
+        
+        rc = mbr_add_partition(label, partition);
+        if (rc != ERR_OK)
+                goto end;
         
         addr = uint32_t_le2host(ebr->pte[1].first_lba) + base;
         
         while (ebr->pte[1].ptype != PT_UNUSED) {
-                
-                ebr = alloc_br();
-                if (ebr == NULL) {
+                rc = block_read_direct(label->device, addr, 1, ebr);
+                if (rc != EOK)
+                        goto end;
+                
+                if (uint16_t_le2host(ebr->signature) != BR_SIGNATURE) {
+                        rc = EINVAL;
+                        goto end;
+                }
+                
+                mbr_part_t *partition = mbr_alloc_partition();
+                if (partition == NULL) {
                         rc = ENOMEM;
                         goto end;
                 }
                 
-                rc = block_read_direct(label->device, addr, 1, ebr);
-                if (rc != EOK) {
-                        goto free_ebr_end;
-                }
-                
-                if (uint16_t_le2host(ebr->signature) != BR_SIGNATURE) {
-                        rc = EINVAL;
-                        goto free_ebr_end;
-                }
-                
-                p = mbr_alloc_partition();
-                if (p == NULL) {
-                        rc = ENOMEM;
-                        goto free_ebr_end;
-                }
-                
-                decode_part(&(ebr->pte[0]), p, addr);
-                mbr_set_flag(p, ST_LOGIC, true);
-                p->ebr = ebr;
-                p->ebr_addr = addr;
-                rc = mbr_add_partition(label, p);
+                decode_part(&ebr->pte[0], partition, addr);
+                mbr_set_flag(partition, ST_LOGIC, true);
+                partition->ebr = ebr;
+                partition->ebr_addr = addr;
+                
+                rc = mbr_add_partition(label, partition);
                 if (rc != ERR_OK)
-                        return EINVAL;
+                        goto end;
                 
                 addr = uint32_t_le2host(ebr->pte[1].first_lba) + base;
@@ -723 +686 @@
         
         rc = EOK;
-        goto end;
-        
-free_ebr_end:
-        free(ebr);
         
 end:
+        // FIXME possible memory leaks
         block_fini(label->device);
         
@@ -734 +694 @@
 }
 
-/** Convert mbr_part_t to pt_entry_t */
-static void encode_part(mbr_part_t * src, pt_entry_t * trgt, uint32_t base, bool ebr)
+/** Encode partition entry */
+static void encode_part(mbr_part_t *src, pt_entry_t *entry, uint32_t base,
+    bool ebr)
 {
         if (src != NULL) {
-                trgt->status = (uint8_t) (src->status & 0xFF);
-                /* ingoring CHS */
-                trgt->first_chs[0] = 0xFE;
-                trgt->first_chs[1] = 0xFF;
-                trgt->first_chs[2] = 0xFF;
-                trgt->last_chs[0] = 0xFE;
-                trgt->last_chs[1] = 0xFF;
-                trgt->last_chs[2] = 0xFF;
-                if (ebr) {      /* encoding reference to EBR */
-                        trgt->ptype = PT_EXTENDED_LBA;
-                        trgt->first_lba = host2uint32_t_le(src->ebr_addr - base);
-                        trgt->length = host2uint32_t_le(src->length + src->start_addr - src->ebr_addr);
-                } else {        /* encoding reference to partition */
-                        trgt->ptype = src->type;
-                        trgt->first_lba = host2uint32_t_le(src->start_addr - base);
-                        trgt->length = host2uint32_t_le(src->length);
-                }
-                
-                if (trgt->ptype == PT_UNUSED)
-                        memset(trgt, 0, sizeof(pt_entry_t));
-        } else {
-                memset(trgt, 0, sizeof(pt_entry_t));
-        }
-}
-
-/** Check whether two partitions overlap
- * 
- * @return              true/false
- */
-static bool check_overlap(mbr_part_t * p1, mbr_part_t * p2)
-{
-        if (p1->start_addr < p2->start_addr && p1->start_addr + p1->length <= p2->start_addr) {
+                entry->status = (uint8_t) src->status & 0xff;
+                
+                /* Ignore CHS */
+                entry->first_chs[0] = 0xfe;
+                entry->first_chs[1] = 0xff;
+                entry->first_chs[2] = 0xff;
+                entry->last_chs[0] = 0xfe;
+                entry->last_chs[1] = 0xff;
+                entry->last_chs[2] = 0xff;
+                
+                if (ebr) {
+                        /* Encode reference to EBR */
+                        entry->ptype = PT_EXTENDED_LBA;
+                        entry->first_lba = host2uint32_t_le(src->ebr_addr - base);
+                        entry->length = host2uint32_t_le(src->length + src->start_addr -
+                            src->ebr_addr);
+                } else {
+                        /* Encode reference to partition */
+                        entry->ptype = src->type;
+                        entry->first_lba = host2uint32_t_le(src->start_addr - base);
+                        entry->length = host2uint32_t_le(src->length);
+                }
+                
+                if (entry->ptype == PT_UNUSED)
+                        memset(entry, 0, sizeof(pt_entry_t));
+        } else
+                memset(entry, 0, sizeof(pt_entry_t));
+}
+
+/** Check whether two partitions overlap */
+static bool check_overlap(mbr_part_t *part1, mbr_part_t *part2)
+{
+        if ((part1->start_addr < part2->start_addr) &&
+            (part1->start_addr + part1->length <= part2->start_addr))
                 return false;
-        } else if (p1->start_addr > p2->start_addr && p2->start_addr + p2->length <= p1->start_addr) {
+        
+        if ((part1->start_addr > part2->start_addr) &&
+            (part2->start_addr + part2->length <= part1->start_addr))
                 return false;
-        }
-
+        
         return true;
 }
 
-/** Check whether one partition encapsulates the other
- * 
- * @return              true/false
- */
-static bool check_encaps(mbr_part_t * inner, mbr_part_t * outer)
-{
-        if (inner->start_addr <= outer->start_addr || outer->start_addr + outer->length <= inner->start_addr) {
+/** Check whether one partition encapsulates the other */
+static bool check_encaps(mbr_part_t *inner, mbr_part_t *outer)
+{
+        if ((inner->start_addr <= outer->start_addr) ||
+            (outer->start_addr + outer->length <= inner->start_addr))
                 return false;
-        } else if (outer->start_addr + outer->length < inner->start_addr + inner->length) {
+        
+        if (outer->start_addr + outer->length < inner->start_addr + inner->length)
                 return false;
-        }
-
+        
         return true;
 }
 
-/** Check whether one partition preceeds the other
- * 
- * @return              true/false
- */
-static bool check_preceeds(mbr_part_t * preceeder, mbr_part_t * precedee)
+/** Check whether one partition preceeds the other */
+static bool check_preceeds(mbr_part_t *preceeder, mbr_part_t *precedee)
 {
         return preceeder->start_addr < precedee->start_addr;
 }
 
-mbr_err_val mbr_add_primary(mbr_label_t *label, mbr_part_t *p)
-{
-        if (label->parts->n_primary == 4) {
+mbr_err_val mbr_add_primary(mbr_label_t *label, mbr_part_t *part)
+{
+        if (label->parts->n_primary == 4)
                 return ERR_PRIMARY_FULL;
-        }
-        
-        /* Check if partition makes space for MBR itself. */
-        if (p->start_addr == 0) {
+        
+        /* Check if partition makes space for MBR itself */
+        if (part->start_addr == 0)
                 return ERR_OUT_BOUNDS;
-        }
-        
-        /* if it's extended, is there any other one? */
-        if ((p->type == PT_EXTENDED || p->type == PT_EXTENDED_LBA) && label->parts->l_extended != NULL) {
+        
+        /* If it is an extended partition, is there any other one? */
+        if (((part->type == PT_EXTENDED) || (part->type == PT_EXTENDED_LBA)) &&
+            (label->parts->l_extended != NULL))
                 return ERR_EXTENDED_PRESENT;
-        }
-        
-        /* find a place and add it */
+        
+        /* Find a place and add it */
         mbr_part_t *iter;
         mbr_part_t *empty = NULL;
@@ -825 +782 @@
                         if (empty == NULL)
                                 empty = iter;
-                } else if (check_overlap(p, iter))
+                } else if (check_overlap(part, iter))
                         return ERR_OVERLAP;
         }
         
-        list_insert_after(&(p->link), &(empty->link));
-        list_remove(&(empty->link));
+        list_insert_after(&part->link, &empty->link);
+        list_remove(&empty->link);
         free(empty);
         
-        label->parts->n_primary += 1;
-        
-        if (p->type == PT_EXTENDED || p->type == PT_EXTENDED_LBA)
-                label->parts->l_extended = &(p->link);
+        label->parts->n_primary++;
+        
+        if ((part->type == PT_EXTENDED) || (part->type == PT_EXTENDED_LBA))
+                label->parts->l_extended = &part->link;
         
         return EOK;
 }
 
-mbr_err_val mbr_add_logical(mbr_label_t *label, mbr_part_t *p)
-{
-        /* is there any extended partition? */
+mbr_err_val mbr_add_logical(mbr_label_t *label, mbr_part_t *part)
+{
+        /* Is there any extended partition? */
         if (label->parts->l_extended == NULL)
                 return ERR_NO_EXTENDED;
         
-        /* is the logical partition inside the extended one? */
-        mbr_part_t *ext = list_get_instance(label->parts->l_extended, mbr_part_t, link);
-        if (!check_encaps(p, ext))
+        /* Is the logical partition inside the extended partition? */
+        mbr_part_t *extended = list_get_instance(label->parts->l_extended, mbr_part_t, link);
+        if (!check_encaps(part, extended))
                 return ERR_OUT_BOUNDS;
         
-        /* find a place for the new partition in a sorted linked list */
+        /* Find a place for the new partition in a sorted linked list */
         bool first_logical = true;
         mbr_part_t *iter;
         mbr_part_foreach (label, iter) {
                 if (mbr_get_flag(iter, ST_LOGIC)) {
-                        if (check_overlap(p, iter)) 
+                        if (check_overlap(part, iter))
                                 return ERR_OVERLAP;
-                        if (check_preceeds(iter, p)) {
-                                /* checking if there's at least one sector of space preceeding */
-                                if ((iter->start_addr + iter->length) >= p->start_addr - 1)
+                        
+                        if (check_preceeds(iter, part)) {
+                                /* Check if there is at least one sector of space preceeding */
+                                if ((iter->start_addr + iter->length) >= part->start_addr - 1)
                                         return ERR_NO_EBR;
-                        } else if (first_logical){
-                                /* First logical partition's EBR is before every other
-                                 * logical partition. Thus we don't check if this partition
-                                 * leaves enough space for it. */
+                        } else if (first_logical) {
+                                /*
+                                 * First logical partition's EBR is before every other
+                                 * logical partition. Thus we do not check if this partition
+                                 * leaves enough space for it.
+                                 */
                                 first_logical = false;
                         } else {
-                                /* checking if there's at least one sector of space following (for following partitions's EBR) */
-                                if ((p->start_addr + p->length) >= iter->start_addr - 1)
+                                /*
+                                 * Check if there is at least one sector of space following
+                                 * (for following partitions's EBR).
+                                 */
+                                if ((part->start_addr + part->length) >= iter->start_addr - 1)
                                         return ERR_NO_EBR;
                         }
@@ -876 +839 @@
         }
         
-        /* alloc EBR if it's not already there */
-        if (p->ebr == NULL) {
-                p->ebr = alloc_br();
-                if (p->ebr == NULL) {
+        /* Allocate EBR if it is not already there */
+        if (part->ebr == NULL) {
+                part->ebr = alloc_br();
+                if (part->ebr == NULL)
                         return ERR_NOMEM;
-                }
-        }
-        
-        /* add it */
-        list_append(&(p->link), &(label->parts->list));
-        label->parts->n_logical += 1;
+        }
+        
+        list_append(&part->link, &label->parts->list);
+        label->parts->n_logical++;
         
         return EOK;
 }
-
-
-