Re: [libvirt] [PATCH] qemu: qemuDomainChangeNet: don't overwrite device info when pci addr is missing
On Tue, Jul 31, 2018 at 04:34:39PM +0200, Katerina Koukiou wrote:
When trying to update an interface's rom settings with an device XML
that is missing the PCI addr element, all new rom settings where not applied.
https://bugzilla.redhat.com/show_bug.cgi?id=1599513
Signed-off-by: Katerina Koukiou
---
Not sure why we chose to overwrite the whole info before though, I hope
that this doesn't cause side problems.
It allows the user to omit parts of the XML that do not need changing.
By dropping the virDomainDeviceInfoCopy call, we start validating the
attributes that weren't provided in the XML.
If we want to keep this convenience functionality, we should also autofill
other fields of virDomainDeviceInfo which weren't provided in the XML.
src/qemu/qemu_hotplug.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1488f0a7c2..f45192b1d3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3445,17 +3445,14 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
goto cleanup;
}
-/* info: if newdev->info is empty, fill it in from olddev,
- * otherwise verify that it matches - nothing is allowed to
- * change. (There is no helper function to do this, so
- * individually check the few feidls of virDomainDeviceInfo that
- * are relevant in this case).
+/* info: if newdev->info.addr.pci is empty, fill it in from olddev,
+ * otherwise verify that it matches.
*/
if (!virDomainDeviceAddressIsValid(>info,
- VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
-virDomainDeviceInfoCopy(>info, >info) < 0) {
-goto cleanup;
+ VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+newdev->info.addr.pci = olddev->info.addr.pci;
}
+
Unrelated whitespace change.
Jano
if (!virPCIDeviceAddressEqual(>info.addr.pci,
>info.addr.pci)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qemuDomainChangeNet: don't overwrite device info when pci addr is missing
On Tue, Jul 31, 2018 at 07:24:00PM +0200, Ján Tomko wrote: > On Tue, Jul 31, 2018 at 05:32:51PM +0200, Katerina Koukiou wrote: > > On Tue, Jul 31, 2018 at 05:15:51PM +0200, Ján Tomko wrote: > > > On Tue, Jul 31, 2018 at 04:34:39PM +0200, Katerina Koukiou wrote: > > > > When trying to update an interface's rom settings with an device XML > > > > that is missing the PCI addr element, all new rom settings where not > > > > applied. > > > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1599513 > > > > > > > > Signed-off-by: Katerina Koukiou > > > > --- > > > > Not sure why we chose to overwrite the whole info before though, I hope > > > > that this doesn't cause side problems. > > > > > > > > > > It allows the user to omit parts of the XML that do not need changing. > > > > But this happens now only if PCI addr is missing. > > > > > > > > By dropping the virDomainDeviceInfoCopy call, we start validating the > > > attributes that weren't provided in the XML. > > > > > > If we want to keep this convenience functionality, we should also autofill > > > other fields of virDomainDeviceInfo which weren't provided in the XML. > > > > The functionality right now is that only if PCI addr is missing, then > > autofilling happens, by overwriting all other passed params. I don't > > really consider it a convenience functionality, more of confusing. > > It is both confusing and convenient. > > > I think overwritting the specific object that is missing is more > > correct. What do you think? > > Well I can at least perform the existing checks for the info before they get overwritten. I 'll repost with this change. Katerina > > Yes. I'm afraid we cannot get rid of the 'change with incomplete XML' > behavior after all the years. > > Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qemuDomainChangeNet: don't overwrite device info when pci addr is missing
On Tue, Jul 31, 2018 at 05:32:51PM +0200, Katerina Koukiou wrote: On Tue, Jul 31, 2018 at 05:15:51PM +0200, Ján Tomko wrote: On Tue, Jul 31, 2018 at 04:34:39PM +0200, Katerina Koukiou wrote: > When trying to update an interface's rom settings with an device XML > that is missing the PCI addr element, all new rom settings where not applied. > > https://bugzilla.redhat.com/show_bug.cgi?id=1599513 > > Signed-off-by: Katerina Koukiou > --- > Not sure why we chose to overwrite the whole info before though, I hope > that this doesn't cause side problems. > It allows the user to omit parts of the XML that do not need changing. But this happens now only if PCI addr is missing. By dropping the virDomainDeviceInfoCopy call, we start validating the attributes that weren't provided in the XML. If we want to keep this convenience functionality, we should also autofill other fields of virDomainDeviceInfo which weren't provided in the XML. The functionality right now is that only if PCI addr is missing, then autofilling happens, by overwriting all other passed params. I don't really consider it a convenience functionality, more of confusing. It is both confusing and convenient. I think overwritting the specific object that is missing is more correct. What do you think? Yes. I'm afraid we cannot get rid of the 'change with incomplete XML' behavior after all the years. Jano signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qemuDomainChangeNet: don't overwrite device info when pci addr is missing
On Tue, Jul 31, 2018 at 05:15:51PM +0200, Ján Tomko wrote:
> On Tue, Jul 31, 2018 at 04:34:39PM +0200, Katerina Koukiou wrote:
> > When trying to update an interface's rom settings with an device XML
> > that is missing the PCI addr element, all new rom settings where not
> > applied.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1599513
> >
> > Signed-off-by: Katerina Koukiou
> > ---
> > Not sure why we chose to overwrite the whole info before though, I hope
> > that this doesn't cause side problems.
> >
>
> It allows the user to omit parts of the XML that do not need changing.
But this happens now only if PCI addr is missing.
>
> By dropping the virDomainDeviceInfoCopy call, we start validating the
> attributes that weren't provided in the XML.
>
> If we want to keep this convenience functionality, we should also autofill
> other fields of virDomainDeviceInfo which weren't provided in the XML.
The functionality right now is that only if PCI addr is missing, then
autofilling happens, by overwriting all other passed params. I don't
really consider it a convenience functionality, more of confusing.
I think overwritting the specific object that is missing is more
correct. What do you think?
>
> >
> > src/qemu/qemu_hotplug.c | 13 +
> > 1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 1488f0a7c2..f45192b1d3 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -3445,17 +3445,14 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> > goto cleanup;
> > }
> >
> > -/* info: if newdev->info is empty, fill it in from olddev,
> > - * otherwise verify that it matches - nothing is allowed to
> > - * change. (There is no helper function to do this, so
> > - * individually check the few feidls of virDomainDeviceInfo that
> > - * are relevant in this case).
> > +/* info: if newdev->info.addr.pci is empty, fill it in from olddev,
> > + * otherwise verify that it matches.
> > */
> > if (!virDomainDeviceAddressIsValid(>info,
> > - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> > &&
> > -virDomainDeviceInfoCopy(>info, >info) < 0) {
> > -goto cleanup;
> > +
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
> > +newdev->info.addr.pci = olddev->info.addr.pci;
> > }
> > +
>
> Unrelated whitespace change.
>
> Jano
>
> > if (!virPCIDeviceAddressEqual(>info.addr.pci,
> > >info.addr.pci)) {
> > virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > --
> > 2.17.1
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: qemuDomainChangeNet: don't overwrite device info when pci addr is missing
When trying to update an interface's rom settings with an device XML
that is missing the PCI addr element, all new rom settings where not applied.
https://bugzilla.redhat.com/show_bug.cgi?id=1599513
Signed-off-by: Katerina Koukiou
---
Not sure why we chose to overwrite the whole info before though, I hope
that this doesn't cause side problems.
src/qemu/qemu_hotplug.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1488f0a7c2..f45192b1d3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3445,17 +3445,14 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
goto cleanup;
}
-/* info: if newdev->info is empty, fill it in from olddev,
- * otherwise verify that it matches - nothing is allowed to
- * change. (There is no helper function to do this, so
- * individually check the few feidls of virDomainDeviceInfo that
- * are relevant in this case).
+/* info: if newdev->info.addr.pci is empty, fill it in from olddev,
+ * otherwise verify that it matches.
*/
if (!virDomainDeviceAddressIsValid(>info,
- VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
-virDomainDeviceInfoCopy(>info, >info) < 0) {
-goto cleanup;
+ VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+newdev->info.addr.pci = olddev->info.addr.pci;
}
+
if (!virPCIDeviceAddressEqual(>info.addr.pci,
>info.addr.pci)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
