make-zfs quick tool could have an onu-like option

Review Request #110 — Created Oct. 28, 2015 and updated

dan.vatca
illumos-gate
general

When working on zfs, the make-zfs is very helpful to reduce build time.
Having an 'onuzfs' option would be great for saving some typing (and remembering) how to run your just compiled changes.

Internal use at Syneto

  • 2
  • 0
  • 0
  • 0
  • 2
Description From Last Updated
I'm not sure which way is typical practice for things like this, but instead of putting sudo on these lines, ... gwr gwr
Just curious: Why gnu tar here? I've always unpacked these with the system tar, i.e.: (cd /mnt ; tar xfjo ... gwr gwr
will
  1. How about an optional argument specifying the BE name?

  2. 
      
will
  1. Never mind, that wouldn't really fit in with the rest of make-zfs. Works for me, LGTM!

  2. 
      
andy_js
  1. I would suggest shortening the name of the new option from "onuzfs" to just "onu". You also need to update the usage line, but apart from that, LGTM.

  2. 
      
dan.vatca
andy_js
  1. It seems to me the do_onu function should probably exit with non-zero in those conditions where action is required before it can complete successfully.

  2. 
      
serbanm
  1. LGTM

  2. 
      
dan.vatca
andy_js
  1. Ship It!
  2. 
      
gwr
  1. 
      
  2. usr/src/tools/quick/make-zfs (Diff revision 3)
     
     

    Just curious: Why gnu tar here?
    I've always unpacked these with the system tar, i.e.:
    (cd /mnt ; tar xfjo ~admin/zfs-1234567.tar.bz2)

    1. Hi Gordon,
      That is just because we've been used to using gnu tar on our distro. If you believe it would be better, I have no issue switching to system tar.

    2. Yes please. That will let this work if the gnu tools are not installed.
      Thanks.

  3. 
      
gwr
  1. 
      
  2. usr/src/tools/quick/make-zfs (Diff revision 3)
     
     

    I'm not sure which way is typical practice for things like this, but instead of putting sudo on these lines, you could just let "beadm create" fail, and when it does (testing the return status) remind the user: Hey, you need to run this as: sudo make-zfs onu

    Tat way seems a little more straight-forward to me.
    It would also work if you didn't install sudo, though that's not a huge concern.

    1. I agree with sudo being out of place on those lines.
      Instead of relying on beadm's return value, I'd rather check the effective UID first and bail if we're not root.
      Checking id -u == 0 is ok?

    2. Sure, that seems OK too.

    3. Updated both concerns.
      One question on RB usage: who is supposed to mark the issues as fixes? The reviewer? Or the auther after adressing the concern?

    4. Not sure. When I have stuff out for review, I've marked issues closed when I think I've fixed them. (and assume the reviewer will tell me if they disagree:)

  3. 
      
dan.vatca
Review request changed

Change Summary:

  1. Check for root instead of using sudo
  2. Use system tar instead of gnu tar

Diff:

Revision 4 (+28 -2)

Show changes

gwr
  1. 
      
  2. 
      
andy_js
  1. Are you going to RTI this anytime soon? This is a really useful feature.

  2. 
      
kmays
  1. Ship It!
  2. 
      
Loading...