[Openvpn-devel] openvpnmsica: make adapter renaming non-fatal

Message ID 20200902133755.308-1-lstipakov@gmail.com
State Changes Requested
Headers show
Series
  • [Openvpn-devel] openvpnmsica: make adapter renaming non-fatal
Related show

Commit Message

Lev Stipakov Sept. 2, 2020, 1:37 p.m.
From: Lev Stipakov <lev@openvpn.net>

For some users renaming adapter mysteriously fails
(https://github.com/OpenVPN/openvpn-build/issues/187),

Since renaming is just a a "nice to have", make it not fatail.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpnmsica/openvpnmsica.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Selva Nair Sept. 2, 2020, 1:49 p.m. | #1
Hi

On Wed, Sep 2, 2020 at 9:39 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> For some users renaming adapter mysteriously fails
> (https://github.com/OpenVPN/openvpn-build/issues/187),
>
> Since renaming is just a a "nice to have", make it not fatail.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpnmsica/openvpnmsica.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpnmsica/openvpnmsica.c
> b/src/openvpnmsica/openvpnmsica.c
> index 31e90bd2..d5b3f09e 100644
> --- a/src/openvpnmsica/openvpnmsica.c
> +++ b/src/openvpnmsica/openvpnmsica.c
> @@ -1100,7 +1100,8 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)
>                  dwResult = tap_set_adapter_name(&guidAdapter, szName);
>                  if (dwResult != ERROR_SUCCESS)
>                  {
> -                    tap_delete_adapter(NULL, &guidAdapter,
> &bRebootRequired);
> +                    /* failed renaming is not a fatal error, continue */
> +                    dwResult = ERROR_SUCCESS;
>

This looks strange. If we are going to rewrite dwResult, why set it and
check it at all, just call
tap_set_adapter() without storing the return value and add the comment.

Or, better, print a warning message saying the rename failed.

Selva


>                  }
>              }
>          }
> --
> 2.17.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 2, 2020 at 9:39 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
<br>
For some users renaming adapter mysteriously fails<br>
(<a href="https://github.com/OpenVPN/openvpn-build/issues/187" rel="noreferrer" target="_blank">https://github.com/OpenVPN/openvpn-build/issues/187</a>),<br>
<br>
Since renaming is just a a &quot;nice to have&quot;, make it not fatail.<br>
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
 src/openvpnmsica/openvpnmsica.c | 3 ++-<br>
 1 file changed, 2 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c<br>
index 31e90bd2..d5b3f09e 100644<br>
--- a/src/openvpnmsica/openvpnmsica.c<br>
+++ b/src/openvpnmsica/openvpnmsica.c<br>
@@ -1100,7 +1100,8 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)<br>
                 dwResult = tap_set_adapter_name(&amp;guidAdapter, szName);<br>
                 if (dwResult != ERROR_SUCCESS)<br>
                 {<br>
-                    tap_delete_adapter(NULL, &amp;guidAdapter, &amp;bRebootRequired);<br>
+                    /* failed renaming is not a fatal error, continue */<br>
+                    dwResult = ERROR_SUCCESS;<br></blockquote><div><br></div><div>This looks strange. If we are going to rewrite dwResult, why set it and check it at all, just call</div><div>tap_set_adapter() without storing the return value and add the comment.</div><div><br></div><div>Or, better, print a warning message saying the rename failed.</div><div><br></div><div>Selva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
                 }<br>
             }<br>
         }<br>
-- <br>
2.17.1<br>
<br>
<br>
<br>
_______________________________________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank">Openvpn-devel@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br>
</blockquote></div></div>
Lev Stipakov Sept. 2, 2020, 1:53 p.m. | #2
Hi,

>>                  if (dwResult != ERROR_SUCCESS)
>>                  {
>> -                    tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired);
>> +                    /* failed renaming is not a fatal error, continue */
>> +                    dwResult = ERROR_SUCCESS;
>
>
> This looks strange. If we are going to rewrite dwResult, why set it and check it at all, just call
> tap_set_adapter() without storing the return value and add the comment.

I think this is good for documentation purposes - that we are aware
that function may fail, but in this case it is not fatal and we continue.

> Or, better, print a warning message saying the rename failed.

Warning is printed inside tap_delete_adapter().
Selva Nair Sept. 2, 2020, 2:22 p.m. | #3
Hi

On Wed, Sep 2, 2020 at 9:54 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> Hi,
>
> >>                  if (dwResult != ERROR_SUCCESS)
> >>                  {
> >> -                    tap_delete_adapter(NULL, &guidAdapter,
> &bRebootRequired);
> >> +                    /* failed renaming is not a fatal error, continue
> */
> >> +                    dwResult = ERROR_SUCCESS;
> >
> >
> > This looks strange. If we are going to rewrite dwResult, why set it and
> check it at all, just call
> > tap_set_adapter() without storing the return value and add the comment.
>
> I think this is good for documentation purposes - that we are aware
> that function may fail, but in this case it is not fatal and we continue.
>

That's achieved also by just adding a comment that we don't check the
return value as failure is not fatal. Anyway, this works too.


>
> > Or, better, print a warning message saying the rename failed.
>
> Warning is printed inside tap_delete_adapter().
>

tap_delete_adapter() is not called here. I was suggesting that if we do
check the return value, let us also print a  warning that renaming failed.
That can't be  done anywhere except just before converting the error
to SUCCESS.

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 2, 2020 at 9:54 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
&gt;&gt;                  if (dwResult != ERROR_SUCCESS)<br>
&gt;&gt;                  {<br>
&gt;&gt; -                    tap_delete_adapter(NULL, &amp;guidAdapter, &amp;bRebootRequired);<br>
&gt;&gt; +                    /* failed renaming is not a fatal error, continue */<br>
&gt;&gt; +                    dwResult = ERROR_SUCCESS;<br>
&gt;<br>
&gt;<br>
&gt; This looks strange. If we are going to rewrite dwResult, why set it and check it at all, just call<br>
&gt; tap_set_adapter() without storing the return value and add the comment.<br>
<br>
I think this is good for documentation purposes - that we are aware<br>
that function may fail, but in this case it is not fatal and we continue.<br></blockquote><div><br></div><div>That&#39;s achieved also by just adding a comment that we don&#39;t check the</div><div>return value as failure is not fatal. Anyway, this works too.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; Or, better, print a warning message saying the rename failed.<br>
<br>
Warning is printed inside tap_delete_adapter().<br></blockquote><div><br></div><div>tap_delete_adapter() is not called here. I was suggesting that if we do </div><div>check the return value, let us also print a  warning that renaming failed. </div><div>That can&#39;t be  done anywhere except just before converting the error</div><div>to SUCCESS.</div><div><br></div><div>Selva</div></div></div>
Lev Stipakov Sept. 2, 2020, 2:53 p.m. | #4
Hi,

>> Warning is printed inside tap_delete_adapter().
>
> tap_delete_adapter() is not called here.

Right, I managed to mistype tap_set_adapter_name().

> I was suggesting that if we do check the return value, let us also print a warning
> that renaming failed. That can't be done anywhere except just before converting the error
> to SUCCESS.

I realized that warning inside tap_set_adapter_name() also
displays a messagebox, which we probably don't want to show.

I'll send a v2 which prints warning and doesn't display a messagebox.

Patch

diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c
index 31e90bd2..d5b3f09e 100644
--- a/src/openvpnmsica/openvpnmsica.c
+++ b/src/openvpnmsica/openvpnmsica.c
@@ -1100,7 +1100,8 @@  ProcessDeferredAction(_In_ MSIHANDLE hInstall)
                 dwResult = tap_set_adapter_name(&guidAdapter, szName);
                 if (dwResult != ERROR_SUCCESS)
                 {
-                    tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired);
+                    /* failed renaming is not a fatal error, continue */
+                    dwResult = ERROR_SUCCESS;
                 }
             }
         }