| Message ID | 1414076175-17034-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/23/2014 08:56 AM, Max Reitz wrote: > When using a relative backing file name, qemu needs to know the > directory of the top image file. For JSON filenames, such a directory > cannot be easily determined (e.g. how do you determine the directory of > a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow > relative filenames for the backing file of BDSs only having a JSON > filename. > Are JSON names the only case where we want to do this, or should we widen it to all non-file protocol names? Then again, the use of relative names is currently being used as a NICE way on glusterfs setups to hide whether two files are both being accessed via gluster protocol or both being accessed via file protocol (if the gluster volume is also mapped to the local file system such as via NFS) - that is, referring to 'base.img' as the backing file of 'top.img' works whether you opened '/path/to/top.img' or 'gluster://host/volume/path/to/top.img', when base.img and top.img are in the same directory of the gluster volume. > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > Just by the way, the reason for using bs->exact_filename in > bdrv_get_full_backing_filename() instead of just testing whether > bs->filename is prefixed by "json:" is that in the future we might have > cases where bs->filename contains a JSON filename, but > bs->exact_filename is set to a non-JSON filename (because there are some > rather vital options, which radically change performance or something > like that, so we want them to be included in bs->filename if they were > specified; but we can still generate a plain filename which results in > the same data read and written, but just in some very different way or > something like that). > Actually, I might write a follow-up patch which makes > bdrv_refresh_filename() always generate an exact_filename if somehow > possible, even if unknown options were specified. This would then be > very useful for this function, but on the other hand, it would no longer > fit the definition of exact_filename (in order to follow that > definition, we have to be certain that we don't omit any vital options > which really change the data read and written). > --- > block.c | 19 +++++++++++++++---- > block/qapi.c | 7 ++++++- > include/block/block.h | 2 +- > 3 files changed, 22 insertions(+), 6 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On 23.10.2014 19:42, Eric Blake wrote: > On 10/23/2014 08:56 AM, Max Reitz wrote: >> When using a relative backing file name, qemu needs to know the >> directory of the top image file. For JSON filenames, such a directory >> cannot be easily determined (e.g. how do you determine the directory of >> a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow >> relative filenames for the backing file of BDSs only having a JSON >> filename. >> > Are JSON names the only case where we want to do this, or should we > widen it to all non-file protocol names? It'll probably work for HTTP, NFS of course and I can see it working for NBD, too, if one is crazy enough to do that (and you're mentioning glusterfs). In general, I think all filenames have some normal unix-path-like sequence as their tail, so relative filenames can work there (and maybe there are even people using it already for all kinds of non-file protocols). > Then again, the use of > relative names is currently being used as a NICE way on glusterfs setups > to hide whether two files are both being accessed via gluster protocol > or both being accessed via file protocol (if the gluster volume is also > mapped to the local file system such as via NFS) - that is, referring to > 'base.img' as the backing file of 'top.img' works whether you opened > '/path/to/top.img' or 'gluster://host/volume/path/to/top.img', when > base.img and top.img are in the same directory of the gluster volume. > >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> Just by the way, the reason for using bs->exact_filename in >> bdrv_get_full_backing_filename() instead of just testing whether >> bs->filename is prefixed by "json:" is that in the future we might have >> cases where bs->filename contains a JSON filename, but >> bs->exact_filename is set to a non-JSON filename (because there are some >> rather vital options, which radically change performance or something >> like that, so we want them to be included in bs->filename if they were >> specified; but we can still generate a plain filename which results in >> the same data read and written, but just in some very different way or >> something like that). >> Actually, I might write a follow-up patch which makes >> bdrv_refresh_filename() always generate an exact_filename if somehow >> possible, even if unknown options were specified. This would then be >> very useful for this function, but on the other hand, it would no longer >> fit the definition of exact_filename (in order to follow that >> definition, we have to be certain that we don't omit any vital options >> which really change the data read and written). >> --- >> block.c | 19 +++++++++++++++---- >> block/qapi.c | 7 ++++++- >> include/block/block.h | 2 +- >> 3 files changed, 22 insertions(+), 6 deletions(-) >> > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks! Max
Am 23.10.2014 um 20:00 hat Max Reitz geschrieben: > On 23.10.2014 19:42, Eric Blake wrote: > >On 10/23/2014 08:56 AM, Max Reitz wrote: > >>When using a relative backing file name, qemu needs to know the > >>directory of the top image file. For JSON filenames, such a directory > >>cannot be easily determined (e.g. how do you determine the directory of > >>a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow > >>relative filenames for the backing file of BDSs only having a JSON > >>filename. > >> > >Are JSON names the only case where we want to do this, or should we > >widen it to all non-file protocol names? > > It'll probably work for HTTP, NFS of course and I can see it working > for NBD, too, if one is crazy enough to do that (and you're > mentioning glusterfs). In general, I think all filenames have some > normal unix-path-like sequence as their tail, so relative filenames > can work there (and maybe there are even people using it already for > all kinds of non-file protocols). Perhaps make path_combine() a BlockDriver callback so that each block driver can implement relative paths the way they make most sense for it, or NULL to forbid them? We can still keep a common implementation for "normal" paths in block.c and reference that in raw-posix, curl, etc. Not sure how tricky it will become to integrate any of this relative file name handling nicely in a future, completely QDict-based bdrv_open()... Kevin
diff --git a/block.c b/block.c index 88f6d9b..6ccc94b 100644 --- a/block.c +++ b/block.c @@ -303,12 +303,17 @@ void path_combine(char *dest, int dest_size, } } -void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz) +void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz, Error **errp) { - if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) { + if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file) || + path_is_absolute(bs->backing_file)) { pstrcpy(dest, sz, bs->backing_file); + } else if (!bs->exact_filename[0] || + strstart(bs->exact_filename, "json:", NULL)) { + error_setg(errp, "Cannot use relative backing file names for '%s'", + bs->filename); } else { - path_combine(dest, sz, bs->filename, bs->backing_file); + path_combine(dest, sz, bs->exact_filename, bs->backing_file); } } @@ -1197,7 +1202,13 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) QDECREF(options); goto free_exit; } else { - bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX); + bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX, &local_err); + if (local_err) { + ret = -EINVAL; + error_propagate(errp, local_err); + QDECREF(options); + goto free_exit; + } } if (!bs->drv || !bs->drv->supports_backing) { diff --git a/block/qapi.c b/block/qapi.c index 1301144..77baa36 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -229,7 +229,12 @@ void bdrv_query_image_info(BlockDriverState *bs, info->backing_filename = g_strdup(backing_filename); info->has_backing_filename = true; bdrv_get_full_backing_filename(bs, backing_filename2, - sizeof(backing_filename2)); + sizeof(backing_filename2), &err); + if (err) { + error_propagate(errp, err); + qapi_free_ImageInfo(info); + return; + } if (strcmp(backing_filename, backing_filename2) != 0) { info->full_backing_filename = diff --git a/include/block/block.h b/include/block/block.h index 341054d..3abc7df 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -387,7 +387,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); void bdrv_get_full_backing_filename(BlockDriverState *bs, - char *dest, size_t sz); + char *dest, size_t sz, Error **errp); int bdrv_is_snapshot(BlockDriverState *bs); int path_is_absolute(const char *path);
When using a relative backing file name, qemu needs to know the directory of the top image file. For JSON filenames, such a directory cannot be easily determined (e.g. how do you determine the directory of a qcow2 BDS directly on top of a quorum BDS?). Therefore, do not allow relative filenames for the backing file of BDSs only having a JSON filename. Signed-off-by: Max Reitz <mreitz@redhat.com> --- Just by the way, the reason for using bs->exact_filename in bdrv_get_full_backing_filename() instead of just testing whether bs->filename is prefixed by "json:" is that in the future we might have cases where bs->filename contains a JSON filename, but bs->exact_filename is set to a non-JSON filename (because there are some rather vital options, which radically change performance or something like that, so we want them to be included in bs->filename if they were specified; but we can still generate a plain filename which results in the same data read and written, but just in some very different way or something like that). Actually, I might write a follow-up patch which makes bdrv_refresh_filename() always generate an exact_filename if somehow possible, even if unknown options were specified. This would then be very useful for this function, but on the other hand, it would no longer fit the definition of exact_filename (in order to follow that definition, we have to be certain that we don't omit any vital options which really change the data read and written). --- block.c | 19 +++++++++++++++---- block/qapi.c | 7 ++++++- include/block/block.h | 2 +- 3 files changed, 22 insertions(+), 6 deletions(-)