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

[Cole Robinson]

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