10:51 a.m.
On 05/15/2012 05:47 PM, Marc-André Lureau wrote:
Hi
Some of your reviews are already adressed in the new patch:
http://www.redhat.com/archives/libvir-list/2012-May/msg00834.html
On Tue, May 15, 2012 at 10:58 PM, Laine Stump <laine(a)laine.org> wrote:
> I think this should be in the common part of <interface> rather than
> user-specific. This isn't something inherently specific to type='user'
> (as a matter of fact, I would like to expand it fairly soon after you're
> done to also work for type='network'), and restricting it as you're
> doing here will just need to be undone later (including moving things
> around in the virDomainNetDef struct, etc). I think instead the RNG,
> parser, and formatter should allow it for all types of interface, and
> the hypervisor driver should log a VIR_ERR_CONFIG_UNSUPPORTED error and
> fail to attach if <forward> is given for an interface type that doesn't
> support it.
As you say, It can be un-restricted once it is implemented for other types.
Yes, but let's avoid the code churn of needing to move it around in the
struct and changing the parser/formatter - it's better for them to have
it in the main structure from the beginning, and just restricted in the
drivers where it is used (or not). That way, as the hypervisor drivers
add support, only the driver code itself will need to be changed (and
only for that particular driver).
>
>> <ref name="interface-options"/>
>> </interleave>
>> </group>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 51d6cb9..4b9b644 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -650,6 +650,10 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode,
>> "static",
>> "auto");
>>
>> +VIR_ENUM_IMPL(virDomainNetForward, VIR_DOMAIN_NET_FORWARD_TYPE_LAST,
>> + "tcp",
>> + "udp")
>> +
>> #define virDomainReportError(code, ...) \
>> virReportErrorHelper(VIR_FROM_DOMAIN, code, __FILE__, \
>> __FUNCTION__, __LINE__, __VA_ARGS__)
>> @@ -1019,8 +1023,22 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>> VIR_FREE(def);
>> }
>>
>> +void
>> +virDomainNetForwardDefFree(virDomainNetForwardDefPtr def)
>> +{
>> + if (!def)
>> + return;
>> +
>> + VIR_FREE(def->host_addr);
>> + VIR_FREE(def->guest_addr);
>> +
>> + VIR_FREE(def);
>> +}
>> +
>> void virDomainNetDefFree(virDomainNetDefPtr def)
>> {
>> + int i;
>> +
>> if (!def)
>> return;
>>
>> @@ -1066,6 +1084,11 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>> break;
>>
>> case VIR_DOMAIN_NET_TYPE_USER:
>> + for (i = 0; i < def->data.user.nforward; i++)
>> + virDomainNetForwardDefFree(def->data.user.forwards[i]);
>> + VIR_FREE(def->data.user.forwards);
>> + break;
>> +
>> case VIR_DOMAIN_NET_TYPE_LAST:
>> break;
>> }
>> @@ -4351,6 +4374,60 @@ error:
>> return ret;
>> }
>>
>> +static virDomainNetForwardDefPtr
>> +virDomainNetForwardDefParseXML(const xmlNodePtr node)
>> +{
>> + char *type = NULL;
>> + char *host_port = NULL;
>> + char *guest_port = NULL;
>> + virDomainNetForwardDefPtr def;
>> +
>> + if (VIR_ALLOC(def) < 0) {
>> + virReportOOMError();
>> + return NULL;
>> + }
>> +
>> + type = virXMLPropString(node, "type");
>> + if (type == NULL) {
>> + def->type = VIR_DOMAIN_NET_FORWARD_TYPE_TCP;
>> + } else if ((def->type = virDomainNetForwardTypeFromString(type)) < 0)
{
>> + virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("unknown forward type '%s'"),
type);
>> + goto error;
>
> If you take this goto, you'll call virDomainNetForwardDefFree on an
> uninitialized def. You should initialize it to NULL.
I am not sure I follow you. Calling virDomainNetForwardDefFree() after
VIR_ALLOC(def) should be okay, no?
You're correct. I guess I was moving over the code too quickly.
> It would be much nicer to make the test work rather than disable
it
> (even for one commit), unless it's really hairy to fix it.
>
>> + <forward type='tcp' host_port='2222'
guest_port='22'/>
>> + <forward type='udp' host='127.0.0.1'
host_port='2242' guest='10.0.2.15' guest_port='42'/>
> Ah, I see. You're adding new XML, so you want it represented in the
> xml2xml tests, but you haven't added the backend, so it will fail the
> xml2argv test? But since unrecognized XML is just ignored, leaving the
> argv file unmodified should do it for you, right?
It will fail because the resulting XML isn't the same, iirc.
(now that I've taken the time to look *carefully*). Okay, I see now that
the problematic test is argv2xml, which of course won't generate the
expected XML, and the test framework isn't setup to allow for a
different xmlout (as is the case for xml2xml), so temporarily disabling
the test really is the most sane thing to do.