Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability

2013-03-13 Thread Yin Olivia-R63875 
Hi Eric,

Thanks for the comments. Could you please help review the next version?

Best Regards,
Olivia

 -Original Message-
 From: Eric Blake [mailto:ebl...@redhat.com]
 Sent: Thursday, February 28, 2013 1:02 AM
 Cc: Yin Olivia-R63875; libvir-list@redhat.com
 Subject: Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
 
 On 02/27/2013 09:56 AM, Eric Blake wrote:
 
  @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
   }
   if (strstr(help, -uuid))
   virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
  +if (strstr(help, -dtb))
  +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
 
  This won't work.  -dtb did not exist prior to qemu 1.2, and for all
  qemu newer than 1.2, we do NOT scrape '-help' output; instead, we use
  QMP query commands.  You need to figure out the corresponding QMP
  command that we can use to tell when dtb support exists in qemu.
 
 I stand (slightly) corrected: it looks like -dtb was added in qemu 1.1
 (qemu commit 379b5c7), which means we DO need -help scraping for that
 version of qemu.  Then, since it predates when QMP queries are reliable,
 you can get away with blindly setting QEMU_CAPS_DTB when targetting a qemu
 new enough to support QMP queries without actually having to query for it.
 But it's still something to be fixed before this patch is ready :)
 
 --
 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org


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

Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability

2013-02-27 Thread Eric Blake 
On 02/27/2013 01:28 AM, Olivia Yin wrote:
 Signed-off-by: Olivia Yin hong-hua@freescale.com
 ---
  src/qemu/qemu_capabilities.c |3 +++
  src/qemu/qemu_capabilities.h |1 +
  2 files changed, 4 insertions(+), 0 deletions(-)

[not a patch review, but a general comment]

You did deep threading:

0/4
\ 1/4
   \ 2/4
  \ 3/4
 \ 4/4

But typically we prefer shallow threading:

0/4
+ 1/4
+ 2/4
+ 3/4
\ 4/4

You may want to investigate the settings you are using with git
format-patch and/or send-email.
 +++ b/src/qemu/qemu_capabilities.c
 @@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
  
rng-random, /* 130 */
rng-egd,
 +   dtb,

OK, so I lied - I'm also doing a patch review:

This uses a TAB, which is forbidden by 'make syntax-check'.

  );
  
  struct _virQEMUCaps {
 @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
  }
  if (strstr(help, -uuid))
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
 +if (strstr(help, -dtb))
 +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);

This won't work.  -dtb did not exist prior to qemu 1.2, and for all qemu
newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP
query commands.  You need to figure out the corresponding QMP command
that we can use to tell when dtb support exists in qemu.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability

2013-02-27 Thread Eric Blake 
On 02/27/2013 09:56 AM, Eric Blake wrote:

 @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
  }
  if (strstr(help, -uuid))
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
 +if (strstr(help, -dtb))
 +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
 
 This won't work.  -dtb did not exist prior to qemu 1.2, and for all qemu
 newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP
 query commands.  You need to figure out the corresponding QMP command
 that we can use to tell when dtb support exists in qemu.

I stand (slightly) corrected: it looks like -dtb was added in qemu 1.1
(qemu commit 379b5c7), which means we DO need -help scraping for that
version of qemu.  Then, since it predates when QMP queries are reliable,
you can get away with blindly setting QEMU_CAPS_DTB when targetting a
qemu new enough to support QMP queries without actually having to query
for it.  But it's still something to be fixed before this patch is ready :)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability

2013-02-27 Thread Yin Olivia-R63875 
Hi Eric,

Thanks for your comments.

 -Original Message-
 From: Eric Blake [mailto:ebl...@redhat.com]
 Sent: Thursday, February 28, 2013 12:57 AM
 To: Yin Olivia-R63875
 Cc: libvir-list@redhat.com
 Subject: Re: [libvirt] [PATCH v3 2/4] qemu: add dtb capability
 
 On 02/27/2013 01:28 AM, Olivia Yin wrote:
  Signed-off-by: Olivia Yin hong-hua@freescale.com
  ---
   src/qemu/qemu_capabilities.c |3 +++
   src/qemu/qemu_capabilities.h |1 +
   2 files changed, 4 insertions(+), 0 deletions(-)
 
 [not a patch review, but a general comment]
 
 You did deep threading:
 
 0/4
 \ 1/4
\ 2/4
   \ 3/4
  \ 4/4
 
 But typically we prefer shallow threading:
 
 0/4
 + 1/4
 + 2/4
 + 3/4
 \ 4/4
 
 You may want to investigate the settings you are using with git format-
 patch and/or send-email.
  +++ b/src/qemu/qemu_capabilities.c
  @@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
 rng-random, /* 130 */
 rng-egd,
  + dtb,
 
 OK, so I lied - I'm also doing a patch review:
 This uses a TAB, which is forbidden by 'make syntax-check'.

I'll fix it in next version.

 
   );
 
   struct _virQEMUCaps {
  @@ -944,6 +945,8 @@ virQEMUCapsComputeCmdFlags(const char *help,
   }
   if (strstr(help, -uuid))
   virQEMUCapsSet(qemuCaps, QEMU_CAPS_UUID);
  +if (strstr(help, -dtb))
  +virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
 
 This won't work.  -dtb did not exist prior to qemu 1.2, and for all qemu
 newer than 1.2, we do NOT scrape '-help' output; instead, we use QMP query
 commands.  You need to figure out the corresponding QMP command that we can
 use to tell when dtb support exists in qemu.

How about add QEMU_CAPS_DTB into virQEMUCapsInitQMPBasic?
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 40022c1..a5bda24 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -210,6 +210,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,

   rng-random, /* 130 */
   rng-egd,
+  dtb,
 );

 struct _virQEMUCaps {
@@ -1177,6 +1178,9 @@ virQEMUCapsComputeCmdFlags(const char *help,

 if (version = 1002000)
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+
+if (version = 11000)
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
 return 0;
 }

@@ -2294,6 +2298,7 @@ virQEMUCapsInitQMPBasic(virQEMUCapsPtr qemuCaps)
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE);
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_SECCOMP_SANDBOX);
 virQEMUCapsSet(qemuCaps, QEMU_CAPS_NO_KVM_PIT);
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_DTB);
 }

 --
 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org


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