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

[Michal Novotny]

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;
}

or something similar to this ?

Thanks,
Michal

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