search

kdave / btrfs-progs

Development of userspace BTRFS tools
GNU General Public License v2.0
527 stars 239 forks source link

Data csum generation inject #823

Open adam900710 opened 1 week ago

adam900710 commented 1 week ago

@kdave Please allow me to handle this case.

This one includes a fix that should be folded into a merged but yet released commit.

kdave commented 1 week ago

Error injection works only when built with -DINJECT.

adam900710 commented 1 week ago

That's already taken into consideration.

The test would check if injection works for the very first commit transaction, if failed, the test case would be skipped.

kdave commented 1 week ago

If the command fails for another reason then it cannot be distinguished if it's the injection or not, eg. when the injection would be enabled and it still fails.

This points out we need a way to determine if the injections are built in or not by querying the btrfs binary (--help or in btrfs version).

And then enable it in the CI workflow files (probably relevant for all). Right now it needs to be enabled by EXTRA_CFLAGS=-DINJECT, I'll do something more convenient.

kdave commented 1 week ago

I like what eg. systemd does with the --version and feature description:

systemd 255 (255.6+suse.30.g3ea0e1dff3)
+PAM +AUDIT +SELINUX +APPARMOR +IMA -SMACK +SECCOMP +GCRYPT +GNUTLS +OPENSSL +ACL +BLKID +CURL +ELFUTILS +FIDO2 +IDN2 -IDN +IPTC +KMOD +LIBCRYPTSETUP +LIBFDISK +PCRE2 +PWQUALITY +P11KIT +QRENCODE +TPM2 +BZIP2 +LZ4 +XZ +ZLIB +ZSTD +BPF_FRAMEWORK -XKBCOMMON -UTMP +SYSVINIT default-hierarchy=unified

We can do the same as there are several optional build features and it's not unified how this can be found out.

adam900710 commented 1 week ago

Personally speaking, I'd prefer to unify most things (including the SAN and debug) into configure options. This means we will no longer do things at make time (disable D= options), but do it like configure --enable-debug --enable-ubsan etc.

So that we can go the same include/config.h for everything.

adam900710 commented 1 week ago

For now, I will go with better stderr/stdout filter to make sure it's the injection got triggered and wait for a proper feature detection (either all configure or systemd like way).

adam900710 commented 1 week ago

And you only need to enable "D=1" for make, as INJECT is already included there.

kdave commented 2 days ago

The D= could be done in configure too but as it is now it allows to do quick rebuilds of the same configured setup. It does not need to be tracked anywhere once done, then run default build, if it fails then rebuild with D=1 or anything else and check results.

kdave commented 2 days ago

For now, I will go with better stderr/stdout filter to make sure it's the injection got triggered

OK.

kdave commented 2 days ago

The built-in feature list in btrfs version is now in devel.