* Prasad Joshi <prasadjoshi124@xxxxxxxxx> wrote: > The code is based on the following QCOW 1 image format specification: > > http://people.gnome.org/~markmc/qcow-image-format-version-1.html > > Changes since V1: > - Variable data type change. > - Added a function qcow_write_l2_table() to write level2 table on to disk. > - Added a comment describing necessity of the fsync() call in qcow_pwrite_with_sync() > - Code reformatting. > > Signed-off-by: Prasad Joshi <prasadjoshi124@xxxxxxxxx> > --- > tools/kvm/qcow.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 170 insertions(+), 1 deletions(-) Just a small stylistic nit which i noticed while skimming the patch. There's a relatively large number of long lines in it, such as: > + if (qcow_pwrite_with_sync(q->fd, l2_table, l2_table_size * sizeof(u64), > + l2_table_offset) < 0) These make the code harder to read. The solution is not to break the line - that makes the code even harder to read! The way to fix overlong lines is to make the code *more compact*. There are several techniques that can be used in this particular case: - drop the qcow_ prefix from qcow.c local function names, so qcow_pwrite_with_sync() becomes pwrite_with_sync() - drop the unnecessary 'with_': so it becomes pwrite_sync() - use shorter variable names and use them everywhere. For example if l2_table is used very often, abbreviate it as 'l2t' - developers working with this code will quickly learn it and it will become a term of art on its own right. (Use it judiciously and do not over-abbreviate though - l2ts for l2_table_size would be a step backwards for example.) - Instead of l2_table_size and l2_table_offset locals, use 'size' and 'offset', - we are already within a l2_table function so it's obvious that this is about L2 table details. etc. There's more techniques like this. Just to show what is possible, here's the 'before' and 'after' state of this function: before: static u64 qcow1_write_l2_table(struct qcow *q, u64 *l2_table) { struct qcow1_header *header = q->header; u64 l2_table_size; u64 l2_table_offset; u64 clust_size; l2_table_size = 1 << header->l2_bits; clust_size = 1 << header->cluster_bits; l2_table_offset = align(get_file_length(q->fd), clust_size); if (qcow_pwrite_with_sync(q->fd, l2_table, l2_table_size * sizeof(u64), l2_table_offset) < 0) return 0; return l2_table_offset; } /* * Write the L2 table: */ static u64 write_l2t(struct qcow *q, u64 *l2t) { struct qcow1_header *header = q->header; u64 size; u64 offset; u64 clust_size; size = 1 << header->l2_bits; clust_size = 1 << header->cluster_bits; offset = align(get_file_length(q->fd), clust_size); if (pwrite_sync(q->fd, l2t, size*sizeof(u64), offset) < 0) return 0; return offset; } See how much clearer the logic is? But there's more cleanups possible. Notice for example the inconsistency in naming here: clust_size = 1 << header->cluster_bits; Why is one perfixed with 'cluster_', while the other with 'clust_'? Rename one to the other. If clust_ reads fine to everyone who hacks this code (it might not) then use that consistently everywhere. size = 1 << header->l2_bits; l2_bits was inconsisent with the original l2_table_size naming. It would be more consistent to name it l2_size_bits in the header - then the 'size' local variable name follows naturally out of that. offset = align(get_file_length(q->fd), clust_size); The get_ in get_file_length() is superfluous - when a function is named file_length() then it's pretty obvious what it does. Btw., file_size() would probably be better. Finally, as 'header' seems to be used very often in this file, it makes sense to abbreviate that as 'h'. With these secondary cleanups in place the function would look like this: /* * Write the L2 table: */ static u64 write_l2t(struct qcow *q, u64 *l2t) { struct qcow1_header *h = q->header; u64 size; u64 clust_size; u64 offset; size = (1 << h->l2_size_bits)*sizeof(u64); clust_size = 1 << h->clust_bits; offset = align(file_size(q->fd), clust_size); if (pwrite_sync(q->fd, l2t, size, offset) < 0) return 0; return offset; } All in one, notice how none of the lines are even close to becoming too long, and notice how much more readable the whole flow has become. Etc. These are just random suggestions and you might end up doing different things, but my main point is that it's *always* possible to avoid overlong lines by writing squeaky clean code. Note that other code within this patch has similar long line line-break uglies which are canaries of other cleanliness problems. Thanks, Ingo [*] Note one more detail: i used two newlines to make the two groups of statements stand out, one the calculation of 'size', the other the calculation of 'offset'. A third newline was used to make the pwrite_sync() stand out alone, separate from the return statement. [**] Note another very small detail - i reordered the local variable definitions to match the exact order of their initialization in the program flow. This makes the logic even more obvious. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
