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

[Michal Novotny]

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 at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat