Re: [libvirt] Regression in allocating ports for serial/parallel devs

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