| Message ID | 1305706755-2816-10-git-send-email-asias.hejun@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Asias He <asias.hejun@gmail.com> wrote: > - fd = open(filename, O_RDONLY); > + /* > + * Be careful! We are opening host block device! > + * Open it readonly since we do not want to break user's data on disk. > + */ > + fd = open(filename, O_RDONLY); > if (fd < 0) > return NULL; btw., this is a repeating pattern i noticed: you align assignment vertically even if it's a stand-alone assignment. We want to apply vertical alignment only when it helps readability - and repeated assingments such as: *job = (struct thread_pool__job) { .kvm = kvm, .data = data, .callback = callback, .mutex = PTHREAD_MUTEX_INITIALIZER }; indeed look *much* better when aligned vertically. Same goes for structure definitions. Thanks for applying those concepts uniformly around tools/kvm/, it makes the code visibly more pleasant to read. But the above standalone assignment of 'fd' does not seem to be such a case: the right side of the assignment just 'floats' freely in space with no other similar assingment next to it giving it structure. Thus the old-fashioned: fd = open(filename, O_RDONLY); if (fd < 0) return NULL; is a lot more readable form IMO. There's many similar examples of isolated assignments looking weird, all around the code. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/disk/blk.c b/tools/kvm/disk/blk.c index 7383967..59294e8 100644 --- a/tools/kvm/disk/blk.c +++ b/tools/kvm/disk/blk.c @@ -3,7 +3,7 @@ /* * raw image and blk dev are similar, so reuse raw image ops. */ -static struct disk_image_operations raw_image_ops = { +static struct disk_image_operations blk_dev_ops = { .read_sector = raw_image__read_sector, .write_sector = raw_image__write_sector, .close = raw_image__close, @@ -17,7 +17,11 @@ struct disk_image *blkdev__probe(const char *filename, struct stat *st) if (!S_ISBLK(st->st_mode)) return NULL; - fd = open(filename, O_RDONLY); + /* + * Be careful! We are opening host block device! + * Open it readonly since we do not want to break user's data on disk. + */ + fd = open(filename, O_RDONLY); if (fd < 0) return NULL; @@ -26,5 +30,10 @@ struct disk_image *blkdev__probe(const char *filename, struct stat *st) return NULL; } - return disk_image__new(fd, size, &raw_image_ops, DISK_IMAGE_MMAP); + /* + * FIXME: This will not work on 32-bit host because we can not + * mmap large disk. There is not enough virtual address space + * in 32-bit host. However, this works on 64-bit host. + */ + return disk_image__new(fd, size, &blk_dev_ops, DISK_IMAGE_MMAP); }
This patch also adds some comments to disk/blk.c Signed-off-by: Asias He <asias.hejun@gmail.com> --- tools/kvm/disk/blk.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-)