Fix restoring SCR root (bsc#1207968)

jreidinger commented 1 year ago

Problem

The issue is that bootloader ( yeah, not network ) crash with calling method on nil. Reason why it gets nil is that it failed to find /etc/sysconfig/bootloader. In logs I see several other failed reads e.g. to /etc/sysconfig/kdump. When manually reproduce it I notice that files are there. So only idea is switched SCR. So I check when it was last changed and it was in save_network to local one, but not back. So problem is that network switch SCR to '/' and do not restore it back.

Solution

Quickly from logs it is obvious that issue is at return from block at https://github.com/yast/yast-network/blob/master/src/lib/network/clients/save_network.rb#L98 where I though that return is from method and replace with break will fix it. But to be sure I write also test program. Another part is issue is that on_local method is not robust enough and e.g. if exception is returned it also do not get restore SCR chroot at https://github.com/yast/yast-network/blob/master/src/lib/network/clients/save_network.rb#L74 .

So to verify this ruby behavior I create testing program:

def test(&block)
  puts "pre block"

  block.call

  puts "post block"
ensure
  puts "ensured block"
end

def a
  test do
    puts "in block"
    return
  end
end

def b
  test do
    puts "in block"
    break
  end
end

def c
  test do
    puts "in block"
  end
end

puts "method a"
a
puts
puts "method b"
b
puts
puts "method c"
c

and get following output ( on both ruby 2.5 and ruby 3.1.2 ):

method a
pre block
in block
ensured block

method b
pre block
in block
ensured block

method c
pre block
in block
post block
ensured block

So conclusion for me is that replacing return with break does not help with skipping calls after block.call which really surprise me ( feel free to enlighten me why ). So only correct solution ( that is correct also to handle properly exceptions ) is to enclose restore of SCR to ensure block.

Testing

~~Added a new unit test~~ sadly save_network unit test is so messy and overmocked that I failed to test new behavior
Tested manually (thanks lada for yupdate, test was easy with using this branch )

coveralls commented 1 year ago

Coverage: 80.438%. Remained the same when pulling b2d82923f96cd8f6247b2a7f120db63e6a07e459 on fix_closing_scr into 6c667c3ab6276ce9da61a7d977e9d7c6ebba7274 on master.

yast-bot commented 1 year ago

:heavy_check_mark: Public Jenkins job #321 successfully finished :heavy_check_mark: Created OBS submit request #1064238

yast-bot commented 1 year ago

:heavy_check_mark: Internal Jenkins job #216 successfully finished :heavy_check_mark: Created IBS submit request #289752

yast / yast-network