| Message ID | 9e1839178f7a85d264eee20a07d4aed6e6ddb7b1.1252543871.git.quintela@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 10 Sep 2009, Juan Quintela wrote: > In previosu series I remove v2 support for RAM (that was the version that was > supported when SaveVM v3 appeared). Now we can't load RAM for any image saved in SaveVM v2, we can as well remove SaveVM v2 entirely. > > Note: That SaveVM RAM was at v2 when General SaveVM support went from v2 to v3 makes talking about versions confusing at least > Please do not commit this patch, we are using the loadvm v2 code actively, we should fix the v2 loading functions instead. If no one else is going to do it, I'll fix it.
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 10 Sep 2009, Juan Quintela wrote: >> In previosu series I remove v2 support for RAM (that was the version that was >> supported when SaveVM v3 appeared). Now we can't load RAM for any image saved in SaveVM v2, we can as well remove SaveVM v2 entirely. >> >> Note: That SaveVM RAM was at v2 when General SaveVM support went from v2 to v3 makes talking about versions confusing at least >> > > Please do not commit this patch, we are using the loadvm v2 code > actively, we should fix the v2 loading functions instead. > If no one else is going to do it, I'll fix it. Details please. What parts do you need? all of it? part of it? Notice that you can only be using it for loading state, writing state is done in V3. We are moving towards V4 (still nothing done), will like to know what are your use case. Later, Juan.
On Thu, 10 Sep 2009, Juan Quintela wrote: > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > On Thu, 10 Sep 2009, Juan Quintela wrote: > >> In previosu series I remove v2 support for RAM (that was the version that was > >> supported when SaveVM v3 appeared). Now we can't load RAM for any image saved in SaveVM v2, we can as well remove SaveVM v2 entirely. > >> > >> Note: That SaveVM RAM was at v2 when General SaveVM support went from v2 to v3 makes talking about versions confusing at least > >> > > > > Please do not commit this patch, we are using the loadvm v2 code > > actively, we should fix the v2 loading functions instead. > > If no one else is going to do it, I'll fix it. > > Details please. > > What parts do you need? all of it? part of it? Notice that you can > only be using it for loading state, writing state is done in V3. We are > moving towards V4 (still nothing done), will like to know what are your > use case. > First of all sorry if I come late into the discussion but I was on vacation. We just need the loading function and we don't even need the ram loading portion of it but just the device state loading. Of course for qemu doesn't make sense to keep only the device loading function around therefore I suggested to keep it all and fix it instead. As I have mentioned before I think it is really important to provide backward compatibility, and we do in xen and xenserver without too much trouble. I am willing to send patches to fix the device state loading functions, and we might already have few fixes in qemu-xen. I realize that my use case is off the tree so you have all the rights not to be interested in it, nonetheless I hope you don't completely discard it because it would make our life difficult.
Stefano Stabellini wrote: > First of all sorry if I come late into the discussion but I was on > vacation. > > We just need the loading function and we don't even need the ram loading > portion of it but just the device state loading. Of course for qemu > doesn't make sense to keep only the device loading function around > therefore I suggested to keep it all and fix it instead. > > As I have mentioned before I think it is really important to provide > backward compatibility, and we do in xen and xenserver without too much > trouble. > I am willing to send patches to fix the device state loading functions, > and we might already have few fixes in qemu-xen. > > I realize that my use case is off the tree so you have all the rights > not to be interested in it, nonetheless I hope you don't completely > discard it because it would make our life difficult. > Practically speaking, we only have the ability to support back to 0.10.0. We simply didn't have the necessary infrastructure in place to support anything older than that. v3 of the savevm protocol came before 0.10.0 so there's no need for us to every support v2 or v1 of the protocol. There are some major changes happen to the savevm infrastructure for 0.12.0. I'd suggest that instead of not removing v2, we remove it, finish up the changes, then you can look at re-adding support for it. Some of the features of v2 may be difficult to carry forward (like the savevm section sizes). And FWIW, I don't necessarily think we'll see a v4 for 0.12.0. I'm not convinced it's needed and/or useful. Regards, Anthony Liguori
On Thu, 10 Sep 2009, Anthony Liguori wrote: > Practically speaking, we only have the ability to support back to > 0.10.0. We simply didn't have the necessary infrastructure in place to > support anything older than that. > v3 of the savevm protocol came before 0.10.0 so there's no need for us > to every support v2 or v1 of the protocol. > > There are some major changes happen to the savevm infrastructure for > 0.12.0. I'd suggest that instead of not removing v2, we remove it, > finish up the changes, then you can look at re-adding support for it. > > Some of the features of v2 may be difficult to carry forward (like the > savevm section sizes). > > And FWIW, I don't necessarily think we'll see a v4 for 0.12.0. I'm not > convinced it's needed and/or useful. > I gave a better look at the code and the situation is not as bad as I thought: the important code for us is not much the single function qemu_loadvm_state_v2 that can easily be replaced and\or rewritten but all the per device state loading functions in C, that fortunately are shared between v2 and v3. I am still unhappy with removing support for v2 and I am inclined to consider it a regression but as long as you keep those functions in I am reasonably happy.
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Thu, 10 Sep 2009, Anthony Liguori wrote: >> Practically speaking, we only have the ability to support back to >> 0.10.0. We simply didn't have the necessary infrastructure in place to >> support anything older than that. >> v3 of the savevm protocol came before 0.10.0 so there's no need for us >> to every support v2 or v1 of the protocol. >> >> There are some major changes happen to the savevm infrastructure for >> 0.12.0. I'd suggest that instead of not removing v2, we remove it, >> finish up the changes, then you can look at re-adding support for it. >> >> Some of the features of v2 may be difficult to carry forward (like the >> savevm section sizes). >> >> And FWIW, I don't necessarily think we'll see a v4 for 0.12.0. I'm not >> convinced it's needed and/or useful. >> > > I gave a better look at the code and the situation is not as bad as I > thought: the important code for us is not much the single > function qemu_loadvm_state_v2 that can easily be replaced and\or > rewritten but all the per device state loading functions in C, that > fortunately are shared between v2 and v3. > I am still unhappy with removing support for v2 and I am inclined to > consider it a regression but as long as you keep those functions in I am > reasonably happy. I think we can't call that a regression: Old in the past SaveVM State v2 is created. Everything works for a while. At some later point SaveVM State v3 is created. Things continue to work fo a while. After this while (In April of this year ram support for V2 got broken, when I fixed it, I found that VGA got garbled and ide didn't work either). Things didn't work for at least 5 months, I haven't seen a complaint. Removing something that hasn't work during 5 months and nobody complained is supposed to be a regression? Now: We have v2 support on tree, that is not working, haven't worked for a long time, and there is not a chance to load an image from the old past. What is better? Remove the old non working code. Or spend time debugging why it stopped working and fixing it? Notice that it is also important that nobody complained that they were unable to load the old images. Later, Juan.
On Fri, 11 Sep 2009, Juan Quintela wrote: > I think we can't call that a regression: > > Old in the past SaveVM State v2 is created. > > Everything works for a while. > > At some later point SaveVM State v3 is created. > > Things continue to work fo a while. > > After this while (In April of this year ram support for V2 got broken, when I fixed > it, I found that VGA got garbled and ide didn't work either). > > Things didn't work for at least 5 months, I haven't seen a complaint. > > Removing something that hasn't work during 5 months and nobody > complained is supposed to be a regression? > > Now: We have v2 support on tree, that is not working, haven't worked for > a long time, and there is not a chance to load an image from the old > past. What is better? Remove the old non working code. Or spend time > debugging why it stopped working and fixing it? Notice that it is also > important that nobody complained that they were unable to load the old images. > No one complained because the people affected by this issue probably don't follow qemu development so closely to have realized that their old images won't work anymore. I do believe that fixing v2 compatibility is important for a large number of qemu users (including kvm and xen). That said, I don't want to fight over this more than necessary, so as long as you keep the per device loading functions in C we can maintain v2 compatibility downstream if upstream is not interested.
Stefano Stabellini wrote: > No one complained because the people affected by this issue probably > don't follow qemu development so closely to have realized that their old > images won't work anymore. > I do believe that fixing v2 compatibility is important for a large > number of qemu users (including kvm and xen). > kvm never supported live migration with v2. As I think I mentioned in another note, I don't want to burden the VMState refactoring with supporting a format that qemu doesn't support upstream. That said, if you want to add back a version of it that makes use of the new infrastructure, I would be willing to take that. In fact, if you're willing to do the cleanup work along with Juan, we can avoid removing it. > That said, I don't want to fight over this more than necessary, so as > long as you keep the per device loading functions in C we can maintain > v2 compatibility downstream if upstream is not interested. > The per device loading functions will be switching to VMState. Regards, Anthony Liguori
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 11 Sep 2009, Juan Quintela wrote: >> I think we can't call that a regression: >> >> Old in the past SaveVM State v2 is created. >> >> Everything works for a while. >> >> At some later point SaveVM State v3 is created. >> >> Things continue to work fo a while. >> >> After this while (In April of this year ram support for V2 got broken, when I fixed >> it, I found that VGA got garbled and ide didn't work either). >> >> Things didn't work for at least 5 months, I haven't seen a complaint. >> >> Removing something that hasn't work during 5 months and nobody >> complained is supposed to be a regression? >> >> Now: We have v2 support on tree, that is not working, haven't worked for >> a long time, and there is not a chance to load an image from the old >> past. What is better? Remove the old non working code. Or spend time >> debugging why it stopped working and fixing it? Notice that it is also >> important that nobody complained that they were unable to load the old images. >> > > No one complained because the people affected by this issue probably > don't follow qemu development so closely to have realized that their old > images won't work anymore. > I do believe that fixing v2 compatibility is important for a large > number of qemu users (including kvm and xen). About this, nobody is wanting to remove useful features. I was removing things that didn't work. The best plan of action if you want v2 support on tree is to fix it. Do what I did: This commit introduces SaveVM v3 commit 9366f4186025e1d8fc3bebd41fb714521c170b6f Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162> Date: Mon Oct 6 14:53:52 2008 +0000 Introduce v3 of savevm protocol I checkout previous commit, savevm one machine. Go to the present and try to load it -> RAM don't work. Search what commit broke it: commit 94a6b54fd6d2d3321066cb4db7abeeb417af9365 Author: pbrook <pbrook@c046a42c-6fe2-441c-8c8c-71466251a162> Date: Sat Apr 11 17:15:54 2009 +0000 Implement dynamic guest ram allocation. Went to the previous one, to see if this was the only broken thing. And found that ide and vga was broken at that point. I stopped searching there. You get that image to load, and I try not to break it with VMState changes. Can you (or anybody else) got v2 to life? If it is working, and you can use it, I don't want to break its support. But if it has been broken for ages and nobody steps it to fix it -> removing is only useful thing that I can think of doing. > That said, I don't want to fight over this more than necessary, so as > long as you keep the per device loading functions in C we can maintain > v2 compatibility downstream if upstream is not interested. Instead of complaining, we can start doing things productively. Can you do a list of the older (device, version) that you are interested in? If we have such a list, we can see how feasible it is maintaing support for them. Later, Juan.
On Fri, 11 Sep 2009, Juan Quintela wrote: > About this, nobody is wanting to remove useful features. I was removing > things that didn't work. The best plan of action if you want v2 support > on tree is to fix it. Do what I did: > > This commit introduces SaveVM v3 > commit 9366f4186025e1d8fc3bebd41fb714521c170b6f > Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162> > Date: Mon Oct 6 14:53:52 2008 +0000 > > Introduce v3 of savevm protocol > > I checkout previous commit, savevm one machine. Go to the present and > try to load it -> RAM don't work. > > Search what commit broke it: > > commit 94a6b54fd6d2d3321066cb4db7abeeb417af9365 > Author: pbrook <pbrook@c046a42c-6fe2-441c-8c8c-71466251a162> > Date: Sat Apr 11 17:15:54 2009 +0000 > > Implement dynamic guest ram allocation. > > Went to the previous one, to see if this was the only broken thing. > And found that ide and vga was broken at that point. > > I stopped searching there. > You get that image to load, and I try not to break it with VMState > changes. Can you (or anybody else) got v2 to life? If it is working, > and you can use it, I don't want to break its support. But if it has > been broken for ages and nobody steps it to fix it -> removing is only > useful thing that I can think of doing. I really want to thank you for understanding; Ian and\or me will try to fix this issue as soon as we can. > > That said, I don't want to fight over this more than necessary, so as > > long as you keep the per device loading functions in C we can maintain > > v2 compatibility downstream if upstream is not interested. > > Instead of complaining, we can start doing things productively. > Can you do a list of the older (device, version) that you are interested > in? If we have such a list, we can see how feasible it is maintaing > support for them. > The list is not very long, and you'll find that most devices still have the same version number: I440FX version 2 (now is 3) PIIX3 version 2 (now is 3) cirrus_vga version 2 (now is 2) vga version 2 (now is 2) mc146818rtc version 1 serial version 2 (now is 3) rtl8139 version 3 (now is 4) ide version 2 (now is 3) pckbd version 3 (now is 3) ps2kbd version 3 (now is 3) ps2mouse version 2 (now is 2) dma version 1 fdc version 2 (now is 2) UHCI usb controller version 1 gpe version 1 pcislots version 1 piix4acpi version 1
diff --git a/savevm.c b/savevm.c index 34145f9..0dcab79 100644 --- a/savevm.c +++ b/savevm.c @@ -1266,47 +1266,6 @@ typedef struct LoadStateEntry { int version_id; } LoadStateEntry; -static int qemu_loadvm_state_v2(QEMUFile *f) -{ - SaveStateEntry *se; - int len, ret, instance_id, record_len, version_id; - int64_t total_len, end_pos, cur_pos; - char idstr[256]; - - total_len = qemu_get_be64(f); - end_pos = total_len + qemu_ftell(f); - for(;;) { - if (qemu_ftell(f) >= end_pos) - break; - len = qemu_get_byte(f); - qemu_get_buffer(f, (uint8_t *)idstr, len); - idstr[len] = '\0'; - instance_id = qemu_get_be32(f); - version_id = qemu_get_be32(f); - record_len = qemu_get_be32(f); - cur_pos = qemu_ftell(f); - se = find_se(idstr, instance_id); - if (!se) { - fprintf(stderr, "qemu: warning: instance 0x%x of device '%s' not present in current VM\n", - instance_id, idstr); - } else { - ret = vmstate_load(f, se, version_id); - if (ret < 0) { - fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n", - instance_id, idstr); - return ret; - } - } - /* always seek to exact end of record */ - qemu_fseek(f, cur_pos + record_len, SEEK_SET); - } - - if (qemu_file_has_error(f)) - return -EIO; - - return 0; -} - int qemu_loadvm_state(QEMUFile *f) { LIST_HEAD(, LoadStateEntry) loadvm_handlers = @@ -1321,8 +1280,10 @@ int qemu_loadvm_state(QEMUFile *f) return -EINVAL; v = qemu_get_be32(f); - if (v == QEMU_VM_FILE_VERSION_COMPAT) - return qemu_loadvm_state_v2(f); + if (v == QEMU_VM_FILE_VERSION_COMPAT) { + fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n"); + return -ENOTSUP; + } if (v != QEMU_VM_FILE_VERSION) return -ENOTSUP;
In previosu series I remove v2 support for RAM (that was the version that was supported when SaveVM v3 appeared). Now we can't load RAM for any image saved in SaveVM v2, we can as well remove SaveVM v2 entirely. Note: That SaveVM RAM was at v2 when General SaveVM support went from v2 to v3 makes talking about versions confusing at least Signed-off-by: Juan Quintela <quintela@redhat.com> --- savevm.c | 47 ++++------------------------------------------- 1 files changed, 4 insertions(+), 43 deletions(-)