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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list