> 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.
> 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.
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).
> 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.
> 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)
> - 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.???
> - vshLookupByFlags
>
> And of course all of the below:
> - vshDomainBlockJob
> - vshDomainJob
> - vshDomainEvent
> - vshDomainEventDefined
> - vshDomainEventUndefined
> - vshDomainEventStarted
> - vshDomainEventSuspended
> - vshDomainEventResumed
> - vshDomainEventStopped
> - vshDomainEventShutdown
> - vshDomainEventPMSuspended
> - vshDomainEventCrashed
> - vshDomainEventWatchdog
> - vshDomainEventIOError
> - vshGraphicsPhase
> - vshGraphicsAddress
> - vshDomainBlockJobStatus
> - vshDomainEventDiskChange
> - vshDomainEventTrayChange
> - vshEventAgentLifecycleState
> - vshEventAgentLifecycleReason
> - vshNetworkEvent
> - vshNetworkEventId
> - vshStorageVol
> - vshUpdateDiskXMLType
> - vshFindDiskType
> - vshUndefineVolume
^Wow, there is quite a lot of these..As I went through the list I found
out, the list is much longer, shame on me.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list