On 05/09/2018 06:58 PM, Daniel Kiper wrote:
> On Tue, Apr 24, 2018 at 09:13:16PM +0200, Goffredo Baroncelli wrote:
>> The code origins from "raid6_recovery.c". The code was changed because the
>> original one assumed that both the disk address and size are multiple of
>> GRUB_DISK_SECTOR_SIZE. This is not true for grub btrfs driver.
>>
>> The code was made more generalized using a function pointer for reading
>> the data from the memory or disk.
>>
>> Signed-off-by: Goffredo Baroncelli <kreij...@inwind.it>
>> ---
>> grub-core/fs/btrfs.c | 211 +++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 204 insertions(+), 7 deletions(-)
>>
>> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
>> index 5c76a68f3..195313a31 100644
>> --- a/grub-core/fs/btrfs.c
>> +++ b/grub-core/fs/btrfs.c
>> @@ -30,6 +30,7 @@
>> #include <grub/i18n.h>
>> #include <grub/btrfs.h>
>> #include <grub/crypto.h>
>> +#include <grub/diskfilter.h>
>>
>> GRUB_MOD_LICENSE ("GPLv3+");
>>
>> @@ -695,19 +696,215 @@ rebuild_raid5 (struct raid56_buffer *buffers,
>> grub_uint64_t nstripes,
>> csize);
>> }
>>
>> +
>> +/* copied from raid6_recover */
>> +/* x**y. */
>> +static grub_uint8_t powx[255 * 2];
>> +/* Such an s that x**s = y */
>> +static unsigned powx_inv[256];
>> +static const grub_uint8_t poly = 0x1d;
>
> Could you define this as a constant?
In the original code from raid6_recover.c is the same. I prefer to not diverge
too much.
>
>> +static void
>> +grub_raid_block_mulx (unsigned mul, char *buf, grub_size_t size)
>> +{
>> + grub_size_t i;
>> + grub_uint8_t *p;
>> +
>> + p = (grub_uint8_t *) buf;
>> + for (i = 0; i < size; i++, p++)
>> + if (*p)
>> + *p = powx[mul + powx_inv[*p]];
>> +}
>> +
>> +static void
>> +grub_raid6_init_table (void)
>> +{
>> + unsigned i;
>> +
>> + grub_uint8_t cur = 1;
>> + for (i = 0; i < 255; i++)
>> + {
>> + powx[i] = cur;
>> + powx[i + 255] = cur;
>> + powx_inv[cur] = i;
>> + if (cur & 0x80)
>> + cur = (cur << 1) ^ poly;
>> + else
>> + cur <<= 1;
>> + }
>> +}
>
> Could not we do this as a static? It is initialized only once.
Ditto
>
>> +static unsigned
>> +mod_255 (unsigned x)
>> +{
>> + while (x > 0xff)
>> + x = (x >> 8) + (x & 0xff);
>> + if (x == 0xff)
>> + return 0;
>> + return x;
>> +}
>> +
>> +typedef grub_err_t (* raid_recover_read_t)(void *data, int disk_nr,
>> + grub_uint64_t addr, void *dest,
>> + grub_size_t size);
>> +#if 0
>> +/*
>> + * code example to be used in raid6_recover.
>> + */
>> +static grub_err_t
>> +raid_recover_read_diskfilter_array (void *data, int disk_nr,
>> + grub_uint64_t sector,
>> + void *buf, grub_size_t size)
>> +{
>> + struct grub_diskfilter_segment *array = data;
>> + return grub_diskfilter_read_node (&array->nodes[disk_nr],
>> + (grub_disk_addr_t)sector,
>> + size >> GRUB_DISK_SECTOR_BITS, buf);
>> +}
>> +#endif
>
> Please drop this.
See below
>
>> +
>> +static grub_err_t
>> +raid_recover_read_raid56_buffer (void *data, int disk_nr, grub_uint64_t
>> addr,
>> + void *dest, grub_size_t size)
>> +{
>> + struct raid56_buffer *buffers = data;
>> +
>> + (void)addr;
>
> "grub_uint64_t addr __attribute__ ((unused))" in prototype definition?
It is ugly ! :-)
>
>> + grub_memcpy(dest, buffers[disk_nr].buf, size);
>> +
>> + grub_errno = buffers[disk_nr].ok ? GRUB_ERR_NONE : GRUB_ERR_READ_ERROR;
>> + return grub_errno;
>> +}
>> +
>> +static grub_err_t
>> +grub_raid6_recover (void *data, grub_uint64_t nstripes, int disknr, int p,
>> + char *buf, grub_uint64_t sector, grub_size_t size,
>> + raid_recover_read_t read_func, int layout)
>> +{
>> + int i, q, pos;
>> + int bad1 = -1, bad2 = -1;
>> + char *pbuf = 0, *qbuf = 0;
>> + static int table_initializated = 0;
>> +
>> + if (!table_initializated)
>> + {
>> + grub_raid6_init_table();
>> + table_initializated = 1;
>> + }
>> +
>> + pbuf = grub_zalloc (size);
>> + if (!pbuf)
>> + goto quit;
>> +
>> + qbuf = grub_zalloc (size);
>> + if (!qbuf)
>> + goto quit;
>> +
>> + q = p + 1;
>> + if (q == (int) nstripes)
>> + q = 0;
>> +
>> + pos = q + 1;
>> + if (pos == (int) nstripes)
>> + pos = 0;
>> +
>> + for (i = 0; i < (int) nstripes - 2; i++)
>> + {
>> + int c;
>> + if (layout & GRUB_RAID_LAYOUT_MUL_FROM_POS)
>> + c = pos;
>> + else
>> + c = i;
>> + if (pos == disknr)
>> + bad1 = c;
>> + else
>> + {
>> + if (!read_func(data, pos, sector, buf, size))
>> + {
>> + grub_crypto_xor (pbuf, pbuf, buf, size);
>> + grub_raid_block_mulx (c, buf, size);
>> + grub_crypto_xor (qbuf, qbuf, buf, size);
>> + }
>> + else
>> + {
>> + /* Too many bad devices */
>> + if (bad2 >= 0)
>> + goto quit;
>> +
>> + bad2 = c;
>> + grub_errno = GRUB_ERR_NONE;
>> + }
>> + }
>> +
>> + pos++;
>> + if (pos == (int) nstripes)
>> + pos = 0;
>> + }
>> +
>> + /* Invalid disknr or p */
>> + if (bad1 < 0)
>> + goto quit;
>> +
>> + if (bad2 < 0)
>> + {
>> + /* One bad device */
>> + if (!read_func(data, p, sector, buf, size))
>> + {
>> + grub_crypto_xor (buf, buf, pbuf, size);
>> + goto quit;
>> + }
>> +
>> + grub_errno = GRUB_ERR_NONE;
>> + if (read_func(data, q, sector, buf, size))
>> + goto quit;
>> +
>> + grub_crypto_xor (buf, buf, qbuf, size);
>> + grub_raid_block_mulx (255 - bad1, buf,
>> + size);
>> + }
>> + else
>> + {
>> + /* Two bad devices */
>> + unsigned c;
>> +
>> + if (read_func(data, p, sector, buf, size))
>> + goto quit;
>> +
>> + grub_crypto_xor (pbuf, pbuf, buf, size);
>> +
>> + if (read_func(data, q, sector, buf, size))
>> + goto quit;
>> +
>> + grub_crypto_xor (qbuf, qbuf, buf, size);
>> +
>> + c = mod_255((255 ^ bad1)
>> + + (255 ^ powx_inv[(powx[bad2 + (bad1 ^ 255)] ^ 1)]));
>> + grub_raid_block_mulx (c, qbuf, size);
>> +
>> + c = mod_255((unsigned) bad2 + c);
>> + grub_raid_block_mulx (c, pbuf, size);
>> +
>> + grub_crypto_xor (pbuf, pbuf, qbuf, size);
>> + grub_memcpy (buf, pbuf, size);
>> + }
>> +
>> +quit:
>
> One space before the label please?
ok
>
>> + grub_free (pbuf);
>> + grub_free (qbuf);
>> +
>> + return grub_errno;
>> +}
>> +
>> +/* end copy */
>> static void
>> rebuild_raid6 (struct raid56_buffer *buffers, grub_uint64_t nstripes,
>> grub_uint64_t csize, grub_uint64_t parities_pos, void *dest,
>> grub_uint64_t stripen)
>>
>> {
>> - (void)buffers;
>> - (void)nstripes;
>> - (void)csize;
>> - (void)parities_pos;
>> - (void)dest;
>> - (void)stripen;
>> - grub_dprintf ("btrfs", "called rebuild_raid6(), NOT IMPLEMENTED\n");
>> + grub_raid6_recover (buffers, nstripes, stripen, parities_pos,
>> + dest, 0, csize,
>> + raid_recover_read_raid56_buffer, 0);
>
> grub_raid6_recover() should be called directly from right place.
replacing raid_recover_read_raid56_buffer() with
raid_recover_read_diskfilter_array(), could allow to use grub_raid6_recover()
even in the grub-core/disk/raid6_recover.c. This is the reason of "double call".
>
> Daniel
>
--
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
