[00:57:10] *** Quits: ziyeyang_ (ziyeyang@nat/intel/x-cbospnurwbpngwnf) (Quit: Leaving) [01:04:42] *** Quits: Param (~Param@157.49.190.18) (Quit: Param) [01:51:21] *** Joins: Param (~Param@157.49.190.18) [02:35:49] *** Quits: Shuhei (caf6fc61@gateway/web/freenode/ip.202.246.252.97) (Ping timeout: 260 seconds) [05:45:20] *** Quits: Param (~Param@157.49.190.18) (Quit: Param) [05:56:43] *** Joins: Param (~Param@157.49.190.18) [06:32:02] *** Quits: mszwed (mszwed@nat/intel/x-khjpqpcklzkgfept) (Read error: Connection reset by peer) [06:33:13] *** Joins: mszwed (mszwed@nat/intel/x-ixocqcggmxgmxnzn) [07:04:05] *** Quits: sbasierx (sbasierx@nat/intel/x-ujvjxsmxaqdtzffg) (Quit: Going offline, see ya! (www.adiirc.com)) [07:33:15] *** Joins: tkulasek (86bfdc49@gateway/web/freenode/ip.134.191.220.73) [09:27:31] jimharris: https://review.gerrithub.io/#/c/400894/ is ready for review (changpe1 already +2'd it), and then we can merge that series [09:58:52] i have a question about the uuids - i was thinking in some cases, we may want the uuid to be automatically provided by the bdev - for example, logical volumes already have a uuid - how would you see that fitting in with this patch? [10:02:46] that sounds like a good idea - I was thinking of auto-generating one if it's not specified, but letting the bdev provide it would be even better [10:03:08] we should add a bdev_get_uuid() interface, then use that as a fallback if the user doesn't specify one in the create call [10:03:41] that should fit into the current model defined by this patch; just retrieve the uuid as the default value of the field and let the user override with a JSON-RPC parameter if they want to [10:04:16] what if we did this the other way around? instead of users specifying the uuid at the upper level - have an optional parameter to specify it for the bdev module rpcs? [10:04:20] just a thought [10:04:50] so then when a bdev gets registered, if no uuid was specified by the bdev module, the bdev.c code can create one [10:05:45] just thinking it might be confusing to allow a user to override the uuid when attaching the bdev to an nvme namespace [10:06:33] then bdev.c could also enforce uuid uniqueness [10:07:20] *** Quits: tkulasek (86bfdc49@gateway/web/freenode/ip.134.191.220.73) (Ping timeout: 260 seconds) [10:07:35] Hey guys, as I await additional review of https://review.gerrithub.io/#/c/401243/, I note in the Change Info Block (upper center) in red, "Cannot Merge". Does this imply that I need to do another pull and rebase with the expectation I may have to hand-merge? [10:08:28] lhodev: exactly. Just rebase on top of current master. [10:10:18] mszwed: thank you. I'll await doing that until collection any additional reviews so that I can incorporate changes and conduct the rebase before the next submission. [10:10:33] jimharris: hmm, that might be better overall, although I think most bdev types won't have a UUID to expose, so the user will end up having to specify it anyway [10:11:37] i agree that most bdev types won't have a UUID to expose - we can provide an optional uuid rpc parameter to let user specify one, or have bdev layer create it automatically when bdev module doesn't specify it [10:14:18] is there a way to expose a volume UUID via iSCSI or vhost-scsi? (as potential other users of this interface) [10:16:43] fairly certain there's an inquiry vpd page for that [10:16:52] * jimharris is looking [10:22:53] it would all be done through something in INQUIRY VPD 0x83 - but not sure exactly which [10:26:25] in any case, I think it would still be nice to have a way to override UUID at the nvmf_tgt level [10:26:34] and only partially because that's what I've already written :) [10:27:08] if we implement the bdev uuid interface, the right thing will happen automatically in most cases, and then the user can intervene if they have a different UUID assignment [10:27:43] i just think it will be confusing to potentially have 2 different uuids for a logical volume [10:29:26] and how this would work with multipath [10:41:31] hmm, seems like lvols don't have a UUID currently - only lvol stores do [10:41:44] would we add that as a new xattr or something? [10:41:54] yes [10:42:04] hmmm - i thought we had uuids for lvols [10:42:23] I thought so too, but I can't find it [10:43:20] the superblob for lvols has a uuid xattr, but normal lvol blobs don't [10:43:33] yeah - well I think we should add it [10:45:18] drv: could you look at https://review.gerrithub.io/#/c/399796/ [10:50:19] we should really not be adding more allocation failure checks with assert() [10:58:30] can you look at https://review.gerrithub.io/#/c/401668/ and tell what you think? If it is ok I will squash it with common_spdk. [10:59:35] I mean the idea itself not the content of common_spdk itself :D [11:00:51] i'm fine with it [11:02:21] i'll wait to +2 the patches until you've squashed this one with the first patch in your series [11:02:31] seems OK to me too [11:03:17] we habe uuids only for lvol stores [11:04:01] lvols take that uuids, add blob id and this way they create their ids [11:07:13] I have update Commands Supported and Effects Log Page Ticket https://review.gerrithub.io/c/401314/3 [11:24:03] After the approval of a patch, if there are conflicts will the SPDK automation take care of rebasing or should we do it manually? [11:32:46] *** Quits: darsto_ (~dstojacx@89-68-135-211.dynamic.chello.pl) (Remote host closed the connection) [11:33:09] *** Joins: darsto_ (~dstojacx@89-68-135-211.dynamic.chello.pl) [11:33:32] *** darsto_ is now known as Guest21738 [11:57:27] *** Quits: Param (~Param@157.49.190.18) (Ping timeout: 240 seconds) [11:59:07] *** Joins: Param (~Param@223.237.224.48) [12:00:50] *** Joins: sethhowe (~sethhowe@134.134.139.72) [12:05:12] *** Quits: Param (~Param@223.237.224.48) (Quit: Param) [13:00:46] *** Guest21738 is now known as darsto_ [13:01:37] drv: about https://review.gerrithub.io/#/c/401217/ and https://review.gerrithub.io/#/c/401218/. Why you think bdev_XXX_dump_config_json() should produce "method"? [13:01:59] if somebody answered to your -1 review on gerrit (basically saying you're wrong) - how do you see it? [13:02:01] we are planning on renaming some of the bdev-related RPCs - a lot of the "construct" ones should be "create", for example [13:02:07] can you set any filter on this? [13:02:22] or do you have to check each patch every now and then? [13:03:12] I don't think there's any easy way to see them (unless you enable email notifications, which are very noisy) [13:03:28] but the reviews that have been commented on most recentlydo get bumped to the top of the list [13:04:27] recently I've tried to filter out -1's from my list to review though unless the patch is assigned to me [13:04:45] and i've been thinking about removing that part of my filter [13:06:06] drv: ok, but this still doesn't justify mixing RPC part with bdev_XXX.c. I tought that we wanted to keep thos layers (JSONRPC in XXX_rpc.c file and JSON configuration) separated. Otherwise why we have separate .c files for JSONRPC handlers? [13:06:39] well, the part emitting the JSON config needs to know about the format of the construct/create/whatever RPC method anyway [13:07:03] so it either needs to expose enough data through APIs so that the dump call can be totally in the RPC part, or split in some way [13:08:08] in general, it is nice to keep the RPC part isolated, but I don't think it's really practical with our current bdev API [13:14:54] darsto_: i take that back - i only filter out patches with -1 if they are assigned to someone else (theory being that if daniel is assigned a patch and it has a -1 on it, i don't bother looking at it until daniel or the submitter gets that -1 cleared) [13:16:08] drv: does astyle not check for if/while/for open brace on same line? [13:16:17] it should [13:16:22] do you have an example where it hasn't? [13:16:34] https://review.gerrithub.io/#/c/401243/3/lib/event/app.c [13:17:37] I think that's fallout from the other astyle bug we've seen before with functions that return enum types [13:17:45] you can see everything is broken after line 504 [13:18:15] we worked around that elsewhere by defining a typedef of that enum and using that as the return value instead [13:23:31] jimharris: noting your "…i only filter out patches with -1 if they are assigned to someone else…", I guess that means instead of waiting for other reviewers to come along, I should submit another series? That kind of bums me, as I was hoping I might get additional feedback about the macro I instantiated (SPDK_MAIN_APP_PARSE_ARGS) which Dariusz didn't care for. [13:26:04] i'm reviewing your patch right now lance :) yours did not get filtered in my view, since it is not assigned to anyone [13:26:34] Thanks Jim! [13:27:05] sethhowe: looks like centos-7 at least is missing eu-readelf - can you check which package that is in and add it to vm_setup.sh? [13:27:43] jimharris: it's clearer to me looking back at the gerrit-review page; i.e. distinction between "Assignee" and "Reviewers" [13:28:23] let's do a real time review here in IRC while we have drv watching [13:28:50] Sure [13:28:51] so I'm thinking we just change spdk_app_parse_args to just return void [13:29:04] instead of the SPDK_MAIN_APP_PARSE_ARGS macro [13:29:18] "we" meaning "lhodev" :) [13:29:38] So, if void, then retain its previous behavior that it would exit() instead of return()'ing? [13:29:55] yes - that's effectively what the macro does anywahs [13:29:56] anyways [13:30:13] That's true, though on a technicality, the macro isn't a library function ;-) [13:30:43] agreed - but that macro would be unlike anything we have in SPDK so far [13:31:02] but you're right - it would just make spdk_app_parse_args go back to returning void (that's what it returns today) [13:32:09] maybe spdk_app_parse_args and spdk_app_parse_args_retval? i don't really like the latter name, but the former could call the latter and do the exit if needed [13:32:22] callers could call spdk_app_parse_args_retval directly if they don't want the exit behavior [13:32:25] I figured I was venturing on new territory with that macro introduction, lol. It's a fine point, but the macro's intent (hence with the inclusion of _MAIN_ in the name) was for use within main and thus simplifying most people's use of this pattern. [13:34:09] I see where you're going with that. I was just h*ll-bent on trying my best to eradicate "if at all possible" any exit() invocations within the lib code. [13:37:02] i'd even be ok with just modifying the seven current callers of spdk_app_parse_args() [13:37:26] How so? [13:37:42] something similar to what darsto_ was proposing [13:38:16] i agree that deviating from 0 == success isn't normal [13:38:34] with the ptr-to-exit argument? [13:39:49] darek suggested returing INT_MAX to mean success - otherwise just exit(rc) (and in the help/usage case, it would return 0 so we'd get a non-error exit) [13:40:01] i'm not sure about using INT_MAX but returning 1 would be fine [13:42:39] any number is fine as long as it's different from EXIT_SUCCESS and EXIT_FAILURE [13:42:48] agreed [13:44:01] Yeah, ok. At the risk of beating a dead horse, I was just a little "less enthusiastic" about return()'ing a non-zero value for success, but I'd prefer to see that over making the function a void and leaving the exit() inside it. [13:47:27] I'll proceed with that implementation (i.e. return non-zero success), elimination of the macro, etc. I guess I could revert the return type back to 'int' instead of the enum to also address the formatting issue. [13:48:41] that is probably fine, as long as it's documented [13:49:53] I had updated the CHANGELOG.md and doxygen comments as part of my original series, so rest assured I'd update again to reflect it properly ;-). [13:51:39] regarding spdk_app_stop() with positive integers... [13:51:48] Yeah? [13:52:06] will it be clear what the difference is between a positive and negative return value to spdk_app_start()? [13:52:28] Clear where? As in the documentation? [13:55:04] just thinking about if a subsystem fails to initialize [13:55:22] those would return positive errnos? [13:56:07] Using gdb, I forced fails at various points. Some of the subsystems invoked from spdk_app_start() merely return something like '-1'. [13:56:23] yeah - those need to be fixed too :) [13:56:37] you're finding some good stuff to cleanup here [13:56:44] That can be another Trello ticket :) [13:57:19] 100% agree that the subsystem should be returning real errnos - i'm just not sure we need to differentiate between failures that happen in spdk_app_start() itself vs. code that gets kicked off by spdk_app_start() - i.e subsystem initialization [13:58:16] I thought about that Jim. My thoughts were that maybe there might be some scenario where an application wants to differentiate and "do something else". That's the policy level of the application (IMHO). [14:00:09] what if we did a pass through all of the spdk_app_stop() calls and made sure they (1) passed real errnos, and (2) SPDK_ERRLOG a good message [14:00:32] there's not much you can do programmatically from that [14:01:18] is there a specific example you have in mind where the spdk_app_start() caller would do something different if it returned >0 vs. < 0? [14:02:44] We could do something like that, for sure. I suspect that's a larger effort that perhaps should be separate from the minimal needed to remove exit()'s from lib code. And, no, I don't have an example of an app "doing something different" on the spdk_app_start() as I just altered the existing ones. However, somebody outside the SPDK-dev world in theory could in the future. [14:03:20] i agree - that would be a separate effort - short term i definitely want to get your patch in so that we remove exits from lib code [14:04:30] Ok, so can I proceed with the previously discussed changes, and then return to the spdk_app_start/stop, etc. in a future effort? [14:05:21] And an FYI: additional grep's through lib code indicated some exit()'s in the tracing and net code so I was gonna look at those next. [14:06:04] yeah - let's do that [14:06:38] i see other places in trace, reactors, etc. where you've found some cases where we do 'bad' things if spdk_app_start fails and then we call spdk_app_fini [14:07:08] i'm wondering if that spdk_app_fini stuff should be done inside of the app.c code and not require an explicit call by the application [14:08:06] Will do, and thank you for the interactive review. Most helpful. Yeah, re: the bad things and spdk_app_fini(). I discovered some of that via forcing fails with gdb to test the behaviors and then encountering some segv's hence those changes. [14:34:48] drv: can you take a look at https://review.gerrithub.io/#/c/400185/ [14:36:17] jimharris: I made a comment, not sure if I am missing something obvious [14:37:58] no - i'll fix it [14:40:48] this was a big in the original code that I moved too - there's another case that also needs to be fixed - I'm going to do that in a separate patch [14:41:05] are you ok with that? [14:42:37] sure [14:55:28] added reply to your comment and pushed new patch [15:16:43] jimharris: changes incorporated, pushed, built/pass. https://review.gerrithub.io/#/c/401243/ [16:36:23] *** Joins: Shuhei (caf6fc61@gateway/web/freenode/ip.202.246.252.97) [16:48:56] Hi Jim, Daniel, would you take a minute and look at https://trello.com/c/pmS56iBI/19-spdk-how-to-remove-the-dependency-of-the-global-parameters-of-nvmf-tgt-and-iscsi-tgt-to-ini-config-file ? [16:50:43] Only NVMf-tgt and iSCSI-tgt need information described only in the .INI config file. [16:51:27] Porting this mechanism to JSON-RPC based config file is not so simple. [16:52:25] Hence my idea is to add the function to load and handle JSON config file in SPDK C library and use it. [16:53:29] I will ask to Pawel this afternoon too. [17:00:51] This is never urgent and gerrit will be easy for you to give feedback. please wait for a while. thanks. [17:06:17] hi Shuhei [17:06:26] i'll read the trello card now [17:07:41] thank you, Jim, I hope question is simple and you spend a minute. [17:09:10] your comments are clear - i'll add some notes to the Trello card [17:09:26] thank you! [17:14:21] i've posted some comments [17:21:10] Hi Jim, thank you, I reply to Trello because Pawel and others can see. [19:16:47] *** Joins: Param (~Param@157.49.166.16) [19:58:08] *** Quits: Param (~Param@157.49.166.16) (Quit: Param) [21:10:02] *** Joins: Param (~Param@157.49.166.16) [21:59:28] *** Quits: Param (~Param@157.49.166.16) (Quit: Param) [23:37:54] *** Joins: ziyeyang_ (~ziyeyang@192.55.54.38) [23:42:23] *** Joins: sbasierx (sbasierx@nat/intel/x-hmtvthremkqgdoaw)