[ovs-dev] [PATCH v2] datapath-windows: Update port property

[Nithin Raju]

hi Eitan,
Thanks for the change. I had a few minor comments. I should be able to ACK a v3.

> +    /* Update properties only for NETDEV ports*/
> +    if (vport == NULL || vport->ovsType != OVS_VPORT_TYPE_NETDEV) {
> +        status = STATUS_DATA_NOT_ACCEPTED;
> +        goto update_port_done;
> +    }

For the HV-internal port that 'ovsType' is going to be OVS_VPORT_TYPE_INTERNAL. So, we'll have to accept that port as well. A better variable to check might be:
if (vport == NULL || vport->hvDeleted) {
    status = STATUS_DATA_NOT_ACCEPTED;
...

I was thinking of renaming 'hvDeleted' to something like 'presentOnHv'. If you want to do it as part of this change, that is fine.

> +
> +    /* Store the nic and the OVS states as Nic Create won't be called */
> +    ovsState = vport->ovsState;
> +    nicState = vport->nicState;
> +    /* Currently only the port friendly name is being updated */
> +    OvsInitVportWithPortParam(vport, portParam);

Can we ascertain that some of the key data members have not changed? Eg. vport->portId, vport->portType, and vport->portState. I have a feeling that we should white-list the properties that are allowed to change and then copy them rather than calling OvsInitVportWithPortParam().

Also, like Ankur pointed out, what does it mean if the Hyper-V name of a port changes while there's a OVS port attached to it? While the vport changes were being done, I was of the opinion that we should not allow the HV friendly name to change while OVS port is attached. Practically it may not matter, but can lead to confusion.

That said, I am ok with allowing the friendly name to change, provided it is well-documented. So, perhaps, you can document this somewhere.

thanks,
Nithin