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

[Cole Robinson]

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