On 12/5/19 1:11 PM, Josef Reidinger wrote:

V Thu, 5 Dec 2019 12:08:40 +0000 David Díaz napsáno:

...

Hi folks!

Yesterday, we got a bug report about a crash at the moment to save the yast2-ftp-server configuration when "Allowing for anonymous upload"

  ...   Caller: /usr/share/YaST2/modules/FtpServer.rb:522:in 'WriteUpload'   Details: undefined method 'mkdir' for #Yast::FileUtilsClass:0x00001002ce2a2c0   ...

After realizing that, probably, the code was written with the FileUtils Ruby module[1] in mind but the Yast::FileUtils module[2] is being called instead, two questions came two my mind

- How much time has been this broken without nobody noticing/reporting it?

  The change was introduced exactly a year ago[3], do it means that the yast2-ftp-server is no longer relevant?

Well, I use that module sometimes, but usually to just setup quickly ftp server, so not so often.

...

- Do we have this problem in more places? I.e. Yast::FileUtils taking the place of Object::FileUtils.

Yes, we had it in past, e.g. String versus Yast::String is also problem and some others. My common recommendation is to always use `::` prefix when you want to use ruby functionality and avoid collision with YaST namespace. Other long term solution on which we are working is to have code under 'src/lib` that does not live in YaST namespace, so for YaST specific calls like Yast::FileUtils you have to use full path.

...

I don't have the answer for any of them ;) but I have a proposal[4] for the last one: to implement the `method_missing` in the Yast::FileUtils and fallback to Object::FileUtils. That way, we could avoid similar crashes.

Well, it helps in this one specific case. I agree it is a bit robust, but can lead to another bugs like If you want to use FileUtils.Exist and use instead FileUtils.exists? which is very similar but later does not respect changed SCR target. and some other methods that just differs in case ( which was already bad for old YCP code which has various mixtures ). Also it does not help for other modules. Also I am not so big fan of this magic as it can often lead to WTF when you read code and wonder how it can work when you see that it calls wrong object. But I am also not strong against as I see some value in it.

Josef

...

What do you think?

[1] https://ruby-doc.org/stdlib-2.6.5/libdoc/fileutils/rdoc/FileUtils.html [2] https://github.com/yast/yast-yast2/blob/master/library/general/src/modules/F... [3] https://github.com/yast/yast-ftp-server/pull/54 [4] https://github.com/yast/yast-yast2/pull/997

I would also avoid to use meta-programming magic like that. Sooner than later it will bite your ass ;). Moreover (and even though it is not published in any official place yet) we have some namespace conventions for the new code [1], for example Y2Storage, Y2ServicesManager, Y2Firewall, Y2Network, etc. So the new code will not live under Yast namespace anymore and it will not be directly affected by this problem. For older code, I would follow the Josef's suggestion: to use the scope resolution operator to always start from the global scope (e.g., ::FileUtils). [1] https://gist.github.com/imobachgs/d90940315bd0471ee00b2a68ba22fbe5 -- José Iván López González YaST Team at SUSE LINUX GmbH IRC: jilopez -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Thu, 2019-12-05 at 15:23 +0000, José Iván López González wrote:

I would also avoid to use meta-programming magic like that. Sooner than later it will bite your ass ;). Moreover (and even though it is not published in any official place yet) we have some namespace conventions for the new code [1], for example Y2Storage, Y2ServicesManager, Y2Firewall, Y2Network, etc. So the new code will not live under Yast namespace anymore and it will not be directly affected by this problem.

For older code, I would follow the Josef's suggestion: to use the scope resolution operator to always start from the global scope (e.g., ::FileUtils).

I usually try to avoid meta-programming, but in this case it does not look that dangerous to me :) It is true that it does not make sense for the new code, good point. On the other hand, it could save us (and our users) from some crashes. However, I guess that as a first step we should do a quick research about the usage of FileUtils in the current code base and then decide how to proceed (if we decide to do something). The time between the end of this sprint and the start of the holidays looks like a good moment to me :) Regards, Imo -- Imobach González Sosa YaST Team at SUSE LINUX GmbH https://imobachgs.github.io/

On 12/5/19 4:53 PM, Ladislav Slezak wrote:

Hi all,

Dne 05. 12. 19 v 13:08 David Díaz napsal(a):

...

- How much time has been this broken without nobody noticing/reporting it?

The change was introduced exactly a year ago[3], do it means that the yast2-ftp-server is no longer relevant?

It's one of those rarely used modules. This quite surely proves that the module is not used and could be possibly dropped.

...

- Do we have this problem in more places? I.e. Yast::FileUtils taking the place of Object::FileUtils.

We had the same problem already several times at different modules.

...

I don't have the answer for any of them ;) but I have a proposal[4] for the last one: to implement the `method_missing` in the Yast::FileUtils and fallback to Object::FileUtils. That way, we could avoid similar crashes.

Um, I do not like that. I'd rather prefer a visible crash than doing some magic in the background which might silently fail or do something slightly different.

As Josef pointed out fixing one case won't help much and the meaning is a bit different so it could cause some nasty surprise.

We should rather fix the original bugs than adding fragile workarounds...

I did a quick grep over the YaST sources to find the potentially dangerous places using this regexp:

  [^:]FileUtils\.[[:lower:]]

This finds all FileUtils (from the Ruby stdlib) calls which do not use the :: prefix. (Fortunately all Yast::FileUtils methods start with an upper case letter, that makes the search easier.)

So to avoid the problems in the future we could fix all places and use the global :: name space prefix at these places.

What do you think about that? There are just 29 places in 9 packages which would need to be fixed (see the attachment, I have filtered out the unit tests and some repositories like the AY integration tests).

That should not be that difficult. Volunteers? ;-)

--

Best Regards

Ladislav Slezák Yast Developer ------------------------------------------------------------------------ SUSE LINUX, s.r.o.                              e-mail: lslezak@suse.cz Lihovarská 1060/12                              tel: +420 284 028 960 190 00 Prague 9                                 fax: +420 284 028 951 Czech Republic                                  http://www.suse.cz/

Thanks Ladislav for the research! I would fix all that 29 occurrences and IMHO it deserves a trello card ;) -- José Iván López González YaST Team at SUSE LINUX GmbH IRC: jilopez -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org