| Message ID | 1288203550-23698-1-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 27 Oct 2010, Anthony Liguori wrote: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > diff --git a/block.c b/block.c > index 1a965b2..00b6f21 100644 > --- a/block.c > +++ b/block.c > @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > BlockDriver *back_drv = NULL; > > bs->backing_hd = bdrv_new(""); > - path_combine(backing_filename, sizeof(backing_filename), > - filename, bs->backing_file); > - if (bs->backing_format[0] != '\0') > - back_drv = bdrv_find_format(bs->backing_format); > + back_drv = bdrv_find_protocol(bs->backing_file); > + if (!back_drv) { > + path_combine(backing_filename, sizeof(backing_filename), > + filename, bs->backing_file); > + if (bs->backing_format[0] != '\0') > + back_drv = bdrv_find_format(bs->backing_format); Sigh.. > + } else { > + pstrcpy(backing_filename, sizeof(backing_filename), > + bs->backing_file); > + } > > /* backing files always opened read-only */ > back_flags = >
On 10/27/2010 02:22 PM, malc wrote: > On Wed, 27 Oct 2010, Anthony Liguori wrote: > > >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> >> diff --git a/block.c b/block.c >> index 1a965b2..00b6f21 100644 >> --- a/block.c >> +++ b/block.c >> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >> BlockDriver *back_drv = NULL; >> >> bs->backing_hd = bdrv_new(""); >> - path_combine(backing_filename, sizeof(backing_filename), >> - filename, bs->backing_file); >> - if (bs->backing_format[0] != '\0') >> - back_drv = bdrv_find_format(bs->backing_format); >> + back_drv = bdrv_find_protocol(bs->backing_file); >> + if (!back_drv) { >> + path_combine(backing_filename, sizeof(backing_filename), >> + filename, bs->backing_file); >> + if (bs->backing_format[0] != '\0') >> + back_drv = bdrv_find_format(bs->backing_format); >> > Sigh.. > Always good to see such clear and constructive comments... I'll respin. Regards, Anthony Liguori > >> + } else { >> + pstrcpy(backing_filename, sizeof(backing_filename), >> + bs->backing_file); >> + } >> >> /* backing files always opened read-only */ >> back_flags = >> >> >
On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > diff --git a/block.c b/block.c > index 1a965b2..00b6f21 100644 > --- a/block.c > +++ b/block.c > @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > BlockDriver *back_drv = NULL; > > bs->backing_hd = bdrv_new(""); > - path_combine(backing_filename, sizeof(backing_filename), > - filename, bs->backing_file); > - if (bs->backing_format[0] != '\0') > - back_drv = bdrv_find_format(bs->backing_format); > + back_drv = bdrv_find_protocol(bs->backing_file); > + if (!back_drv) { > + path_combine(backing_filename, sizeof(backing_filename), > + filename, bs->backing_file); > + if (bs->backing_format[0] != '\0') > + back_drv = bdrv_find_format(bs->backing_format); > + } else { > + pstrcpy(backing_filename, sizeof(backing_filename), > + bs->backing_file); > + } > > /* backing files always opened read-only */ > back_flags = > -- > 1.7.0.4 I think this makes sense. Now it is possible to specify backing files that are relative to QEMU's current working directory using file:filename. I don't see harm in this. Stefan
Am 28.10.2010 10:30, schrieb Stefan Hajnoczi: > On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> >> diff --git a/block.c b/block.c >> index 1a965b2..00b6f21 100644 >> --- a/block.c >> +++ b/block.c >> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >> BlockDriver *back_drv = NULL; >> >> bs->backing_hd = bdrv_new(""); >> - path_combine(backing_filename, sizeof(backing_filename), >> - filename, bs->backing_file); >> - if (bs->backing_format[0] != '\0') >> - back_drv = bdrv_find_format(bs->backing_format); >> + back_drv = bdrv_find_protocol(bs->backing_file); >> + if (!back_drv) { >> + path_combine(backing_filename, sizeof(backing_filename), >> + filename, bs->backing_file); >> + if (bs->backing_format[0] != '\0') >> + back_drv = bdrv_find_format(bs->backing_format); >> + } else { >> + pstrcpy(backing_filename, sizeof(backing_filename), >> + bs->backing_file); >> + } >> >> /* backing files always opened read-only */ >> back_flags = >> -- >> 1.7.0.4 > > I think this makes sense. > > Now it is possible to specify backing files that are relative to > QEMU's current working directory using file:filename. I don't see > harm in this. It would be more consistent if it was relative to the image, but there's no meaningful way to do that for arbitrary protocols. It's definitely not worse than before the change. Kevin
On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote: > On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > > > diff --git a/block.c b/block.c > > index 1a965b2..00b6f21 100644 > > --- a/block.c > > +++ b/block.c > > @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > > BlockDriver *back_drv = NULL; > > > > bs->backing_hd = bdrv_new(""); > > - path_combine(backing_filename, sizeof(backing_filename), > > - filename, bs->backing_file); > > - if (bs->backing_format[0] != '\0') > > - back_drv = bdrv_find_format(bs->backing_format); > > + back_drv = bdrv_find_protocol(bs->backing_file); > > + if (!back_drv) { > > + path_combine(backing_filename, sizeof(backing_filename), > > + filename, bs->backing_file); > > + if (bs->backing_format[0] != '\0') > > + back_drv = bdrv_find_format(bs->backing_format); > > + } else { > > + pstrcpy(backing_filename, sizeof(backing_filename), > > + bs->backing_file); > > + } > > > > /* backing files always opened read-only */ > > back_flags = > > -- > > 1.7.0.4 > > I think this makes sense. > > Now it is possible to specify backing files that are relative to > QEMU's current working directory using file:filename. I don't see > harm in this. Shouldn't a backing file be treated as relative to the image file pointing to it, rather than the QEMU working directory. eg so you can do # qemu-img create backing.img # qemu-img create -o backing_file=file:backing.img main.img And have main.img be able to resolve backing.img in its same directory, no matter what directory QEMU itself is executing from Regards, Daniel
On Thu, Oct 28, 2010 at 10:35:02AM +0100, Daniel P. Berrange wrote: > On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote: > > On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > > > > > diff --git a/block.c b/block.c > > > index 1a965b2..00b6f21 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > > > BlockDriver *back_drv = NULL; > > > > > > bs->backing_hd = bdrv_new(""); > > > - path_combine(backing_filename, sizeof(backing_filename), > > > - filename, bs->backing_file); > > > - if (bs->backing_format[0] != '\0') > > > - back_drv = bdrv_find_format(bs->backing_format); > > > + back_drv = bdrv_find_protocol(bs->backing_file); > > > + if (!back_drv) { > > > + path_combine(backing_filename, sizeof(backing_filename), > > > + filename, bs->backing_file); > > > + if (bs->backing_format[0] != '\0') > > > + back_drv = bdrv_find_format(bs->backing_format); > > > + } else { > > > + pstrcpy(backing_filename, sizeof(backing_filename), > > > + bs->backing_file); > > > + } > > > > > > /* backing files always opened read-only */ > > > back_flags = > > > -- > > > 1.7.0.4 > > > > I think this makes sense. > > > > Now it is possible to specify backing files that are relative to > > QEMU's current working directory using file:filename. I don't see > > harm in this. > > Shouldn't a backing file be treated as relative to the image file pointing > to it, rather than the QEMU working directory. eg so you can do > > # qemu-img create backing.img > # qemu-img create -o backing_file=file:backing.img main.img > > And have main.img be able to resolve backing.img in its same directory, > no matter what directory QEMU itself is executing from This works relative to main.img: # qemu-img create -o backing_file=backing.img main.img but this works relative to QEMU's cwd: # qemu-img create -o backing_file=file:backing.img main.img As Kevin mentioned, special-casing the file: protocol isn't ideal. We shouldn't make assumptions about specific protocols. Stefan
Am 28.10.2010 11:35, schrieb Daniel P. Berrange: > On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote: >> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>> >>> diff --git a/block.c b/block.c >>> index 1a965b2..00b6f21 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >>> BlockDriver *back_drv = NULL; >>> >>> bs->backing_hd = bdrv_new(""); >>> - path_combine(backing_filename, sizeof(backing_filename), >>> - filename, bs->backing_file); >>> - if (bs->backing_format[0] != '\0') >>> - back_drv = bdrv_find_format(bs->backing_format); >>> + back_drv = bdrv_find_protocol(bs->backing_file); >>> + if (!back_drv) { >>> + path_combine(backing_filename, sizeof(backing_filename), >>> + filename, bs->backing_file); >>> + if (bs->backing_format[0] != '\0') >>> + back_drv = bdrv_find_format(bs->backing_format); >>> + } else { >>> + pstrcpy(backing_filename, sizeof(backing_filename), >>> + bs->backing_file); >>> + } >>> >>> /* backing files always opened read-only */ >>> back_flags = >>> -- >>> 1.7.0.4 >> >> I think this makes sense. >> >> Now it is possible to specify backing files that are relative to >> QEMU's current working directory using file:filename. I don't see >> harm in this. > > Shouldn't a backing file be treated as relative to the image file pointing > to it, rather than the QEMU working directory. eg so you can do > > # qemu-img create backing.img > # qemu-img create -o backing_file=file:backing.img main.img > > And have main.img be able to resolve backing.img in its same directory, > no matter what directory QEMU itself is executing from The problem is that this wouldn't work in the general case. It's rather an exception that it makes sense for file: backing files with file: images. Consider this: # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img Without this patch, you'll end up with /tmp/nbd:foo:1234, which is probably not what you wanted. With a patch that would work for file: you would get a hardly better path nbd:/tmp/foo:1234 Kevin
On Thu, Oct 28, 2010 at 11:49:58AM +0200, Kevin Wolf wrote: > Am 28.10.2010 11:35, schrieb Daniel P. Berrange: > > On Thu, Oct 28, 2010 at 09:30:09AM +0100, Stefan Hajnoczi wrote: > >> On Wed, Oct 27, 2010 at 7:19 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > >>> > >>> diff --git a/block.c b/block.c > >>> index 1a965b2..00b6f21 100644 > >>> --- a/block.c > >>> +++ b/block.c > >>> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > >>> BlockDriver *back_drv = NULL; > >>> > >>> bs->backing_hd = bdrv_new(""); > >>> - path_combine(backing_filename, sizeof(backing_filename), > >>> - filename, bs->backing_file); > >>> - if (bs->backing_format[0] != '\0') > >>> - back_drv = bdrv_find_format(bs->backing_format); > >>> + back_drv = bdrv_find_protocol(bs->backing_file); > >>> + if (!back_drv) { > >>> + path_combine(backing_filename, sizeof(backing_filename), > >>> + filename, bs->backing_file); > >>> + if (bs->backing_format[0] != '\0') > >>> + back_drv = bdrv_find_format(bs->backing_format); > >>> + } else { > >>> + pstrcpy(backing_filename, sizeof(backing_filename), > >>> + bs->backing_file); > >>> + } > >>> > >>> /* backing files always opened read-only */ > >>> back_flags = > >>> -- > >>> 1.7.0.4 > >> > >> I think this makes sense. > >> > >> Now it is possible to specify backing files that are relative to > >> QEMU's current working directory using file:filename. I don't see > >> harm in this. > > > > Shouldn't a backing file be treated as relative to the image file pointing > > to it, rather than the QEMU working directory. eg so you can do > > > > # qemu-img create backing.img > > # qemu-img create -o backing_file=file:backing.img main.img > > > > And have main.img be able to resolve backing.img in its same directory, > > no matter what directory QEMU itself is executing from > > The problem is that this wouldn't work in the general case. It's rather > an exception that it makes sense for file: backing files with file: > images. Consider this: > > # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img > > Without this patch, you'll end up with /tmp/nbd:foo:1234, which is > probably not what you wanted. With a patch that would work for file: you > would get a hardly better path nbd:/tmp/foo:1234 Since we know the protocol, there could be a per-protocol function used for resolving the backing store URI relative to the master URI. That would avoid needing to special case file: in the shared generic code. Daniel
On 10/28/2010 04:51 AM, Daniel P. Berrange wrote: >> The problem is that this wouldn't work in the general case. It's rather >> an exception that it makes sense for file: backing files with file: >> images. Consider this: >> >> # qemu-img create -o backing_file=nbd:foo:1234 /tmp/main.img >> >> Without this patch, you'll end up with /tmp/nbd:foo:1234, which is >> probably not what you wanted. With a patch that would work for file: you >> would get a hardly better path nbd:/tmp/foo:1234 >> > Since we know the protocol, there could be a per-protocol function used > for resolving the backing store URI relative to the master URI. That > would avoid needing to special case file: in the shared generic code. > Relative resolution of a backing files makes me very nervous. Any time a disk image can reasonably resolve to something other than what the user expected is potentially a very nasty security issue. The less obvious the resolution, the worse the problem becomes. I think resolution based on current path is probably the most obvious implementation. Regards, Anthony Liguori > Daniel >
Am 27.10.2010 20:19, schrieb Anthony Liguori: > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > diff --git a/block.c b/block.c > index 1a965b2..00b6f21 100644 > --- a/block.c > +++ b/block.c > @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, > BlockDriver *back_drv = NULL; > > bs->backing_hd = bdrv_new(""); > - path_combine(backing_filename, sizeof(backing_filename), > - filename, bs->backing_file); > - if (bs->backing_format[0] != '\0') > - back_drv = bdrv_find_format(bs->backing_format); > + back_drv = bdrv_find_protocol(bs->backing_file); > + if (!back_drv) { If no protocol is specified, bdrv_find_protocol doesn't return NULL but the file: driver. This breaks all backing file related qemu-iotests cases because qcow2 backing files are opened as raw files now. Kevin
On 11/04/2010 07:54 AM, Kevin Wolf wrote: > Am 27.10.2010 20:19, schrieb Anthony Liguori: > >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> >> diff --git a/block.c b/block.c >> index 1a965b2..00b6f21 100644 >> --- a/block.c >> +++ b/block.c >> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >> BlockDriver *back_drv = NULL; >> >> bs->backing_hd = bdrv_new(""); >> - path_combine(backing_filename, sizeof(backing_filename), >> - filename, bs->backing_file); >> - if (bs->backing_format[0] != '\0') >> - back_drv = bdrv_find_format(bs->backing_format); >> + back_drv = bdrv_find_protocol(bs->backing_file); >> + if (!back_drv) { >> > If no protocol is specified, bdrv_find_protocol doesn't return NULL but > the file: driver. > An ugly way to handle this would be to do if (strstr(bs->backing_file, ":") == NULL) instead. A deeper refactoring could return NULL in bdrv_find_protocol and fixup the callers to default to file: if none are specified. What do you think? Regards, Anthony Liguori > This breaks all backing file related qemu-iotests cases because qcow2 > backing files are opened as raw files now. > > Kevin >
Am 04.11.2010 14:14, schrieb Anthony Liguori: > On 11/04/2010 07:54 AM, Kevin Wolf wrote: >> Am 27.10.2010 20:19, schrieb Anthony Liguori: >> >>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >>> >>> diff --git a/block.c b/block.c >>> index 1a965b2..00b6f21 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, >>> BlockDriver *back_drv = NULL; >>> >>> bs->backing_hd = bdrv_new(""); >>> - path_combine(backing_filename, sizeof(backing_filename), >>> - filename, bs->backing_file); >>> - if (bs->backing_format[0] != '\0') >>> - back_drv = bdrv_find_format(bs->backing_format); >>> + back_drv = bdrv_find_protocol(bs->backing_file); >>> + if (!back_drv) { >>> >> If no protocol is specified, bdrv_find_protocol doesn't return NULL but >> the file: driver. >> > > An ugly way to handle this would be to do if (strstr(bs->backing_file, > ":") == NULL) instead. Ugly indeed. > A deeper refactoring could return NULL in bdrv_find_protocol and fixup > the callers to default to file: if none are specified. NULL is already used for errors, so we'd have to have something like int bdrv_find_protocol(const char *filename, BlockDriver **drv) in order to be able to distinguish "invalid protocol" from "no explicit protocol". You could rename this function and retain a bdrv_find_protocol with the old prototype as a wrapper that returns file instead of NULL (there are several callers that expect this behaviour, so probably it makes sense to have it in a central place). Does that sound reasonable? Kevin
diff --git a/block.c b/block.c index 1a965b2..00b6f21 100644 --- a/block.c +++ b/block.c @@ -603,10 +603,16 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags, BlockDriver *back_drv = NULL; bs->backing_hd = bdrv_new(""); - path_combine(backing_filename, sizeof(backing_filename), - filename, bs->backing_file); - if (bs->backing_format[0] != '\0') - back_drv = bdrv_find_format(bs->backing_format); + back_drv = bdrv_find_protocol(bs->backing_file); + if (!back_drv) { + path_combine(backing_filename, sizeof(backing_filename), + filename, bs->backing_file); + if (bs->backing_format[0] != '\0') + back_drv = bdrv_find_format(bs->backing_format); + } else { + pstrcpy(backing_filename, sizeof(backing_filename), + bs->backing_file); + } /* backing files always opened read-only */ back_flags =
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>