[getting back to this after *loong* time, sorry] On Fri, Jul 17, 2015 at 02:30:20PM +0200, Erik Skultety wrote:
Actually, I found out this is easier to review as a one patch with various diff options used for various parts of the patch. Some questions and suggestions below. Why vshClientHooks and vshCmdGrp aren't in the vshControl struct? If we move the client helpers into a library (I think this stuff can be in src/util/virshell.c for example), then it won't be thread-safe. Moving those to the control structure will also be cleaner.It would be nice, but since readline-defined user callbacks do not contain any opaque argument in their signature, you can only rely on statically declared data, therefore you can't do this with command groups, although it does work for client hooks at least.
That's fair. Let's keep it as a static global then.
Some things are still broken up, like readline stuff. It should be either completely hidden or completely exposed. For example, vshReadline{Init,Deinit} should be moved into vsh{Init,Deinit}. Commands from former virshCmds (except connect) should be in vsh.c so each client can use them in their cmdGroups definition without re-implementing them.Well, I thought off 2 possible approaches how to tackle this one. The first one is (though not my favorite) to simply move handler implementations and structures initialization to vsh.c. As we do specify all the command groups and options in fixed arrays, we would have to create a completely new group for the generic commands available to every client. In virsh for example, that would leave us with 'connect' command only which we should create a separate group to make it all work. To be honest, I don't like this idea at all.
Me neither.
The other idea however, might be nicer, but much more complex and it's for a debate if we really want that and if so, I think it should be a separate follow-up patch, definitely not part of v2. So, the idea is to implement command group register mechanism using linked lists. That way, each client would have to register every group individually. And because lists are easily extensible, we could just add the 'connect' command to the 'virsh' group by implementing some internal APIs to command group management (add/update/whatever).
That's nice, but I had a different thing in mind. I though that vsh.c would create and export common command definitions and it'd be up to the client whether it uses them in one of its command groups or not. Easier than the first version and more extensible then the second one.
vsh is not doing the argument parsing, but that's be fine. I would, at least, wrap some options in a function that can be called from multiple clients, but that's one of the nice-to-have things that can be done later.There is a reason for this. In my opinion, each client should be responsible for parsing their own command line arguments. It doesn't really matter that all the clients most likely will implement usage, help, connect arguments...Being generic in this case would require each client to provide their callbacks for generic options, their specific options list and callbacks to handle these specific options as well. Maybe I just don't see it the way you see it :), but I still think, in this specific case we shouldn't try to split the code even more.
I saw it the other way around. I thought there could be a teeny tiny function for parsing common arguments that all clients *could* call, but as I said, that's not necessary and not thought through. Just a possible future idea.
vshInit() is declared with ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); but handles passed NULLs properly, that declaration should be fixed.Fixed in v2.Also there are some whitespace problems (e.g. with parameters of virshLookupDomainBy), but considering how many of them are there inside the files already, it's nothing compared to the size of this refactor.I tried to fix in v2 as many as I could find.The exclude_file_name_regexp--sc_avoid_strcase should only contain ^tools/vsh\.h$$, not virsh.h.Fixed.Anyway, here's a list of things that should be changed (either from virsh to vsh or vice versa) split into categories (feel free to disagree with any): Totally: - virshCommandOptTimeoutToMs^^ Something tells me I overlooked this one and pushed v2 to my forked repo without it...Sigh, v3 it is..- VIRSH_MAX_XML_FILE - virshPrettyCapacity - vshFindDisk - vshSnapshotListCollect Most likely: - vshCatchInt - virshPrintJobProgress^^currently this is only used for migration and blockjobs and so far, we haven't talked about some time consuming admin jobs, so I'd say either we leave it for good or we can move it to the lib some time later (possibly with virsh-edit stuff, see below)
We can, yes. I'm just really afraid of these two clients having more and more re-implemented in both of them. Basically I don't want these to end up as qemu and lxc drivers :)
- virshTreePrint(internal) with virshTreeLookup typedef - virshPrintRaw - virshAskReedit - virshEdit* - virshAllowedEscapeChar^^ This one should be client dependent, whatever the reasons to different allowed sets of escape chars might beMaybe (i.e. not needed now, but might be nice): - virshWatchJob - virsh-edit stuff^^ I'm not sure how you meant this one, did you mean just renaming the macros inside and the module itself or you meant a complete refactor, spliting the module, etc.???
I meant just a rename. Martin
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list