- Download Diff

Summary:

7540 loader zfs should check all labels

Review Request #250 — Created Nov. 7, 2016 and submitted April 5, 2017, 6:48 a.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	7540
Depends On:
Commit:	0a4b58f...
Reviewers
Groups:	general
People:

Description

Testing Done

Verified the boot; also verified the label read and data from reads (txg and ub).

Issues

0
0
9
0
All Issues: 9

Description	From	Last Updated
style - function name on new line.	yuripv	Feb. 13, 2017, 12:34 p.m.
style - function name on new line.	yuripv	Feb. 13, 2017, 12:36 p.m.
style - function name on new line.	yuripv	Feb. 13, 2017, 12:37 p.m.
style - function name on new line.	yuripv	Feb. 13, 2017, 12:38 p.m.
some suspicious bracketing here ( () ( ? : () ) )	yuripv	Feb. 13, 2017, 12:47 p.m.
!= 0	yuripv	Feb. 13, 2017, 12:50 p.m.
whitespace after sizeof	yuripv	Feb. 13, 2017, 12:52 p.m.
This is better written as (without val = 0): if (nvlist_find(...) == 0 && val != 0) return (EIO); And, ...	yuripv	Feb. 13, 2017, 12:59 p.m.
you could really combine these 2 ifs into just one as resulting assignment is the same: if (up->ub_txg > spa->spa_uberblock.ub_txg ...	yuripv	Feb. 13, 2017, 1:05 p.m.

Summary:

-	loader zfs should check all labels
+	7540 loader zfs should check all labels

Description:

~		loader zfs should check all labels
	~	7540 loader zfs should check all labels

Testing Done:

Verified the boot; also verified the label read and data from reads (txg and ub).

Bugs:

+	7540

Ship it!

```
Ship It!
```

usr/src/boot/sys/boot/efi/boot1/zfs_module.c (Diff revision 1)

An explicit cast maybe?

tsoome Feb. 10, 2017, 11:14 a.m.

void * does not need the cast, in my opinion such casts do not add extra value anyhow, because we already know the type.

usr/src/boot/sys/boot/efi/libefi/efipart.c (Diff revision 1)
```
Should these be here for illumos?
```
1. tsoome Feb. 10, 2017, 11:14 a.m.
  Leftover, I'll remove it.

usr/src/boot/sys/boot/efi/boot1/zfs_module.c (Diff revision 1)
Show all issues
```
style - function name on new line.
```
usr/src/boot/sys/boot/efi/loader/main.c (Diff revision 1)
Show all issues
```
style - function name on new line.
```
usr/src/boot/sys/boot/i386/gptzfsboot/zfsboot.c (Diff revision 1)
Show all issues
```
style - function name on new line.
```
usr/src/boot/sys/boot/i386/loader/main.c (Diff revision 1)
Show all issues
```
style - function name on new line.
```

usr/src/boot/sys/boot/zfs/zfsimpl.c (Diff revision 1)

Show all issues

some suspicious bracketing here ( () ( ? : () ) )

yuripv Feb. 10, 2017, 11:21 a.m.

Ah, got it, could you please put the condition in brackets, i.e., (l < VDEV_LABELS / 2) ?, so I'd stop asking stupid questions :-)

tsoome Feb. 10, 2017, 11:26 a.m.

you want also look at: http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/vdev_label.c#155

tsoome Feb. 13, 2017, 1:30 p.m.

I actually split it up, there is no reason to have such hard to read construct.

usr/src/boot/sys/boot/zfs/zfsimpl.c (Diff revision 1)
Show all issues
```
!= 0
```
usr/src/boot/sys/boot/zfs/zfsimpl.c (Diff revision 1)
Show all issues
```
whitespace after sizeof
```

usr/src/boot/sys/boot/zfs/zfsimpl.c (Diff revision 1)

Show all issues

This is better written as (without val = 0):

if (nvlist_find(...) == 0 && val != 0)
        return (EIO);


And, BTW, about that '0' arg which is supposed to be int* elementsp - should we use NULL there instead?

tsoome Feb. 13, 2017, 1:30 p.m.

yap, replaced them. Also removed unneeded 0 assignment from dnode_cache_obj and duplicate zfs_crc64_table.

usr/src/boot/sys/boot/zfs/zfsimpl.c (Diff revision 1)

Show all issues

you could really combine these 2 ifs into just one as resulting assignment is the same:

if (up->ub_txg > spa->spa_uberblock.ub_txg ||
    (up->ub_txg == spa->spa_uberblock.ub_txg &&
    up->ub_timestamp > spa->spa_uberblock.ub_timestamp))
        spa->spa_uberblock = *up;

Commit:

-	21a22da1b520667572648f3023c2ddb49b6b8037
+	323ab1786735befdf5fba9bc7e43be53c78bdb11

Diff:

Revision 2 (+220 -108)

Show changes

	usr/src/boot/sys/boot/efi/boot1/zfs_module.c
	usr/src/boot/sys/boot/efi/libefi/efipart.c
	usr/src/boot/sys/boot/efi/loader/main.c
	usr/src/boot/sys/boot/i386/gptzfsboot/zfsboot.c
	usr/src/boot/sys/boot/i386/libi386/biosdisk.c
	usr/src/boot/sys/boot/i386/loader/main.c
	usr/src/boot/sys/boot/zfs/libzfs.h
	usr/src/boot/sys/boot/zfs/zfs.c
	1 more

Change Summary:

Just attaching version number, no functional change.

Commit:

-	323ab1786735befdf5fba9bc7e43be53c78bdb11
+	3344eaed7e58e15f032836900af779229af40043

Diff:

Revision 3 (+221 -107)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/boot1/zfs_module.c
	usr/src/boot/sys/boot/efi/libefi/efipart.c
	usr/src/boot/sys/boot/efi/loader/main.c
	usr/src/boot/sys/boot/i386/gptzfsboot/zfsboot.c
	usr/src/boot/sys/boot/i386/libi386/biosdisk.c
	usr/src/boot/sys/boot/i386/loader/main.c
	usr/src/boot/sys/boot/zfs/libzfs.h
	2 more

Change Summary:

rebase on 7987 - version update.

Commit:

-	3344eaed7e58e15f032836900af779229af40043
+	0a4b58f75735c3afcb201f256820f1b1ac6ac9e3

Diff:

Revision 4 (+221 -107)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/boot1/zfs_module.c
	usr/src/boot/sys/boot/efi/libefi/efipart.c
	usr/src/boot/sys/boot/efi/loader/main.c
	usr/src/boot/sys/boot/i386/gptzfsboot/zfsboot.c
	usr/src/boot/sys/boot/i386/libi386/biosdisk.c
	usr/src/boot/sys/boot/i386/loader/main.c
	usr/src/boot/sys/boot/zfs/libzfs.h
	2 more

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

7540 loader zfs should check all labels

Summary:

Description:

Testing Done:

Bugs:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)