Bug #64040
openPGBackend unhandled throw exceptions
0%
Description
See `PGBackend::interruptible_future<> PGBackend::write_same()`:
if (op.writesame.data_length == 0 ||
len % op.writesame.data_length != 0 ||
op.writesame.data_length != osd_op.indata.length()) {
throw crimson::osd::invalid_argument();
}
any catch-throw usage should probably be replaced by errorator instead and handling the error should result in replying
the correct error message back. This is being tested for example in NeoRadosMisc.WriteSame and NeoRadosIo.Limits (which are now skipped):
// Write length must be a multiple of the pattern length
co_await expect_error_code(execute(oid, WriteOp{}
.writesame(0, samelen - 1, patbl)),
sys::errc::invalid_argument);
Updated by Jose J Palacios Perez about 2 months ago
I was curious about this, and having a look at src/crimson/common/errorator.h (which looks quite scary), particularly at invarg (within ct_error), and make_exception_future() method from the class futurize. Will continue to looking to try understand better, and also if I can find some examples.
Updated by Jose J Palacios Perez about 2 months ago
Looking at the same source (src/crimson/osd/pg_backend.cc), there are uses of ct_error() instead of throw exception, for example
PGBackend::write_iertr::future<>·PGBackend::writefull( ... )
:
if·(op.extent.length·!=·osd_op.indata.length())·{
return·crimson::ct_error::invarg::make();$
}
:
so I think that is the intention of the fix. I'd prepare a patchset shortly.
Updated by Jose J Palacios Perez about 2 months ago
I think for those methods with signature XX::interruptible_future<> we can replace a throw by the corresponding crimson::ct_error, I am looking at eg
PGBackend::ll_read_ierrorator::future<>
PGBackend::omap_get_keys(
:
)
:
try·{$
..auto·p·=·osd_op.indata.cbegin();$
..decode(start_after,·p);$
..decode(max_return,·p);$
}·catch·(buffer::error&)·{$
..throw·crimson::osd::invalid_argument{};
}$
which ll_read_ierrorator is ::crimson::interruptible::interruptible_errorator<...>, so I wonder if for the unit test such method that can throw an exception?
will continue looking for further instances where the replace is required.
Updated by Jose J Palacios Perez about 2 months ago
Nope, that didn't work:
/ceph/src/crimson/osd/pg_backend.cc: In member function ‘PGBackend::interruptible_future<> PGBackend::write_same(ObjectState&, const OSDOp&, ceph::os::Transaction&, osd_op_params_t&, object_stat_sum_t&)’:
/ceph/src/crimson/osd/pg_backend.cc:751:43: error: could not convert
‘crimson::unthrowable_wrapper<const std::error_code&, ((const std::error_code&)(& crimson::ec<22>))>::make()’
from
‘const crimson::unthrowable_wrapper<const std::error_code&, ((const std::error_code&)(
& crimson::ec<22>))>’
to
‘PGBackend::interruptible_future<>’ {aka ‘crimson::interruptible::interruptible_future_detail<crimson::osd::IOInterruptCondition, seastar::future<> >’}
751 | return crimson::ct_error::invarg::make();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
| |
| const crimson::unthrowable_wrapper<const std::error_code&, ((const std::error_code&)(& crimson::ec<22>))>
will continue looking ...
Updated by Jose J Palacios Perez about 2 months ago
Need to understand the difference between
PGBackend::interruptible_future<> PGBackend::write_same(..)
and
PGBackend::write_iertr::future<>·PGBackend::writefull( ... )