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

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?