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 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=79c3fe4d1681cd94598d2bd42e38a98f51cb645d >>>>>>> >>>>>>> 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 Ok, I understand. I'll write the patch for this today. Michal -- Michal Novotny <minovotn@xxxxxxxxxx>, RHCE Virtualization Team (xen userspace), Red Hat -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list