Bug 1221222

AFAICS that's the libstorage-ng bindings.

-> Arvin

AFAIS the bindings work. I have added an example using create_lvm_lv
and that works with swig 4.2.1:

https://github.com/openSUSE/libstorage-ng/blob/master/bindings/ruby/examples/lvm.rb

I assume YaST passes the wrong types to create_lvm_lv. Maybe the
new swig version is more strict in that regard (the changelog addition
itself is several hundreds lines long so I cannot say for sure).
In the YaST code for me it is unclear where the conversion from
DiskSize to unsigned long long is done.

This does not at all look like a generic problem with DiskSize; in that entire build log only two methods are failing:

> % grep 'Wrong arguments for overloaded method' build.log | perl -pe 's/^\s*//' | sort -u
> Wrong arguments for overloaded method 'Lvm.Vg.create_lvm_lv'.
> Wrong arguments for overloaded method 'LvmVg.create_lvm_lv'.


...which begs the question: What is special about those two methods?

In many of the failed tests, I see this:

Failure/Error: before { devicegraph_stub("trivial_lvm_and_other_partitions") }

where that YAML file yast-storage-ng/test/data/devicegraphs/trivial_lvm_and_other_partitions.yml has this section about the LV:


- lvm_vg:
    vg_name: vg0
    lvm_pvs:
        - lvm_pv:
            blk_device: /dev/sda1

    lvm_lvs:
        - lvm_lv:
            size:         unlimited
            lv_name:      lv1
            file_system:  btrfs
            mount_point:  /


The FakeDeviceFactory which is used throughout the yast-storage-ng unit tests checks various keys that it receives from a YAML file, e.g. "size", and converts the corresponding value to a DiskSize object. The special value "unlimited" is translated to -1 which means "unlimited".

But it's -1 which certainly cannot be converted to an unsigned long long.


I guess in this context it means "take everything that is available", i.e. the sum of all associated PVs. This used to work before, and the code is quite trivial:

https://github.com/yast/yast-storage-ng/blob/master/src/lib/y2storage/fake_device_factory.rb#L772 

  lvm_lv = parent.create_lvm_lv(lv_name, size)

'size' should now be DiskSize("unlimited"), i.e. -1.

'parent' *should* be     

  # @param parent [Object] volume group object

which I hope is a Ruby object here.

But SWIG seems to be confused about that; look closely at the exact alternatives that it proposes:

> storage::LvmLv LvmVg.create_lvm_lv(std::string const &lv_name, storage::LvType lv_type, unsigned long long size)
> storage::LvmLv * LvmVg.create_lvm_lv(std::string const &lv_name, unsigned long long size)


I.e. a non-pointer storage::LvmLv object vs. a storage::LvmLv pointer.

(In reply to Stefan Hundhammer from comment #4)

> https://github.com/yast/yast-storage-ng/blob/master/src/lib/y2storage/
> fake_device_factory.rb#L772 
> 
>   lvm_lv = parent.create_lvm_lv(lv_name, size)
> 
> 'size' should now be DiskSize("unlimited"), i.e. -1.

One entry in the swig changelog includes "Ruby negative number check". Maybe swig
now detects that -1 is not a unsigned long long and thus cannot decide which function
to use.

> But SWIG seems to be confused about that; look closely at the exact
> alternatives that it proposes:
> 
> > storage::LvmLv LvmVg.create_lvm_lv(std::string const &lv_name, storage::LvType lv_type, unsigned long long size)
> > storage::LvmLv * LvmVg.create_lvm_lv(std::string const &lv_name, unsigned long long size)
> 
> I.e. a non-pointer storage::LvmLv object vs. a storage::LvmLv pointer.

Yes, I also noticed the wrong non-pointer.

(In reply to Arvin Schnell from comment #5)

> One entry in the swig changelog includes "Ruby negative number check". Maybe
> swig
> now detects that -1 is not a unsigned long long and thus cannot decide which
> function
> to use.

That seems to be the case. If I pass a size of -1 in the new example
mentioned above it fails with the new swig version. If I compile the
bindings with the old swig version swig does not complain and passes
-1 alias 16 EiB to libstorage-ng.

OK, so this could be fixed with a one-liner in the FakeDeviceFactory: If the requested size for an LV is 'unlimited' (-1), use a huge size instead like 16 EiB.

I guess that only works because somewhere in libstorage-ng this is limited by the available size for that LV, right?

(In reply to Stefan Hundhammer from comment #7)
> OK, so this could be fixed with a one-liner in the FakeDeviceFactory: If the
> requested size for an LV is 'unlimited' (-1), use a huge size instead like
> 16 EiB.
> 
> I guess that only works because somewhere in libstorage-ng this is limited
> by the available size for that LV, right?

No, the volume group is overcommitted in that case. Maybe the result is
not really needed in the testcases.

PR with a fix:

  https://github.com/yast/yast-storage-ng/pull/1374

this will become available as yast2-storage-5.0.9.

New PR since the previous one did not find agreement:

  https://github.com/yast/yast-storage-ng/pull/1375

this will become available as yast2-storage-5.0.9.

Merged PR #1375, but Jenkins is down right now. An auto-submission will follow as soon as it's back up.

Submitted manually to OBS Factory:

https://build.opensuse.org/request/show/1157596

PR for SLE-15-SP6:

  https://github.com/yast/yast-storage-ng/pull/1378

SR to IBS for SLE-15-SP6:

  https://build.suse.de/request/show/325354