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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list