Re: [libvirt] [PATCH v4 1/3] add new virDomainCoreDumpWithFormat API

2014-03-05 Thread qiaonuo...@cn.fujitsu.com 
On 03/04/2014 07:41 PM, Daniel P. Berrange wrote:
 diff --git a/src/test/test_driver.c b/src/test/test_driver.c
 index b724f82..605b0d1 100644
 --- a/src/test/test_driver.c
 +++ b/src/test/test_driver.c
 @@ -2427,9 +2427,10 @@ testDomainRestore(virConnectPtr conn,
   return testDomainRestoreFlags(conn, path, NULL, 0);
   }

 -static int testDomainCoreDump(virDomainPtr domain,
 -  const char *to,
 -  unsigned int flags)
 +static int testDomainCoreDumpWithFormat(virDomainPtr domain,
 +const char *to,
 +unsigned int dumpformat,
 +unsigned int flags)
   {
   testConnPtr privconn = domain-conn-privateData;
   int fd = -1;
 @@ -2479,6 +2480,12 @@ static int testDomainCoreDump(virDomainPtr domain,
   }
   }

 +if (dumpformat  VIR_DUMP_FORMAT_KDUMP_SNAPPY) {
 +virReportSystemError(errno,
 + _(invalid value of dumpformat: %d), 
 dumpformat);
 +goto cleanup;
 +}

 This should be done in the libvirt.c entry point, comparing against
 VIR_DUMP_FORMAT_LAST


Is it OK, if I change the check to following one

+/* dump the core of domain to file to */
+if (virDomainCoreDumpWithFormat(domain, to, dumpformat, flags)  0) {
+goto cleanup;
+}


 Regards,
 Daniel


-- 
Regards
Qiao Nuohan

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

Re: [libvirt] [PATCH v4 1/3] add new virDomainCoreDumpWithFormat API

2014-03-05 Thread Daniel P. Berrange 
On Wed, Mar 05, 2014 at 12:00:59PM +, qiaonuo...@cn.fujitsu.com wrote:
 On 03/04/2014 07:41 PM, Daniel P. Berrange wrote:
  diff --git a/src/test/test_driver.c b/src/test/test_driver.c
  index b724f82..605b0d1 100644
  --- a/src/test/test_driver.c
  +++ b/src/test/test_driver.c
  @@ -2427,9 +2427,10 @@ testDomainRestore(virConnectPtr conn,
return testDomainRestoreFlags(conn, path, NULL, 0);
}
 
  -static int testDomainCoreDump(virDomainPtr domain,
  -  const char *to,
  -  unsigned int flags)
  +static int testDomainCoreDumpWithFormat(virDomainPtr domain,
  +const char *to,
  +unsigned int dumpformat,
  +unsigned int flags)
{
testConnPtr privconn = domain-conn-privateData;
int fd = -1;
  @@ -2479,6 +2480,12 @@ static int testDomainCoreDump(virDomainPtr domain,
}
}
 
  +if (dumpformat  VIR_DUMP_FORMAT_KDUMP_SNAPPY) {
  +virReportSystemError(errno,
  + _(invalid value of dumpformat: %d), 
  dumpformat);
  +goto cleanup;
  +}
 
  This should be done in the libvirt.c entry point, comparing against
  VIR_DUMP_FORMAT_LAST
 
 
 Is it OK, if I change the check to following one
 
 +/* dump the core of domain to file to */
 +if (virDomainCoreDumpWithFormat(domain, to, dumpformat, flags)  0) {
 +goto cleanup;
 +}

Huh, I don't really see what you mean here.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v4 1/3] add new virDomainCoreDumpWithFormat API

2014-03-05 Thread qiaonuo...@cn.fujitsu.com 
On 03/05/2014 08:42 PM, Daniel P. Berrange wrote:
 On Wed, Mar 05, 2014 at 12:00:59PM +, qiaonuo...@cn.fujitsu.com wrote:
 On 03/04/2014 07:41 PM, Daniel P. Berrange wrote:
 diff --git a/src/test/test_driver.c b/src/test/test_driver.c
 index b724f82..605b0d1 100644
 --- a/src/test/test_driver.c
 +++ b/src/test/test_driver.c
 @@ -2427,9 +2427,10 @@ testDomainRestore(virConnectPtr conn,
return testDomainRestoreFlags(conn, path, NULL, 0);
}

 -static int testDomainCoreDump(virDomainPtr domain,
 -  const char *to,
 -  unsigned int flags)
 +static int testDomainCoreDumpWithFormat(virDomainPtr domain,
 +const char *to,
 +unsigned int dumpformat,
 +unsigned int flags)
{
testConnPtr privconn = domain-conn-privateData;
int fd = -1;
 @@ -2479,6 +2480,12 @@ static int testDomainCoreDump(virDomainPtr domain,
}
}

 +if (dumpformat   VIR_DUMP_FORMAT_KDUMP_SNAPPY) {
 +virReportSystemError(errno,
 + _(invalid value of dumpformat: %d), 
 dumpformat);
 +goto cleanup;
 +}

 This should be done in the libvirt.c entry point, comparing against
 VIR_DUMP_FORMAT_LAST


 Is it OK, if I change the check to following one

 +/* dump the core of domain to file to */
 +if (virDomainCoreDumpWithFormat(domain, to, dumpformat, flags)  0) {
 +goto cleanup;
 +}

 Huh, I don't really see what you mean here.

parameter of testXXX should be used, or it won't go through make. My computer
output the following message. I just want to check is it OK to use
virDomainCoreDumpWithFormat here.

cut
...
   CC qemu/libvirt_driver_qemu_impl_la-qemu_capabilities.lo
   CC qemu/libvirt_driver_qemu_impl_la-qemu_command.lo
   CC qemu/libvirt_driver_qemu_impl_la-qemu_domain.lo
cc1: warnings being treated as errors
test/test_driver.c: In function 'testDomainCoreDumpWithFormat':
test/test_driver.c:2432: error: unused parameter 'dumpformat' 
[-Wunused-parameter]
make[3]: *** [test/libvirt_driver_test_la-test_driver.lo] Error 1
make[3]: *** Waiting for unfinished jobs
make[3]: Leaving directory `/work/qemu/libvirt/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/work/qemu/libvirt/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/work/qemu/libvirt'
make: *** [all] Error 2
cut


 Regards,
 Daniel


-- 
Regards
Qiao Nuohan

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

Re: [libvirt] [PATCH v4 1/3] add new virDomainCoreDumpWithFormat API

2014-03-05 Thread Daniel P. Berrange 
On Wed, Mar 05, 2014 at 12:48:00PM +, qiaonuo...@cn.fujitsu.com wrote:
 On 03/05/2014 08:42 PM, Daniel P. Berrange wrote:
  On Wed, Mar 05, 2014 at 12:00:59PM +, qiaonuo...@cn.fujitsu.com wrote:
  On 03/04/2014 07:41 PM, Daniel P. Berrange wrote:
  diff --git a/src/test/test_driver.c b/src/test/test_driver.c
  index b724f82..605b0d1 100644
  --- a/src/test/test_driver.c
  +++ b/src/test/test_driver.c
  @@ -2427,9 +2427,10 @@ testDomainRestore(virConnectPtr conn,
 return testDomainRestoreFlags(conn, path, NULL, 0);
 }
 
  -static int testDomainCoreDump(virDomainPtr domain,
  -  const char *to,
  -  unsigned int flags)
  +static int testDomainCoreDumpWithFormat(virDomainPtr domain,
  +const char *to,
  +unsigned int dumpformat,
  +unsigned int flags)
 {
 testConnPtr privconn = domain-conn-privateData;
 int fd = -1;
  @@ -2479,6 +2480,12 @@ static int testDomainCoreDump(virDomainPtr domain,
 }
 }
 
  +if (dumpformat   VIR_DUMP_FORMAT_KDUMP_SNAPPY) {
  +virReportSystemError(errno,
  + _(invalid value of dumpformat: %d), 
  dumpformat);
  +goto cleanup;
  +}
 
  This should be done in the libvirt.c entry point, comparing against
  VIR_DUMP_FORMAT_LAST
 
 
  Is it OK, if I change the check to following one
 
  +/* dump the core of domain to file to */
  +if (virDomainCoreDumpWithFormat(domain, to, dumpformat, flags)  0) {
  +goto cleanup;
  +}
 
  Huh, I don't really see what you mean here.
 
 parameter of testXXX should be used, or it won't go through make. My computer
 output the following message. I just want to check is it OK to use
 virDomainCoreDumpWithFormat here.
 
 cut
 ...
CC qemu/libvirt_driver_qemu_impl_la-qemu_capabilities.lo
CC qemu/libvirt_driver_qemu_impl_la-qemu_command.lo
CC qemu/libvirt_driver_qemu_impl_la-qemu_domain.lo
 cc1: warnings being treated as errors
 test/test_driver.c: In function 'testDomainCoreDumpWithFormat':
 test/test_driver.c:2432: error: unused parameter 'dumpformat' 
 [-Wunused-parameter]

Ok, so for the test driver you should do something like this


  if (dumpformat != VIR_DUMP_FORMAT_RAW) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
);
 return -1;
  }

ie, only accept raw format

The check for format value being out of range should be in libvirt.c

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v4 1/3] add new virDomainCoreDumpWithFormat API

2014-03-04 Thread Daniel P. Berrange 
On Mon, Mar 03, 2014 at 10:27:24AM +0800, qiaonuohan wrote:
 --memory-only option is introduced without compression supported. Therefore,
 this is a freature regression of virsh dump. Now qemu has support dumping 
 memory
 in kdump-compressed format. This patch is adding new 
 virDomainCoreDumpWithFormat
 API, so that the format in which qemu dump domain's memory can be specified.
 
 Signed-off-by: Qiao Nuohan qiaonuo...@cn.fujitsu.com
 ---
  include/libvirt/libvirt.h.in |  31 ++
  src/driver.h |   7 +++
  src/libvirt.c| 100 
 +++
  src/libvirt_public.syms  |   5 +++
  src/remote/remote_driver.c   |   1 +
  src/remote/remote_protocol.x |  15 ++-
  src/remote_protocol-structs  |   7 +++
  src/test/test_driver.c   |  19 ++--
  8 files changed, 181 insertions(+), 4 deletions(-)
 
 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index 295d551..12d64ab 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in

 diff --git a/src/libvirt.c b/src/libvirt.c
 index dcf6b53..5a6a576 100644
 --- a/src/libvirt.c
 +++ b/src/libvirt.c
 @@ -2999,6 +2999,106 @@ error:
  return -1;
  }
  
 +/**
 + * virDomainCoreDumpWithFormat:
 + * @domain: a domain object
 + * @to: path for the core file
 + * @dumpformat: format of domain memory's dump
 + * @flags: bitwise-OR of virDomainCoreDumpFlags
 + *
 + * This method will dump the core of a domain on a given file for analysis.
 + * Note that for remote Xen Daemon the file path will be interpreted in
 + * the remote host. Hypervisors may require  the user to manually ensure
 + * proper permissions on the file named by @to.
 + *
 + * If @flags includes VIR_DUMP_MEMORY_ONLY and dumpformat is set, libvirt
 + * will ask qemu dump domain's memory in kdump-compressed format.
 + *
 + * If @flags includes VIR_DUMP_CRASH, then leave the guest shut off with
 + * a crashed state after the dump completes.  If @flags includes
 + * VIR_DUMP_LIVE, then make the core dump while continuing to allow
 + * the guest to run; otherwise, the guest is suspended during the dump.
 + * VIR_DUMP_RESET flag forces reset of the quest after dump.
 + * The above three flags are mutually exclusive.
 + *
 + * Additionally, if @flags includes VIR_DUMP_BYPASS_CACHE, then libvirt
 + * will attempt to bypass the file system cache while creating the file,
 + * or fail if it cannot do so for the given system; this can allow less
 + * pressure on file system cache, but also risks slowing saves to NFS.
 + *
 + * Returns 0 in case of success and -1 in case of failure.
 + */
 +int
 +virDomainCoreDumpWithFormat(virDomainPtr domain, const char *to, unsigned int
 +dumpformat, unsigned int flags)
 +{
 +virConnectPtr conn;
 +
 +VIR_DOMAIN_DEBUG(domain, to=%s, flags=%x, to, flags);
 +
 +virResetLastError();
 +
 +virCheckDomainReturn(domain, -1);
 +conn = domain-conn;
 +
 +virCheckReadOnlyGoto(conn-flags, error);
 +virCheckNonNullArgGoto(to, error);
 +
 +if (!(flags  VIR_DUMP_MEMORY_ONLY) 
 +(dumpformat == VIR_DUMP_FORMAT_KDUMP_ZLIB ||
 + dumpformat == VIR_DUMP_FORMAT_KDUMP_LZO ||
 + dumpformat == VIR_DUMP_FORMAT_KDUMP_SNAPPY)) {
 +virReportInvalidArg(flags, %s,
 +_(compression format is only work with 
 memory-only));
 +goto error;
 +}

This should be something checked on the QEMU driver since it is an
implmentation restriction, not an API restriction.

This should validate that dumpformat  VIR_DUMP_FORMAT_LAST though

 +
 +if ((flags  VIR_DUMP_CRASH)  (flags  VIR_DUMP_LIVE)) {
 +virReportInvalidArg(flags, %s,
 +_(crash and live flags are mutually 
 exclusive));
 +goto error;
 +}
 +
 +if ((flags  VIR_DUMP_CRASH)  (flags  VIR_DUMP_RESET)) {
 +virReportInvalidArg(flags, %s,
 + _(crash and reset flags are mutually exclusive));
 +goto error;
 +}
 +
 +if ((flags  VIR_DUMP_LIVE)  (flags  VIR_DUMP_RESET)) {
 +virReportInvalidArg(flags, %s,
 +_(live and reset flags are mutually 
 exclusive));
 +goto error;
 +}
 +
 +if (conn-driver-domainCoreDumpWithFormat) {
 +int ret;
 +char *absolute_to;
 +
 +/* We must absolutize the file path as the save is done out of 
 process */
 +if (virFileAbsPath(to, absolute_to)  0) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(could not build absolute core file path));
 +goto error;
 +}
 +
 +ret = conn-driver-domainCoreDumpWithFormat(domain, absolute_to,
 + dumpformat, flags);
 +
 +VIR_FREE(absolute_to);
 +
 +if (ret  0)
 +goto error;
 +return ret;
 +}
 +
 +