| Message ID | 20200902133755.308-1-lstipakov@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] openvpnmsica: make adapter renaming non-fatal | expand |
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 <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>> 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 <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><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 "nice to have", make it not fatail.<br> <br> Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><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(&guidAdapter, szName);<br> if (dwResult != ERROR_SUCCESS)<br> {<br> - tap_delete_adapter(NULL, &guidAdapter, &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>
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().
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 <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> 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> >> if (dwResult != ERROR_SUCCESS)<br> >> {<br> >> - tap_delete_adapter(NULL, &guidAdapter, &bRebootRequired);<br> >> + /* failed renaming is not a fatal error, continue */<br> >> + dwResult = ERROR_SUCCESS;<br> ><br> ><br> > This looks strange. If we are going to rewrite dwResult, why set it and check it at all, just call<br> > 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's achieved also by just adding a comment that we don'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> > 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't be done anywhere except just before converting the error</div><div>to SUCCESS.</div><div><br></div><div>Selva</div></div></div>
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.
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; } } }