Re: [libvirt] [PATCH v5 3/9] pvs: add functions to list domains and get info

2012-05-02 Thread Dmitry Guryanov 

On 05/02/2012 03:11 AM, Eric Blake wrote:

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



.

+/*
+ * 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.




Yes, you're right, I have to add comment about it.

--
Dmitry Guryanov

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 3/9] pvs: add functions to list domains and get info

2012-05-01 Thread Eric Blake 
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)
 +{
 +

[libvirt] [PATCH v5 3/9] pvs: add functions to list domains and get info

2012-04-20 Thread Dmitry Guryanov 
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
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.
Quering devices information will be added in the next patches.

PVS does't support non-persistent domains - you can't run
a domain having only disk image, it must always be registered
in system.

Functions for quering domain info have been just copied from
test driver with some changes - they extract needed data from
previouly created list of virDomainObj objects.

changes in v4:
* fix indent in preprocessor directives in pvs_driver.h
* add pvs_driver.c to POTFILES.in

Signed-off-by: Dmitry Guryanov dgurya...@parallels.com
---
 po/POTFILES.in   |1 +
 src/Makefile.am  |3 +-
 src/pvs/pvs_driver.c |  516 +-
 src/pvs/pvs_driver.h |   14 ++
 src/pvs/pvs_utils.c  |  101 ++
 5 files changed, 633 insertions(+), 2 deletions(-)
 create mode 100644 src/pvs/pvs_utils.c

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 4ea544b..07ccb7c 100644
--- a/po/POTFILES.in
+++ 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
diff --git a/src/Makefile.am b/src/Makefile.am
index 8057f54..1dd27c3 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -468,7 +468,8 @@ HYPERV_DRIVER_EXTRA_DIST =  
\
 
 PVS_DRIVER_SOURCES =   
\
pvs/pvs_driver.h
\
-   pvs/pvs_driver.c
+   pvs/pvs_driver.c
\
+   pvs/pvs_utils.c
 
 NETWORK_DRIVER_SOURCES =   \
network/bridge_driver.h network/bridge_driver.c
diff --git a/src/pvs/pvs_driver.c b/src/pvs/pvs_driver.c
index 3e48a76..736aa55 100644
--- a/src/pvs/pvs_driver.c
+++ b/src/pvs/pvs_driver.c
@@ -50,12 +50,13 @@
 #include configmake.h
 #include storage_file.h
 #include nodeinfo.h
-#include json.h
+#include domain_conf.h
 
 #include pvs_driver.h
 
 #define VIR_FROM_THIS VIR_FROM_PVS
 
+void pvsFreeDomObj(void *p);
 static virCapsPtr pvsBuildCapabilities(void);
 static int pvsClose(virConnectPtr conn);
 
@@ -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);
+};
+
 static virCapsPtr
 pvsBuildCapabilities(void)
 {
@@ -125,6 +134,218 @@ pvsGetCapabilities(virConnectPtr conn)
 return xml;
 }
 
+/*
+ * Must be called with privconn-lock held
+ */
+static virDomainObjPtr
+pvsLoadDomain(pvsConnPtr privconn, virJSONValuePtr jobj)
+{
+virDomainObjPtr dom = NULL;
+virDomainDefPtr def = NULL;
+pvsDomObjPtr pdom = NULL;
+virJSONValuePtr jobj2, jobj3;
+const char *tmp;
+char *endptr;
+unsigned long mem;
+unsigned int x;
+
+if (VIR_ALLOC(def)  0)
+goto no_memory;
+
+def-virtType = VIR_DOMAIN_VIRT_PVS;
+def-id = -1;
+
+tmp = virJSONValueObjectGetString(jobj, Name);
+if (!tmp) {
+pvsParseError();
+goto cleanup;
+}
+if (!(def-name = strdup(tmp)))
+goto no_memory;
+
+tmp = virJSONValueObjectGetString(jobj, ID);
+if (!tmp) {
+pvsParseError();
+goto cleanup;
+}
+
+if (virUUIDParse(tmp, def-uuid)  0) {
+pvsError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(UUID in config file malformed));
+goto cleanup;
+}
+
+tmp = virJSONValueObjectGetString(jobj, Description);
+if (!tmp) {
+pvsParseError();
+goto cleanup;
+}
+if (!(def-description = strdup(tmp)))
+goto no_memory;
+
+jobj2 = virJSONValueObjectGet(jobj, Hardware);
+if (!jobj2) {
+pvsParseError();
+goto cleanup;
+}
+
+jobj3 = virJSONValueObjectGet(jobj2, cpu);
+if (!jobj3) {
+pvsParseError();
+goto cleanup;
+}
+
+if (virJSONValueObjectGetNumberUint(jobj3, cpus, x)  0) {
+pvsParseError();
+goto cleanup;
+}
+def-vcpus = x;
+def-maxvcpus = x;
+
+jobj3 = virJSONValueObjectGet(jobj2, memory);
+if (!jobj3) {
+pvsParseError();
+goto cleanup;
+}
+
+tmp = virJSONValueObjectGetString(jobj3, size);
+
+if (virStrToLong_ul(tmp, endptr, 10, mem)  0) {
+pvsParseError();
+goto cleanup;
+}
+
+if (!STREQ(endptr, Mb)) {
+pvsParseError();
+goto cleanup;
+}
+
+