6602 lofi should support labeled devices

Review Request #120 — Created Nov. 28, 2015 and submitted

tsoome
illumos-gate
6602
2365e62...
general

6602 lofi should support labeled devices

first test:
lofiadm -a and lofiadm -la in GZ/LZ, same for delete, mapping files with/without content.

NOTE: with labeled lofi in local zone, the current code needs device match entries, like:
<device match="/dev/lofictl"/>
<device match="/dev/dsk/c0t1d0"/>
<device match="/dev/rdsk/c0t1d0
"/>
also, to mount filesystem, fs-allowed property must be set accordingly.

if the entry in zone config is missing, device node is not created even if file mapping succeeds; lofiadm will stay in loop (to find device nodes) for 2 minutes.

for second test, I have updated usbgen script (in distro-const package), to use labeled device for usb image creation:
root@beastie:/# ./usbgen /rpool/dc/media/OpenIndiana_Text_X86.iso /mnt/OpenIndiana_Text_X86.usb /tmp
fmthard: New volume table of contents now in place.
/dev/rdsk/c4t2d0s0: 1601536 sectors in 782 cylinders of 64 tracks, 32 sectors
782.0MB in 49 cyl groups (16 c/g, 16.00MB/g, 7680 i/g)
super-block backups (for fsck -F ufs -o b=#) at:
32, 32832, 65632, 98432, 131232, 164032, 196832, 229632, 262432, 295232,
1279232, 1312032, 1344832, 1377632, 1410432, 1443232, 1476032, 1508832,
1541632, 1574432
Copying ISO contents to USB image...
..................................................
..................................................
..................................................
..................................................
..................................................
....................
1339424 blocks
bootblock written for partition 0, 213 sectors starting at 50 (abs 2098)
stage1 written to partition 0 sector 0 (abs 2048)
stage1 written to master boot sector
=== ./usbgen completed at Mon Feb 1 00:55:16 EET 2016

updated usbgen is running ok in both global and local zone.

Additional tests after all the updates:

root@test:/home/tsoome# zlogin lz
[Connected to zone 'lz' pts/2]
Last login: Tue Jun 7 02:08:55 on pts/2
The Illumos Project SunOS 5.11 illumos-gate May 2016
root@lz:/root# lofiadm -a OpenIndiana_Text_X86.usb
/dev/lofi/1
root@lz:/root# lofiadm
Block Device File Options
/dev/lofi/1 /root/OpenIndiana_Text_X86.usb -
root@lz:/root# lofiadm -d OpenIndiana_Text_X86.usb
root@lz:/root# lofiadm -la OpenIndiana_Text_X86.usb
/dev/dsk/c0t1d0p0
root@lz:/root# lofiadm
Block Device File Options
/dev/dsk/c0t1d0p0 /root/OpenIndiana_Text_X86.usb Labeled
root@lz:/root# lofiadm -d OpenIndiana_Text_X86.usb
root@lz:/root# zone
zoneadm zonecfg zonename zonestat
root@lz:/root# zonename
lz
root@lz:/root# lofiadm
Block Device File Options
root@lz:/root#

  • 0
  • 0
  • 94
  • 10
  • 104
Description From Last Updated
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
jeffpc
  1. 
      
  2. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 16)
     
     

    you don't need the if statement, nvlist_free(NULL) is a no-op

  3. 
      
tsoome
tsoome
tsoome
tsoome
jeffpc
  1. 
      
  2. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 20)
     
     

    no need for the if as nvlist_free(nvl) is a no-op.

  3. 
      
tsoome
igork
  1. Ship It!
  2. 
      
yuripv
  1. 
      
  2. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 21)
     
     

    Use snprintf() here, easier to read and you can check for overflows.

  3. usr/src/cmd/devfsadm/disk_link.c (Diff revision 21)
     
     

    != 0

  4. usr/src/cmd/devfsadm/disk_link.c (Diff revision 21)
     
     
    {}
  5. 
      
yuripv
  1. 
      
  2. usr/src/uts/common/os/log_sysevent.c (Diff revision 21)
     
     

    Is separate function really needed given that it's called from one place only?

    Also, please do this in separate issue as it seems generic enough and isn't lofi-only.

    1. in fact it is lofi only, and it is kind of ugly workaround for an problem (IMO). The real solution would be to have controller data in single place (modctl?), and have interface to query such information from one place. I did implement this as separate function to make it easy to remove it, once more proper interface is in place. Right now lofi does keep its own cache of own interfaces, and dev_add/dev_rm messages are setting the content for that cache. The message passing may seem generic, but I have never thought about it as such. I would rather keep it as is for now and consider it lofi specific, but IMO it is topic to think about and have possible alternate solution in future.

  3. usr/src/uts/common/os/log_sysevent.c (Diff revision 21)
     
     
    != 0
  4. usr/src/uts/common/os/log_sysevent.c (Diff revision 21)
     
     

    Check ret? Or don't assing it.

  5. 
      
tsoome
yuripv
  1. 
      
  2. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 22)
     
     

    POSIX defines PATH_MAX as including terminating null character.

  3. usr/src/cmd/devfsadm/disk_link.c (Diff revision 22)
     
     

    Can't this ever fail?

    1. since its scanning string, there are no IO related issues (pointed by fgetc(3C)), arguments are present, so no EINVAL, the input string is device node name, so the format should match as well, so I think, it should not...

  4. usr/src/cmd/lofiadm/main.c (Diff revision 22)
     
     

    Can't these be just one line with ",Labeled"?

  5. usr/src/cmd/lofiadm/main.c (Diff revision 22)
     
     

    Can't these be just one line with ",Removable"?

  6. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    Style nit: enclose in {}, as else block is {}.

  7. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    Don't really need {} here.

  8. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    What's 20?

  9. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    What's 10?

  10. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    What's 10?

  11. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    Why is this commented out?

    1. It is leftover from picking code segments. The very first glance was, perhaps we can use devid to prepare image with zfs pool (for usb sticks), but unless I got it all wrong, it really does not help as the device tree (and therefore devid cache) will be built after rootfs is mounted. So, I just remove that commented out block.

  12. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     
  13. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     

    Remove? Or better #ifdef DEBUG.

  14. usr/src/uts/common/io/lofi.c (Diff revision 22)
     
     
  15. 
      
tsoome
yuripv
  1. I've run the lofi test suite from STC with these changes, everything seems fine.

  2. 
      
gdamore
  1. I only got about half way through so far... but I have some feedback...

  2. usr/src/cmd/devfsadm/devfsadm.h (Diff revision 23)
     
     

    I feel that placing this into devfsadm core is the wrong approach. Instead, these should probably be handled in the devfsadm misc module. Look at "misc_link.c" for examples. Alternatively, disk_link.c.

    1. Actually, if you check a bit more into how the devfsadm is implemented, the real cleanup calls used in misc_link.c and disk_link.c are all in core. And the reason is, the cleanup is using dev & devices path static global variables set in core as the result of the initialization, and those values are either relative to root (/) or alternate root (set by -r command line switch). So the option there is either to expose/pass device path (prefix) variables to modules, or have cleanup callbacks in core. As you did conclude in another spot - how unfortunate. But I'm really not in position to rewrite/redesign devfsadm implementation and I'll be very happy to leave it for someone else:)

    2. I'm not thrilled with your response. The "real cleanup calls" are generic routines that are not specific to one module, so that's not such a problme.

      So it seems that the problem is dev_path is fixed, because you're trying to look for the lofi path in the device link, because you want to be sure the node is a lofi node. I really don't like the core to know the specifics of individual subsystems, since this can break if we ever change it. (The fact that you're doing this work now tells me that change in the future is not out of the question. :-)

      If we wanted that approach, we wouldn't bother writing separate modules, and we'd just have a monolithic devfsadm. That's not the current design.

      Here are two alternative approaches:

      a) Decorate the device node or change its type so that it isn't just "disk", but "disk,lofi" or somesuch (maybe this won't work though? I'm not sure I fully understand the context in which this operates.) If you can figure out how to express the minor node or the link so that the plugin can verify that its for lofi without having to do readlink, that would definitely be nicer IMO. (The problem I see is that I'm not sure you have access to the rich data that is in the device tree at this point.)

      b) Create a function in devfsadm to expose the global variable (e.g. const char *devfsadm_dev_path(). This is probably the simplest. This may help future devfsadm module implementors too, so its multiuse unlike your current one-off special for lofi.

      I'd really like you to move this function (and make the various proposed changes) -- doing option "b" above is fairly trivial, and represents minimal effort and keeps the boundaries a bit clearer.

    3. Yep, You are right. Used approach b) as the "disk" is not arbitrary string, but class name, making things more complicated. As discussed in mail, however, for future note, there is an possibility of adding driver name to remove struct as there is one in create struct, allowing to implement specific callbacks. However, since such change is not trivial, and the option b) wont close any doors, I leave this idea to air:)

  3. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    What kind of noise are you talking about here? I am not thrilled with teaching devfsadm core about lofi-specific details.

    1. when device node is getting removed, the kenel will generate following events:

      ./sysevent.d

      PUBLISHER CLASS SUBCLASS
      SUNW:kern:ddi EC_devfs ESC_devfs_minor_remove
      SUNW:kern:ddi EC_devfs ESC_devfs_minor_remove
      SUNW:kern:ddi EC_devfs ESC_devfs_devi_remove
      SUNW:usr:devfsadmd:215 EC_dev_remove lofi
      SUNW:kern:iscsi EC_iSCSI ESC_static_start
      SUNW:kern:iscsi EC_iSCSI ESC_static_end
      SUNW:kern:iscsi EC_iSCSI ESC_send_targets_start
      SUNW:kern:iscsi EC_iSCSI ESC_send_targets_end
      SUNW:kern:iscsi EC_iSCSI ESC_slp_start
      SUNW:kern:iscsi EC_iSCSI ESC_slp_end
      SUNW:kern:iscsi EC_iSCSI ESC_isns_start
      SUNW:kern:iscsi EC_iSCSI ESC_isns_end
      SUNW:usr:devfsadmd:215 EC_dev_branch ESC_dev_branch_remove

      2x ESC_devfs_minor_remove is happening because of disks have 2 device nodes; so what devfsadm is doing, its "ignoring" first ESC_devfs_minor_remove event and run clean up on "last" ESC_devfs_minor_remove to avoid running lofi cleanup twice as this cleanup is removing both raw and block device node in one run. Also note this is how current lofi code is done, the difference from having lofi+cmlb is about getting labeled lofi devices with subclass type ESC_DISK (as indirect result of cmlb_attach()), while non-labeled lofi devices are still labeled as subclass ESC_LOFI.

      The "noise" would be lookup_disk_dev_name() for labeled lofi device or lookup_lofi_dev_name() for traditional lofi device returning NULL and causing error message being printed in case of ESC_DISK subclass. And once again, the lofi specific details are already there, just for traditional non-labeled case (ESC_LOFI subclass).

    2. It seems unfortunate that we have such special casing here for LOFI. It would have been better IMO if we could have treated lofi devices more normally. Or perhaps have cmlb suppress the events for lofi nodes. I'm not sure. Anyway, now I understand the problem, and I'm ok, if less than thrilled, with your response.

  4. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    Ah, I see lofi knowledge was already present here. How unfortunate.

  5. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    Language, instead of "For... " try "If ...". I'd reword as "If the the minor iS NULL, we use a constant value instead".

    1. rewrote it, included detailed event information for both EC_dev_add and EC_dev_remove, hopefully this code block will make more sense now:)

  6. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    Actually, this logic seems kind of goofy. You set the minor, but then its going to match the next if statement and bail out. In fact, the whole logic here is redundant, since if this case hits, then necessarily the next one will too.

    1. fixed the comment block (see the response above).

  7. usr/src/cmd/devfsadm/disk_link.c (Diff revision 23)
     
     

    Yeah, see devfsadm_rm_lofi_disk should be in this file ... as this is the only consumer of it.

    1. As explained in response for comment about devfsadm.h, it does not seem to be possible.

    2. And I think it is -- see my reply.

  8. usr/src/cmd/devfsadm/lofi_link.c (Diff revision 23)
     
     

    These paths don't seem to mtach your comment at 75/76.

    1. the snprintf() there is creating the link name, not referenced name. The first argument to devfsadm_mklink() is the path appended to dev_path to create link name.

  9. usr/src/cmd/devfsadm/mapfile-vers (Diff revision 23)
     
     

    Remove this when you relocate the function to disk_misc.c

    1. and this is also already commented... :)

  10. usr/src/cmd/format/menu_fdisk.c (Diff revision 23)
     
     

    This branch needs an explanatory comment .. why are lofi disks treated specially here?

  11. usr/src/cmd/format/menu_fdisk.c (Diff revision 23)
     
     

    Again, why do we skip this check for lofi?

  12. usr/src/cmd/lofiadm/main.c (Diff revision 23)
     
     

    This seems like a seperate issue to lofi?

  13. usr/src/man/man1m/lofiadm.1m (Diff revision 23)
     
     

    Passive voice.. reword as "Labeled devices may not be specified." or somesuch.

  14. usr/src/man/man1m/lofiadm.1m (Diff revision 23)
     
     

    in the zone configuration...

    1. rewrote sections in manual.

  15. usr/src/uts/common/fs/vfs.c (Diff revision 23)
     
     

    While here, should this be explicity (void) in the arguments list? (C99 doesn't permit empty argument lists I think...)

  16. usr/src/uts/common/io/lofi.c (Diff revision 23)
     
     

    do you really need to doubly dereference this to get the lock? Is this really a separate mutex? (that would be unusual, but I didn't check).

    1. yes it is separate mutex, I did copy this kstat update from lofi_strategy() (https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/io/lofi.c#L1402) the lofi_strategy() itself can not be used there of course as lofi_tg_rdwr() is callback used from cmlb. Also I did want to keep the non-labeled lofi logic about IO (of course the partition checks had to be introduced, but those are well isolated), so I kept kstat handling exactly as it was. In fact, initially I did miss this kstat_waitq_enter() call and thats how the issue #6599 was found...

    2. OK with your reply -- I didn't follow up myself.

  17. 
      
rm
  1. I'd like to see a bit more of a design written up somewhere,
    particularly addressing the safety of renaming the /devices entries and
    the impact that'll have on conumsers. This hsould also touch on the
    general use of cmbl and sysevents.

    I will come back and finish reviewing lofiadm changes once the manual
    page is updated and the answer to the undocumented options is clarified.

    The sysevent usage here makes it clear that a better description /
    architecture description will seriously facilitate the review. Can that
    be written up somewhere?

    Particularly how all the devices coming and going are designed to
    interact with lofi and with sysevents, particularly the plan to recover
    from lost sysevents. With those in place, I'll have more context and be
    able to actually perform a proper review of the cmlb / lofi changes.

    1. I add the description here, so other reviewers can see it as well.

      The idea behind this work is to add an additional way to create block device mapping on top of file, to emulate disk devices by supporting partition table by using cmlb module implementing the actual partition related interfaces. At the same time, lofi should still continue to support traditional method for creating device mapping. The labeled device support is needed to be able to prepare more complex disk images and to support installation of boot program on such image.

      From user point of view, this change will introduce new command line option for lofiadm command (-l) to specify the labeled mapping should be created, and as an result, instead of traditional /dev/[r]lofi/X device, such mapping will provice an access to created device via /dev/[r]dsk device tree.

      As we do not have distinct notion for whole disk devices (there is ...p0 device and device name without slice part, but used only with GPT partitioning), I did decide to follow an approach used in Oracle Solaris and when labeled mapping is created, the lofiadm will report back the device name based on p0 notation, and for consistency, this approach is taken for both x86 and sparc platforms. Using p0 device name resultied in need to adjust the cmlb driver to create needed minor node on both platforms.

      The much bigger issue is about how lofiadm could get the disk name created for labeled mapping. The problem is, device links providing device names in /dev tree, are created by devfsadm[d], which is running only in global zone. So when the driver is generating minor node(s) for devices, sysevent is generated to announce devfsadm about new device nodes, devfsadm will create the device links and generate sysevents about new device links (and similar chain will happen for device removal).

      So my initial idea was just to listen for sysevents from devfsadm with lofiadm command, but unfortunately this approach can not be used in local zones, as sysevents are not supported by local zones.

      Therefore the followup idea was to listen for such events with lofi driver and return the mapped disk name as response to ioctl() which was used to create the mapping. Unfortunately devfsadm generated events are not distributed inside the kernel, so I was in need for mechanism to relay devfsadm events to lofi driver from log_sysevent.c.

      In this implementation lofi driver is receiving EC_DEV_ADD message from devfsadm to create devlink name entry in cache maintained by lofi driver, and EC_DEV_REMOVE message to remove cached entry. Cache is implemented as nvlist with NV_UNIQUE_NAME and lofi instance number used as name.

      The overall result is [too] complex and depending on devfsadm to post events about device link creation, but for time being I really dont see any other existing mechanism to use for such purpose. Still the solution appeares to work reasonably well at least for my own (limited) testing and afterall, the devfsadmd does seem to work quite reliably.

      Also the device link name information passing is not really vital for lofi mappings, this function is built to help lofiadm to report back the device name without overloading the system; The last resort to find device name for mapped labeled device would be to browse /dev/dsk tree and test every device node for lofi instance number returned as an response for creating the mapping. This option is still possible, but I think the event processing is better solution than resolving symbolic links in /dev/dsk tree and testing for lofi instance ID.

      In case the event from devfsadm is late or missing at all, the lofiadm will not be able to report back the correct device name in case of labeled devices; however lofiadm can still be used to retrieve the device names while run without arguments, and to remove the mapping, user can check for device names manually from /dev/dsk tree or for example, from disk listing of format command or remove the mapping using the file name used to create the mapping.

      To use cmlb interfaces by lofi driver itself, I did update lofi driver to create new instance, as using instances as opposed to minor numbers do simplify the mapped device identification and overall housekeeping for mappings. Such change is reflected in lofi device names in /devices tree, but not for links in /dev tree, so for user commands the change should not be visible.

      Also, as partitioning is created to support preparation of disk images (such as for usb sticks), the implementation does not support combining compression/encryprion for labeled mappings.

      In case of labeled mapping, the ioctl() calls are passed to cmlb_ioctl(), achieving support for partition related functions. For non-labeled case, only lofi internal ioctl implementations are used.

      The first lofi instance, instance 0, is used as control device with device node /dev/lofictl and first mapped device is instance 1. This behavior also does match old implementation.

      I hope I have covered at least most important parts there.

  2. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    Shouldn't this actually be an error case? What happens if you end up opening and reading something else in there due to truncation?

  3. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    Is there a reason that you're only checking for the substring and allowing it to be anywhere? Isn't this going to always be under /devices as it shouldn't be a symlink in the NGZ, though it is possible for someone to create one there.

  4. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    The language here sounds a bit of and is a bit confusing. Does something like this match what you're trying to say:

    'When we're removing a labeled lofi device, a final sysevent will be generated when the block and character device nodes are removed. We filter these events out as there should be nothing left for us to do.'

    However, I'd like to hear a bit more about why this is okay. Presumably the reason is that we'd only bother telling users about the individual block and character devices as opposed to the labled device as a whole?

  5. usr/src/cmd/devfsadm/devfsadm.c (Diff revision 23)
     
     

    I think this comment would be clearer if it said something like the following, assuming my understanding of your intentions is corect:

    If we have a labeled lofi device, The first entry has no minor associated with, but is always the raw disk device. Therefore we assign it an explicit minor node.

  6. usr/src/cmd/devfsadm/disk_link.c (Diff revision 23)
     
     

    Can you make this snprintf while we're here? That's a terrifying sprintf.

  7. usr/src/cmd/devfsadm/mapfile-vers (Diff revision 23)
     
     

    Please do not adjust other companies copyright statements.

  8. usr/src/cmd/lofiadm/main.c (Diff revision 23)
     
     

    What's this doing here? This is undocumented.

  9. usr/src/cmd/lofiadm/main.c (Diff revision 23)
     
     

    What's this doing here, it's undocumented.

  10. usr/src/man/man1m/lofiadm.1m (Diff revision 23)
     
     

    This section is very confusing. All in all, the definition of -l and the act of labeling a device is very unclear to the user. It's telling me that data may be accessed through slices or partitions, but how do those come into existence. Presumably they don't exist the first time we do this and things will have to be created, etc.

    Here, the 'Assocated device is associated with device file ...' sentence doesn't really make sense. You'll want to rewrite that.

    More broadly, I'd actually add a specific section on labeled devices in the description (use a subsection). In that section I'd want to be able to answer the following different questions:

    • What does it mean to lable the device?
    • How does partitioning it work?
    • How do devices come into existence with the act of partioning?
    • What happens if I try to edit the partition table while there are active minor devices from the partition table?

    etc.

  11. usr/src/man/man1m/lofiadm.1m (Diff revision 23)
     
     

    This in its own paragraph is somewhat confusing. I think it'd be clearer if this were part of the previous paragraph and it said something like:

    'A device may not be specified when using a labeled lofi device.'

  12. usr/src/man/man1m/lofiadm.1m (Diff revision 23)
     
     

    This section is very unclear. I as a naive user have no idea what it means to label a device. What is a label? What does it do for me? See the comment earlier in the description. This two should be reworked together.

  13. usr/src/uts/common/io/cmlb.c (Diff revision 23)
     
     

    What does node availability actually mean? Can we clarify the comment here?

  14. usr/src/uts/common/io/cmlb.c (Diff revision 23)
     
     

    It appears that _SUNOS_VTOC_16 is only defined on x86 platforms and not on SPARC platforms. Can you describe what it was safe to change this and eliminate the other case?

    What are the implications for SPARC with this?

    1. yes, _SUNOS_VTOC_16 is still defined only on x86 platform, and the part != P0_RAW_DISK part of the if statement is making sure the p0 partition will get assigned the size, while queries for other zero sized partitions will be responded with EINVAL.

      Now allowing p0 partition to be present in case of labeled lofi on both platforms means the same partition number check should be present for both platforms and it means the preprocessor #if .. is no longer needed as we do want to fill p0 data.

      The P0_RAW_DISK is defined as NDKMAP (8 in case of sparc and 16 in case of x86), so it has unique value and does not conflict with entries in VTOC. Also an array for partition entries is allocated by NSDMAP (NDKMAP + 1 for sparc), there is space in partition array.

      As the p0 is only enabled for lofi mapped labeled devices on sparc, normal disks will not be matched by P0_RAW_DISK, as part is always < NDKMAP for normal disks. But if by any reason the part == 8 will be queried on sparc, the cl_map[P0_RAW_DISK] is initialised in cmlb_validate_geometry(), and (valid) values will be returned. Normally drivers are calling cmlb_partinfo() based on existing partition, I think only sd is iterating over 0 to NDSMAP, but since sd has its own definition for NDSMAP, it will not create issues.

      So the implications for sparc are that in case of labeled lofi, we should have p0 size properly set, even if there is no other practical use from such fact than we can present this device node as an result of creating lofi mapping and use it to remove the mapping.

  15. usr/src/uts/common/io/cmlb.c (Diff revision 23)
     
     

    There are enough lines here so please use { } around bits.

  16. usr/src/uts/common/os/log_sysevent.c (Diff revision 23)
     
     

    Rather than punctuate and captialize this, fold it into the comment at the top of the function and eliminate this.

  17. usr/src/uts/common/os/log_sysevent.c (Diff revision 23)
     
     

    Can you describe more of the architecture here? For example, what happens to downstream folks if we never send this sysevent downstream?

    Also, why are we binding and creating this on every event? Presume that we have to deal with a high rate of these for some reason, could this end up creating suprious errors as opposed to keeping around a single global bind?

  18. usr/src/uts/common/os/log_sysevent.c (Diff revision 23)
     
     

    Can you describe the architecture here? This feels like we're relying on these sysevents for other subsystems to function correctly. Is that actually the case?

    If so, how are failures being tracked and logged. You certainly can't just blindly assume that sysevent_evc_publish will succeed even if you're using EVCH_SLEEP.

  19. usr/src/uts/common/os/log_sysevent.c (Diff revision 23)
     
     

    This name seems a bit confusing. The idea of posting an event seems like a common thing that most folks using sysevents will think about. This isn't a generic event posting mechanism, can you think about a better name?

  20. usr/src/uts/common/sys/cmlb_impl.h (Diff revision 23)
     
     

    This comment on its own seems a little tricky. Perhaps you want to say, something to the effect that on SPARC we want to allow an entry for the entire disk (p0) which is used by lofi.

  21. usr/src/uts/common/sys/cmlb_impl.h (Diff revision 23)
     
     

    Can you confirm that NSDMAP doesn't actually make it out to userland and get embedded in anything? From my brief look it appears that a lot of maximum partition values are used here for sd and others.

    1. cmlb_impl.h is not installed into /usr/include; sd is using its own instance of NSDMAP definition in http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/scsi/targets/sddef.h#100

      I think it is as safe as it can get as there is published /usr/include/sys/scsi/targets/sddef.h

  22. 
      
tsoome
tsoome
tsoome
tsoome
rm
  1. 
      
  2. usr/src/cmd/devfsadm/disk_link.c (Diff revision 27)
     
     

    This code all makes me wonder how 20 is the right magic number.

    1. as discussed, set to 23 as the largest string from uint32 is 10 chars, so we have 2x 10 + 2 for t and d and terminating 0. Just to be at safe side.

  3. usr/src/cmd/format/menu_fdisk.c (Diff revision 27)
     
     
    This sentence sounds a bit odd. How about 'Labeled device support in lofi provides the p0 partition on both x86 and sparc.'
  4. usr/src/cmd/format/menu_fdisk.c (Diff revision 27)
     
     
    This sentence sounds a bit odd. How about 'Labeled device support in lofi provides the p0 partition on both x86 and sparc.'
  5. usr/src/cmd/lofiadm/main.c (Diff revision 27)
     
     
    I think this comment could be made more clear. How about:
    
    'If devicename does not exist, then devicename contains the name of the device to be created.'
    
    The bit about unlabeled devices, is that referring to the input set of devicename or something else?
    1. Yes, it is the case when user is trying to assign mapping to specific lofi instance. Since there is no way to assign controller number
      (devfsadm has its own internal logic to assign controller number), only unlabeled mappings can have such user settable device names. Well to be exact, once the system has created first labeled mapping, user can know controller number, but I think, implementing all the proper checks makes it all too complicated to be worth.

  6. usr/src/cmd/lofiadm/main.c (Diff revision 27)
     
     

    This needs to take a buffer length for path. You should not be relying on strcpy and sprintf. Please correctly use bounds checking functions like:

    • strlcpy
    • snprintf
    • strlcat
  7. usr/src/cmd/lofiadm/main.c (Diff revision 27)
     
     

    Do not use strcpy, use strncpy/strlcpy. All buffer operations should be bounded.

  8. usr/src/cmd/lofiadm/main.c (Diff revision 27)
     
     

    Do not use strcat! All buffer operations should be bounded, e.g. strncat/strlcat.

  9. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     
    You should not be using \fB in mandoc. You can format these and the dozen other versions by doing something like:
    
    .Sy lofi
    
    I'm not going to mark all of these.
  10. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     

    I think you want commas around this.

  11. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     

    This sentence sounds a bit off. How about, 'Before using these devices, users should create or verify the partition tables by using tools like format(1M) and fdisk(1M).'

  12. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     
    How about:
    
    'Once the device has been appropriately partitoined, the labeled device can be used as a nromal disk to create...'
  13. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     

    I think 'on system reboot' could be a bit more general as it happens on power loss, etc. Not just when someone types reboot.

    How about, '.. are not permenanent and not persisted by the system. If power is lost or the system is rebooted, then they will need to be created again.'

  14. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     
    Some minor suggestions, how about:
    
    The partition table requires space from the mapped file.
    .Sy lofi
    does no support converting previously created unlableld loopback device images to labeled loopback devices. If an unlabeled device is used as a labeld device, writing to it will corrupt it.
  15. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     
    Shouldn't this be a .Sh, not a .Ss?
    1. The mdoc(5) does not actually enforce it, it is only suggesting:

      Since the DESCRIPTION section usually contains most of the text of
      a manual, longer manuals often use the Ss macro to form
      subsections.  In very long manuals, the DESCRIPTION may be split
      into multiple sections, each started by an Sh macro followed by a
      non-standard section name, and each having several subsections,
      like in the present mdoc manual.
      

      As lofiadm manual is not that long, I figured it makes sense to have Options and Operands as subsections. Also in individual sections, the Options and Operands would be non-standard section names which by itself would be allowed, but not well justified due to the length of the manual. Based on concerns above, I'd keep .Ss and drop this issue.

    2. That's a bug in our mdoc(5) that got missed when we adapted it for our manual page conventions. If you look at all of our section 1 and section 1M manual pages, that's how they're written. It should be kept to match the style of everything else.

  16. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     

    Why is this the case? That seems like an odd restriction. Aren't they used in the same command invocation? Reviewing the implementation it doesn't seem like -l needs to come before -a due to both being covered by the use of getopt().

    1. Yes it is bad wording, meant with -a as -l by itself, or with alternate options, is not allowed.

  17. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     
    This should be a .Sh.
    1. Same explanation as with Options above.

  18. usr/src/man/man1m/lofiadm.1m (Diff revision 27)
     
     

    I get that you wanted to add this, but this seems wrong for a couple reasons. First off, I doubt it was uncommitted to begin with. Secondly, you likely want to specify something like we do with a lot of other commands do separating the output format from the command line syntax.

  19. usr/src/uts/common/io/cmlb.c (Diff revision 27)
     
     

    Is there a reason that these are cast to void?

    1. me being sloppy. implemented the proper error check now.

  20. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    The big theory statement needs to be updated with the addition of labeled devices and how it changes things.

  21. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    The big theory statement needs to be updated with the addition of labeled devices and how it changes things.

  22. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Captialize and punctuate.

  23. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Should this relaly be a sleeping allocation?

    1. I think it should; the issue is, when cmlb is calling DK_TG_READ callback, the default case for return value would be EINVAL/EIO and it would be wrong result. Also as I did check the blkdev for example, it is using same logic, except only with case of dump (polled mode) the KM_NOSLEEP is used, but lofi is not implementing dump anyhow.

  24. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    The sentence is a bit confusing. Not sure what direction has what information and what we're trying to gather.

    Also, in general please capitalize sentences.

  25. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Capitalize and punctuate.

    Maybe 'to determine the capacity, we return an error if the cookie is set. If we don't, lofi_attach will deadlock.'

    But how will it deadlock?

    1. I did rewrite the comment a bit, hopefully for better.
      Perhaps the deadlock is bad term there, replaced it with "stuck"; the problem is, when mapping is created (ioctl()), the lofi will create new instance, lofi_attach() will be called and eventually cmlb_attach(), which will use callback to get device capacity from lofi. Since mapping is not yet done, lsp->ls_vp_ready is B_FALSE and nothing will change it as lofi_attach is waiting cmlb_attach() to return, all those steps are happening in single thread. Thats why I replaced "deadlock" with "stuck", as deadlock may direct one thinking about threads.

  26. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    So the value of tg_cookie is not part of the invariant here?

    1. yes, the cookie is set only with cmlb_attach call to recognise if the mapping is still in process. Other possible calls of cmlb api are coming from different thread(s) and can therefore wait till mapping is complete.

  27. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    I think I understand why this is now necessary, but would apprecaite confirmation.

    1. I am not entirely sure if it is strictly necessary, but since lofi mappings can get stacked (mapping image, mount file system, map image from this mounted file system...), I did put it in for safety, to make sure the caches are written down. I dont know the file system/page cache interface well enough to tell for sure if VOP_PUTPAGE() is definitely needed or not.

  28. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    This comment would be a bit more helpful if it explained why these things were true. e.g. why is a non-blocking open allowed to continue and why is a blocking one forced to fail.

  29. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    I think you want the fnvlist related functions here.

  30. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Capitalization and punctuation. This sentence sounds a bit odd as well.

  31. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    explicit comparison

  32. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Does unbinding gaurantee that we unsubscribe?

    1. thats how sysevent_evc_unbind() is implemented, its calling evch_chunsubscribe() first, then evch_chunbind().

  33. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Please use { } around that else case.

  34. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    Capitalization and punctuation.

  35. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     
    This is missing articles. Try:
    
    'With the addition of encryption, we must be careful that the key is wiped before the kernel's data structures are freed so it cannot accidentally slip out to userland through unitialized data elsewhere.'
    1. This comment was not added by me, but updated nevertheless;)

  36. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    These two functions simplify

  37. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    I think these two lines have extraneous commas.

  38. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    No strcpy!

  39. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     
    Capitalize
  40. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    What was the compiler complaining about here?

    1. Apparently it's leftover from some earlier development stage where nvlist processing was not yet implemented. Removed and confirmed clean build.

  41. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    This sentence could probably be rephrased to sound a bit better. Perhaps

    'We need to copy the device link name for lofiadm.' Though it seems almost a bit redundant.

  42. usr/src/uts/common/io/lofi.c (Diff revision 27)
     
     

    The minor number

    Need capitalization.

  43. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     

    device link names to the

  44. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     

    s/response//

  45. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     
    Missing some articles in this line. How about:
    
    '... to announce the device name from the /dev tree ...'
  46. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     

    as the device name

  47. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     

    have a better

  48. usr/src/uts/common/os/log_sysevent.c (Diff revision 27)
     
     

    Can we bump a counter in the case of failure?

    1. As discussed, will leave as is as there seem to be no existing counters for events. It may be worth implementing kstat for sysevent/event channels, but I figure, it would be task for separate project.

  49. usr/src/uts/common/sys/lofi.h (Diff revision 27)
     
     

    With all the lofiadm stack allocations are we properly initializing these members there?

    1. As much as I have walked through, I think yes. I also did add bzero() to lofiadm add_mapping() to zero out the structure just for safe side.

  50. usr/src/uts/common/sys/lofi.h (Diff revision 27)
     
     

    Missing structure prefix.

  51. 
      
tsoome
tsoome
tsoome
tsoome
rm
  1. Thanks for doing a bunch of the clean up here.

  2. usr/src/cmd/devfsadm/disk_link.c (Diff revisions 27 - 30)
     
     
    This can overflow the buffer. The problem is that the buffer is PATH_MAX bytes. If the link is longer than the size of the buffer, it'll return PATH_MAX. However buf[PATH_MAX] indexes beyond the end of the buffer.
  3. usr/src/man/man1m/lofiadm.1m (Diff revisions 27 - 30)
     
     
    missing 'a' before loopback device
  4. usr/src/man/man1m/lofiadm.1m (Diff revisions 27 - 30)
     
     

    missing 'a' before labeled loopback

  5. usr/src/man/man1m/lofiadm.1m (Diff revisions 27 - 30)
     
     
    New line after the fdisk reference.
  6. usr/src/man/man1m/lofiadm.1m (Diff revisions 27 - 30)
     
     
    Insert 'the' before mapped file.
  7. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
tsoome
Review request changed

Status: Closed (submitted)

Loading...