vagrant-landrush / landrush

A Vagrant plugin that provides a simple DNS server for Vagrant guests
MIT License
668 stars 80 forks source link

Fix landrush not working if executed as admin #357

Open spocky opened 4 years ago

spocky commented 4 years ago

Issue

On Windows 10, running a vagrant up on an admin (power)shell crashes with OpenKey': The system cannot find the file specified. in update_network_adapter (as described here)

Cause

It seems that get_network_guid couldn't find the right network adapter while enumerating the registry (and indeed, no adapter has the required address, which might be caused by an underlying issue I've not explored). In that special case, the return value is the size of the interfaces array (17 in my case), which makes update_network_adapter try to open registry address SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces\17 and crash.

Fix

Returning nil if the value couldn't be found in the adapters array, will fix the issue as update_network_adapter will then proceed to manual configuration and won't try to open an inexistent registry key.

davividal commented 4 years ago

Could you write one test for that? The windows laptop I used to test my changes is unaccessible thanks to corona.

Your description make sense, but I'm not really sure that simply returning nil will solve that. As far as I can remember, when a method does not have an explicit return, it will return nil.

irb(main):001:0> def foo()
irb(main):002:1>   end
=> :foo
irb(main):003:0> a = foo()
=> nil
irb(main):004:0> a
=> nil

The linked issue seems like an exception to me. So maybe we just need to add a rescue clause?

spocky commented 3 years ago

Sorry for the late reply :/

Could you write one test for that? The windows laptop I used to test my changes is unaccessible thanks to corona.

Well, I would if I knew how to do it. To be honest, I'm not a ruby developer and the first time I ever opened a ruby file was to debug this error :)

Your description make sense, but I'm not really sure that simply returning nil will solve that. As far as I can remember, when a method does not have an explicit return, it will return nil.

I understand and due to my inexperience, I obviously can't discuss how this works internally, but this nil fixes the error I had. It's as if without it, the returned value was the array size.

Reading this tutorial, it seems like :

If we don’t do anything else, then a method will return the value that was returned from the last evaluated statement.

In our case, that would probably be the 'interfaces' array (or its size, in order to loop in the 'do'), which does totally explain the error I had.

The linked issue seems like an exception to me. So maybe we just need to add a rescue clause?

Wouldn't this be catched by the "rescue StandardError" (I'm assuming it's like a generic "catch(Exception e)") just below ?

davividal commented 3 years ago

Your description make sense, but I'm not really sure that simply returning nil will solve that. As far as I can remember, when a method does not have an explicit return, it will return nil.

I understand and due to my inexperience, I obviously can't discuss how this works internally, but this nil fixes the error I had. It's as if without it, the returned value was the array size.

Reading this tutorial, it seems like :

If we don’t do anything else, then a method will return the value that was returned from the last evaluated statement.

In our case, that would probably be the 'interfaces' array (or its size, in order to loop in the 'do'), which does totally explain the error I had.

Ops! You are right! I was thinking about other languages when I wrote this.

KuiKui commented 3 years ago

@davividal Can you merge and tag this PR ? 🙏

davividal commented 3 years ago

@KuiKui no, I can't. @hferentschik can you? :)

KuiKui commented 2 years ago

Hi @hferentschik, Can you read/merge this PR, please ? 🙏