On 04/20/2012 10:01 AM, Dmitry Guryanov wrote:
PVS driver is 'stateless', like vmware or openvz drivers. It collects information about domains during startup using command-line utility prlctl. VMs in PVS identified by UUIDs
s/identified/are identified/
or unique names, which can be used as respective fields in virDomainDef structure. Currently only basic info, like description, virtual cpus number and memory amount implemented.
s/amount implemented/amount, is implemented/
Quering devices information will be added in the next patches.
s/Quering/Querying/
PVS does't support non-persistent domains - you can't run
s/does't/doesn't/
a domain having only disk image, it must always be registered in system.
Functions for quering domain info have been just copied from
s/quering/querying/
test driver with some changes - they extract needed data from previouly created list of virDomainObj objects.
s/previouly/previously/
+++ b/po/POTFILES.in @@ -165,6 +165,7 @@ src/xenapi/xenapi_driver.c src/xenapi/xenapi_utils.c src/xenxs/xen_sxpr.c src/xenxs/xen_xm.c +src/pvs/pvs_driver.c tools/console.c tools/libvirt-guests.init.sh tools/virsh.c
I think this hunk belongs in 1/9.
@@ -77,6 +78,14 @@ pvsDefaultConsoleType(const char *ostype ATTRIBUTE_UNUSED) return VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL; }
+void +pvsFreeDomObj(void *p) +{ + pvsDomObjPtr pdom = (pvsDomObjPtr) p; + + VIR_FREE(pdom);
Simpler to just write VIR_FREE(p) and avoid the intermediate variable. Also, this function can safely be marked static.
+/* + * Must be called with privconn->lock held + */ +static virDomainObjPtr +pvsLoadDomain(pvsConnPtr privconn, virJSONValuePtr jobj) +{
Long, but looks okay.
+/* + * Must be called with privconn->lock held + */ +static int +pvsLoadDomains(pvsConnPtr privconn, const char *domain_name) +{ + int count, i; + virJSONValuePtr jobj; + virJSONValuePtr jobj2; + virDomainObjPtr dom = NULL; + int ret = -1; + + jobj = pvsParseOutput(PRLCTL, "list", "-j", "-a", + "-i", "-H", domain_name, NULL);
I guess you can call this command with a domain name, to limit to one output, or with no argument, to list all domains, given...
@@ -150,6 +371,9 @@ pvsOpenDefault(virConnectPtr conn) if (virDomainObjListInit(&privconn->domains) < 0) goto error;
+ if (pvsLoadDomains(privconn, NULL)) + goto error;
this usage.
+static int +pvsDomainIsPersistent(virDomainPtr dom ATTRIBUTE_UNUSED) +{ + return 1; +}
I'm wondering if we should at least validate that dom still exists in our hash table. But I can live with this implementation.
+ +static int +pvsDomainGetState(virDomainPtr domain, + int *state, int *reason, unsigned int flags) +{ + pvsConnPtr privconn = domain->conn->privateData; + virDomainObjPtr privdom; + int ret = -1; + virCheckFlags(0, -1); + + pvsDriverLock(privconn); + privdom = virDomainFindByName(&privconn->domains, domain->name); + pvsDriverUnlock(privconn); + + if (privdom == NULL) { + pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
Use of __FUNCTION__ as the error message is not a good habit, since pvsError _already_ adds the function name automatically. You should instead provide a real message, such as _("Domain '%s' not found"), domain->name.
+static char * +pvsDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) +{ + pvsConnPtr privconn = domain->conn->privateData; + virDomainDefPtr def; + virDomainObjPtr privdom; + char *ret = NULL; + + /* Flags checked by virDomainDefFormat */ + + pvsDriverLock(privconn); + privdom = virDomainFindByName(&privconn->domains, domain->name); + pvsDriverUnlock(privconn); + + if (privdom == NULL) { + pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
Likewise (and probably throughout the series, so I'll quit pointing it out).
# define pvsError(code, ...) \ virReportErrorHelper(VIR_FROM_TEST, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) # define PRLCTL "prlctl" +# define pvsParseError() \ + virReportErrorHelper(VIR_FROM_TEST, VIR_ERR_OPERATION_FAILED, __FILE__, \ + __FUNCTION__, __LINE__, "Can't parse prlctl output")
Mark this string for translation: _("Can't parse prlctl output")
+ +struct pvsDomObj { + int id; + char *uuid; + char *os; +};
+typedef struct pvsDomObj *pvsDomObjPtr;
struct _pvsConn { virMutex lock; @@ -48,4 +60,6 @@ typedef struct _pvsConn *pvsConnPtr;
int pvsRegister(void);
+virJSONValuePtr pvsParseOutput(const char *binary, ...);
Mark this with ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL.
+ +static int +pvsDoCmdRun(char **outbuf, const char *binary, va_list list) +{ + virCommandPtr cmd = virCommandNew(binary); + const char *arg; + int exitstatus; + char *scmd = NULL; + char *sstatus = NULL; + int ret = -1; + + while ((arg = va_arg(list, const char *)) != NULL) + virCommandAddArg(cmd, arg); + + if (outbuf) + virCommandSetOutputBuffer(cmd, outbuf); + + scmd = virCommandToString(cmd); + if (!scmd) + goto cleanup; + + if (virCommandRun(cmd, &exitstatus)) {
If you pass NULL as the second argument here,
+ pvsError(VIR_ERR_INTERNAL_ERROR, + _("Failed to execute command '%s'"), scmd); + goto cleanup; + } + + if (exitstatus) { + sstatus = virCommandTranslateStatus(exitstatus); + pvsError(VIR_ERR_INTERNAL_ERROR, + _("Command '%s' finished with errors: %s"), scmd, sstatus);
then virCommand will take care of ensuring a 0 exit status, and with more uniform error reporting. No need to reinvent the work that someone else will do for you :) The only reason to ever pass an &exitstatus parameter is if you validly expect non-zero exit status.
+/* + * Run command and parse its JSON output, return + * pointer to virJSONValue or NULL in case of error. + */ +virJSONValuePtr +pvsParseOutput(const char *binary, ...) +{ + char *outbuf; + virJSONValuePtr jobj = NULL; + va_list list; + int ret; + + va_start(list, binary); + ret = pvsDoCmdRun(&outbuf, binary, list); + va_end(list); + if (ret) + return NULL; + + jobj = virJSONValueFromString(outbuf); + if (!jobj) + pvsError(VIR_ERR_INTERNAL_ERROR, "%s: %s", + _("invalid output from prlctl"), outbuf); + + return jobj;
You leak outbuf if you reach the virJSONValueFromString() call. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org