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

[Michal Novotny]

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?

Thanks,
Michal

-- 
Michal Novotny <minovotn at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat