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

On Mon, Jun 29, 2015 at 05:37:34PM +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. 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