| Message ID | 20190730172508.19911-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: Let check -r all repair some snapshot bits | expand |
On 7/30/19 12:24 PM, Max Reitz wrote: > The qcow2 specification says to ignore unknown extra data fields in > snapshot table entries. Currently, we discard it whenever we update the > image, which is a bit different from "ignore". > > This patch makes the qcow2 driver keep all unknown extra data fields > when updating an image's snapshot table. The cover letter questioned whether we want this, but I think we do. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.h | 5 ++++ > block/qcow2-snapshot.c | 59 +++++++++++++++++++++++++++++++++++------- > 2 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/block/qcow2.h b/block/qcow2.h > index 175708cee0..290a48b77e 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -61,6 +61,9 @@ > * space for snapshot names and IDs */ > #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS) > > +/* Maximum amount of extra data per snapshot table entry to accept */ > +#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024 > + > /* Bitmap header extension constraints */ > #define QCOW2_MAX_BITMAPS 65535 > #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS) > @@ -178,6 +181,8 @@ typedef struct QCowSnapshot { > uint32_t date_sec; > uint32_t date_nsec; > uint64_t vm_clock_nsec; > + uint32_t extra_data_size; > + void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */ Is char* going to be any easier to use than void*? > +++ b/block/qcow2-snapshot.c > @@ -80,30 +80,52 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp) > + > + /* Read known extra data */ > ret = bdrv_pread(bs->file, offset, &extra, > - MIN(sizeof(extra), extra_data_size)); > + MIN(sizeof(extra), sn->extra_data_size)); > if (ret < 0) { > error_setg_errno(errp, -ret, "Failed to read snapshot table"); > goto fail; > } > - offset += extra_data_size; > + offset += MIN(sizeof(extra), sn->extra_data_size); > > - if (extra_data_size >= 8) { > + if (sn->extra_data_size >= 8) { While touching this, is it worth spelling it: if (sn->extra_data_size >= sizeof(extra.vm_state_size_large)) { > sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large); > } > > - if (extra_data_size >= 16) { > + if (sn->extra_data_size >= 16) { and a similar use of sizeof() instead of hard-coded 16 here? > sn->disk_size = be64_to_cpu(extra.disk_size); > } else { > sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE; > } > > + if (sn->extra_data_size > sizeof(extra)) { > + /* Store unknown extra data */ > + size_t unknown_extra_data_size = > + sn->extra_data_size - sizeof(extra); > + > + sn->unknown_extra_data = g_malloc(unknown_extra_data_size); > + ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data, > + unknown_extra_data_size); We're doing two separate bdrv_pread()s. Would it be better to do a single bdrv_preadv into a vector composed of &extra and &unknown_extra_data, for less I/O? (Then again, this micro-optimization is probably in the noise in the long run) > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to read snapshot table"); > + goto fail; > + } > + offset += unknown_extra_data_size; > + } > + > /* Read snapshot ID */ > sn->id_str = g_malloc(id_str_size + 1); > ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size); > @@ -161,7 +183,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs) > sn = s->snapshots + i; > offset = ROUND_UP(offset, 8); > offset += sizeof(h); > - offset += sizeof(extra); > + offset += MAX(sizeof(extra), sn->extra_data_size); > offset += strlen(sn->id_str); > offset += strlen(sn->name); > > @@ -208,7 +230,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs) > h.date_sec = cpu_to_be32(sn->date_sec); > h.date_nsec = cpu_to_be32(sn->date_nsec); > h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec); > - h.extra_data_size = cpu_to_be32(sizeof(extra)); > + h.extra_data_size = cpu_to_be32(MAX(sizeof(extra), > + sn->extra_data_size)); > > memset(&extra, 0, sizeof(extra)); > extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size); > @@ -233,6 +256,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs) > } > offset += sizeof(extra); > > + if (sn->extra_data_size > sizeof(extra)) { > + size_t unknown_extra_data_size = > + sn->extra_data_size - sizeof(extra); > + > + /* qcow2_read_snapshots() ensures no unbounded allocation */ > + assert(unknown_extra_data_size <= INT_MAX); Should we make this tighter: assert(unknown_extra_data_size < QCOW_MAX_SNAPSHOT_EXTRA_DATA); or even assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA - sizeof(extra)); > + assert(sn->unknown_extra_data); > + > + ret = bdrv_pwrite(bs->file, offset, sn->unknown_extra_data, > + unknown_extra_data_size); > + if (ret < 0) { > + goto fail; > + } > + offset += unknown_extra_data_size; > + } > + > ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size); > if (ret < 0) { > goto fail; > @@ -375,6 +414,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) > sn->date_sec = sn_info->date_sec; > sn->date_nsec = sn_info->date_nsec; > sn->vm_clock_nsec = sn_info->vm_clock_nsec; > + sn->extra_data_size = sizeof(QCowSnapshotExtraData); > > /* Allocate the L1 table of the snapshot and copy the current one there. */ > l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t)); > @@ -646,6 +686,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, > * The snapshot is now unused, clean up. If we fail after this point, we > * won't recover but just leak clusters. > */ > + g_free(sn.unknown_extra_data); > g_free(sn.id_str); > g_free(sn.name); > > Tweaks to assertions are minor enough that I'm okay with: Reviewed-by: Eric Blake <eblake@redhat.com>
On 30.07.19 19:56, Eric Blake wrote: > On 7/30/19 12:24 PM, Max Reitz wrote: >> The qcow2 specification says to ignore unknown extra data fields in >> snapshot table entries. Currently, we discard it whenever we update the >> image, which is a bit different from "ignore". >> >> This patch makes the qcow2 driver keep all unknown extra data fields >> when updating an image's snapshot table. > > The cover letter questioned whether we want this, but I think we do. > >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2.h | 5 ++++ >> block/qcow2-snapshot.c | 59 +++++++++++++++++++++++++++++++++++------- >> 2 files changed, 55 insertions(+), 9 deletions(-) >> >> diff --git a/block/qcow2.h b/block/qcow2.h >> index 175708cee0..290a48b77e 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -61,6 +61,9 @@ >> * space for snapshot names and IDs */ >> #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS) >> >> +/* Maximum amount of extra data per snapshot table entry to accept */ >> +#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024 >> + >> /* Bitmap header extension constraints */ >> #define QCOW2_MAX_BITMAPS 65535 >> #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS) >> @@ -178,6 +181,8 @@ typedef struct QCowSnapshot { >> uint32_t date_sec; >> uint32_t date_nsec; >> uint64_t vm_clock_nsec; >> + uint32_t extra_data_size; >> + void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */ > > Is char* going to be any easier to use than void*? > >> +++ b/block/qcow2-snapshot.c > >> @@ -80,30 +80,52 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp) > >> + >> + /* Read known extra data */ >> ret = bdrv_pread(bs->file, offset, &extra, >> - MIN(sizeof(extra), extra_data_size)); >> + MIN(sizeof(extra), sn->extra_data_size)); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Failed to read snapshot table"); >> goto fail; >> } >> - offset += extra_data_size; >> + offset += MIN(sizeof(extra), sn->extra_data_size); >> >> - if (extra_data_size >= 8) { >> + if (sn->extra_data_size >= 8) { > > While touching this, is it worth spelling it: > if (sn->extra_data_size >= sizeof(extra.vm_state_size_large)) { > >> sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large); >> } >> >> - if (extra_data_size >= 16) { >> + if (sn->extra_data_size >= 16) { > > and a similar use of sizeof() instead of hard-coded 16 here? Well, the most verbose spelling would be offsetof() + sizeof() both times. Hm. I’ll see how it looks. I think it is obvious enough what it means as it is, and the full offsetof() + sizeof() might actually be less obvious just because it’s longer and thus takes longer to read. >> sn->disk_size = be64_to_cpu(extra.disk_size); >> } else { >> sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE; >> } >> >> + if (sn->extra_data_size > sizeof(extra)) { >> + /* Store unknown extra data */ >> + size_t unknown_extra_data_size = >> + sn->extra_data_size - sizeof(extra); >> + >> + sn->unknown_extra_data = g_malloc(unknown_extra_data_size); >> + ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data, >> + unknown_extra_data_size); > > We're doing two separate bdrv_pread()s. Would it be better to do a > single bdrv_preadv into a vector composed of &extra and > &unknown_extra_data, for less I/O? (Then again, this micro-optimization > is probably in the noise in the long run) Interesting idea, we could even add the ID and name string into that vector. But I’m not sure whether it’s really useful. (I’ll take a look anyway, because it sounds interesting.) >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Failed to read snapshot table"); >> + goto fail; >> + } >> + offset += unknown_extra_data_size; >> + } >> + >> /* Read snapshot ID */ >> sn->id_str = g_malloc(id_str_size + 1); >> ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size); >> @@ -161,7 +183,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> sn = s->snapshots + i; >> offset = ROUND_UP(offset, 8); >> offset += sizeof(h); >> - offset += sizeof(extra); >> + offset += MAX(sizeof(extra), sn->extra_data_size); >> offset += strlen(sn->id_str); >> offset += strlen(sn->name); >> >> @@ -208,7 +230,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> h.date_sec = cpu_to_be32(sn->date_sec); >> h.date_nsec = cpu_to_be32(sn->date_nsec); >> h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec); >> - h.extra_data_size = cpu_to_be32(sizeof(extra)); >> + h.extra_data_size = cpu_to_be32(MAX(sizeof(extra), >> + sn->extra_data_size)); >> >> memset(&extra, 0, sizeof(extra)); >> extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size); >> @@ -233,6 +256,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> } >> offset += sizeof(extra); >> >> + if (sn->extra_data_size > sizeof(extra)) { >> + size_t unknown_extra_data_size = >> + sn->extra_data_size - sizeof(extra); >> + >> + /* qcow2_read_snapshots() ensures no unbounded allocation */ >> + assert(unknown_extra_data_size <= INT_MAX); > > Should we make this tighter: > assert(unknown_extra_data_size < QCOW_MAX_SNAPSHOT_EXTRA_DATA); > or even > assert(unknown_extra_data_size <= > QCOW_MAX_SNAPSHOT_EXTRA_DATA - sizeof(extra)); Hm, I wanted to signal that it can fit into the int taken by bdrv_pwrite(). But then I should use BDRV_REQUEST_MAX_BYTES, actually. I’m not sure. It doesn’t really matter that it’s less than QCOW_MAX_SNAPSHOT_EXTRA_DATA here. What does matter is that it doesn’t exceed BDRV_REQUEST_MAX_BYTES. >> + assert(sn->unknown_extra_data); >> + >> + ret = bdrv_pwrite(bs->file, offset, sn->unknown_extra_data, >> + unknown_extra_data_size); >> + if (ret < 0) { >> + goto fail; >> + } >> + offset += unknown_extra_data_size; >> + } >> + >> ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size); >> if (ret < 0) { >> goto fail; >> @@ -375,6 +414,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) >> sn->date_sec = sn_info->date_sec; >> sn->date_nsec = sn_info->date_nsec; >> sn->vm_clock_nsec = sn_info->vm_clock_nsec; >> + sn->extra_data_size = sizeof(QCowSnapshotExtraData); >> >> /* Allocate the L1 table of the snapshot and copy the current one there. */ >> l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t)); >> @@ -646,6 +686,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, >> * The snapshot is now unused, clean up. If we fail after this point, we >> * won't recover but just leak clusters. >> */ >> + g_free(sn.unknown_extra_data); >> g_free(sn.id_str); >> g_free(sn.name); >> >> > > Tweaks to assertions are minor enough that I'm okay with: > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks for reviewing! Max
On 31.07.19 10:54, Max Reitz wrote: > On 30.07.19 19:56, Eric Blake wrote: >> On 7/30/19 12:24 PM, Max Reitz wrote: [...] >>> + if (sn->extra_data_size > sizeof(extra)) { >>> + /* Store unknown extra data */ >>> + size_t unknown_extra_data_size = >>> + sn->extra_data_size - sizeof(extra); >>> + >>> + sn->unknown_extra_data = g_malloc(unknown_extra_data_size); >>> + ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data, >>> + unknown_extra_data_size); >> >> We're doing two separate bdrv_pread()s. Would it be better to do a >> single bdrv_preadv into a vector composed of &extra and >> &unknown_extra_data, for less I/O? (Then again, this micro-optimization >> is probably in the noise in the long run) > > Interesting idea, we could even add the ID and name string into that > vector. But I’m not sure whether it’s really useful. > > (I’ll take a look anyway, because it sounds interesting.) I did, and it was actually really nice. I liked it. (I don’t hink the performance is important, but it was actually just simpler.) But then it turned out that it won’t work with patch 8, because after that we may want to skip parts of the unknown extra data – and skipping doesn’t work with bdrv_preadv()... (So all the simplicity is lost, and that’s what I was interested in.) But it was indeed quite cool. :-/ Max
diff --git a/block/qcow2.h b/block/qcow2.h index 175708cee0..290a48b77e 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -61,6 +61,9 @@ * space for snapshot names and IDs */ #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS) +/* Maximum amount of extra data per snapshot table entry to accept */ +#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024 + /* Bitmap header extension constraints */ #define QCOW2_MAX_BITMAPS 65535 #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS) @@ -178,6 +181,8 @@ typedef struct QCowSnapshot { uint32_t date_sec; uint32_t date_nsec; uint64_t vm_clock_nsec; + uint32_t extra_data_size; + void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */ } QCowSnapshot; struct Qcow2Cache; diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 6295ba4c9c..b037c6e5fd 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -37,6 +37,7 @@ void qcow2_free_snapshots(BlockDriverState *bs) for(i = 0; i < s->nb_snapshots; i++) { g_free(s->snapshots[i].name); g_free(s->snapshots[i].id_str); + g_free(s->snapshots[i].unknown_extra_data); } g_free(s->snapshots); s->snapshots = NULL; @@ -51,7 +52,6 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp) QCowSnapshot *sn; int i, id_str_size, name_size; int64_t offset; - uint32_t extra_data_size; int ret; if (!s->nb_snapshots) { @@ -80,30 +80,52 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp) sn->date_sec = be32_to_cpu(h.date_sec); sn->date_nsec = be32_to_cpu(h.date_nsec); sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec); - extra_data_size = be32_to_cpu(h.extra_data_size); + sn->extra_data_size = be32_to_cpu(h.extra_data_size); id_str_size = be16_to_cpu(h.id_str_size); name_size = be16_to_cpu(h.name_size); - /* Read extra data */ + if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) { + ret = -EFBIG; + error_setg(errp, "Too much extra metadata in snapshot table " + "entry %i", i); + goto fail; + } + + /* Read known extra data */ ret = bdrv_pread(bs->file, offset, &extra, - MIN(sizeof(extra), extra_data_size)); + MIN(sizeof(extra), sn->extra_data_size)); if (ret < 0) { error_setg_errno(errp, -ret, "Failed to read snapshot table"); goto fail; } - offset += extra_data_size; + offset += MIN(sizeof(extra), sn->extra_data_size); - if (extra_data_size >= 8) { + if (sn->extra_data_size >= 8) { sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large); } - if (extra_data_size >= 16) { + if (sn->extra_data_size >= 16) { sn->disk_size = be64_to_cpu(extra.disk_size); } else { sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE; } + if (sn->extra_data_size > sizeof(extra)) { + /* Store unknown extra data */ + size_t unknown_extra_data_size = + sn->extra_data_size - sizeof(extra); + + sn->unknown_extra_data = g_malloc(unknown_extra_data_size); + ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data, + unknown_extra_data_size); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to read snapshot table"); + goto fail; + } + offset += unknown_extra_data_size; + } + /* Read snapshot ID */ sn->id_str = g_malloc(id_str_size + 1); ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size); @@ -161,7 +183,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs) sn = s->snapshots + i; offset = ROUND_UP(offset, 8); offset += sizeof(h); - offset += sizeof(extra); + offset += MAX(sizeof(extra), sn->extra_data_size); offset += strlen(sn->id_str); offset += strlen(sn->name); @@ -208,7 +230,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs) h.date_sec = cpu_to_be32(sn->date_sec); h.date_nsec = cpu_to_be32(sn->date_nsec); h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec); - h.extra_data_size = cpu_to_be32(sizeof(extra)); + h.extra_data_size = cpu_to_be32(MAX(sizeof(extra), + sn->extra_data_size)); memset(&extra, 0, sizeof(extra)); extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size); @@ -233,6 +256,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs) } offset += sizeof(extra); + if (sn->extra_data_size > sizeof(extra)) { + size_t unknown_extra_data_size = + sn->extra_data_size - sizeof(extra); + + /* qcow2_read_snapshots() ensures no unbounded allocation */ + assert(unknown_extra_data_size <= INT_MAX); + assert(sn->unknown_extra_data); + + ret = bdrv_pwrite(bs->file, offset, sn->unknown_extra_data, + unknown_extra_data_size); + if (ret < 0) { + goto fail; + } + offset += unknown_extra_data_size; + } + ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size); if (ret < 0) { goto fail; @@ -375,6 +414,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) sn->date_sec = sn_info->date_sec; sn->date_nsec = sn_info->date_nsec; sn->vm_clock_nsec = sn_info->vm_clock_nsec; + sn->extra_data_size = sizeof(QCowSnapshotExtraData); /* Allocate the L1 table of the snapshot and copy the current one there. */ l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t)); @@ -646,6 +686,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, * The snapshot is now unused, clean up. If we fail after this point, we * won't recover but just leak clusters. */ + g_free(sn.unknown_extra_data); g_free(sn.id_str); g_free(sn.name);
The qcow2 specification says to ignore unknown extra data fields in snapshot table entries. Currently, we discard it whenever we update the image, which is a bit different from "ignore". This patch makes the qcow2 driver keep all unknown extra data fields when updating an image's snapshot table. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2.h | 5 ++++ block/qcow2-snapshot.c | 59 +++++++++++++++++++++++++++++++++++------- 2 files changed, 55 insertions(+), 9 deletions(-)