Created attachment 153967 [details] patch to libxo-ify iscsictl I've added libxo support to iscsictl, see attached patch. Example output: # ./iscsictl --libxo json,pretty { "iscsi-information": { "target": [ { "name": "iqn.2014-01.SomeHostName.istgt:SomeTargetName", "portal": "10.0.0.1:1234 ", "state": "Connected", "lun": [ { "id": 0, "device": "da1" }, { "id": 1, "device": "da2" }, { "id": 2, "device": "da3" }, { "id": 3, "device": "da4" } ] } ] } } # ./iscsictl -v --libxo json,pretty { "iscsi-information": { "target": [ { "initiator": { "name": "iqn.1994-09.org.freebsd:SomeName", "portal": "", "alias": "" }, "target": { "name": "iqn.2014-01.SomeHostName.istgt:SomeTargetName", "portal": "10.0.0.1:1234", "alias": "SomeAliasName" }, "auth": { "user": "iqn.1991-05.com.freebsd:SomeUser", "secret": "superSecretText", "mutualUser": "", "mutualSecret": "" }, "session": { "id": 4, "type": "Normal", "state": "Connected" }, "failureReason": "", "digest": { "header": "None", "data": "None" }, "dataSegmentLen": 131072, "immediateData": "Yes", "iSER": "No", "offloadDriver": "None", "lun": [ { "id": 0, "device": "da1" }, { "id": 1, "device": "da2" }, { "id": 2, "device": "da3" }, { "id": 3, "device": "da4" } ] } ] } } Marie Helene / Savagedlight
Created attachment 153993 [details] patch to libxo-ify iscsictl Updated patch so that comments adhere to style(9).
My motivation for implementing this is to enable bhyve to be backed by iSCSI LUNs without guessing which block device to use. This patch will enable bhyve (and other tools) to access the correct block device, given a hypothetical configuration of: disk { type: iscsi, target: someTargetName, lun: 4 } PS: I did a diff on output between original iscsictl and the patched version, and they were identical in default/text mode. (only tested with one connected target)
Created attachment 154027 [details] patch to LibXo-ify iscsictl Updated patch: New variable name now adheres to style(9). Root element of json/xml output renamed to 'iscsictl' as per Allan Judes suggestion on IRC.
Do you really need "lun ids" there? They are confusing, because they don't match actual LUN ids, as can be seen by "camcontrol devlist -v".
Few more nits: 1. You didn't convert err/warn calls in parse.y. 2. The <err.h> include is no longer needed. 3. Hm, this is curious - fields like "name" or "portal" are padded with spaces not only in "text" output - where it's needed for, well, aesthetic reasons, but also for JSON and XML outputs. Is that something that could be fixed, or a libxo deficiency?
Created attachment 154087 [details] Patch to LibXo-ify iscsictl periphs.c: Fixed typo in xo_emit which referred to lunNo instead of lun_no periphs.c: Moved lun_no declaration outside of the outtermost do-while loop, to persist value in cases where do-while does its thing iscsictl.c: Fixed formatting of "target->portal" and "target->name" value fields, so that encoded (json, xml) output doesn't have padding or length constrictions, while text output still does. Didn't find any others which needed this fix. parse.y: converted errors and warnings. Overall: Removed #include<err.h>
Thanks for the feedback! I've updated the patch to fix the things you mentioned (see comment associated with the patch file for details) > Do you really need "lun ids" there? They are confusing, because they don't match actual LUN ids, as can be seen by "camcontrol devlist -v". I might have made a fundamentally wrong assumption here. My train of thought was that as periphs.c is iterating the busses and attached devices, it would get the attached devices in the order of LUN number. This is what I observed on my system, and assumed I understood that piece of code correctly. It may however be a fluke, in which case that sub-tree of the json/xml output should be renamed to 'peripherals' (or code be added to retrieve correct LUN number). I did spot one error which could mess up the LUN numbers even if the aforementioned assumption holds true, which has now been fixed. The "lun_no" variable would previously reset to 0 every time periphs.c had to send a new query, which according to comments would happen if it has to iterate more than 100 devices. What do you think?
Created attachment 154090 [details] Patch to LibXo-ify iscsictl Forgot one err() in parse.y. Must have forgotten to compile after converting pase.y.
Thing is, LUNs are not neccessarily consecutive and starting at 0. You can setup the target to export luns 2, 4, and 42; displaying them as 0, 1, and 2 would be confusing. So, I'd suggest to either remove the LUN number altogether, or take it from CAM - I believe it should be in periph_result->target_lun. I'm not sure how to display it in traditional (text) output; perhaps you could just skip it, basically leaving the output as it is now. One more minor nit - comments in cases like below are redundant: /* Start Container: auth */ xo_open_container("auth");
Thank you for explaining just how wrong I was! I really appreciate the explanation for why my assumption was wrong, and the suggestion for how to achieve what I was trying to do. :) I'd prefer to leave text/default output as-is for compatibility reasons, and if you think so as well, I'll leave it as-is. Minor nit on redundant comments: They made sense at the time, but I agree. xo_open_* and xo_close_* already explains what's going on there. I've now updated my local code to provide proper LUN IDs. I'll upload a new patch tomorrow, after testing it in every way I can think of. I think it might be useful to also present information about which scbus+target (example: scbus3 target 0) the devices are attached at. I'm thinking of adding that information directly before the LUNs in the json/xml output. What do you think of that? For now, example output (not including scbus/target info): # ./iscsictl --libxo json,pretty { "iscsictl": { "target": [ { "name": "iqn.2014-01.HOSTNAME.istgt:SOMENAME", "portal": "1.2.3.4:1234", "state": "Connected", "lun": [ { "id": 0, "device": "da1" }, { "id": 1, "device": "da2" }, { "id": 3, "device": "da3" } ] } ] } } # camcontrol devlist -v (...) scbus3 on iscsi1 bus 0: <FREEBSD CTLDISK 0001> at scbus3 target 0 lun 0 (pass2,da1) <FREEBSD CTLDISK 0001> at scbus3 target 0 lun 1 (pass3,da2) <FREEBSD CTLDISK 0001> at scbus3 target 0 lun 3 (pass4,da3) (...)
Thanks! And yes, reporting bus and target IDs is a good idea.
Created attachment 154227 [details] Patch to LibXo-ify iscsictl Changes: iscsictl.c: Removed redundant comments which existed in previous versions of this patch periphs.c: Now stores 'target_id' and 'path_id' from iteration of devices/peripherals, and emits them for LibXo json/xml after the LUNs list. periphs.c: LUNs list and new 'target' and 'path' elements wrapped in element 'bus'. Example output: # ./iscsictl --libxo json,pretty { "iscsictl": { "target": [ { "name": "iqn.2014-01.SOMEHOST.istgt:SOMETARGET1", "portal": "1.2.3.4:1234", "state": "Connected", "bus": { "lun": [ { "id": 0, "device": "da2" }, { "id": 1, "device": "da3" }, { "id": 3, "device": "da4" } ], "path": 4, "target": 0 } }, { "name": "iqn.2014-01.SOMEHOST.istgt:TARGET2", "portal": "1.2.3.4:1234", "state": "Connected", "bus": { "lun": [ { "id": 2, "device": "da1" } ], "path": 3, "target": 0 } } ] } } # camcontrol devlist -v | grep -i freebsd <FREEBSD CTLDISK 0001> at scbus3 target 0 lun 2 (pass2,da1) <FREEBSD CTLDISK 0001> at scbus4 target 0 lun 0 (pass3,da2) <FREEBSD CTLDISK 0001> at scbus4 target 0 lun 1 (pass4,da3) <FREEBSD CTLDISK 0001> at scbus4 target 0 lun 3 (pass5,da4)
I've now added an updated patch to this PR, see associated comment for details on that. I'm unsure about how best to name/structure the output related to bus id and target. Currently, what camcontrol shows as 'scbus3' is shown by this iscsictl patch as '"path": 3'. As I feel this is a bit ambigious, I wonder if it's safe to assume the bus will always be scbus for iSCSI, and if so, if it's ok to name it "scbus" instead of "path" in the output. PS: I forgot to make the newest patch mark the old one as obsolete, and can't figure out how to obsolete the old patch right now.
Thanks. Before I commit it I first need to talk to libxo maintainer to fix a little problem with xo.h - routines such as xo_err() are not marked __dead2, which leads to false warnings from the compiler, which, in turn, results in a failed build.
I found two more problems. First, the target_id and path_id might be uninitialized if there are no LUNs (basically an "empty target"; perhaps not common, but possible). If you look at the code, you will notice that it's actually completely redundant, because we already know target id (0) and scbus (equal to session id); that's how we find the LUNs for the session. Second - I'm not sure what's going on there, but try to add iscsi session (iscsictl -A whatever) without iscsid running. With the patch, the failure reason is missing. Last - I think I'd like to rename some of the fields. Namely: "path" -> "scbus"; "target" (the instance) -> "session", as it describes a session, not just the target; the second "session" and "digest" containers looks kind of redundant; also "bus" -> "devices".
Hey, thanks for the feedback. I'll be looking at it this week. :)
* iscsictl -A whatever when iscsid isn't running: There was a missing closing tag for the format specification. Also a missing \n. :) * target_id and path_id being uninitialized: DOH! I should totally have caught that. I'll be using a boolean to check if it has been set. Added benefit: will only be setting these variables once. * periph_result->target always being 0: Ok. However, I think it's prudent to still use the looked-up value, as we'll know this info is correct even if someone would make the iscsi subsystem expose multiple targets per scbus at some point in the future. * Naming of the 'target' xo_instance: Good catch. I agree that 'session' would be more accurate for the topmost 'target'. It may also be unfortunate to have nested elements/containers with the same name as one of its parents. * 'Session' and 'Digest' containers: Yep, everything in these relate to the session itself, and it would make sense to place this information on the 'root' of each session. Session ID vs scbus: It seems like 'periph_result->path_id' isn't related to 'bus_result->unit_number' (session id). Example: After connecting and reconnecting all iSCSI targets a couple of times (-Ra and -Aa), the "Session ID" for the two targets are 4 and 5 respectively. Whereas camcontrol says the devices are connected to scbus3 and scbus4. Repeatedly reconnecting the targets keeps incrementing the session ID, but they keep appearing under scbus3 and scbus4. I'm currently outputting scbus=-1 and target=-1 when no scbus/target is found. Does this sound okay? Marie Helene
Sorry for delay. As for bus and target - well, all this code is in a single file, so if someone changes the iscsi subsystem to report more than one target, than this routine will need changing anyway. As for bus and target numbers: take a look at we look them up: we iterate over the table, looking for bus: if ((int)bus_result->unit_number != session_id) { ... and then we report LUNs. So, we always know the bus number (passed as the parameter to print_periphs() routine) and target number (0). The only unknown is LUN. One more thing: in the Makefile, instead of LDADD you can use LIBADD, like this: LIBADD= xo
(Also: do you perhaps have this on github? Would be much faster that way :-)
A commit references this bug: Author: trasz Date: Sun Apr 12 09:36:52 UTC 2015 New revision: 281461 URL: https://svnweb.freebsd.org/changeset/base/281461 Log: Add libxo(3) support to iscsictl(8). PR: 198396 Submitted by: Marie Helene Kvello-Aune <marieheleneka at gmail.com> MFC after: 1 month Sponsored by: The FreeBSD Foundation Changes: head/usr.bin/iscsictl/Makefile head/usr.bin/iscsictl/iscsictl.c head/usr.bin/iscsictl/parse.y head/usr.bin/iscsictl/periphs.c
Committed with few minor changes: whitespace/style(9) fixes and workaround for build with GCC. Thanks for cooperation! I'll close the PR after MFC to 10-STABLE.
According to marcel@, there are no plans to MFC libxo(3) to 10-STABLE.