Bug #64040
open
PGBackend unhandled throw exceptions
Added by Matan Breizman 4 months ago.
Updated 2 months ago.
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);
- Description updated (diff)
- Tags set to good-first-issue
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.
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.
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.
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 ...
Need to understand the difference between
PGBackend::interruptible_future<> PGBackend::write_same(..)
and
PGBackend::write_iertr::future<>·PGBackend::writefull( ... )
Also available in: Atom
PDF