| Message ID | cover.1554419814.git.crobinso@redhat.com |
|---|---|
| Headers | show |
| Series | qemu: use domCaps for validation | expand |
ping On 4/4/19 7:37 PM, Cole Robinson wrote: > I'm trying to remove some hurdles and pitfalls WRT extending > domaincapabilities data. One issue is that it's too easy to add > invalid data to it, or let the data become out of date. > > For example the first two patches of this series add <rng model=X> > domcaps reporting. The logic to fill in the domcaps data from qemuCaps > is nearly identical to the logic we use to validate rng->model in > qemuDomainRNGDefValidate. If just those patches are added, and later > a new qemu rng model was introduced, a future patch could easily > miss updated domaincapabilities output. > > This series aims to set up a pattern to prevent these types of issues > from sneaking in. A function virDomainCapsDeviceDefValidate is added > which will use domcaps data to perform validation against a devicedef. > The existing qemu <rng> model validation is moved there. This ensures > that any future <rng> model additions, if they want to work in the > qemu driver, effectively need to extend domaincapabilities as well. > It's also theoretically useful for other drivers too. > > One issue is that at DomainDefValidate time we don't have domCaps > handy, or any cache layer for domCaps assembling. Patch #4 adds > a domCapsCache hashtable to the virQEMUCaps class for caching domCaps > builds based on the full tuple of emulator+machine+arch+virttype. > If qemuCaps need to be regenerated, the domCaps cache is wiped out > for us so we don't need to worry about the data being stale, it's > tied to the lifetime of a qemuCaps instance. > > Cole Robinson (7): > conf: domcaps: Report device <rng> > qemu: capabilities: fill in domcaps <rng> > qemu: conf: add virQEMUDriverGetDomainCapabilities > qemu: conf: Cache domCaps in qemuCaps > conf: domcaps: Add virDomainCapsDeviceDefValidate > qemu: domain: Call virDomainCapsDeviceDefValidate > qemu: Move rng model validation to domcaps > > docs/formatdomaincaps.html.in | 35 ++++++++ > docs/schemas/domaincaps.rng | 10 +++ > src/conf/domain_capabilities.c | 83 ++++++++++++++++++ > src/conf/domain_capabilities.h | 14 ++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_capabilities.c | 41 +++++++++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_conf.c | 84 +++++++++++++++++++ > src/qemu/qemu_conf.h | 7 ++ > src/qemu/qemu_domain.c | 38 +++------ > src/qemu/qemu_driver.c | 18 +--- > .../qemu_1.7.0.x86_64.xml | 9 ++ > .../qemu_2.12.0-virt.aarch64.xml | 11 +++ > .../qemu_2.12.0.ppc64.xml | 11 +++ > .../qemu_2.12.0.s390x.xml | 11 +++ > .../qemu_2.12.0.x86_64.xml | 11 +++ > .../qemu_2.6.0-virt.aarch64.xml | 11 +++ > .../qemu_2.6.0.aarch64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 11 +++ > .../qemu_2.6.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 11 +++ > .../qemu_2.8.0-tcg.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 11 +++ > .../qemu_2.8.0.x86_64.xml | 11 +++ > .../qemu_2.9.0-q35.x86_64.xml | 11 +++ > .../qemu_2.9.0-tcg.x86_64.xml | 11 +++ > .../qemu_2.9.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 11 +++ > .../qemu_4.0.0.x86_64.xml | 11 +++ > 29 files changed, 488 insertions(+), 40 deletions(-) > - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Tried the patch series. Unfortunately I didn't get the <rng> element reported in virsh capabilities, however I am not sure if this hardware (Lenovo T480) has a hardware RNG device to report anyways. I'll check it later when I have the chance. It is worth noticing that patch 3 is conflicting with current master. Extra code in qemu_driver.c that shifted the lines that the patch is changing (trivial) and, in qemu_conf.c, I had to change the call to virQEMUCapsFillDomainCaps since it is now taking an extra argument (added by 5b9819eedc). This change was enough to get the patch rolling: +++ b/src/qemu/qemu_conf.c @@ -1424,6 +1424,7 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver, goto cleanup; if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps, + driver->privileged, cfg->firmwares, cfg->nfirmwares) < 0) goto cleanup; Thanks, DHB On 4/4/19 8:37 PM, Cole Robinson wrote: > I'm trying to remove some hurdles and pitfalls WRT extending > domaincapabilities data. One issue is that it's too easy to add > invalid data to it, or let the data become out of date. > > For example the first two patches of this series add <rng model=X> > domcaps reporting. The logic to fill in the domcaps data from qemuCaps > is nearly identical to the logic we use to validate rng->model in > qemuDomainRNGDefValidate. If just those patches are added, and later > a new qemu rng model was introduced, a future patch could easily > miss updated domaincapabilities output. > > This series aims to set up a pattern to prevent these types of issues > from sneaking in. A function virDomainCapsDeviceDefValidate is added > which will use domcaps data to perform validation against a devicedef. > The existing qemu <rng> model validation is moved there. This ensures > that any future <rng> model additions, if they want to work in the > qemu driver, effectively need to extend domaincapabilities as well. > It's also theoretically useful for other drivers too. > > One issue is that at DomainDefValidate time we don't have domCaps > handy, or any cache layer for domCaps assembling. Patch #4 adds > a domCapsCache hashtable to the virQEMUCaps class for caching domCaps > builds based on the full tuple of emulator+machine+arch+virttype. > If qemuCaps need to be regenerated, the domCaps cache is wiped out > for us so we don't need to worry about the data being stale, it's > tied to the lifetime of a qemuCaps instance. > > Cole Robinson (7): > conf: domcaps: Report device <rng> > qemu: capabilities: fill in domcaps <rng> > qemu: conf: add virQEMUDriverGetDomainCapabilities > qemu: conf: Cache domCaps in qemuCaps > conf: domcaps: Add virDomainCapsDeviceDefValidate > qemu: domain: Call virDomainCapsDeviceDefValidate > qemu: Move rng model validation to domcaps > > docs/formatdomaincaps.html.in | 35 ++++++++ > docs/schemas/domaincaps.rng | 10 +++ > src/conf/domain_capabilities.c | 83 ++++++++++++++++++ > src/conf/domain_capabilities.h | 14 ++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_capabilities.c | 41 +++++++++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_conf.c | 84 +++++++++++++++++++ > src/qemu/qemu_conf.h | 7 ++ > src/qemu/qemu_domain.c | 38 +++------ > src/qemu/qemu_driver.c | 18 +--- > .../qemu_1.7.0.x86_64.xml | 9 ++ > .../qemu_2.12.0-virt.aarch64.xml | 11 +++ > .../qemu_2.12.0.ppc64.xml | 11 +++ > .../qemu_2.12.0.s390x.xml | 11 +++ > .../qemu_2.12.0.x86_64.xml | 11 +++ > .../qemu_2.6.0-virt.aarch64.xml | 11 +++ > .../qemu_2.6.0.aarch64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 11 +++ > .../qemu_2.6.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 11 +++ > .../qemu_2.8.0-tcg.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 11 +++ > .../qemu_2.8.0.x86_64.xml | 11 +++ > .../qemu_2.9.0-q35.x86_64.xml | 11 +++ > .../qemu_2.9.0-tcg.x86_64.xml | 11 +++ > .../qemu_2.9.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 11 +++ > .../qemu_4.0.0.x86_64.xml | 11 +++ > 29 files changed, 488 insertions(+), 40 deletions(-) > <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <font size="+1">Tried the patch series. Unfortunately I didn't get the <rng> element<br> reported in virsh capabilities, however I am not sure if this hardware<br> (Lenovo T480) has a hardware RNG device to report anyways. I'll check<br> it later when I have the chance.<br> <br> It is worth noticing that patch 3 is conflicting with current master. Extra<br> code in qemu_driver.c that shifted the lines that the patch is changing<br> (trivial) and, in qemu_conf.c, I had to change the call to<br> virQEMUCapsFillDomainCaps since it is now taking an extra argument<br> (added by 5b9819eedc). This change was enough to get the patch rolling:<br> <br> <br> +++ b/src/qemu/qemu_conf.c<br> @@ -1424,6 +1424,7 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,<br> goto cleanup;<br> <br> if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,<br> + driver->privileged,<br> cfg->firmwares, cfg->nfirmwares) < 0)<br> goto cleanup;<br> <br> <br> <br> Thanks,<br> <br> <br> DHB<br> </font><br> <div class="moz-cite-prefix">On 4/4/19 8:37 PM, Cole Robinson wrote:<br> </div> <blockquote type="cite" cite="mid:cover.1554419814.git.crobinso@redhat.com"> <pre class="moz-quote-pre" wrap="">I'm trying to remove some hurdles and pitfalls WRT extending domaincapabilities data. One issue is that it's too easy to add invalid data to it, or let the data become out of date. For example the first two patches of this series add <rng model=X> domcaps reporting. The logic to fill in the domcaps data from qemuCaps is nearly identical to the logic we use to validate rng->model in qemuDomainRNGDefValidate. If just those patches are added, and later a new qemu rng model was introduced, a future patch could easily miss updated domaincapabilities output. This series aims to set up a pattern to prevent these types of issues from sneaking in. A function virDomainCapsDeviceDefValidate is added which will use domcaps data to perform validation against a devicedef. The existing qemu <rng> model validation is moved there. This ensures that any future <rng> model additions, if they want to work in the qemu driver, effectively need to extend domaincapabilities as well. It's also theoretically useful for other drivers too. One issue is that at DomainDefValidate time we don't have domCaps handy, or any cache layer for domCaps assembling. Patch #4 adds a domCapsCache hashtable to the virQEMUCaps class for caching domCaps builds based on the full tuple of emulator+machine+arch+virttype. If qemuCaps need to be regenerated, the domCaps cache is wiped out for us so we don't need to worry about the data being stale, it's tied to the lifetime of a qemuCaps instance. Cole Robinson (7): conf: domcaps: Report device <rng> qemu: capabilities: fill in domcaps <rng> qemu: conf: add virQEMUDriverGetDomainCapabilities qemu: conf: Cache domCaps in qemuCaps conf: domcaps: Add virDomainCapsDeviceDefValidate qemu: domain: Call virDomainCapsDeviceDefValidate qemu: Move rng model validation to domcaps docs/formatdomaincaps.html.in | 35 ++++++++ docs/schemas/domaincaps.rng | 10 +++ src/conf/domain_capabilities.c | 83 ++++++++++++++++++ src/conf/domain_capabilities.h | 14 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 41 +++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_conf.c | 84 +++++++++++++++++++ src/qemu/qemu_conf.h | 7 ++ src/qemu/qemu_domain.c | 38 +++------ src/qemu/qemu_driver.c | 18 +--- .../qemu_1.7.0.x86_64.xml | 9 ++ .../qemu_2.12.0-virt.aarch64.xml | 11 +++ .../qemu_2.12.0.ppc64.xml | 11 +++ .../qemu_2.12.0.s390x.xml | 11 +++ .../qemu_2.12.0.x86_64.xml | 11 +++ .../qemu_2.6.0-virt.aarch64.xml | 11 +++ .../qemu_2.6.0.aarch64.xml | 11 +++ .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 11 +++ .../qemu_2.6.0.x86_64.xml | 11 +++ .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 11 +++ .../qemu_2.8.0-tcg.x86_64.xml | 11 +++ .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 11 +++ .../qemu_2.8.0.x86_64.xml | 11 +++ .../qemu_2.9.0-q35.x86_64.xml | 11 +++ .../qemu_2.9.0-tcg.x86_64.xml | 11 +++ .../qemu_2.9.0.x86_64.xml | 11 +++ .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 11 +++ .../qemu_4.0.0.x86_64.xml | 11 +++ 29 files changed, 488 insertions(+), 40 deletions(-) </pre> </blockquote> <br> </body> </html> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 4/5/19 1:37 AM, Cole Robinson wrote: > I'm trying to remove some hurdles and pitfalls WRT extending > domaincapabilities data. One issue is that it's too easy to add > invalid data to it, or let the data become out of date. > > For example the first two patches of this series add <rng model=X> > domcaps reporting. The logic to fill in the domcaps data from qemuCaps > is nearly identical to the logic we use to validate rng->model in > qemuDomainRNGDefValidate. If just those patches are added, and later > a new qemu rng model was introduced, a future patch could easily > miss updated domaincapabilities output. > > This series aims to set up a pattern to prevent these types of issues > from sneaking in. A function virDomainCapsDeviceDefValidate is added > which will use domcaps data to perform validation against a devicedef. > The existing qemu <rng> model validation is moved there. This ensures > that any future <rng> model additions, if they want to work in the > qemu driver, effectively need to extend domaincapabilities as well. > It's also theoretically useful for other drivers too. > > One issue is that at DomainDefValidate time we don't have domCaps > handy, or any cache layer for domCaps assembling. Patch #4 adds > a domCapsCache hashtable to the virQEMUCaps class for caching domCaps > builds based on the full tuple of emulator+machine+arch+virttype. > If qemuCaps need to be regenerated, the domCaps cache is wiped out > for us so we don't need to worry about the data being stale, it's > tied to the lifetime of a qemuCaps instance. > > Cole Robinson (7): > conf: domcaps: Report device <rng> > qemu: capabilities: fill in domcaps <rng> > qemu: conf: add virQEMUDriverGetDomainCapabilities > qemu: conf: Cache domCaps in qemuCaps > conf: domcaps: Add virDomainCapsDeviceDefValidate > qemu: domain: Call virDomainCapsDeviceDefValidate > qemu: Move rng model validation to domcaps > > docs/formatdomaincaps.html.in | 35 ++++++++ > docs/schemas/domaincaps.rng | 10 +++ > src/conf/domain_capabilities.c | 83 ++++++++++++++++++ > src/conf/domain_capabilities.h | 14 ++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_capabilities.c | 41 +++++++++ > src/qemu/qemu_capabilities.h | 1 + > src/qemu/qemu_conf.c | 84 +++++++++++++++++++ > src/qemu/qemu_conf.h | 7 ++ > src/qemu/qemu_domain.c | 38 +++------ > src/qemu/qemu_driver.c | 18 +--- > .../qemu_1.7.0.x86_64.xml | 9 ++ > .../qemu_2.12.0-virt.aarch64.xml | 11 +++ > .../qemu_2.12.0.ppc64.xml | 11 +++ > .../qemu_2.12.0.s390x.xml | 11 +++ > .../qemu_2.12.0.x86_64.xml | 11 +++ > .../qemu_2.6.0-virt.aarch64.xml | 11 +++ > .../qemu_2.6.0.aarch64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 11 +++ > .../qemu_2.6.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 11 +++ > .../qemu_2.8.0-tcg.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 11 +++ > .../qemu_2.8.0.x86_64.xml | 11 +++ > .../qemu_2.9.0-q35.x86_64.xml | 11 +++ > .../qemu_2.9.0-tcg.x86_64.xml | 11 +++ > .../qemu_2.9.0.x86_64.xml | 11 +++ > .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 11 +++ > .../qemu_4.0.0.x86_64.xml | 11 +++ > 29 files changed, 488 insertions(+), 40 deletions(-) > ACK if you address Daniel's and mine findings. Sorry for delayed review. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 4/29/19 8:35 AM, Michal Privoznik wrote: > On 4/5/19 1:37 AM, Cole Robinson wrote: >> I'm trying to remove some hurdles and pitfalls WRT extending >> domaincapabilities data. One issue is that it's too easy to add >> invalid data to it, or let the data become out of date. >> >> For example the first two patches of this series add <rng model=X> >> domcaps reporting. The logic to fill in the domcaps data from qemuCaps >> is nearly identical to the logic we use to validate rng->model in >> qemuDomainRNGDefValidate. If just those patches are added, and later >> a new qemu rng model was introduced, a future patch could easily >> miss updated domaincapabilities output. >> >> This series aims to set up a pattern to prevent these types of issues >> from sneaking in. A function virDomainCapsDeviceDefValidate is added >> which will use domcaps data to perform validation against a devicedef. >> The existing qemu <rng> model validation is moved there. This ensures >> that any future <rng> model additions, if they want to work in the >> qemu driver, effectively need to extend domaincapabilities as well. >> It's also theoretically useful for other drivers too. >> >> One issue is that at DomainDefValidate time we don't have domCaps >> handy, or any cache layer for domCaps assembling. Patch #4 adds >> a domCapsCache hashtable to the virQEMUCaps class for caching domCaps >> builds based on the full tuple of emulator+machine+arch+virttype. >> If qemuCaps need to be regenerated, the domCaps cache is wiped out >> for us so we don't need to worry about the data being stale, it's >> tied to the lifetime of a qemuCaps instance. >> >> Cole Robinson (7): >> conf: domcaps: Report device <rng> >> qemu: capabilities: fill in domcaps <rng> >> qemu: conf: add virQEMUDriverGetDomainCapabilities >> qemu: conf: Cache domCaps in qemuCaps >> conf: domcaps: Add virDomainCapsDeviceDefValidate >> qemu: domain: Call virDomainCapsDeviceDefValidate >> qemu: Move rng model validation to domcaps >> >> docs/formatdomaincaps.html.in | 35 ++++++++ >> docs/schemas/domaincaps.rng | 10 +++ >> src/conf/domain_capabilities.c | 83 ++++++++++++++++++ >> src/conf/domain_capabilities.h | 14 ++++ >> src/libvirt_private.syms | 1 + >> src/qemu/qemu_capabilities.c | 41 +++++++++ >> src/qemu/qemu_capabilities.h | 1 + >> src/qemu/qemu_conf.c | 84 +++++++++++++++++++ >> src/qemu/qemu_conf.h | 7 ++ >> src/qemu/qemu_domain.c | 38 +++------ >> src/qemu/qemu_driver.c | 18 +--- >> .../qemu_1.7.0.x86_64.xml | 9 ++ >> .../qemu_2.12.0-virt.aarch64.xml | 11 +++ >> .../qemu_2.12.0.ppc64.xml | 11 +++ >> .../qemu_2.12.0.s390x.xml | 11 +++ >> .../qemu_2.12.0.x86_64.xml | 11 +++ >> .../qemu_2.6.0-virt.aarch64.xml | 11 +++ >> .../qemu_2.6.0.aarch64.xml | 11 +++ >> .../domaincapsschemadata/qemu_2.6.0.ppc64.xml | 11 +++ >> .../qemu_2.6.0.x86_64.xml | 11 +++ >> .../domaincapsschemadata/qemu_2.7.0.s390x.xml | 11 +++ >> .../qemu_2.8.0-tcg.x86_64.xml | 11 +++ >> .../domaincapsschemadata/qemu_2.8.0.s390x.xml | 11 +++ >> .../qemu_2.8.0.x86_64.xml | 11 +++ >> .../qemu_2.9.0-q35.x86_64.xml | 11 +++ >> .../qemu_2.9.0-tcg.x86_64.xml | 11 +++ >> .../qemu_2.9.0.x86_64.xml | 11 +++ >> .../domaincapsschemadata/qemu_3.0.0.s390x.xml | 11 +++ >> .../qemu_4.0.0.x86_64.xml | 11 +++ >> 29 files changed, 488 insertions(+), 40 deletions(-) >> > > ACK if you address Daniel's and mine findings. > > Sorry for delayed review. > So I completely forgot about this series and never even saw the ACKs! I rebased the patches and they applied cleanly besides the driver->privileged piece that was already mentioned. So I addressed all the original review comments, add RB tags for you and Daniel, and pushed now. So no worries on the review delay :) Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list