src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 52 +-------------------------- src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 32 +++++------------ 6 files changed, 95 insertions(+), 77 deletions(-)
There is some validation of NetDefs (<interface>) that can't be done until runtime, due to not knowing the exact configuration until that time. This needs to happen in 3 places: 1) in the qemu commandline when a new guest is started, 2) when an <interface> is hotplugged, and 3) when an <interface> is updated. Until now, there some (but not all) validation copy-pasted between (1) and (2), and none of that was present for (3). These patches move all the validation from (1) (which is the most complete) into a separate function, and then call that function from all three places, so the exact same validation is done in all cases. (These patches are a followup to a patch I sent *long* ago in a naive attempt to fix https://bugzilla.redhat.com/1502754 - my original patch did fix the problem when starting a guest with an invalid <interface> config, but not when hotplugging or updating such an interface.) NB: I noticed that if someone tries to specify <bandwidth> for an interface type that doesn't support it, we only give a warning, not an error, due to a fear that there are existing management apps that add <bandwidth> to all types of interfaces, and we don't want them to suddenly fail to run (I got this info from the BZ associated with the commit that added the warning). This seems to me to be going a bit too far - I think that (at least upstream) we should turn this into an error, and let the regression testing of said management apps catch the behavior change so they can fix their code. (insert Kermit-drinking-coffee meme here) Laine Stump (3): conf: make arg to virDomainNetGetActualVirtPortProfile() a const qemu: move runtime netdev validation into a separate function qemu: call common NetDef validation for hotplug and device update src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 52 +-------------------------- src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 32 +++++------------ 6 files changed, 95 insertions(+), 77 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 9/13/19 4:52 PM, Laine Stump wrote: > There is some validation of NetDefs (<interface>) that can't be done > until runtime, due to not knowing the exact configuration until that > time. This needs to happen in 3 places: 1) in the qemu commandline > when a new guest is started, 2) when an <interface> is hotplugged, and > 3) when an <interface> is updated. Until now, there some (but not all) > validation copy-pasted between (1) and (2), and none of that was > present for (3). These patches move all the validation from (1) (which > is the most complete) into a separate function, and then call that > function from all three places, so the exact same validation is done > in all cases. > > (These patches are a followup to a patch I sent *long* ago in a naive > attempt to fix https://bugzilla.redhat.com/1502754 - my original patch > did fix the problem when starting a guest with an invalid <interface> > config, but not when hotplugging or updating such an interface.) > > NB: I noticed that if someone tries to specify <bandwidth> for an > interface type that doesn't support it, we only give a warning, not an > error, due to a fear that there are existing management apps that add > <bandwidth> to all types of interfaces, and we don't want them to > suddenly fail to run (I got this info from the BZ associated with the > commit that added the warning). This seems to me to be going a bit too > far - I think that (at least upstream) we should turn this into an > error, and let the regression testing of said management apps catch > the behavior change so they can fix their code. (insert > Kermit-drinking-coffee meme here) Yes, that was exactly the reasoning we had when introducing the warning. Initially, when I implemented QoS it did not error out on unsupported interface type, but I guess we can do so now. > > > Laine Stump (3): > conf: make arg to virDomainNetGetActualVirtPortProfile() a const > qemu: move runtime netdev validation into a separate function > qemu: call common NetDef validation for hotplug and device update > > src/conf/domain_conf.c | 2 +- > src/conf/domain_conf.h | 2 +- > src/qemu/qemu_command.c | 52 +-------------------------- > src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_domain.h | 4 +++ > src/qemu/qemu_hotplug.c | 32 +++++------------ > 6 files changed, 95 insertions(+), 77 deletions(-) > Reviewed-by: Michal Privoznik <mprivozn@redhat.com> but please see my comment to 2/3 before pushing. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 9/13/19 4:02 PM, Michal Prívozník wrote: > On 9/13/19 4:52 PM, Laine Stump wrote: >> There is some validation of NetDefs (<interface>) that can't be done >> until runtime, due to not knowing the exact configuration until that >> time. This needs to happen in 3 places: 1) in the qemu commandline >> when a new guest is started, 2) when an <interface> is hotplugged, and >> 3) when an <interface> is updated. Until now, there some (but not all) >> validation copy-pasted between (1) and (2), and none of that was >> present for (3). These patches move all the validation from (1) (which >> is the most complete) into a separate function, and then call that >> function from all three places, so the exact same validation is done >> in all cases. >> >> (These patches are a followup to a patch I sent *long* ago in a naive >> attempt to fix https://bugzilla.redhat.com/1502754 - my original patch >> did fix the problem when starting a guest with an invalid <interface> >> config, but not when hotplugging or updating such an interface.) >> >> NB: I noticed that if someone tries to specify <bandwidth> for an >> interface type that doesn't support it, we only give a warning, not an >> error, due to a fear that there are existing management apps that add >> <bandwidth> to all types of interfaces, and we don't want them to >> suddenly fail to run (I got this info from the BZ associated with the >> commit that added the warning). This seems to me to be going a bit too >> far - I think that (at least upstream) we should turn this into an >> error, and let the regression testing of said management apps catch >> the behavior change so they can fix their code. (insert >> Kermit-drinking-coffee meme here) > > Yes, that was exactly the reasoning we had when introducing the warning. > Initially, when I implemented QoS it did not error out on unsupported > interface type, but I guess we can do so now. Yep, I completely understand your reluctance at the time to make it a full fledged error - especially if the patch was going to be backported to a downstream build during a minor update :-) I'm going to add some more things to this new validation function; I'll see if I can slip in a patch for this too. > >> >> >> Laine Stump (3): >> conf: make arg to virDomainNetGetActualVirtPortProfile() a const >> qemu: move runtime netdev validation into a separate function >> qemu: call common NetDef validation for hotplug and device update >> >> src/conf/domain_conf.c | 2 +- >> src/conf/domain_conf.h | 2 +- >> src/qemu/qemu_command.c | 52 +-------------------------- >> src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++ >> src/qemu/qemu_domain.h | 4 +++ >> src/qemu/qemu_hotplug.c | 32 +++++------------ >> 6 files changed, 95 insertions(+), 77 deletions(-) >> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com> but please see my > comment to 2/3 before pushing. > Thanks! -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.