Skip to content
This repository has been archived by the owner on Jun 25, 2023. It is now read-only.

Fix landrush not working if executed as admin #357

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spocky
Copy link

@spocky spocky commented Apr 23, 2020

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
Copy link
Collaborator

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
Copy link
Author

spocky commented Oct 8, 2020

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
Copy link
Collaborator

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
Copy link

KuiKui commented Dec 7, 2020

@davividal Can you merge and tag this PR ? 🙏

@davividal
Copy link
Collaborator

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

@KuiKui
Copy link

KuiKui commented Aug 23, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants