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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]