5:12 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2018-August/msg01208.html
diff to v1:
- did what Andrea suggested
Michal Prívozník (2):
device_conf: Split virDomainDeviceInfoClear
conf: Parse guestfwd channel device info again
src/conf/device_conf.c | 10 ++++++++--
src/conf/device_conf.h | 1 +
src/conf/domain_conf.c | 13 ++++++-------
3 files changed, 15 insertions(+), 9 deletions(-)
--
2.16.4
5:12 a.m.
New subject: [libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear
Some callers might want to clear the address but not the alias.
Split the virDomainDeviceInfoClear() function to allow that.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/device_conf.c | 10 ++++++++--
src/conf/device_conf.h | 1 +
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index d69f94fadf..8fa6bb4fe0 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -54,9 +54,8 @@ virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
}
void
-virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
{
- VIR_FREE(info->alias);
memset(&info->addr, 0, sizeof(info->addr));
info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
VIR_FREE(info->romfile);
@@ -65,6 +64,13 @@ virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
info->isolationGroupLocked = false;
}
+void
+virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+{
+ VIR_FREE(info->alias);
+ virDomainDeviceInfoClearAddress(info);
+}
+
void
virDomainDeviceInfoFree(virDomainDeviceInfoPtr info)
{
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a31ce9c376..1d0e9e6925 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -179,6 +179,7 @@ struct _virDomainDeviceInfo {
int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
virDomainDeviceInfoPtr src);
+void virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
void virDomainDeviceInfoFree(virDomainDeviceInfoPtr info);
--
2.16.4
6:15 a.m.
New subject: [libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear
On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
void
-virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
{
- VIR_FREE(info->alias);
memset(&info->addr, 0, sizeof(info->addr));
info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
VIR_FREE(info->romfile);
Now virDomainDeviceInfoClearAddress() clears out *way more* than
just the address, including romfile and other information not
visible in the context... It should really only call memset and
reset info->type in order for the name not to be very misleading.
[...]
+void
+virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
+{
+ VIR_FREE(info->alias);
+ virDomainDeviceInfoClearAddress(info);
+}
romfile and friends should be cleared out here along with the alias
and, of course, the address :)
With that fixed
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
6:45 a.m.
New subject: [libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear
On 08/21/2018 01:15 PM, Andrea Bolognani wrote:
On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
> void
> -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> +virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
> {
> - VIR_FREE(info->alias);
> memset(&info->addr, 0, sizeof(info->addr));
> info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
> VIR_FREE(info->romfile);
Now virDomainDeviceInfoClearAddress() clears out *way more* than
just the address, including romfile and other information not
visible in the context... It should really only call memset and
reset info->type in order for the name not to be very misleading.
[...]
> +void
> +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> +{
> + VIR_FREE(info->alias);
> + virDomainDeviceInfoClearAddress(info);
> +}
romfile and friends should be cleared out here along with the alias
and, of course, the address :)
With that fixed
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
Actually, this is not going to work either. If somebody specifies:
<master startport='1'/> into guestfwd XML we will parse it and format it
back. The only solution I see is to have a flag to
virDomainDeviceInfoParseXML() that parses just alias and nothing else.
Which of course doesn't fit into the rest of the flags it already has.
So the flag has to be the opposite: allow alias parsing and then fix all
the places where it is called.
Sigh. This is gotten too far for such unusual corner case. I'm just
going to close the bug as WONTFIX.
Thanks anyway.
Michal
7:12 a.m.
New subject: [libvirt] [PATCH v2 1/2] device_conf: Split virDomainDeviceInfoClear
On Tue, 2018-08-21 at 13:45 +0200, Michal Privoznik wrote:
On 08/21/2018 01:15 PM, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
> > void
> > -virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> > +virDomainDeviceInfoClearAddress(virDomainDeviceInfoPtr info)
> > {
> > - VIR_FREE(info->alias);
> > memset(&info->addr, 0, sizeof(info->addr));
> > info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
> > VIR_FREE(info->romfile);
>
> Now virDomainDeviceInfoClearAddress() clears out *way more* than
> just the address, including romfile and other information not
> visible in the context... It should really only call memset and
> reset info->type in order for the name not to be very misleading.
>
> [...]
> > +void
> > +virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> > +{
> > + VIR_FREE(info->alias);
> > + virDomainDeviceInfoClearAddress(info);
> > +}
>
> romfile and friends should be cleared out here along with the alias
> and, of course, the address :)
Actually, this is not going to work either. If somebody specifies:
<master startport='1'/> into guestfwd XML we will parse it and format it
back. The only solution I see is to have a flag to
virDomainDeviceInfoParseXML() that parses just alias and nothing else.
Which of course doesn't fit into the rest of the flags it already has.
So the flag has to be the opposite: allow alias parsing and then fix all
the places where it is called.
Sigh. This is gotten too far for such unusual corner case. I'm just
going to close the bug as WONTFIX.
Thanks anyway.
You're right, I failed to consider that.
Given the situation I'd say that we should merge the initial fix
and err on the side of parsing/formatting information that doesn't
apply to the device rather than skipping information that does: the
former is suboptimal, but the latter is outright broken.
I'll ACK the v1 patch.
--
Andrea Bolognani / Red Hat / Virtualization
5:12 a.m.
New subject: [libvirt] [PATCH v2 2/2] conf: Parse guestfwd channel device info again
https://bugzilla.redhat.com/show_bug.cgi?id=1610072
Due to historical reasons we were not parsing device info on
guestfwd channel. Sure, it doesn't make much sense to parse
<address/> but it surely makes sense to parse its alias (which
might be an user alias).
This partially reverts commit
47a3dd46ead20e6fdc30bcdc1b8e707e250d33da which fixed
https://bugzilla.redhat.com/show_bug.cgi?id=1172526.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c254801cd..abbc6e8a85 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12807,14 +12807,13 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
}
}
+ if (virDomainDeviceInfoParseXML(xmlopt, node,
+ &def->info, flags) < 0)
+ goto error;
+
if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
- def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
- VIR_DEBUG("Ignoring device address for gustfwd channel");
- } else if (virDomainDeviceInfoParseXML(xmlopt, node,
- &def->info, flags) < 0) {
- goto error;
- }
-
+ def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD)
+ virDomainDeviceInfoClearAddress(&def->info);
if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
--
2.16.4
6:18 a.m.
New subject: [libvirt] [PATCH v2 2/2] conf: Parse guestfwd channel device info again
On Tue, 2018-08-21 at 12:12 +0200, Michal Privoznik wrote:
[...]
if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL
&&
- def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
- VIR_DEBUG("Ignoring device address for gustfwd channel");
- } else if (virDomainDeviceInfoParseXML(xmlopt, node,
- &def->info, flags) < 0) {
- goto error;
- }
-
+ def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD)
+ virDomainDeviceInfoClearAddress(&def->info);
Our style guidelines require curly braces around the if body in this
case - which is great, because it means your diff can be smaller and
more readable! :)
With that fixed
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2285
days inactive
2285
days old
6 comments
2 participants
participants (2)
-
Andrea Bolognani -
Michal Privoznik