Added driver for QCOW1 format #218
Conversation
There was a problem hiding this comment.
Thanks for your PR! Looks like a potentially useful piece of code.
I made a number of comments inline. Apart from that, it is evident that you didn't run Ccheck (our C style checker) on the code. Run 'make ccheck' from the source root. This gives many errors in your files including incorrect use of CRLF line endings and missing LF at the end of file.
| #include "qcow_bd.h" | ||
|
|
||
| static FILE *img; | ||
| static QCowHeader header; |
There was a problem hiding this comment.
In HelenOS identifiers should use lower_case not CamelCase. Type names should end with _t so it should be something like qcow_header_t.
| printf("Usage: " NAME " [-b <block_size>] <image_file> <device_name>\n"); | ||
| } | ||
|
|
||
| static void initialize_state() |
There was a problem hiding this comment.
This is not a valid function definition in C. A function with no parameters needs to be daclared as name(void).
| } | ||
|
|
||
| /* Return 0 for if this system uses big endian, 1 for little endian. */ | ||
| static bool isLittleEndian() { |
There was a problem hiding this comment.
There is no need to check for the system's endianness. Use macros defined in libc's byteorder.h [u]intt{le|be}2host to convert from fixed byteorder to the CPU's byteorder and host2[u]intt{le|be} to convert from CPU byteorder to fixed byteorder.
| return (*((uint8_t*)(&i))) == 0x67; | ||
| } | ||
|
|
||
| static errno_t qcow_bd_init(const char *fname) |
There was a problem hiding this comment.
Functions should, in general, have Doxygen DOC header. If they are missing it in existing code, it's an omission that should be fixed. Newly written code should be more diligent.
|
|
||
| /* Swap values to big-endian */ | ||
| if (isLittleEndian()) { | ||
| header.magic = __builtin_bswap32(header.magic); |
There was a problem hiding this comment.
This is not good coding style. You should not modify the header in place like that. I would decode the data to a different structure. Not just different instance of the same type. I would argue that on-disk and in-core representations should be different types altogether (see, for example, uspace/srv/net/udp/udp.{c|h}.
| return ENOTSUP; | ||
| } | ||
|
|
||
| initialize_state(); |
There was a problem hiding this comment.
Why don't you pass state as an argument? Also, state_initialize() would be more appropriate naming scheme. Ideally you'd go all the way to eliminate global variables. I'm willing to let it pass here, but at least some effort could be made.
| } | ||
|
|
||
| /** From the offset of the given block compute its offset which is relative from the start of qcow file. */ | ||
| static errno_t get_block_offset(uint64_t *offset) |
There was a problem hiding this comment.
It makes no sense to use an in-out argument here, it's just horribly confusing. Also ideally this would have a pointer to the Qcow instance object as the first argument. Something like static errno_t qcow_get_block_offset(qcow_t *, uint64_t voffset, uint64_t *offset). Note that you are using both fields from state and header here. An instance structure would need to contain all that information.
| const void *buf, size_t size) | ||
| { | ||
| // TODO | ||
| return EOK; |
There was a problem hiding this comment.
This should return ENOTSUP.
No description provided.