* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2017-12-19 14:20 ` Max Reitz
@ 2017-12-20 4:34 ` seeteena
2017-12-20 13:18 ` Dr. David Alan Gilbert
2017-12-21 23:14 ` Eric Blake
2 siblings, 0 replies; 13+ messages in thread
From: seeteena @ 2017-12-20 4:34 UTC (permalink / raw)
To: Max Reitz, Dr. David Alan Gilbert, lma; +Cc: qemu-devel
On 12/19/2017 07:50 PM, Max Reitz wrote:
> On 2017-12-18 10:24, seeteena wrote:
>>
>> On 12/15/2017 02:48 PM, Max Reitz wrote:
>>> On 2017-12-13 05:50, seeteena wrote:
>>>> Hi David,
>>>>
>>>> While creating snapshots when tag name '0' used in the very first and followed by tag name '1' then snapshot created with tag name '0' is getting erased.
>>>> A snapshot is identified by a name computed either from an id, which is basically a numerical counter starting at 1 for qcow2, or from a tag, which is a string (provided
>>>> by the user or automatically computed).
>>>>
>>>> (qemu) savevm 0
>>>>
>>>> This creates a snapshot with tag '0' and id '1'.
>>> That's something someone has complained about already, as far as I
>>> remember, and this is indeed an issue.
>>>
>>>>> (qemu) savevm 1
>>>> This deletes snapshot with name '1' (ie, with id '1') and creates snapshot with tag '1' and id '1'.
>>> I think this is the issue, not info snapshots.
>>>
>>>> From the output of 'info snapshots' id '1' is not seen for tag '0' instead seeing an empty field.
>>>>
>>>> ID TAG VM SIZE DATE VM CLOCK
>>>> -- 0 338M 2017-10-16 13:44:35 00:02:07.491
>>>>
>>>> If an ID shown as '1' in the above then user can understand the above documented stuff i.e "If there is already a snapshot with the same tag or ID, it is replaced".
>>>> The '--' in ID field is annoying.
>>> Maybe, but this patch is wrong still. Commit 3a1ee711904 says exactly why:
>>>
>>>> The patch uses snapshot name instead of snapshot id to determine whether a
>>>> snapshot is fully available and uses '--' instead of snapshot id in output
>>>> because the snapshot id is not guaranteed to be the same on all images.
>>> info snapshots first shows a list of snapshots that are present on all
>>> disks. However, those are matched by name and not by ID, so the ID is
>>> not necessarily the same.
>>>
>>> Therefore, we can only print it if it is. Sure, we can do that, but
>>> your patch is missing that check.
>> As you referred I had checked Commit 3a1ee711904. Does it mean that we
>> need to add the check option for ID as well along with name to get the
>> list of snapshots that are present on all disks with my patch ?
>> so info snapshots shows list of snapshots that are present on all disks
>> that matched either by ID or name.
>>
>> like
>> if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) ||
>> (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0)
> Kind of true, but I think that first this is a separate issue and
> secondly, this can get rather hairy. (See below under (3).)
>
> So there are three things:
>
> (1) We probably should not allow snapshot names that could be IDs.
> Easiest way to solve this: Names have to start with a non-digit.
>
> (2) If we want to print a global snapshot's common ID, we need to affirm
> that this ID is indeed the same on all disks before we can print it.
> Same for names, but currently the name is always the same on all disks
> because that is how we identify global snapshots.
>
> (3) You can give an ID to loadvm and then it will load the snapshot with
> that ID from all disks. So if you have snapshots with a common ID on
> all disks, these are kind of global snapshots, too, even though they
> don't share a name. Thus, they should probably be included in the
> listing (this is what you have just proposed).
> I don't like this at all, though. A snapshot's ID is not really
> user-controlled, it's just some auto-generated number. Therefore, just
> because the ID of a snapshot matches across multiple disks, this doesn't
> mean that they are related whatsoever.
> So, first, I don't think loadvm should work with IDs (at least not
> across multiple disks). But I don't think this really needs to be fixed.
> On the other hand, I really don't think info snapshots should list
> snapshots that match by ID, because a matching ID does not mean that
> snapshots are actually related. A matching name usually does, though,
> so I think what we currently do is sufficient and the right way to do it.
>
> Max
>
Sounds great.. Thank you Max for the detail explanation. I will go
fixing (1) that would fix savevm issue (savevm overwrites snapshots when
tags 0 and 1 is used consecutively) so we don't want ID to expose
through info snapshots and will continue with the current implementation.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2017-12-19 14:20 ` Max Reitz
2017-12-20 4:34 ` seeteena
@ 2017-12-20 13:18 ` Dr. David Alan Gilbert
2017-12-21 23:14 ` Eric Blake
2 siblings, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2017-12-20 13:18 UTC (permalink / raw)
To: Max Reitz; +Cc: seeteena, lma, qemu-devel
* Max Reitz (mreitz@redhat.com) wrote:
> On 2017-12-18 10:24, seeteena wrote:
> >
> >
> > On 12/15/2017 02:48 PM, Max Reitz wrote:
> >> On 2017-12-13 05:50, seeteena wrote:
> >>> Hi David,
> >>>
> >>> While creating snapshots when tag name '0' used in the very first and followed by tag name '1' then snapshot created with tag name '0' is getting erased.
> >>> A snapshot is identified by a name computed either from an id, which is basically a numerical counter starting at 1 for qcow2, or from a tag, which is a string (provided
> >>> by the user or automatically computed).
> >>>
> >>> (qemu) savevm 0
> >>>
> >>> This creates a snapshot with tag '0' and id '1'.
> >> That's something someone has complained about already, as far as I
> >> remember, and this is indeed an issue.
> >>
> >>>> (qemu) savevm 1
> >>> This deletes snapshot with name '1' (ie, with id '1') and creates snapshot with tag '1' and id '1'.
> >> I think this is the issue, not info snapshots.
> >>
> >>> From the output of 'info snapshots' id '1' is not seen for tag '0' instead seeing an empty field.
> >>>
> >>> ID TAG VM SIZE DATE VM CLOCK
> >>> -- 0 338M 2017-10-16 13:44:35 00:02:07.491
> >>>
> >>> If an ID shown as '1' in the above then user can understand the above documented stuff i.e "If there is already a snapshot with the same tag or ID, it is replaced".
> >>> The '--' in ID field is annoying.
> >> Maybe, but this patch is wrong still. Commit 3a1ee711904 says exactly why:
> >>
> >>> The patch uses snapshot name instead of snapshot id to determine whether a
> >>> snapshot is fully available and uses '--' instead of snapshot id in output
> >>> because the snapshot id is not guaranteed to be the same on all images.
> >> info snapshots first shows a list of snapshots that are present on all
> >> disks. However, those are matched by name and not by ID, so the ID is
> >> not necessarily the same.
> >>
> >> Therefore, we can only print it if it is. Sure, we can do that, but
> >> your patch is missing that check.
> > As you referred I had checked Commit 3a1ee711904. Does it mean that we
> > need to add the check option for ID as well along with name to get the
> > list of snapshots that are present on all disks with my patch ?
> > so info snapshots shows list of snapshots that are present on all disks
> > that matched either by ID or name.
> >
> > like
> > if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) ||
> > (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0)
>
> Kind of true, but I think that first this is a separate issue and
> secondly, this can get rather hairy. (See below under (3).)
>
> So there are three things:
>
> (1) We probably should not allow snapshot names that could be IDs.
> Easiest way to solve this: Names have to start with a non-digit.
Is this something we're in a position to change, or is it part of the
API that user programs can end up using?
Dave
> (2) If we want to print a global snapshot's common ID, we need to affirm
> that this ID is indeed the same on all disks before we can print it.
> Same for names, but currently the name is always the same on all disks
> because that is how we identify global snapshots.
>
> (3) You can give an ID to loadvm and then it will load the snapshot with
> that ID from all disks. So if you have snapshots with a common ID on
> all disks, these are kind of global snapshots, too, even though they
> don't share a name. Thus, they should probably be included in the
> listing (this is what you have just proposed).
> I don't like this at all, though. A snapshot's ID is not really
> user-controlled, it's just some auto-generated number. Therefore, just
> because the ID of a snapshot matches across multiple disks, this doesn't
> mean that they are related whatsoever.
> So, first, I don't think loadvm should work with IDs (at least not
> across multiple disks). But I don't think this really needs to be fixed.
> On the other hand, I really don't think info snapshots should list
> snapshots that match by ID, because a matching ID does not mean that
> snapshots are actually related. A matching name usually does, though,
> so I think what we currently do is sufficient and the right way to do it.
>
> Max
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2017-12-19 14:20 ` Max Reitz
2017-12-20 4:34 ` seeteena
2017-12-20 13:18 ` Dr. David Alan Gilbert
@ 2017-12-21 23:14 ` Eric Blake
2017-12-22 7:37 ` Markus Armbruster
2 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2017-12-21 23:14 UTC (permalink / raw)
To: Max Reitz, seeteena, Dr. David Alan Gilbert, lma; +Cc: qemu-devel
On 12/19/2017 08:20 AM, Max Reitz wrote:
> So there are three things:
>
> (1) We probably should not allow snapshot names that could be IDs.
> Easiest way to solve this: Names have to start with a non-digit.
Yes, that would be a nice change. It is not strictly backwards
compatible (so we'd still have to cope with images that didn't follow
the rule, whether created by older qemu or by non-qemu implementations
of qcow2), but would alleviate a lot of confusion.
>
> (2) If we want to print a global snapshot's common ID, we need to affirm
> that this ID is indeed the same on all disks before we can print it.
> Same for names, but currently the name is always the same on all disks
> because that is how we identify global snapshots.
>
> (3) You can give an ID to loadvm and then it will load the snapshot with
> that ID from all disks. So if you have snapshots with a common ID on
> all disks, these are kind of global snapshots, too, even though they
> don't share a name. Thus, they should probably be included in the
> listing (this is what you have just proposed).
> I don't like this at all, though. A snapshot's ID is not really
> user-controlled, it's just some auto-generated number. Therefore, just
> because the ID of a snapshot matches across multiple disks, this doesn't
> mean that they are related whatsoever.
> So, first, I don't think loadvm should work with IDs (at least not
> across multiple disks). But I don't think this really needs to be fixed.
> On the other hand, I really don't think info snapshots should list
> snapshots that match by ID, because a matching ID does not mean that
> snapshots are actually related. A matching name usually does, though,
> so I think what we currently do is sufficient and the right way to do it.
>
> Max
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2017-12-21 23:14 ` Eric Blake
@ 2017-12-22 7:37 ` Markus Armbruster
2018-01-02 6:42 ` seeteena
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-12-22 7:37 UTC (permalink / raw)
To: Eric Blake; +Cc: Max Reitz, seeteena, Dr. David Alan Gilbert, lma, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 12/19/2017 08:20 AM, Max Reitz wrote:
>
>> So there are three things:
>>
>> (1) We probably should not allow snapshot names that could be IDs.
>> Easiest way to solve this: Names have to start with a non-digit.
>
> Yes, that would be a nice change. It is not strictly backwards
> compatible (so we'd still have to cope with images that didn't follow
> the rule, whether created by older qemu or by non-qemu implementations
> of qcow2), but would alleviate a lot of confusion.
I recommend to restrict ID strings to letters, digits, '-', '.', '_',
starting with a letter. Use id_wellformed() to check.
If backward compatibility is an issue, deprecate offending IDs (with a
suitable warning), and kill them off after the customary grace period.
IDs embedded in image files and such you may have to keep working
somehow indefinitely.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2017-12-22 7:37 ` Markus Armbruster
@ 2018-01-02 6:42 ` seeteena
2018-01-02 8:24 ` Markus Armbruster
0 siblings, 1 reply; 13+ messages in thread
From: seeteena @ 2018-01-02 6:42 UTC (permalink / raw)
To: Markus Armbruster, Eric Blake
Cc: Max Reitz, Dr. David Alan Gilbert, lma, qemu-devel
On 12/22/2017 01:07 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 12/19/2017 08:20 AM, Max Reitz wrote:
>>
>>> So there are three things:
>>>
>>> (1) We probably should not allow snapshot names that could be IDs.
>>> Easiest way to solve this: Names have to start with a non-digit.
>> Yes, that would be a nice change. It is not strictly backwards
>> compatible (so we'd still have to cope with images that didn't follow
>> the rule, whether created by older qemu or by non-qemu implementations
>> of qcow2), but would alleviate a lot of confusion.
> I recommend to restrict ID strings to letters, digits, '-', '.', '_',
> starting with a letter. Use id_wellformed() to check.
char *id_generate(IdSubSystems id) autogenates ID in the format eg-
"#block146" with starting with '#' always.
restricting ID strings to use id_wellformed() means we need to force ID
to start with letter.
#define ID_SPECIAL_CHAR '#' needs to change to (some letter)
#define ID_SPECIAL_CHAR 'A'
>
> If backward compatibility is an issue, deprecate offending IDs (with a
> suitable warning), and kill them off after the customary grace period.
>
> IDs embedded in image files and such you may have to keep working
> somehow indefinitely.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2018-01-02 6:42 ` seeteena
@ 2018-01-02 8:24 ` Markus Armbruster
2018-01-04 16:24 ` seeteena
0 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2018-01-02 8:24 UTC (permalink / raw)
To: seeteena; +Cc: Eric Blake, qemu-devel, lma, Dr. David Alan Gilbert, Max Reitz
seeteena <s1seetee@linux.vnet.ibm.com> writes:
> On 12/22/2017 01:07 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 12/19/2017 08:20 AM, Max Reitz wrote:
>>>
>>>> So there are three things:
>>>>
>>>> (1) We probably should not allow snapshot names that could be IDs.
>>>> Easiest way to solve this: Names have to start with a non-digit.
>>> Yes, that would be a nice change. It is not strictly backwards
>>> compatible (so we'd still have to cope with images that didn't follow
>>> the rule, whether created by older qemu or by non-qemu implementations
>>> of qcow2), but would alleviate a lot of confusion.
>> I recommend to restrict ID strings to letters, digits, '-', '.', '_',
>> starting with a letter. Use id_wellformed() to check.
> char *id_generate(IdSubSystems id) autogenates ID in the format eg-
> "#block146" with starting with '#' always.
> restricting ID strings to use id_wellformed() means we need to force
> ID to start with letter.
>
> #define ID_SPECIAL_CHAR '#' needs to change to (some letter)
> #define ID_SPECIAL_CHAR 'A'
Ensuring auto-generated IDs cannot clash with IDs chosen by the user is
a good idea. We generally fail to do that in QEMU.
An obvious way to do it is to require the user's IDs to start with a
letter, and auto-generated IDs to start with another suitable character.
In theory, starting with '#' is problematic, because it needs to be
quoted in shell at the beginning of words. Only matters if
auto-generated IDs make sense in command lines, which I guess they
currently don't. I'd prefer a 'shell-safe' character all the same.
>> If backward compatibility is an issue, deprecate offending IDs (with a
>> suitable warning), and kill them off after the customary grace period.
>>
>> IDs embedded in image files and such you may have to keep working
>> somehow indefinitely.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v1] hmp: 'info snapshots' not showing the id
2018-01-02 8:24 ` Markus Armbruster
@ 2018-01-04 16:24 ` seeteena
0 siblings, 0 replies; 13+ messages in thread
From: seeteena @ 2018-01-04 16:24 UTC (permalink / raw)
To: Markus Armbruster
Cc: Eric Blake, qemu-devel, lma, Dr. David Alan Gilbert, Max Reitz
On 01/02/2018 01:54 PM, Markus Armbruster wrote:
> seeteena <s1seetee@linux.vnet.ibm.com> writes:
>
>> On 12/22/2017 01:07 PM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 12/19/2017 08:20 AM, Max Reitz wrote:
>>>>
>>>>> So there are three things:
>>>>>
>>>>> (1) We probably should not allow snapshot names that could be IDs.
>>>>> Easiest way to solve this: Names have to start with a non-digit.
>>>> Yes, that would be a nice change. It is not strictly backwards
>>>> compatible (so we'd still have to cope with images that didn't follow
>>>> the rule, whether created by older qemu or by non-qemu implementations
>>>> of qcow2), but would alleviate a lot of confusion.
>>> I recommend to restrict ID strings to letters, digits, '-', '.', '_',
>>> starting with a letter. Use id_wellformed() to check.
>> char *id_generate(IdSubSystems id) autogenates ID in the format eg-
>> "#block146" with starting with '#' always.
>> restricting ID strings to use id_wellformed() means we need to force
>> ID to start with letter.
>>
>> #define ID_SPECIAL_CHAR '#' needs to change to (some letter)
>> #define ID_SPECIAL_CHAR 'A'
> Ensuring auto-generated IDs cannot clash with IDs chosen by the user is
> a good idea. We generally fail to do that in QEMU.
>
> An obvious way to do it is to require the user's IDs to start with a
> letter, and auto-generated IDs to start with another suitable character.
> In theory, starting with '#' is problematic, because it needs to be
> quoted in shell at the beginning of words. Only matters if
> auto-generated IDs make sense in command lines, which I guess they
> currently don't. I'd prefer a 'shell-safe' character all the same.
I was using loaned system from other team for recreate and it appears
there is some issue in that system and I am not able to recreate the
issue now.
QEMU 2.11.50 monitor - type 'help' for more information
(qemu) savevm 0
Error while creating snapshot on 'rootdisk'
(qemu) savevm 0
Error while creating snapshot on 'rootdisk'
(qemu) savevm 0
Error while creating snapshot on 'rootdisk'
(qemu) savevm 1
Error while creating snapshot on 'rootdisk'
(qemu)
>
>>> If backward compatibility is an issue, deprecate offending IDs (with a
>>> suitable warning), and kill them off after the customary grace period.
>>>
>>> IDs embedded in image files and such you may have to keep working
>>> somehow indefinitely.
^ permalink raw reply [flat|nested] 13+ messages in thread