4:32 a.m.
[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