Re: svn commit: r226554 - head/sys/boot/common

[Andriy Gapon]

on 20/10/2011 02:48 Pawel Jakub Dawidek said the following:
 Author: pjd
 Date: Wed Oct 19 23:48:15 2011
 New Revision: 226554
 URL: http://svn.freebsd.org/changeset/base/226554
 
 Log:
   Fix missing return when LOADER_GPT_SUPPORT is defined, but 
 LOADER_MBR_SUPPORT
   is not.
   
   MFC after:  3 days
 
 Modified:
   head/sys/boot/common/disk.c
 
 Modified: head/sys/boot/common/disk.c
 ==
 --- head/sys/boot/common/disk.c   Wed Oct 19 23:44:38 2011
 (r226553)
 +++ head/sys/boot/common/disk.c   Wed Oct 19 23:48:15 2011
 (r226554)
 @@ -776,9 +776,9 @@ disk_open(struct disk_devdesc *dev)
  
  #ifdef LOADER_GPT_SUPPORT
   rc = disk_opengpt(dev);
 - if (rc)
  #endif
  #ifdef LOADER_MBR_SUPPORT
 + if (rc)
   rc = disk_openmbr(dev);
  #endif
  

I believe that this fix is incorrect.  Now with !LOADER_GPT_SUPPORT and
LOADER_MBR_SUPPORT the preprocessed code would be:

rc = 0;
/*
 * While we are reading disk metadata, make sure we do it relative
 * to the start of the disk
 */
dev-d_offset = 0;

if (rc)
rc = disk_openmbr(dev);

Please see my take at the problem:
https://gitorious.org/~avg/freebsd/avgbsd/commit/49adde51339791a7bf0e2c75f24eeda2bc886da5

-- 
Andriy Gapon
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r226552 - head/sys/boot/zfs

[Andriy Gapon]

on 20/10/2011 02:40 Pawel Jakub Dawidek said the following:
 Author: pjd
 Date: Wed Oct 19 23:40:37 2011
 New Revision: 226552
 URL: http://svn.freebsd.org/changeset/base/226552
 
 Log:
   Never pass NULL block pointer when reading. This is neither expected nor
   handled by lower layers like vdev_raidz, which uses bp for checksum
   verification. This bug could lead to NULL pointer reference and resets
   during boot.
   
   MFC after:  3 days
 
 Modified:
   head/sys/boot/zfs/zfsimpl.c
 
 Modified: head/sys/boot/zfs/zfsimpl.c
 ==
 --- head/sys/boot/zfs/zfsimpl.c   Wed Oct 19 23:37:30 2011
 (r226551)
 +++ head/sys/boot/zfs/zfsimpl.c   Wed Oct 19 23:40:37 2011
 (r226552)
 @@ -988,7 +988,8 @@ zio_read_gang(spa_t *spa, const blkptr_t
   break;
   if (!vdev || !vdev-v_read)
   return (EIO);
 - if (vdev-v_read(vdev, NULL, zio_gb, offset, SPA_GANGBLOCKSIZE))
 +
 + if (vdev-v_read(vdev, bp, zio_gb, offset, SPA_GANGBLOCKSIZE))
   return (EIO);
  
   for (i = 0; i  SPA_GBH_NBLKPTRS; i++) {


I believe that this commit is incorrect.
It does appear to do the right thing for raidz, but I believe that it breaks the
simple case when v_read is vdev_read_phys.  Now because the bp argument is not
NULL, the latter will try to read BP_GET_PSIZE(bp) bytes instead of
SPA_GANGBLOCKSIZE bytes.

I believe that my patch that I shared (much) earlier handles this issue
correctly and integrally:
http://article.gmane.org/gmane.os.freebsd.devel.file-systems/13130
Did you find anything wrong with it?
If not, then maybe we can get back to discussing it?

-- 
Andriy Gapon
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r226553 - in head/sys: boot/zfs cddl/boot/zfs

[Andriy Gapon]

on 20/10/2011 02:44 Pawel Jakub Dawidek said the following:
 Author: pjd
 Date: Wed Oct 19 23:44:38 2011
 New Revision: 226553
 URL: http://svn.freebsd.org/changeset/base/226553
 
 Log:
   Always pass data size for checksum verification function, as using
   physical block size declared in bp may not always be what we want.
   For example in case of gang block header physical block size declared
   in bp is much larger than SPA_GANGBLOCKSIZE (512 bytes) and checksum
   calculation failed. This bug could lead to accessing unallocated
   memory and resets/failures during boot.
   
   MFC after:  3 days

I believe that this change is going in the wrong direction.  I'd rather prefer
that we decreased number of parameters :-)  Se my other email.

Besides I do not see where we currently really verify checksum of a re-assembled
gang block.

-- 
Andriy Gapon
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r226554 - head/sys/boot/common

[Pawel Jakub Dawidek]

On Thu, Oct 20, 2011 at 10:06:35AM +0300, Andriy Gapon wrote:
 on 20/10/2011 02:48 Pawel Jakub Dawidek said the following:
  Author: pjd
  Date: Wed Oct 19 23:48:15 2011
  New Revision: 226554
  URL: http://svn.freebsd.org/changeset/base/226554
  
  Log:
Fix missing return when LOADER_GPT_SUPPORT is defined, but 
  LOADER_MBR_SUPPORT
is not.

MFC after:3 days
  
  Modified:
head/sys/boot/common/disk.c
  
  Modified: head/sys/boot/common/disk.c
  ==
  --- head/sys/boot/common/disk.c Wed Oct 19 23:44:38 2011
  (r226553)
  +++ head/sys/boot/common/disk.c Wed Oct 19 23:48:15 2011
  (r226554)
  @@ -776,9 +776,9 @@ disk_open(struct disk_devdesc *dev)
   
   #ifdef LOADER_GPT_SUPPORT
  rc = disk_opengpt(dev);
  -   if (rc)
   #endif
   #ifdef LOADER_MBR_SUPPORT
  +   if (rc)
  rc = disk_openmbr(dev);
   #endif
   
 
 I believe that this fix is incorrect.  Now with !LOADER_GPT_SUPPORT and
 LOADER_MBR_SUPPORT the preprocessed code would be:
 
   rc = 0;
   /*
* While we are reading disk metadata, make sure we do it relative
* to the start of the disk
*/
   dev-d_offset = 0;
 
   if (rc)
   rc = disk_openmbr(dev);

 Please see my take at the problem:
 https://gitorious.org/~avg/freebsd/avgbsd/commit/49adde51339791a7bf0e2c75f24eeda2bc886da5

Yes, you are right. Feel free to commit your fix.

-- 
Pawel Jakub Dawidek   http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://yomoli.com


pgp8Xg8B5H4Rx.pgp
Description: PGP signature

Re: svn commit: r226552 - head/sys/boot/zfs

[Pawel Jakub Dawidek]

On Thu, Oct 20, 2011 at 10:15:09AM +0300, Andriy Gapon wrote:
 I believe that this commit is incorrect.
 It does appear to do the right thing for raidz, but I believe that it breaks 
 the
 simple case when v_read is vdev_read_phys.  Now because the bp argument is not
 NULL, the latter will try to read BP_GET_PSIZE(bp) bytes instead of
 SPA_GANGBLOCKSIZE bytes.
 
 I believe that my patch that I shared (much) earlier handles this issue
 correctly and integrally:
 http://article.gmane.org/gmane.os.freebsd.devel.file-systems/13130
 Did you find anything wrong with it?
 If not, then maybe we can get back to discussing it?

You see. My memory is very short:) and I remembered that you were
working on gang blocks in ZFS boot code, but could find the commit, so I
went ahead with my changes. Those changes still didn't fix one problem
for me. Your patch seems to be much more complete and correct and it
fixes all my problems.

Please, wait no longer and commit your patch! We have to have this in 9.0.

-- 
Pawel Jakub Dawidek   http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://yomoli.com


pgpInie59zysm.pgp
Description: PGP signature

Re: svn commit: r226552 - head/sys/boot/zfs

[Andriy Gapon]

on 20/10/2011 12:35 Pawel Jakub Dawidek said the following:
 On Thu, Oct 20, 2011 at 10:15:09AM +0300, Andriy Gapon wrote:
 I believe that this commit is incorrect.
 It does appear to do the right thing for raidz, but I believe that it breaks 
 the
 simple case when v_read is vdev_read_phys.  Now because the bp argument is 
 not
 NULL, the latter will try to read BP_GET_PSIZE(bp) bytes instead of
 SPA_GANGBLOCKSIZE bytes.

 I believe that my patch that I shared (much) earlier handles this issue
 correctly and integrally:
 http://article.gmane.org/gmane.os.freebsd.devel.file-systems/13130
 Did you find anything wrong with it?
 If not, then maybe we can get back to discussing it?
 
 You see. My memory is very short:) and I remembered that you were
 working on gang blocks in ZFS boot code, but could find the commit, so I
 went ahead with my changes. Those changes still didn't fix one problem
 for me. Your patch seems to be much more complete and correct and it
 fixes all my problems.
 
 Please, wait no longer and commit your patch! We have to have this in 9.0.
 

Unfortunately, at the moment I can only afford to spend at most 1/2 hour a day 
for
FreeBSD (and those go mostly into reading and writing emails) and my development
environment is in a little bit of disarray.  I will try to find some time, but 
if
you have a chance to review, possibly rework and commit those changes, that 
would
be really great and appreciated.

Thank you!
-- 
Andriy Gapon
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

svn commit: r226564 - head/share/mk

[Jayachandran C.]

Author: jchandra
Date: Thu Oct 20 13:41:37 2011
New Revision: 226564
URL: http://svn.freebsd.org/changeset/base/226564

Log:
  Build 'dtc' by default for MIPS.
  
  The device tree compiler is needed during the kernel build to compile
  DTS files to DTB.
  
  Reviewed by:  stas

Modified:
  head/share/mk/bsd.own.mk

Modified: head/share/mk/bsd.own.mk
==
--- head/share/mk/bsd.own.mkThu Oct 20 09:57:27 2011(r226563)
+++ head/share/mk/bsd.own.mkThu Oct 20 13:41:37 2011(r226564)
@@ -434,8 +434,9 @@ __DEFAULT_YES_OPTIONS+=CLANG
 .else
 __DEFAULT_NO_OPTIONS+=CLANG
 .endif
-# FDT is needed only for arm and powerpc (and not powerpc64)
-.if ${__T} == arm || ${__T} == armeb || ${__T} == powerpc
+# FDT is needed only for arm, mips and powerpc (and not powerpc64)
+.if ${__T} == arm || ${__T} == armeb || ${__T} == powerpc || \
+${__T:Mmips*}
 __DEFAULT_YES_OPTIONS+=FDT
 .else
 __DEFAULT_NO_OPTIONS+=FDT
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

svn commit: r226568 - in head/sys: boot/zfs cddl/boot/zfs

[Pawel Jakub Dawidek]

Author: pjd
Date: Thu Oct 20 15:42:38 2011
New Revision: 226568
URL: http://svn.freebsd.org/changeset/base/226568

Log:
  - Correctly read gang header from raidz.
  - Decompress assembled gang block data if compressed.
  - Verify checksum of a gang header.
  - Verify checksum of assembled gang block data.
  - Verify checksum of uber block.
  
  Submitted by: avg
  MFC after:3 days

Modified:
  head/sys/boot/zfs/zfsimpl.c
  head/sys/cddl/boot/zfs/zfssubr.c

Modified: head/sys/boot/zfs/zfsimpl.c
==
--- head/sys/boot/zfs/zfsimpl.c Thu Oct 20 15:34:17 2011(r226567)
+++ head/sys/boot/zfs/zfsimpl.c Thu Oct 20 15:42:38 2011(r226568)
@@ -347,7 +347,7 @@ vdev_read_phys(vdev_t *vdev, const blkpt
rc = vdev-v_phys_read(vdev, vdev-v_read_priv, offset, buf, psize);
if (rc)
return (rc);
-   if (bp  zio_checksum_verify(bp, buf, offset, psize))
+   if (bp  zio_checksum_verify(bp, buf))
return (EIO);
 
return (0);
@@ -798,6 +798,7 @@ vdev_probe(vdev_phys_read_t *read, void 
BP_SET_PSIZE(bp, sizeof(vdev_phys_t));
BP_SET_CHECKSUM(bp, ZIO_CHECKSUM_LABEL);
BP_SET_COMPRESS(bp, ZIO_COMPRESS_OFF);
+   DVA_SET_OFFSET(BP_IDENTITY(bp), off);
ZIO_SET_CHECKSUM(bp.blk_cksum, off, 0, 0, 0);
if (vdev_read_phys(vtmp, bp, vdev_label, off, 0))
return (EIO);
@@ -940,7 +941,7 @@ vdev_probe(vdev_phys_read_t *read, void 
BP_SET_COMPRESS(bp, ZIO_COMPRESS_OFF);
ZIO_SET_CHECKSUM(bp.blk_cksum, off, 0, 0, 0);
 
-   if (vdev_read_phys(vdev, NULL, upbuf, off, 
VDEV_UBERBLOCK_SIZE(vdev)))
+   if (vdev_read_phys(vdev, bp, upbuf, off, 0))
continue;
 
if (up-ub_magic != UBERBLOCK_MAGIC)
@@ -973,35 +974,39 @@ ilog2(int n)
 }
 
 static int
-zio_read_gang(spa_t *spa, const blkptr_t *bp, const dva_t *dva, void *buf)
+zio_read_gang(spa_t *spa, const blkptr_t *bp, void *buf)
 {
+   blkptr_t gbh_bp;
zio_gbh_phys_t zio_gb;
-   vdev_t *vdev;
-   int vdevid;
-   off_t offset;
+   char *pbuf;
int i;
 
-   vdevid = DVA_GET_VDEV(dva);
-   offset = DVA_GET_OFFSET(dva);
-   STAILQ_FOREACH(vdev, spa-spa_vdevs, v_childlink)
-   if (vdev-v_id == vdevid)
-   break;
-   if (!vdev || !vdev-v_read)
-   return (EIO);
+   /* Artificial BP for gang block header. */
+   gbh_bp = *bp;
+   BP_SET_PSIZE(gbh_bp, SPA_GANGBLOCKSIZE);
+   BP_SET_LSIZE(gbh_bp, SPA_GANGBLOCKSIZE);
+   BP_SET_CHECKSUM(gbh_bp, ZIO_CHECKSUM_GANG_HEADER);
+   BP_SET_COMPRESS(gbh_bp, ZIO_COMPRESS_OFF);
+   for (i = 0; i  SPA_DVAS_PER_BP; i++)
+   DVA_SET_GANG(gbh_bp.blk_dva[i], 0);
 
-   if (vdev-v_read(vdev, bp, zio_gb, offset, SPA_GANGBLOCKSIZE))
+   /* Read gang header block using the artificial BP. */
+   if (zio_read(spa, gbh_bp, zio_gb))
return (EIO);
 
+   pbuf = buf;
for (i = 0; i  SPA_GBH_NBLKPTRS; i++) {
blkptr_t *gbp = zio_gb.zg_blkptr[i];
 
if (BP_IS_HOLE(gbp))
continue;
-   if (zio_read(spa, gbp, buf))
+   if (zio_read(spa, gbp, pbuf))
return (EIO);
-   buf = (char*)buf + BP_GET_PSIZE(gbp);
+   pbuf += BP_GET_PSIZE(gbp);
}
- 
+
+   if (zio_checksum_verify(bp, buf))
+   return (EIO);
return (0);
 }
 
@@ -1024,46 +1029,41 @@ zio_read(spa_t *spa, const blkptr_t *bp,
if (!dva-dva_word[0]  !dva-dva_word[1])
continue;
 
-   if (DVA_GET_GANG(dva)) {
-   error = zio_read_gang(spa, bp, dva, buf);
-   if (error != 0)
-   continue;
-   } else {
-   vdevid = DVA_GET_VDEV(dva);
-   offset = DVA_GET_OFFSET(dva);
-   STAILQ_FOREACH(vdev, spa-spa_vdevs, v_childlink) {
-   if (vdev-v_id == vdevid)
-   break;
-   }
-   if (!vdev || !vdev-v_read)
-   continue;
+   vdevid = DVA_GET_VDEV(dva);
+   offset = DVA_GET_OFFSET(dva);
+   STAILQ_FOREACH(vdev, spa-spa_vdevs, v_childlink) {
+   if (vdev-v_id == vdevid)
+   break;
+   }
+   if (!vdev || !vdev-v_read)
+   continue;
 
-   size = BP_GET_PSIZE(bp);
+   size = BP_GET_PSIZE(bp);
+   if (vdev-v_read == vdev_raidz_read) {
align = 1ULL  vdev-v_top-v_ashift;
if (P2PHASE(size, align) != 0)

svn commit: r226569 - head/sys/boot/common

[Pawel Jakub Dawidek]

Author: pjd
Date: Thu Oct 20 15:46:54 2011
New Revision: 226569
URL: http://svn.freebsd.org/changeset/base/226569

Log:
  With LOADER_MBR_SUPPORT defined and LOADER_GPT_SUPPORT undefined we would
  never call disk_openmbr().
  
  Submitted by: avg
  MFC after:3 days

Modified:
  head/sys/boot/common/disk.c

Modified: head/sys/boot/common/disk.c
==
--- head/sys/boot/common/disk.c Thu Oct 20 15:42:38 2011(r226568)
+++ head/sys/boot/common/disk.c Thu Oct 20 15:46:54 2011(r226569)
@@ -776,10 +776,11 @@ disk_open(struct disk_devdesc *dev)
 
 #ifdef LOADER_GPT_SUPPORT
rc = disk_opengpt(dev);
+   if (rc == 0)
+   return (0);
 #endif
 #ifdef LOADER_MBR_SUPPORT
-   if (rc)
-   rc = disk_openmbr(dev);
+   rc = disk_openmbr(dev);
 #endif
 
return (rc);
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r226402 - head/sys/netinet

[Gleb Smirnoff]

On Sat, Oct 15, 2011 at 11:51:21PM +0300, Nikolay Denev wrote:
N  Log:
N   Add support for IPv4 /31 prefixes, as described in RFC3021.
N  
N   To run a /31 network, participating hosts MUST drop support
N   for directed broadcasts, and treat the first and last addresses
N   on subnet as unicast. The broadcast address for the prefix
N   should be the link local broadcast address, INADDR_BROADCAST.
...
N That's great! Thanks!
N Any plans for MFC?

Merged to stable/9.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

svn commit: r226583 - head/cddl/contrib/opensolaris/cmd/zpool

[Pawel Jakub Dawidek]

Author: pjd
Date: Thu Oct 20 21:01:50 2011
New Revision: 226583
URL: http://svn.freebsd.org/changeset/base/226583

Log:
  Make all the lines align properly.
  
  MFC after:3 days

Modified:
  head/cddl/contrib/opensolaris/cmd/zpool/zpool_main.c

Modified: head/cddl/contrib/opensolaris/cmd/zpool/zpool_main.c
==
--- head/cddl/contrib/opensolaris/cmd/zpool/zpool_main.cThu Oct 20 
20:33:31 2011(r226582)
+++ head/cddl/contrib/opensolaris/cmd/zpool/zpool_main.cThu Oct 20 
21:01:50 2011(r226583)
@@ -3377,7 +3377,7 @@ print_scan_status(pool_scan_stat_t *ps)
double fraction_done;
char processed_buf[7], examined_buf[7], total_buf[7], rate_buf[7];
 
-   (void) printf(gettext( scan: ));
+   (void) printf(gettext(  scan: ));
 
/* If there's never been a scan, there's not much to say. */
if (ps == NULL || ps-pss_func == POOL_SCAN_NONE ||
@@ -3457,7 +3457,7 @@ print_scan_status(pool_scan_stat_t *ps)
/*
 * do not print estimated time if hours_left is more than 30 days
 */
-   (void) printf(gettext(%s scanned out of %s at %s/s),
+   (void) printf(gettext(%s scanned out of %s at %s/s),
examined_buf, total_buf, rate_buf);
if (hours_left  (30 * 24)) {
(void) printf(gettext(, %lluh%um to go\n),
@@ -3468,10 +3468,10 @@ print_scan_status(pool_scan_stat_t *ps)
}
 
if (ps-pss_func == POOL_SCAN_RESILVER) {
-   (void) printf(gettext(%s resilvered, %.2f%% done\n),
+   (void) printf(gettext(%s resilvered, %.2f%% done\n),
processed_buf, 100 * fraction_done);
} else if (ps-pss_func == POOL_SCAN_SCRUB) {
-   (void) printf(gettext(%s repaired, %.2f%% done\n),
+   (void) printf(gettext(%s repaired, %.2f%% done\n),
processed_buf, 100 * fraction_done);
}
 }
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org