ami-iit / yarp-openmct

Repo for YARP and OpenMCT integration.
BSD 3-Clause "New" or "Revised" License
6 stars 1 forks source link

Follow-up of pull request #74 #77

Open nunoguedelha opened 2 years ago

nunoguedelha commented 2 years ago

It's worth gathering our discussions in a new issue in order to track the investigation. It should have lower priority than the 3 next fixes in the reference list #33 , but it's worth addressing the raised points.

Originally posted by @nunoguedelha in https://github.com/ami-iit/yarp-openmct/issues/74#issuecomment-965562105


@S-Dafarra : What happens if you put the address of a different machine?

@nunoguedelha : HaHaaaa! I knew you were gonna ask that! I have no idea. There might be internal mechanisms or shenanigans connecting to a remote machine and launching a server. But I doubt it.

It's something worth checking. At the end, if localhost or the machine's self address are the only left choices, then we should only allow those and use a keyword for the machine's self IP, like... selfIPaddress, and resolve it in the Node.js/Javascript scripts.

What do you think?

@S-Dafarra :

At the end, if localhost or the machine's self address are the only left choices, then we should only allow those and use a keyword for the machine's self IP, like... selfIPaddress, and resolve it in the Node.js/Javascript scripts.

Yeah, I posed the question leaving some room for some fancy mechanism of Node.js I did not know about 😄 . But in general, I think we don't need that feature. If we need to do that, we could resort to ssh. Also resolving automatically the address could be dangerous if you have multiple network cards. Can we check if the address we set is actually the address of the machine, and throw a warning otherwise?

If we leave the possibility to the user to specify the address, it is quite probable he/she will mess up. I simply want to reduce that chance.

@nunoguedelha :

But in general, I think we don't need that feature. If we need to do that, we could resort to ssh

Yes it's safer to stay simple and only consider using the address(es) of the machine.

Can we check if the address we set is actually the address of the machine, and throw a warning otherwise? If we leave the possibility to the user to specify the address, it is quite probable he/she will mess up. I simply want to reduce that chance.

Agree, I think it's possible, I have to investigate. We get the possible IP addresses and throw a warning if none of them matches the one set by the user.


@S-Dafarra : Wouldn't it be possible to also get this list directly from the json file?

@nunoguedelha : It's a good point, actually, I intend to simplify this function as well as the update. But I shouldn't need to save a field telling if the entry is already flat or not. It would be simpler to systematically call flatten and do the check in there. Btw, I think it's already possible to do it without additional checks, I have to take a look.

@S-Dafarra : I see. My main point would be to simplify the logic for adding new ports. Having a single json file where you specify what you want would be already nice, but clearly this would be for future works

nunoguedelha commented 2 years ago

Reading https://www.ni.com/en-us/support/documentation/supplemental/11/best-practices-for-using-multiple-network-interfaces--nics--with.html ...