[Bug 1209162] New: yast2-s390: wrong management if DIAG in some situations
https://bugzilla.suse.com/show_bug.cgi?id=1209162 Bug ID: 1209162 Summary: yast2-s390: wrong management if DIAG in some situations Classification: openSUSE Product: openSUSE Tumbleweed Version: Current Hardware: S/390 OS: openSUSE Tumbleweed Status: NEW Severity: Normal Priority: P5 - None Component: YaST2 Assignee: yast2-maintainers@suse.de Reporter: ancor@suse.com QA Contact: jsrain@suse.com Found By: --- Blocker: --- Disclaimer: I found this bug by reading the source code of yast2-s390. Unfortunately I don't have access to an s/390 system with DIAG-capable devices, so I cannot confirm any of my observations or assertions. First of all, let's take a look to how the options "Set DIAG On" and "Set DIAG off" work in yast2-s390. I find the behavior very weird, but I can confirm by reading the code that it's indeed intentional. 1) When the value of the DIAG flag is changed in YaST for an enabled device, the change is done immediately. That implies disabling the device and enabling it again with the new value. 2) When the flag is changed for a disabled device, the new desired value is stored by YaST in memory but not written to the system configuration. The change will only have effect if the device is enabled afterwards via YaST during the same YaST execution (the change is lost if YaST quits without ever enabling the device). 3) In the UI, DIAG is always displayed as 'no' for disabled devices. For enabled devices the correct/current value is displayed. And now, let me describe the bug which I believe was introduced in this change (remember I diagnosed the problem by reading the code, with no empirical confirmation): https://github.com/yast/yast-s390/pull/93 I believe that now YaST forgets the desired value of the DIAG flag for disabled DASDs in some situation. Let me illustrate it with two possible scenarios. Scenario a) When YaST starts, a given DASD is already enabled and with DIAG=1 YaST is used to disable it YaST is used to enable it again Before the mentioned change (eg. Leap 15.3) the device is enabled with DIAG=1 After the change (eg. Leap 15.4) it's enabled with DIAG=0 Scenario b) When YaST starts, there is a disabled DASD YaST is used to "Set Diag On" The, YaST is used to enable the device Before the change (eg. Leap 15.3), the device is enabled with DIAG=1 After the change (eg. Leap 15.4), it's enabled with DIAG=0 I believe the root of the evil resides in two things: - The attribute Dasd#diag_wanted is not properly initialized - The mentioned attribute is reset to nil after every operation -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 Ancor Gonzalez Sosa <ancor@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|yast2-s390: wrong |yast2-s390: wrong |management if DIAG in some |management of DIAG in some |situations |situations -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c1 --- Comment #1 from Stefan Hundhammer <shundhammer@suse.com> --- I read through the sources to understand what all that code does, and I can't claim that I was very successful. But on that way, I came across this: https://github.com/yast/yast-s390/blob/master/src/include/s390/dasd/dialogs.... dasd.format_watend = format Notice "watend", not "wanted". That can't be right. -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c2 --- Comment #2 from Stefan Hundhammer <shundhammer@suse.com> --- https://github.com/yast/yast-s390/blob/master/src/lib/y2s390/dasds_reader.rb...
def use_diag?(dasd) diag_file = "/sys/#{dasd.sysfs_id}/device/use_diag" use_diag = Yast::SCR.Read(Yast.path(".target.string"), diag_file) if File.exist?(diag_file) use_diag.to_i == 1 end
If the file does not exist, "use_diag" will remain nil, and "use_diag.to_i" will throw a nil.NilClass exception because "use_diag.to_i" will evaluate to "nil.to_i". -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c3 Stefan Hundhammer <shundhammer@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kanderssen@suse.com Flags| |needinfo?(kanderssen@suse.c | |om) --- Comment #3 from Stefan Hundhammer <shundhammer@suse.com> --- Knut, please have a look. Ancor couldn't really do actual testing, he came to those conclusions from reading the source code. I also read it, and I was a bit overwhelmed by the complexity of setting such a simple flag. Please also check out the small issues that I found; maybe they are important, maybe they are not. If you test that DIAG and find that it works as expected, so much the better. But somebody who knows his way around s/390 DASDs should really give this a try. -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c4 --- Comment #4 from Knut Alejandro Anderssen Gonz�lez <kanderssen@suse.com> --- (In reply to Stefan Hundhammer from comment #2)
https://github.com/yast/yast-s390/blob/master/src/lib/y2s390/dasds_reader. rb#L172-L176
def use_diag?(dasd) diag_file = "/sys/#{dasd.sysfs_id}/device/use_diag" use_diag = Yast::SCR.Read(Yast.path(".target.string"), diag_file) if File.exist?(diag_file) use_diag.to_i == 1 end
If the file does not exist, "use_diag" will remain nil, and "use_diag.to_i" will throw a nil.NilClass exception because "use_diag.to_i" will evaluate to "nil.to_i".
This was already expected and it will be evaluated to 0, which is not enabled -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c5 Knut Alejandro Anderssen Gonz�lez <kanderssen@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(kanderssen@suse.c | |om) | --- Comment #5 from Knut Alejandro Anderssen Gonz�lez <kanderssen@suse.com> --- (In reply to Stefan Hundhammer from comment #3)
Knut, please have a look.
Ancor couldn't really do actual testing, he came to those conclusions from reading the source code.
I also read it, and I was a bit overwhelmed by the complexity of setting such a simple flag. Please also check out the small issues that I found; maybe they are important, maybe they are not.
If you test that DIAG and find that it works as expected, so much the better. But somebody who knows his way around s/390 DASDs should really give this a try.
We were testing it together as part of the support of DASD management devices in the installer. Unfortunately we do not have a machine with diagnose access, so will try to ask Nikolay and Ihno for testing it and help us. -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 Stefan Hundhammer <shundhammer@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nikolay.gueorguiev@suse.com Flags| |needinfo?(nikolay.gueorguie | |v@suse.com) -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 Stefan Hundhammer <shundhammer@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(nikolay.gueorguie | |v@suse.com) | -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 Stefan Hundhammer <shundhammer@suse.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CONFIRMED URL| |https://trello.com/c/Kbl9wi | |nE Assignee|yast2-maintainers@suse.de |yast-internal@suse.de -- You are receiving this mail because: You are on the CC list for the bug.
https://bugzilla.suse.com/show_bug.cgi?id=1209162 https://bugzilla.suse.com/show_bug.cgi?id=1209162#c6 --- Comment #6 from Stefan Hundhammer <shundhammer@suse.com> --- Moving to Trello. -- You are receiving this mail because: You are on the CC list for the bug.
participants (1)
-
bugzilla_noreply＠suse.com