On Mon, Jun 29, 2015 at 05:37:34PM +0200, Erik Skultety wrote:
The idea behind this is that in order to introduce virt-admin client (and later some commands/APIs), there are lots of methods in virsh that can be easily reused by other potential clients like command and command argument passing or error reporting. !!! IMPORTANT !!! These patches cannot be compiled separately, the series is split more or less logically into chunks only to be more readable by the reviewer. I started this at least 4 times from scratch and still haven't found a way that splitting virsh could be done with several independent applicable commits, rather than having one massive commit in the end.
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.
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.
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.
vshInit() is declared with ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
but handles passed NULLs properly, that declaration should be fixed.
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.
The exclude_file_name_regexp--sc_avoid_strcase should only contain
^tools/vsh\.h$$, not virsh.h.
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
- VIRSH_MAX_XML_FILE
- virshPrettyCapacity
- vshFindDisk
- vshSnapshotListCollect
Most likely:
- vshCatchInt
- virshPrintJobProgress
- virshTreePrint(internal) with virshTreeLookup typedef
- virshPrintRaw
- virshAskReedit
- virshEdit*
- virshAllowedEscapeChar
Maybe (i.e. not needed now, but might be nice):
- virshWatchJob
- virsh-edit stuff
- 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
Martin
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list