[libvirt] [PATCH 00/13] Move generic virsh data to a separate module vsh

[Martin Kletzander]

[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 be
>>
>> Maybe (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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150804/e36637b7/attachment-0001.sig>