Hi Michal, The following commit introduced a regression: http://libvirt.org/git/?p=libvirt.git;a=commit;h=79c3fe4d1681cd94598d2bd4... 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 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=79c3fe4d1681cd94598d2bd4... >> >> 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/13/2011 04:36 AM, Michal Novotny wrote: > On 04/05/2011 06:30 PM, Cole Robinson 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=79c3fe4d1681cd94598d2bd4... >>>> >>>> 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 > 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 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 06:30 PM, Cole Robinson 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=79c3fe4d1681cd94598d2bd4... >>>>>> >>>>>> 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 >>> 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