Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed handling of unformatted DASD devices during autoinstallation (fate#313228) #3

Merged
merged 6 commits into from Jan 24, 2014

Conversation

Projects
None yet
4 participants
@jsrain
Copy link
Member

commented Nov 28, 2013

Unfortunately, I could not test the code until having an S/390 build...
Changelog entry comes later

@aschnell

This comment has been minimized.

Copy link

commented on src/modules/DASDController.rb in cddab09 Dec 13, 2013

act_ret will just be the error of the last device. What if something in between goes wrong?

@jsrain

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

I have added FIXMEs on activation error handling - so that overall exist code is returned. It is not that trivial, since a more inteligent error handling would need to be added (in case of multiple disks returning different exit codes).

@kobliha

This comment has been minimized.

Copy link
Member

commented Jan 8, 2014

What's the status of this review?

@jsrain

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2014

I hope that I answered Arvin's question (there was no error handling, so no regression, and doing it properly IMO means proper handling of error and non-error, but still non-zero exit codes - which should be handled separately from this enhancement).

@kobliha

This comment has been minimized.

Copy link
Member

commented Jan 8, 2014

@aschnell See above, what's the verdict?

@aschnell

This comment has been minimized.

Copy link
Member

commented Jan 15, 2014

Is it tested? I don't see any documentation update.

@jsrain

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2014

I have enhanced it in regard of other related features. The testing was only limited, due to the overall status of the S/390 snapshots.

else
DASDController.DeactivateDisk(channel, diag)
end
if act_ret == 8

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

hmm, why eight? constant will be better here

This comment has been minimized.

Copy link
@jsrain

jsrain Jan 23, 2014

Author Member

adding a comment explaining the number

end
if unformatted_disks.size > 0

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

having it in separate method will be much better

end
if unformatted_disks.size > 0
if unformatted_disks.size == 1
popup = Builtins.sformat(_("Device %1 is not formatted. Format device now?"), unformatted_disks[0])

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

I think this is wrong way how to make distinction betweeen various number of devices. _n is better solution. @lslezak should know details

# for autoinst, format unformatted disks later
if (! Mode.autoinst) && Popup.ContinueCancel(popup)
unformatted_disks.each do | channel |
cmd = Builtins.sformat(

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

for new code I prefer pure ruby so cmd = "ls '/sys/bus/ccw/devices/#{channel}/block/' | tr -d '\n'"

"ls '/sys/bus/ccw/devices/%1/block/' | tr -d '\n'",
channel
)
disk = Convert.convert(

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

convert is useless here simple disk = SCR.Execute(path(".target.bash_output"), cmd) is enough

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

and naming result as disk I found quite confusing

:from => "any",
:to => "map <string, any>"
)
if Ops.get_integer(disk, "exit", -1) == 0 &&

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

if disk["exit"] == 0 && !disk["stdout"].empty?

Ops.greater_than( Builtins.size(Ops.get_string(disk, "stdout", "")), 0)
DASDController.FormatDisks(
[Builtins.sformat("/dev/%1", Ops.get_string(disk, "stdout", ""))],
1

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

what mean this 1?

This comment has been minimized.

Copy link
@jsrain

jsrain Jan 23, 2014

Author Member

Looks like this part can have some more optimalization, but I don't dare to look at it until I have a possible testing environment; adding a FIXME

if Ops.get_integer(disk, "exit", -1) == 0 &&
Ops.greater_than( Builtins.size(Ops.get_string(disk, "stdout", "")), 0)
DASDController.FormatDisks(
[Builtins.sformat("/dev/%1", Ops.get_string(disk, "stdout", ""))],

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

simple ["/dev/#{disk["stdout"]}"]

[Builtins.sformat("/dev/%1", Ops.get_string(disk, "stdout", ""))],
1
)
diag = Ops.get(DASDController.diag, channel, false)

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

to get boolean and false if not found usually in ruby is used !!DASDController.diag[channel]

@@ -485,7 +524,15 @@ def DASDDialog

ret = Ops.get_symbol(event, "ID")

if ret == :filter
if ret == :select_all

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

nice option for refactoring to convert it to case statement

This comment has been minimized.

Copy link
@jsrain

jsrain Jan 23, 2014

Author Member

the same, but here the comment below explains it

to_format = Builtins.add(to_format, dev_name) if dev_name != nil
to_format << device
# unformtted disk, manual (not AutoYaS)
elsif act_ret == 8

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

again same 8 or different one?

message = Builtins.sformat(_("Device %1 is not formatted. Format device now?"), unformatted_devices[0])
else
message = BUiltins.sformat(_("There are %1 unformatted devices. Format them now?"), unformatted_devices.size)
end

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jan 23, 2014

Member

It looks like I already see this code :)

This comment has been minimized.

Copy link
@jsrain

jsrain Jan 23, 2014

Author Member

yes, as I stated above, the formatting part would deserve some more love, but I don't want to touch it more than necessary until I have a way to test it (read: working S/390 snapshot)

@jreidinger

This comment has been minimized.

Copy link
Member

commented Jan 23, 2014

I found some ruby issues, but nothing critical, so if you are under pressure by feature freeze you can improve it after it.

@jreidinger

This comment has been minimized.

Copy link

commented on src/include/s390/dasd/dialogs.rb in c96309e Jan 23, 2014

it can create nil, so if you really want only boolean, then you need to add !!

@kobliha

This comment has been minimized.

Copy link
Member

commented Jan 23, 2014

@jreidinger Thanks, we need these S390 features to be done till feature freeze, let's add what needs to be done "better" into, e.g. Trello. Real issues should be fixed before merging, fixing cosmetic issues should be delayed after FF.

@jreidinger

This comment has been minimized.

Copy link
Member

commented Jan 24, 2014

OK, I expect some cleaning when we get proper s390. LGTM

jsrain added a commit that referenced this pull request Jan 24, 2014

Merge pull request #3 from jsrain/master
Changed handling of unformatted DASD devices during autoinstallation (fate#313228)

@jsrain jsrain merged commit 5414957 into yast:master Jan 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.