| Message ID | 1359392680-15838-4-git-send-email-benoit@irqsave.net |
|---|---|
| State | New |
| Headers | show |
Am 28.01.2013 18:04, schrieb Benoît Canet: > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > blockdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 0ce45c5..b1f388b 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -800,7 +800,8 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) > /* We will manually add the backing_hd field to the bs later */ > states->new_bs = bdrv_new(""); > ret = bdrv_open(states->new_bs, new_image_file, > - flags | BDRV_O_NO_BACKING, drv); > + flags | BDRV_O_NO_BACKING, > + path_has_protocol(new_image_file) ? NULL : drv); > if (ret != 0) { > error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); > goto delete_and_fail; Wait, what's happening here? I don't understand this patch and how it's related to snapshotting non-file protocols (if this is even what you mean). What is your exact scenario, what does the existing code do in it, and how does this change improve it? An empty commit message is definitely not appropriate for such a change. In any case, using NULL as drv for bdrv_open() looks plain wrong. Kevin
> Wait, what's happening here? I don't understand this patch and how it's > related to snapshotting non-file protocols (if this is even what you > mean). What is your exact scenario, what does the existing code do in > it, and how does this change improve it? An empty commit message is > definitely not appropriate for such a change. > > In any case, using NULL as drv for bdrv_open() looks plain wrong. When passing drv bdrv_open tries to open the url as qcow2 or another plain file format and fail. bdrv_file_open is not a better option because it won't return a BlockDriverState constructed in the same way as the old one. (raw as bs and quorum as ->file) I agree that this patch is a hack and I am looking for a better way of doing it. Benoît
Am 29.01.2013 14:07, schrieb Benoît Canet: >> Wait, what's happening here? I don't understand this patch and how it's >> related to snapshotting non-file protocols (if this is even what you >> mean). What is your exact scenario, what does the existing code do in >> it, and how does this change improve it? An empty commit message is >> definitely not appropriate for such a change. >> >> In any case, using NULL as drv for bdrv_open() looks plain wrong. > > When passing drv bdrv_open tries to open the url as qcow2 or another plain file > format and fail. What exactly is a "plain file format" and what is the difference between it and other file formats? Anyway, this is not a problem description, let alone a description of your exact scenario. If qcow2 is specified as the format in the QMP command, why is it then wrong to try and open the file as qcow2? Why does it fail? What is the correct driver? > bdrv_file_open is not a better option because it won't return a BlockDriverState > constructed in the same way as the old one. > (raw as bs and quorum as ->file) > > I agree that this patch is a hack and I am looking for a better way of doing it. Let's talk about the problem first before discussing solutions. Kevin
Le Tuesday 29 Jan 2013 à 14:25:42 (+0100), Kevin Wolf a écrit : > Am 29.01.2013 14:07, schrieb Benoît Canet: > >> Wait, what's happening here? I don't understand this patch and how it's > >> related to snapshotting non-file protocols (if this is even what you > >> mean). What is your exact scenario, what does the existing code do in > >> it, and how does this change improve it? An empty commit message is > >> definitely not appropriate for such a change. > >> > >> In any case, using NULL as drv for bdrv_open() looks plain wrong. > > > > When passing drv bdrv_open tries to open the url as qcow2 or another plain file > > format and fail. > > What exactly is a "plain file format" and what is the difference between > it and other file formats? Quorum is a protocol which encapsulate n BlockDriverState. > > Anyway, this is not a problem description, let alone a description of > your exact scenario. A scenario would be: A user want to create a new quorum snapshot made of n qcow2 snapshots. The user specify qcow2 as format so the new quorum files are created as qcow2. Then the code try to open the quorum file as a qcow2 file and it fail. > > If qcow2 is specified as the format in the QMP command, why is it then > wrong to try and open the file as qcow2? Why does it fail? What is the > correct driver? It's correct to format the individual quorum files as qcow2 so specifying qcow2 in the qmp command seems ok. But opening a "quorum:" url which agregate these qcow2 files will fail if drv is the qcow2 driver. Would passing the quorum protocol driver to bdrv_open be ok ? Benoît > > > bdrv_file_open is not a better option because it won't return a BlockDriverState > > constructed in the same way as the old one. > > (raw as bs and quorum as ->file) > > > > I agree that this patch is a hack and I am looking for a better way of doing it. > > Let's talk about the problem first before discussing solutions. > > Kevin
Am 29.01.2013 14:45, schrieb Benoît Canet: > Le Tuesday 29 Jan 2013 à 14:25:42 (+0100), Kevin Wolf a écrit : >> Am 29.01.2013 14:07, schrieb Benoît Canet: >>>> Wait, what's happening here? I don't understand this patch and how it's >>>> related to snapshotting non-file protocols (if this is even what you >>>> mean). What is your exact scenario, what does the existing code do in >>>> it, and how does this change improve it? An empty commit message is >>>> definitely not appropriate for such a change. >>>> >>>> In any case, using NULL as drv for bdrv_open() looks plain wrong. >>> >>> When passing drv bdrv_open tries to open the url as qcow2 or another plain file >>> format and fail. >> >> What exactly is a "plain file format" and what is the difference between >> it and other file formats? > > Quorum is a protocol which encapsulate n BlockDriverState. So it's not a file format, neither plain nor otherwise. >> Anyway, this is not a problem description, let alone a description of >> your exact scenario. > > A scenario would be: > A user want to create a new quorum snapshot made of n qcow2 snapshots. > The user specify qcow2 as format so the new quorum files are created as qcow2. > Then the code try to open the quorum file as a qcow2 file and it fail. Like this? base1 [file] --- base1 [qcow2] --\ base2 [file] --- base2 [qcow2] --->-- quorum --- snap.qcow2 base3 [file] --- base3 [qcow2] --/ Or like this? (This is the only one where quorum is really used as a protocol) base1 [file] --\ base2 [file] --->-- quorum --- base [qcow2] --- snap.qcow2 base3 [file] --/ Or even this one? base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ I think the last one is what you really want, but it's certainly not the case that is enabled by this patch. >> If qcow2 is specified as the format in the QMP command, why is it then >> wrong to try and open the file as qcow2? Why does it fail? What is the >> correct driver? > > It's correct to format the individual quorum files as qcow2 so specifying qcow2 > in the qmp command seems ok. > But opening a "quorum:" url which agregate these qcow2 files will fail if drv is > the qcow2 driver. > Would passing the quorum protocol driver to bdrv_open be ok ? A quorum: URL opened with the qcow2 driver should succeed and result in something like the second case mentioned above. Kevin
> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ > base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum > base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ > > I think the last one is what you really want, but it's certainly not the > case that is enabled by this patch. Yes I am trying to get this configuration: quorum always on top. Benoît > > >> If qcow2 is specified as the format in the QMP command, why is it then > >> wrong to try and open the file as qcow2? Why does it fail? What is the > >> correct driver? > > > > It's correct to format the individual quorum files as qcow2 so specifying qcow2 > > in the qmp command seems ok. > > But opening a "quorum:" url which agregate these qcow2 files will fail if drv is > > the qcow2 driver. > > Would passing the quorum protocol driver to bdrv_open be ok ? > > A quorum: URL opened with the qcow2 driver should succeed and result in > something like the second case mentioned above. > > Kevin
> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ > base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum > base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ I tried to default snapshot creation to qcow2 in quorum.c and specify quorum as format in the snapshot_blkdev command line. It doesn't work (crash). Do you have any idea on how to open while obtaining the third case ? Benoît
Am 29.01.2013 16:03, schrieb Benoît Canet: >> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ >> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum >> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ > > I tried to default snapshot creation to qcow2 in quorum.c and specify > quorum as format in the snapshot_blkdev command line. > It doesn't work (crash). > Do you have any idea on how to open while obtaining the third case ? Wait for BlockBackends and then implement proper stay-on-top filters. Kevin
On 01/29/2013 07:27 AM, Benoît Canet wrote: >> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ >> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum >> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ For that matter, I think it would be useful to have a quorum where the data is duplicated between different formats, such as: file1 [raw] --\ base2 [raw] --- snap2.qcow2 [qcow2] -->-- quorum base3 [qed] --- snap3.qed [qed] --/ In that case, you NEED to be able to pass in the protocol of each underlying device _as part of opening the quorum_, something like: quorum:2/3:file1[raw]:snap2.qcow2[qcow2]:snap3.qed[qed] or some such punctuation. And of course, that means you'd have to extend your escaping mechanism to allow escaping of whatever syntax you use for declaring a per-member format.
Le Tuesday 29 Jan 2013 à 16:21:43 (+0100), Kevin Wolf a écrit : > Am 29.01.2013 16:03, schrieb Benoît Canet: > >> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ > >> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum > >> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ > > > > I tried to default snapshot creation to qcow2 in quorum.c and specify > > quorum as format in the snapshot_blkdev command line. > > It doesn't work (crash). > > Do you have any idea on how to open while obtaining the third case ? > > Wait for BlockBackends and then implement proper stay-on-top filters. Any idea to disable properly snapshot creation for quorum and allowing it to be merged in a not so far future ? Benoît > > Kevin >
Am 29.01.2013 17:24, schrieb Eric Blake: > On 01/29/2013 07:27 AM, Benoît Canet wrote: >>> base1 [file] --- base1 [qcow2] --- snap1.qcow2 --\ >>> base2 [file] --- base2 [qcow2] --- snap2.qcow2 --->-- quorum >>> base3 [file] --- base3 [qcow2] --- snap3.qcow2 --/ > > For that matter, I think it would be useful to have a quorum where the > data is duplicated between different formats, such as: > > file1 [raw] --\ > base2 [raw] --- snap2.qcow2 [qcow2] -->-- quorum > base3 [qed] --- snap3.qed [qed] --/ > > In that case, you NEED to be able to pass in the protocol of each > underlying device _as part of opening the quorum_, something like: > > quorum:2/3:file1[raw]:snap2.qcow2[qcow2]:snap3.qed[qed] > > or some such punctuation. And of course, that means you'd have to > extend your escaping mechanism to allow escaping of whatever syntax you > use for declaring a per-member format. Let me rephrase that: quorum really needs -blockdev. And quite possibly the data that can be passed to the snapshot QMP commands today isn't sufficient to describe what we want to describe. :-/ Kevin
diff --git a/blockdev.c b/blockdev.c index 0ce45c5..b1f388b 100644 --- a/blockdev.c +++ b/blockdev.c @@ -800,7 +800,8 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) /* We will manually add the backing_hd field to the bs later */ states->new_bs = bdrv_new(""); ret = bdrv_open(states->new_bs, new_image_file, - flags | BDRV_O_NO_BACKING, drv); + flags | BDRV_O_NO_BACKING, + path_has_protocol(new_image_file) ? NULL : drv); if (ret != 0) { error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); goto delete_and_fail;
Signed-off-by: Benoit Canet <benoit@irqsave.net> --- blockdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)