Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image
On Thu, Jun 30, 2011 at 2:57 AM, Fam Zheng famc...@gmail.com wrote:
On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng famc...@gmail.com wrote:
+ /* trim the quotation marks around */
+ if (fname[0] == '') {
+ memmove(fname, fname + 1, strlen(fname) + 1);
This copies 1 byte too many, just strlen(fname) will do.
I meant to copy the NULL terminator too.
Yes. The problem is the copying starts at fname + 1 but strlen(3)
starts at fname. So there is already an extra byte. The + 1 adds an
additional byte after the NUL.
Stefan
Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image
On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng famc...@gmail.com wrote:
+/* find an option value out of descriptor file */
+static int vmdk_parse_description(const char *desc, const char *opt_name,
+ char *buf, int buf_size)
+{
+ char *opt_pos, *opt_end;
+ const char *end = desc + strlen(desc);
+
+ opt_pos = strstr(desc, opt_name);
+ if (!opt_pos) {
+ return -1;
+ }
+ /* Skip =\ following opt_name */
+ opt_pos += strlen(opt_name) + 2;
+ if (opt_pos = end) {
+ return -1;
+ }
This can produce false positives because strstr(desc, opt_name) will
match anything that contains the opt_name string. Also we don't
verify that =\ follow the opt_name. Luckily we only use this for
createType which will hopefully never cause false positives.
+ opt_end = opt_pos;
+ while (opt_end end *opt_end != '') {
+ opt_end++;
+ }
+ if (opt_end == end || buf_size opt_end - opt_pos + 1) {
+ return -1;
+ }
+ pstrcpy(buf, opt_end - opt_pos + 1, opt_pos);
+ return 0;
+}
+
+static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
+ const char *desc_file_path)
+{
+ int ret = 0;
+ int r;
+ char access[11];
+ char type[11];
+ char fname[512];
+ const char *p = desc;
+ int64_t sectors = 0;
+ int64_t flat_offset;
+
+ while (*p) {
+ if (strncmp(p, RW, strlen(RW))) {
+ goto next_line;
+ }
This check is duplicated below and can be removed.
+ /* parse extent line:
+ * RW [size in sectors] FLAT file-name.vmdk OFFSET
+ * or
+ * RW [size in sectors] SPARSE file-name.vmdk
+ */
+ flat_offset = -1;
+ sscanf(p, %10s %lld %10s %512s,
+ access, sectors, type, fname);
Please check the sscanf(3) return value to ensure the format string
matched. The value of access[], type[], fname[], sectors will be
unchanged at the point where sscanf(3) fails so your checks will not
work.
Declared as char fname[512] but using %512s format string. The
format string needs to be 511 to leave space for the NUL terminator.
+ if (!strcmp(type, FLAT)) {
+ sscanf(p, %10s %lld %10s %512s %lld,
+ access, sectors, type, fname, flat_offset);
+ if (flat_offset == -1) {
+ return -EINVAL;
+ }
+ }
+
+ /* trim the quotation marks around */
+ if (fname[0] == '') {
+ memmove(fname, fname + 1, strlen(fname) + 1);
This copies 1 byte too many, just strlen(fname) will do.
+ if (fname[strlen(fname) - 1] == '') {
+ fname[strlen(fname) - 1] = '\0';
+ }
+ }
If starts as fname[] = {'', '\0'} then this if statement checks
fname[-1] == ''!
+ if (!(strlen(access) sectors strlen(type) strlen(fname))) {
+ goto next_line;
+ }
These can be replaced by checking the sscanf(3) return value above.
Validating sectors is still a good idea though.
+ if (strcmp(type, FLAT) strcmp(type, SPARSE)) {
+ goto next_line;
+ }
+ if (strcmp(access, RW)) {
+ goto next_line;
+ }
+ ret++;
This variable is unused.
+ /* save to extents array */
+ if (!strcmp(type, FLAT)) {
+ /* FLAT extent */
+ char extent_path[PATH_MAX];
+ BlockDriverState *extent_file;
+ BlockDriver *drv;
+ VmdkExtent *extent;
+
+ extent_file = bdrv_new();
+ drv = bdrv_find_format(file);
+ if (!drv) {
bdrv_delete(extent_file);
+ return -EINVAL;
+ }
+ path_combine(extent_path, sizeof(extent_path),
+ desc_file_path, fname);
+ r = bdrv_open(extent_file, extent_path,
+ BDRV_O_RDWR | BDRV_O_NO_BACKING, drv);
We should honor the bs-open_flags. Otherwise
cache=none|writeback|writethrough|unsafe is ignored on the actual
extent files.
+ if (r) {
bdrv_delete(extent_file);
+ return -EINVAL;
+ }
+ extent = vmdk_add_extent(bs, extent_file, true, sectors,
+ 0, 0, 0, 0, sectors);
+ extent-flat_start_offset = flat_offset;
+ } else {
+ /* SPARSE extent, not supported for now */
+ fprintf(stderr,
+ VMDK: Not supported extent type \%s\.\n, type);
+ return -ENOTSUP;
+ }
+next_line:
+ /* move to next line */
+ while (*p *p != '\n') {
+ p++;
+ }
+ p++;
+ }
+ return 0;
+}
+
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
+{
+ int ret;
+ char buf[2048];
+ char ct[128];
+ BDRVVmdkState *s = bs-opaque;
+
+ ret = bdrv_pread(bs-file, 0, buf, sizeof(buf));
+ if (ret 0) {
+ return
Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image
On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng famc...@gmail.com wrote:
+/* find an option value out of descriptor file */
+static int vmdk_parse_description(const char *desc, const char *opt_name,
+ char *buf, int buf_size)
+{
+ char *opt_pos, *opt_end;
+ const char *end = desc + strlen(desc);
+
+ opt_pos = strstr(desc, opt_name);
+ if (!opt_pos) {
+ return -1;
+ }
+ /* Skip =\ following opt_name */
+ opt_pos += strlen(opt_name) + 2;
+ if (opt_pos = end) {
+ return -1;
+ }
This can produce false positives because strstr(desc, opt_name) will
match anything that contains the opt_name string. Also we don't
verify that =\ follow the opt_name. Luckily we only use this for
createType which will hopefully never cause false positives.
+ opt_end = opt_pos;
+ while (opt_end end *opt_end != '') {
+ opt_end++;
+ }
+ if (opt_end == end || buf_size opt_end - opt_pos + 1) {
+ return -1;
+ }
+ pstrcpy(buf, opt_end - opt_pos + 1, opt_pos);
+ return 0;
+}
+
+static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
+ const char *desc_file_path)
+{
+ int ret = 0;
+ int r;
+ char access[11];
+ char type[11];
+ char fname[512];
+ const char *p = desc;
+ int64_t sectors = 0;
+ int64_t flat_offset;
+
+ while (*p) {
+ if (strncmp(p, RW, strlen(RW))) {
+ goto next_line;
+ }
This check is duplicated below and can be removed.
+ /* parse extent line:
+ * RW [size in sectors] FLAT file-name.vmdk OFFSET
+ * or
+ * RW [size in sectors] SPARSE file-name.vmdk
+ */
+ flat_offset = -1;
+ sscanf(p, %10s %lld %10s %512s,
+ access, sectors, type, fname);
Please check the sscanf(3) return value to ensure the format string
matched. The value of access[], type[], fname[], sectors will be
unchanged at the point where sscanf(3) fails so your checks will not
work.
Declared as char fname[512] but using %512s format string. The
format string needs to be 511 to leave space for the NUL terminator.
+ if (!strcmp(type, FLAT)) {
+ sscanf(p, %10s %lld %10s %512s %lld,
+ access, sectors, type, fname, flat_offset);
+ if (flat_offset == -1) {
+ return -EINVAL;
+ }
+ }
+
+ /* trim the quotation marks around */
+ if (fname[0] == '') {
+ memmove(fname, fname + 1, strlen(fname) + 1);
This copies 1 byte too many, just strlen(fname) will do.
I meant to copy the NULL terminator too.
+ if (fname[strlen(fname) - 1] == '') {
+ fname[strlen(fname) - 1] = '\0';
+ }
+ }
If starts as fname[] = {'', '\0'} then this if statement checks
fname[-1] == ''!
+ if (!(strlen(access) sectors strlen(type) strlen(fname))) {
+ goto next_line;
+ }
These can be replaced by checking the sscanf(3) return value above.
Validating sectors is still a good idea though.
+ if (strcmp(type, FLAT) strcmp(type, SPARSE)) {
+ goto next_line;
+ }
+ if (strcmp(access, RW)) {
+ goto next_line;
+ }
+ ret++;
This variable is unused.
+ /* save to extents array */
+ if (!strcmp(type, FLAT)) {
+ /* FLAT extent */
+ char extent_path[PATH_MAX];
+ BlockDriverState *extent_file;
+ BlockDriver *drv;
+ VmdkExtent *extent;
+
+ extent_file = bdrv_new();
+ drv = bdrv_find_format(file);
+ if (!drv) {
bdrv_delete(extent_file);
+ return -EINVAL;
+ }
+ path_combine(extent_path, sizeof(extent_path),
+ desc_file_path, fname);
+ r = bdrv_open(extent_file, extent_path,
+ BDRV_O_RDWR | BDRV_O_NO_BACKING, drv);
We should honor the bs-open_flags. Otherwise
cache=none|writeback|writethrough|unsafe is ignored on the actual
extent files.
+ if (r) {
bdrv_delete(extent_file);
+ return -EINVAL;
+ }
+ extent = vmdk_add_extent(bs, extent_file, true, sectors,
+ 0, 0, 0, 0, sectors);
+ extent-flat_start_offset = flat_offset;
+ } else {
+ /* SPARSE extent, not supported for now */
+ fprintf(stderr,
+ VMDK: Not supported extent type \%s\.\n, type);
+ return -ENOTSUP;
+ }
+next_line:
+ /* move to next line */
+ while (*p *p != '\n') {
+ p++;
+ }
+ p++;
+ }
+ return 0;
+}
+
+static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
+{
+ int ret;
+ char buf[2048];
+
