[libvirt] Regression in allocating ports for serial/parallel devs
Hi Michal, The following commit introduced a regression: http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3... Now, defining a guest with XML like <serial type='pty'/> <serial type='null'/> <serial type='stdio'/> Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is: --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; } + def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY; But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default. Additionally we should add a qemuxml2xml test for this to prevent against future regressions. Thanks, Cole
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole
Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have? There's a regression testing but maybe it's not for qemuxml2xml test. Adding DV to the CC list for his opinion whether he has some better idea about this. Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole
Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml Thanks, Cole
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example
tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml
Thanks, Cole I'm sorry for the delay to get to this and I've been thinking about what you wrote and I'm still not sure how exactly it should work. Based on
On 04/05/2011 06:30 PM, Cole Robinson wrote: the virDomainChrDefParseXML() function there's: if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } used for the allocation of the virDomainChrDefPtr (since it's declared as "virDomainChrDefPtr def") so basically just adding: def->target.port = -1; below those lines would be sufficient, won't it ? Or do you prefer having some function like virChrDefNew declared as: static virDomainChrDefPtr virChrDefNew() { virDomainChrDefPtr def; if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; } def->target.port = -1; return def; } or something similar to this ? Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 04/13/2011 04:36 AM, Michal Novotny wrote:
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example
tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml
Thanks, Cole I'm sorry for the delay to get to this and I've been thinking about what you wrote and I'm still not sure how exactly it should work. Based on
On 04/05/2011 06:30 PM, Cole Robinson wrote: the virDomainChrDefParseXML() function there's:
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
used for the allocation of the virDomainChrDefPtr (since it's declared as "virDomainChrDefPtr def") so basically just adding:
def->target.port = -1;
below those lines would be sufficient, won't it ? Or do you prefer having some function like virChrDefNew declared as:
static virDomainChrDefPtr virChrDefNew() { virDomainChrDefPtr def;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
def->target.port = -1; return def; }
That is basically the function I was envisioning, but I don't think it should be static, since there are users outside domain_conf who populate ChrDefPtr (xen and vmware at least). - Cole
On 04/13/2011 02:21 PM, Cole Robinson wrote:
On 04/13/2011 04:36 AM, Michal Novotny wrote:
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example
tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml
Thanks, Cole I'm sorry for the delay to get to this and I've been thinking about what you wrote and I'm still not sure how exactly it should work. Based on
On 04/05/2011 06:30 PM, Cole Robinson wrote: the virDomainChrDefParseXML() function there's:
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
used for the allocation of the virDomainChrDefPtr (since it's declared as "virDomainChrDefPtr def") so basically just adding:
def->target.port = -1;
below those lines would be sufficient, won't it ? Or do you prefer having some function like virChrDefNew declared as:
static virDomainChrDefPtr virChrDefNew() { virDomainChrDefPtr def;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
def->target.port = -1; return def; }
That is basically the function I was envisioning, but I don't think it should be static, since there are users outside domain_conf who populate ChrDefPtr (xen and vmware at least).
- Cole So do you think I should introduce the function I mentioned and use it instead of VIR_ALLOC(def) calls?
Thanks, Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
On 04/13/2011 08:44 AM, Michal Novotny wrote:
On 04/13/2011 02:21 PM, Cole Robinson wrote:
On 04/13/2011 04:36 AM, Michal Novotny wrote:
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote:
Hi Michal,
The following commit introduced a regression:
http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3...
Now, defining a guest with XML like
<serial type='pty'/> <serial type='null'/> <serial type='stdio'/>
Will allocate <target port='0'/> to all 3. The reason is that target.port is never set to -1 unless the user specified some <target> XML. A simple fix is:
--- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, return NULL; }
+ def->target.port = -1; + type = virXMLPropString(node, "type"); if (type == NULL) { def->source.type = VIR_DOMAIN_CHR_TYPE_PTY;
But that doesn't solve the problem for users who are building ChrDef's by hand, like when converting between formats as xen and vmware drivers do. I didn't look at those users so they may be safe, but the interface should be improved. Maybe add a ChrDefNew function that sets the -1 default.
Additionally we should add a qemuxml2xml test for this to prevent against future regressions.
Thanks, Cole Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example
tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml
Thanks, Cole I'm sorry for the delay to get to this and I've been thinking about what you wrote and I'm still not sure how exactly it should work. Based on
On 04/05/2011 06:30 PM, Cole Robinson wrote: the virDomainChrDefParseXML() function there's:
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
used for the allocation of the virDomainChrDefPtr (since it's declared as "virDomainChrDefPtr def") so basically just adding:
def->target.port = -1;
below those lines would be sufficient, won't it ? Or do you prefer having some function like virChrDefNew declared as:
static virDomainChrDefPtr virChrDefNew() { virDomainChrDefPtr def;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
def->target.port = -1; return def; }
That is basically the function I was envisioning, but I don't think it should be static, since there are users outside domain_conf who populate ChrDefPtr (xen and vmware at least).
- Cole So do you think I should introduce the function I mentioned and use it instead of VIR_ALLOC(def) calls?
Yes, that's correct. Thanks, Cole
On 04/13/2011 02:46 PM, Cole Robinson wrote:
On 04/13/2011 08:44 AM, Michal Novotny wrote:
On 04/13/2011 02:21 PM, Cole Robinson wrote:
On 04/13/2011 04:36 AM, Michal Novotny wrote:
On 04/05/2011 12:17 PM, Michal Novotny wrote:
On 04/05/2011 06:13 PM, Cole Robinson wrote: > Hi Michal, > > The following commit introduced a regression: > > http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd42e3... > > Now, defining a guest with XML like > > <serial type='pty'/> > <serial type='null'/> > <serial type='stdio'/> > > Will allocate <target port='0'/> to all 3. The reason is that > target.port is never set to -1 unless the user specified some <target> > XML. A simple fix is: > > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -3265,6 +3265,8 @@ virDomainChrDefParseXML(virCapsPtr caps, > return NULL; > } > > + def->target.port = -1; > + > type = virXMLPropString(node, "type"); > if (type == NULL) { > def->source.type = VIR_DOMAIN_CHR_TYPE_PTY; > > But that doesn't solve the problem for users who are building ChrDef's > by hand, like when converting between formats as xen and vmware drivers > do. I didn't look at those users so they may be safe, but the interface > should be improved. Maybe add a ChrDefNew function that sets the -1 default. > > Additionally we should add a qemuxml2xml test for this to prevent > against future regressions. > > Thanks, > Cole Hi Cole, do you think it's worth implementing the ChrDefNew besides the ChrDef which we already have?
ChrDefNew would be a constructor function, that would return an allocated ChrDefPtr. In fact I'd argue we should have these constructors for every device type, some others definitely need it at the moment (like controller which uses a similar -1 default).
There's a regression testing but maybe it's not for qemuxml2xml test.
Not sure I understand this sentence, but qemuxml2xml test is for round trip XML testing, which is what is needed here. There is already a couple examples where the round trip XML is expected to change, see for example
tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-console-compat-auto.xml tests/qemuxml2xmloutdata/qemuxml2xmlout-console-compat-auto.xml
Thanks, Cole I'm sorry for the delay to get to this and I've been thinking about what you wrote and I'm still not sure how exactly it should work. Based on
On 04/05/2011 06:30 PM, Cole Robinson wrote: the virDomainChrDefParseXML() function there's:
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
used for the allocation of the virDomainChrDefPtr (since it's declared as "virDomainChrDefPtr def") so basically just adding:
def->target.port = -1;
below those lines would be sufficient, won't it ? Or do you prefer having some function like virChrDefNew declared as:
static virDomainChrDefPtr virChrDefNew() { virDomainChrDefPtr def;
if (VIR_ALLOC(def) < 0) { virReportOOMError(); return NULL; }
def->target.port = -1; return def; }
That is basically the function I was envisioning, but I don't think it should be static, since there are users outside domain_conf who populate ChrDefPtr (xen and vmware at least).
- Cole So do you think I should introduce the function I mentioned and use it instead of VIR_ALLOC(def) calls?
Yes, that's correct.
Thanks, Cole
Ok, I understand. I'll write the patch for this today. Michal -- Michal Novotny <minovotn@redhat.com>, RHCE Virtualization Team (xen userspace), Red Hat
participants (2)
-
Cole Robinson -
Michal Novotny