Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[Andrea Bolognani]

On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1248277
> 
> When count <= 0, the client exit without set an error.
> 
> Signed-off-by: Luyao Huang 
> ---
>  tools/virsh-domain.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index f7edeeb..b6da684 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd 
> *cmd)
>  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>  return false;
>  
> -if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 
> 0)
> +if (vshCommandOptInt(ctl, cmd, "count", &count) < 0)
>  goto cleanup;
> +if (count <= 0) {
> +vshError(ctl, _("Invalid value '%d' for number of virtual 
> CPUs"), count);
> +goto cleanup;
> +}
>  
>  /* none of the options were specified */
>  if (!current && flags == 0) {

Nice catch, but I would go for the following diff instead:

-8<-
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a61656f..4b8ebbd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6819,7 +6819,7 @@ static bool
 cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom;
-int count = 0;
+unsigned int count = 0;
 bool ret = false;
 bool maximum = vshCommandOptBool(cmd, "maximum");
 bool config = vshCommandOptBool(cmd, "config");
@@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
 if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
 return false;
 
-if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0)
+if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0)
+goto cleanup;
+
+if (count == 0) {
+vshError(ctl,
+ _("Numeric value '%s' for <%s> option is malformed or
out of range"),
+ "0", "count");
 goto cleanup;
+}
 
 /* none of the options were specified */
 if (!current && flags == 0) {
->8-

It's slightly more awkward, but his way we make sure the
error message is the same whether the value used for the
count option is "foo", "0" or "-20". vshCommandOptUInt()
already uses that error message internally.

Does it look good to you?

Cheers.


-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [python PATCH] iothread: Fix crash if virDomainGetIOThreadInfo returns error

[Peter Krempa]

The cleanup portion of libvirt_virDomainGetIOThreadInfo would try to
clean the returned structures but the count of iothreads was set to -1.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1248295
---
 libvirt-override.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 45c8afc..2398228 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -2104,8 +2104,10 @@ libvirt_virDomainGetIOThreadInfo(PyObject *self 
ATTRIBUTE_UNUSED,
 py_iothrinfo = NULL;

 cleanup:
-for (i = 0; i < niothreads; i++)
-virDomainIOThreadInfoFree(iothrinfo[i]);
+if (niothreads > 0) {
+for (i = 0; i < niothreads; i++)
+virDomainIOThreadInfoFree(iothrinfo[i]);
+}
 VIR_FREE(iothrinfo);
 Py_XDECREF(py_iothrinfo);
 return py_retval;
-- 
2.4.5

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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[Peter Krempa]

On Thu, Jul 30, 2015 at 09:29:15 +0200, Andrea Bolognani wrote:
> On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1248277
> > 
> > When count <= 0, the client exit without set an error.
> > 
> > Signed-off-by: Luyao Huang 
> > ---
> >  tools/virsh-domain.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index f7edeeb..b6da684 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd 
> > *cmd)
> >  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> >  return false;
> >  
> > -if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 
> > 0)
> > +if (vshCommandOptInt(ctl, cmd, "count", &count) < 0)
> >  goto cleanup;
> > +if (count <= 0) {
> > +vshError(ctl, _("Invalid value '%d' for number of virtual 
> > CPUs"), count);
> > +goto cleanup;
> > +}
> >  
> >  /* none of the options were specified */
> >  if (!current && flags == 0) {
> 
> Nice catch, but I would go for the following diff instead:
> 
> -8<-
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index a61656f..4b8ebbd 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6819,7 +6819,7 @@ static bool
>  cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
>  {
>  virDomainPtr dom;
> -int count = 0;
> +unsigned int count = 0;
>  bool ret = false;
>  bool maximum = vshCommandOptBool(cmd, "maximum");
>  bool config = vshCommandOptBool(cmd, "config");
> @@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
>  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>  return false;
>  
> -if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0)
> +if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0)
> +goto cleanup;
> +
> +if (count == 0) {
> +vshError(ctl,
> + _("Numeric value '%s' for <%s> option is malformed or
> out of range"),

Since we know that here we are trying to set 0 cpus which is wrong we
can also say that explicitly in the error message rather than copying
the generic one.

Peter


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

Re: [libvirt] [python PATCH] iothread: Fix crash if virDomainGetIOThreadInfo returns error

[Erik Skultety]

On 30/07/15 09:36, Peter Krempa wrote:
> The cleanup portion of libvirt_virDomainGetIOThreadInfo would try to
> clean the returned structures but the count of iothreads was set to -1.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1248295
> ---
>  libvirt-override.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libvirt-override.c b/libvirt-override.c
> index 45c8afc..2398228 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -2104,8 +2104,10 @@ libvirt_virDomainGetIOThreadInfo(PyObject *self 
> ATTRIBUTE_UNUSED,
>  py_iothrinfo = NULL;
> 
>  cleanup:
> -for (i = 0; i < niothreads; i++)
> -virDomainIOThreadInfoFree(iothrinfo[i]);
> +if (niothreads > 0) {
> +for (i = 0; i < niothreads; i++)
> +virDomainIOThreadInfoFree(iothrinfo[i]);
> +}
>  VIR_FREE(iothrinfo);
>  Py_XDECREF(py_iothrinfo);
>  return py_retval;
> 
ACK

Erik

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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[Andrea Bolognani]

On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
> 
> Since we know that here we are trying to set 0 cpus which is wrong we
> can also say that explicitly in the error message rather than copying
> the generic one.

Using the generic one means we don't have to add yet another
translatable string, though. Or did you mean to use a
completely different error message?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [python PATCH] iothread: Fix crash if virDomainGetIOThreadInfo returns error

[Peter Krempa]

On Thu, Jul 30, 2015 at 09:49:51 +0200, Erik Skultety wrote:
> 
> 
> On 30/07/15 09:36, Peter Krempa wrote:
> > The cleanup portion of libvirt_virDomainGetIOThreadInfo would try to
> > clean the returned structures but the count of iothreads was set to -1.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1248295
> > ---
> >  libvirt-override.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)

...

> > 
> ACK

Pushed; Thanks.

Peter


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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[Peter Krempa]

On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:
> On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
> > 
> > Since we know that here we are trying to set 0 cpus which is wrong we
> > can also say that explicitly in the error message rather than copying
> > the generic one.
> 
> Using the generic one means we don't have to add yet another
> translatable string, though. Or did you mean to use a
> completely different error message?

Completely different. Something along: "Can't set 0 processors for a VM"



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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[lhuang]

On 07/30/2015 03:29 PM, Andrea Bolognani wrote:

On Thu, 2015-07-30 at 10:57 +0800, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1248277

When count <= 0, the client exit without set an error.

Signed-off-by: Luyao Huang 
---
  tools/virsh-domain.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f7edeeb..b6da684 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6744,8 +6744,12 @@ cmdSetvcpus(vshControl *ctl, const vshCmd
*cmd)
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
  
-if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <=

0)
+if (vshCommandOptInt(ctl, cmd, "count", &count) < 0)
  goto cleanup;
+if (count <= 0) {
+vshError(ctl, _("Invalid value '%d' for number of virtual
CPUs"), count);
+goto cleanup;
+}
  
  /* none of the options were specified */

  if (!current && flags == 0) {

Nice catch, but I would go for the following diff instead:

-8<-
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a61656f..4b8ebbd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6819,7 +6819,7 @@ static bool
  cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  {
  virDomainPtr dom;
-int count = 0;
+unsigned int count = 0;
  bool ret = false;
  bool maximum = vshCommandOptBool(cmd, "maximum");
  bool config = vshCommandOptBool(cmd, "config");
@@ -6846,8 +6846,15 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
  
-if (vshCommandOptInt(ctl, cmd, "count", &count) < 0 || count <= 0)

+if (vshCommandOptUInt(ctl, cmd, "count", &count) < 0)
+goto cleanup;
+
+if (count == 0) {
+vshError(ctl,
+ _("Numeric value '%s' for <%s> option is malformed or
out of range"),
+ "0", "count");
  goto cleanup;
+}
  
  /* none of the options were specified */

  if (!current && flags == 0) {
->8-

It's slightly more awkward, but his way we make sure the
error message is the same whether the value used for the
count option is "foo", "0" or "-20". vshCommandOptUInt()
already uses that error message internally.

Does it look good to you?


I think a clear error is works for me, i am not good at the error 
message for a long time ;)


Thanks a lot for your quick review.


Cheers.




Luyao

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

[libvirt] Build failed in Jenkins: libvirt-syntax-check #3732

[Jenkins CI]

See 

--
Started by upstream project "libvirt-build" build number 4256
Building on master in workspace 

[workspace] $ /bin/sh -xe /tmp/hudson3362415966189127246.sh
+ make syntax-check
  GENbracket-spacing-check
GFDL_version
0.86 GFDL_version
TAB_in_indentation
0.55 TAB_in_indentation
Wundef_boolean
0.37 Wundef_boolean
avoid_attribute_unused_in_header
0.42 avoid_attribute_unused_in_header
avoid_ctype_macros
0.91 avoid_ctype_macros
avoid_if_before_free
6.76 avoid_if_before_free
avoid_strcase
0.76 avoid_strcase
avoid_write
0.49 avoid_write
bindtextdomain
0.41 bindtextdomain
cast_of_argument_to_free
0.80 cast_of_argument_to_free
cast_of_x_alloc_return_value
0.82 cast_of_x_alloc_return_value
changelog
0.36 changelog
const_long_option
0.80 const_long_option
copyright_check
1.08 copyright_check
copyright_format
2.63 copyright_format
copyright_usage
2.26 copyright_usage
correct_id_types
0.91 correct_id_types
curly_braces_style
1.56 curly_braces_style
error_message_period
0.70 error_message_period
error_message_warn_fatal
0.61 error_message_warn_fatal
flags_debug
1.56 flags_debug
flags_usage
1.73 flags_usage
forbid_const_pointer_typedef
1.81 forbid_const_pointer_typedef
forbid_manual_xml_indent
0.79 forbid_manual_xml_indent
libvirt_unmarked_diagnostics
2.23 libvirt_unmarked_diagnostics
m4_quote_check
0.43 m4_quote_check
makefile_TAB_only_indentation
0.42 makefile_TAB_only_indentation
makefile_at_at_check
0.35 makefile_at_at_check
makefile_conditionals
0.41 makefile_conditionals
po_check
15.53 po_check
preprocessor_indentation
0.73 preprocessor_indentation
prohibit_HAVE_MBRTOWC
0.91 prohibit_HAVE_MBRTOWC
prohibit_PATH_MAX
1.05 prohibit_PATH_MAX
prohibit_VIR_ERR_NO_MEMORY
0.79 prohibit_VIR_ERR_NO_MEMORY
prohibit_access_xok
0.84 prohibit_access_xok
prohibit_always-defined_macros
2.06 prohibit_always-defined_macros
prohibit_always_true_header_tests
0.94 prohibit_always_true_header_tests
prohibit_argmatch_without_use
0.66 prohibit_argmatch_without_use
prohibit_asprintf
1.57 prohibit_asprintf
prohibit_assert_without_use
0.71 prohibit_assert_without_use
prohibit_atoi
0.82 prohibit_atoi
prohibit_backup_files
0.26 prohibit_backup_files
prohibit_c_ctype_without_use
0.69 prohibit_c_ctype_without_use
prohibit_canonicalize_without_use
0.61 prohibit_canonicalize_without_use
prohibit_cloexec_without_use
0.66 prohibit_cloexec_without_use
prohibit_close
1.45 prohibit_close
prohibit_close_stream_without_use
0.76 prohibit_close_stream_without_use
prohibit_config_h_in_headers
0.44 prohibit_config_h_in_headers
prohibit_cross_inclusion
13.40 prohibit_cross_inclusion
prohibit_ctype_h
0.75 prohibit_ctype_h
prohibit_cvs_keyword
0.74 prohibit_cvs_keyword
prohibit_defined_have_decl_tests
0.79 prohibit_defined_have_decl_tests
prohibit_devname
0.76 prohibit_devname
prohibit_diagnostic_without_format
src/util/virfile.c:820:virReportError(VIR_ERR_INTERNAL_ERROR,
src/util/virfile.c-821-   _("Failed to load nbd module: "
src/util/virfile.c-822- "administratively prohibited"));
src/util/virfile.c:829:virReportError(VIR_ERR_INTERNAL_ERROR,
src/util/virfile.c-830-   _("Failed to load nbd 
module"));
maint.mk: found diagnostic without %
make: *** [sc_prohibit_diagnostic_without_format] Error 1
Build step 'Execute shell' marked build as failure

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

Re: [libvirt] [PATCH] virsh: fix no error when pass a count <= 0 to setvcpus

[Andrea Bolognani]

On Thu, 2015-07-30 at 10:05 +0200, Peter Krempa wrote:
> On Thu, Jul 30, 2015 at 09:51:15 +0200, Andrea Bolognani wrote:
> > On Thu, 2015-07-30 at 09:45 +0200, Peter Krempa wrote:
> > > 
> > > Since we know that here we are trying to set 0 cpus which is 
> > > wrong we
> > > can also say that explicitly in the error message rather than 
> > > copying
> > > the generic one.
> > 
> > Using the generic one means we don't have to add yet another
> > translatable string, though. Or did you mean to use a
> > completely different error message?
> 
> Completely different. Something along: "Can't set 0 processors for a 
> VM"

I see. I don't feel strongly either way, I just want to avoid
having a bunch of very similar but non-identical messages; if
you feel a custom error message is warranted in this situation
then go right ahead.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] Build failed in Jenkins: libvirt-syntax-check #3732

[Cedric Bosdonnat]

On Thu, 2015-07-30 at 10:18 +0200, Jenkins CI wrote:
> See 
> 
> --
> Started by upstream project "libvirt-build" build number 4256
> Building on master in workspace 
> 
> [workspace] $ /bin/sh -xe /tmp/hudson3362415966189127246.sh
> + make syntax-check
>   GENbracket-spacing-check
> GFDL_version
> 0.86 GFDL_version
> TAB_in_indentation
> 0.55 TAB_in_indentation
> Wundef_boolean
> 0.37 Wundef_boolean
> avoid_attribute_unused_in_header
> 0.42 avoid_attribute_unused_in_header
> avoid_ctype_macros
> 0.91 avoid_ctype_macros
> avoid_if_before_free
> 6.76 avoid_if_before_free
> avoid_strcase
> 0.76 avoid_strcase
> avoid_write
> 0.49 avoid_write
> bindtextdomain
> 0.41 bindtextdomain
> cast_of_argument_to_free
> 0.80 cast_of_argument_to_free
> cast_of_x_alloc_return_value
> 0.82 cast_of_x_alloc_return_value
> changelog
> 0.36 changelog
> const_long_option
> 0.80 const_long_option
> copyright_check
> 1.08 copyright_check
> copyright_format
> 2.63 copyright_format
> copyright_usage
> 2.26 copyright_usage
> correct_id_types
> 0.91 correct_id_types
> curly_braces_style
> 1.56 curly_braces_style
> error_message_period
> 0.70 error_message_period
> error_message_warn_fatal
> 0.61 error_message_warn_fatal
> flags_debug
> 1.56 flags_debug
> flags_usage
> 1.73 flags_usage
> forbid_const_pointer_typedef
> 1.81 forbid_const_pointer_typedef
> forbid_manual_xml_indent
> 0.79 forbid_manual_xml_indent
> libvirt_unmarked_diagnostics
> 2.23 libvirt_unmarked_diagnostics
> m4_quote_check
> 0.43 m4_quote_check
> makefile_TAB_only_indentation
> 0.42 makefile_TAB_only_indentation
> makefile_at_at_check
> 0.35 makefile_at_at_check
> makefile_conditionals
> 0.41 makefile_conditionals
> po_check
> 15.53 po_check
> preprocessor_indentation
> 0.73 preprocessor_indentation
> prohibit_HAVE_MBRTOWC
> 0.91 prohibit_HAVE_MBRTOWC
> prohibit_PATH_MAX
> 1.05 prohibit_PATH_MAX
> prohibit_VIR_ERR_NO_MEMORY
> 0.79 prohibit_VIR_ERR_NO_MEMORY
> prohibit_access_xok
> 0.84 prohibit_access_xok
> prohibit_always-defined_macros
> 2.06 prohibit_always-defined_macros
> prohibit_always_true_header_tests
> 0.94 prohibit_always_true_header_tests
> prohibit_argmatch_without_use
> 0.66 prohibit_argmatch_without_use
> prohibit_asprintf
> 1.57 prohibit_asprintf
> prohibit_assert_without_use
> 0.71 prohibit_assert_without_use
> prohibit_atoi
> 0.82 prohibit_atoi
> prohibit_backup_files
> 0.26 prohibit_backup_files
> prohibit_c_ctype_without_use
> 0.69 prohibit_c_ctype_without_use
> prohibit_canonicalize_without_use
> 0.61 prohibit_canonicalize_without_use
> prohibit_cloexec_without_use
> 0.66 prohibit_cloexec_without_use
> prohibit_close
> 1.45 prohibit_close
> prohibit_close_stream_without_use
> 0.76 prohibit_close_stream_without_use
> prohibit_config_h_in_headers
> 0.44 prohibit_config_h_in_headers
> prohibit_cross_inclusion
> 13.40 prohibit_cross_inclusion
> prohibit_ctype_h
> 0.75 prohibit_ctype_h
> prohibit_cvs_keyword
> 0.74 prohibit_cvs_keyword
> prohibit_defined_have_decl_tests
> 0.79 prohibit_defined_have_decl_tests
> prohibit_devname
> 0.76 prohibit_devname
> prohibit_diagnostic_without_format
> src/util/virfile.c:820:virReportError(VIR_ERR_INTERNAL_ERROR,
> src/util/virfile.c-821-   _("Failed to load nbd module: "
> src/util/virfile.c-822- "administratively 
> prohibited"));
> src/util/virfile.c:829:virReportError(VIR_ERR_INTERNAL_ERROR,
> src/util/virfile.c-830-   _("Failed to load nbd 
> module"));
> maint.mk: found diagnostic without %
> make: *** [sc_prohibit_diagnostic_without_format] Error 1
> Build step 'Execute shell' marked build as failure

Oops, I forgot that one. Fixed now.

--
Cedric

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

[libvirt] Jenkins build is back to normal : libvirt-syntax-check #3733

[Jenkins CI]

See 

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

[libvirt] [PATCH v11 0/5] nodeinfo: Add support for subcores

[Andrea Bolognani]

Only patch 1/5 has been updated, all the other patches
are the same as v8:

  2/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01045.html
  3/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01048.html
  4/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01049.html
  5/5 https://www.redhat.com/archives/libvir-list/2015-July/msg01050.html

I'm including the full history below.

Cheers.


Changes in v11:

  * don't declare variables only used inside a code block
guarded by #ifdef outside of said code block

  * use virBitmapNextSetBit() instead of going through
all possible index values and calling
virBitmapIsBitSet() for each one

Changes in v10:

  * don't attempt to close a file that wasn't opened

  * report a detailed error to help with diagnosis

Changes in v9:

  * take into account the fact that kvm might not be loaded
or even installed

Changes in v8:

  * shortened test names so that make dist doesn't
stop working again

Changes in v7:

  * rebased on top of master now that the series this one
builds on have been merged

Changes in v6:

  * updated to work on top of

  [PATCH v2 00/10] nodeinfo: Various cleanups

Changes in v5:

  * streamlined the logic used to decide whether the subcore
configuration is valid and moved it to a separate function

  * split the tests into separate commits for easier review and
to hopefully avoid having trouble with the list due to the
message size

Changes in v4:

  * removed a printf() statement

  * fixed typo in a commit message

Changes in v3:

  * the function to get the number of threads per subcore
has been moved to the from virarch.c, which deals with
architecture names only and is therefore not the right
place to read host configuration, to nodeinfo.c where
the rest of this stuff lives

  * said function has also been given a shorter name

  * the "valid subcore mode" boolean has been removed:
threads_per_subcore will be a positive number if
subcores should be taken into account, and if that's
not the case (x86 host, tainted configuration) it
will simply be zero, so now the code needs to keep
track of a single variable instead of two

  * the test case has been renamed to be more
descriptive

  * the test data has been cleaned up by removing all
cpu/cpu*/node* links, which prevented 'make dist'
from working due to recursive linking


Andrea Bolognani (3):
  tests: Add subcores1 nodeinfo test
  tests: Add subcores2 nodeinfo test
  tests: Add subcores3 nodeinfo test

Shivaprasad G Bhat (2):
  nodeinfo: Fix output on PPC64 KVM hosts
  tests: Prepare for subcore tests

 src/libvirt_private.syms   |   1 +
 src/nodeinfo.c | 153 -
 src/nodeinfo.h |   1 +
 tests/Makefile.am  |   6 +
 [...]
 tests/nodeinfomock.c   |  35 +
 tests/nodeinfotest.c   |   8 +-
 1348 files changed, 2134 insertions(+), 6 deletions(-)
 [...]
 create mode 100644 tests/nodeinfomock.c

-- 
2.4.3

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

[libvirt] [PATCH v11 1/5] nodeinfo: Fix output on PPC64 KVM hosts

[Andrea Bolognani]

From: Shivaprasad G Bhat 

The nodeinfo is reporting incorrect number of cpus and incorrect host
topology on PPC64 KVM hosts. The KVM hypervisor on PPC64 needs only
the primary thread in a core to be online, and the secondaries offlined.
While scheduling a guest in, the kvm scheduler wakes up the secondaries to
run in guest context.

The host scheduling of the guests happen at the core level(as only primary
thread is online). The kvm scheduler exploits as many threads of the core
as needed by guest. Further, starting POWER8, the processor allows splitting
a physical core into multiple subcores with 2 or 4 threads each. Again, only
the primary thread in a subcore is online in the host. The KVM-PPC
scheduler allows guests to exploit all the offline threads in the subcore,
by bringing them online when needed.
(Kernel patches on split-core 
http://www.spinics.net/lists/kvm-ppc/msg09121.html)

Recently with dynamic micro-threading changes in ppc-kvm, makes sure
to utilize all the offline cpus across guests, and across guests with
different cpu topologies.
(https://www.mail-archive.com/kvm@vger.kernel.org/msg115978.html)

Since the offline cpus are brought online in the guest context, it is safe
to count them as online. Nodeinfo today discounts these offline cpus from
cpu count/topology calclulation, and the nodeinfo output is not of any help
and the host appears overcommited when it is actually not.

The patch carefully counts those offline threads whose primary threads are
online. The host topology displayed by the nodeinfo is also fixed when the
host is in valid kvm state.

Signed-off-by: Shivaprasad G Bhat 
Signed-off-by: Andrea Bolognani 
---
 src/libvirt_private.syms |   1 +
 src/nodeinfo.c   | 153 +--
 src/nodeinfo.h   |   1 +
 3 files changed, 151 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ff322d6..0517c24 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1010,6 +1010,7 @@ nodeGetMemoryParameters;
 nodeGetMemoryStats;
 nodeGetOnlineCPUBitmap;
 nodeGetPresentCPUBitmap;
+nodeGetThreadsPerSubcore;
 nodeSetMemoryParameters;
 
 
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index ba633a1..7da055c 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -31,6 +31,12 @@
 #include 
 #include 
 #include "conf/domain_conf.h"
+#include 
+#include 
+
+#if HAVE_LINUX_KVM_H
+# include 
+#endif
 
 #if defined(__FreeBSD__) || defined(__APPLE__)
 # include 
@@ -392,13 +398,14 @@ virNodeParseSocket(const char *dir,
  * filling arguments */
 static int
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7)
-ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(9)
 virNodeParseNode(const char *node,
  virArch arch,
  virBitmapPtr present_cpus_map,
  virBitmapPtr online_cpus_map,
+ int threads_per_subcore,
  int *sockets,
  int *cores,
  int *threads,
@@ -492,7 +499,18 @@ virNodeParseNode(const char *node,
 continue;
 
 if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
-(*offline)++;
+if (threads_per_subcore > 0 &&
+cpu % threads_per_subcore != 0 &&
+virBitmapIsBitSet(online_cpus_map,
+  cpu - (cpu % threads_per_subcore))) {
+/* Secondary offline threads are counted as online when
+ * subcores are in use and the corresponding primary
+ * thread is online */
+processors++;
+} else {
+/* But they are counted as offline otherwise */
+(*offline)++;
+}
 continue;
 }
 
@@ -545,6 +563,12 @@ virNodeParseNode(const char *node,
 *cores = core;
 }
 
+if (threads_per_subcore > 0) {
+/* The thread count ignores offline threads, which means that only
+ * only primary threads have been considered so far. If subcores
+ * are in use, we need to also account for secondary threads */
+*threads *= threads_per_subcore;
+}
 ret = processors;
 
  cleanup:
@@ -563,6 +587,41 @@ virNodeParseNode(const char *node,
 return ret;
 }
 
+/* Check whether the host subcore configuration is valid.
+ *
+ * A valid configuration is one where no secondary thread is online;
+ * the primary thread in a subcore is always the first one */
+static bool
+nodeHasValidSubcoreConfiguration(const char *sysfs_prefix,
+ int threads_per_subcore)
+{
+virBitmapPtr online_cpus = NULL;
+int cpu = -1;
+bool ret = false;
+
+/* No point in checking if subcores are not in use */
+if (threads_per_subcore <= 0)
+goto cleanup;
+
+if

Re: [libvirt] ambiguous ret of qemuDomainDetachVirtioDiskDevice

[zhang bo]

On 2015/7/28 16:33, Ján Tomko wrote:

> On Tue, Jul 28, 2015 at 04:25:13PM +0800, zhang bo wrote:
>> static int
>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>>  virDomainObjPtr vm,
>>  virDomainDiskDefPtr detach)
>> {
>> ...
>>
>> rc = qemuDomainWaitForDeviceRemoval(vm);
>> if (rc == 0 || rc == 1)
>> ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
>> else
>> ret = 0;  /*the return value of 2 is dismissed here, which refers to 
>> ETIMEOUT.*/
>> 
>> }
>>
>> 
>>
>> If it timeouts when qemu tries to del the device, the return value would be 
>> modified from 2 to 0 in 
>> function qemuDomainDetachVirtioDiskDevice(), which means that, the users 
>> would be misleaded that 
>> the device has been deleted, however, the device maybe probably failed to be 
>> detached after timeout and
>> still in use. 
>>
> 
> This is intentional and documented:
> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags
> 
> Unplugging a disk requires guest cooperation, so the best we can do is
> ask qemu to unplug it and wait for a while.
> 
>> That is to say, the function qemuDomainDetachVirtioDiskDevice()'s return 
>> value is ambiguous when it's 0, 
>> maybe successful, or timeout. Will it be better to pass ETIMEOUT to user? or 
>> any other advises? for example,
>> let users themselves dumpxml the guest to check whether the device has been 
>> actually detached or not? 
> 
> Either dump the XML, or wait for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
> event, as the API documentation suggests.
> 
> Jan


It seems to have fixed the problem by dumping the XML or wait for the 
DEVICE_REMOVED event. However, it seems to 
make nova or other apps to do more checking work, they need to dump XML or wait 
the event even if the device has
already been successfully removed. which is unnecessary. 

I think, it's better to return ETIMEOUT and let nova to dumpxml or wait the 
event at this situation, rather than always
doing that job.

It maybe a better design, what's your opinion?

-- 
Oscar
oscar.zhan...@huawei.com  

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

Re: [libvirt] [PATCH v10 1/5] nodeinfo: Fix output on PPC64 KVM hosts

[Andrea Bolognani]

On Wed, 2015-07-29 at 22:30 -0400, John Ferlan wrote:
> 
> > +online_cpus = nodeGetOnlineCPUBitmap(sysfs_prefix);
> > +if (online_cpus == NULL)
> > +goto cleanup;
> > +
> > +nonline_cpus = virBitmapSize(online_cpus);
> > +
> > +for (cpu = 0; cpu < nonline_cpus; cpu++) {
> > +
> > +/* Skip primary threads */
> > +if (cpu % threads_per_subcore == 0)
> > +continue;
> > +
> > +/* A single online subthread is enough to make the
> > + * configuration invalid */
> > +if (virBitmapIsBitSet(online_cpus, cpu))
> > +goto cleanup;
> > +}
> 
> Could virBitmapNextSetBit be used?  Where if the returned pos (cpu) %
> threads_per_subcore != 0, then jump to cleanup?
> 
> I think both work, I just didn't know how large nonline_cpus could be
> and since the bitmap code has a way to return 'next' bit set, we 
> could
> use it.

It works, and it looks nicer too! Thanks for the tip :)

> > +int
> > +nodeGetThreadsPerSubcore(virArch arch)
> > +{
> > +const char *kvmpath = "/dev/kvm";
> > +int kvmfd;
> 
> These could move inside the if below.
> 
> I'm thinking of one particular architecture where we consistently get
> compilation errors when there's variables that aren't used (which 
> would
> be the case if HAVE_LINUX_KVM_H or KVM_CAP_PPC_SMT were not true

Done.

> > +if ((kvmfd = open(kvmpath, O_RDONLY)) < 0) {
> > +/* This can happen when running as a regular user if
> > + * permissions are tight enough, in which case 
> > erroring out
> > + * is better than silently falling back and reporting
> > + * different nodeinfo depending on the user */
> > +virReportSystemError(errno,
> > + _("Failed to open '%s'"),
> > + kvmpath);
> > +threads_per_subcore = kvmfd;
> 
> I would just set to -1 here directly rather than the return failure 
> from
> open

Done.

> Beyond those - things look OK to me.

Just to be sure, did you check the rest of the series as well?

> Let me know if you want to make
> changes... Probably should wait until DV generates the release before
> pushing though...

I've just sent out v11 which addresses all your remarks. It
should definitely only be pushed once the new release is out.

Thanks for the review :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote:
> Introduce a new element in shmem device element, this
> could help users to change the shm label to a specified
> label.
> 
> Signed-off-by: Luyao Huang 
> ---
>  docs/formatdomain.html.in |  7 ++
>  docs/schemas/domaincommon.rng |  3 +++
>  src/conf/domain_conf.c| 55 
> ++-
>  src/conf/domain_conf.h|  5 
>  4 files changed, 59 insertions(+), 11 deletions(-)

As already mentioned, this must include additions to the qemu tests
suite for XML to XML conversion.

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 2/4] security: add security part for shmem device

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:47PM +0800, Luyao Huang wrote:
> A new api to help set/restore the shmem deivce dac/selinux label.

s/deivce/device/

> Signed-off-by: Luyao Huang 
> ---
>  src/libvirt_private.syms|  2 ++
>  src/security/security_dac.c | 67 +++
>  src/security/security_driver.h  | 11 +++
>  src/security/security_manager.c | 38 ++
>  src/security/security_manager.h |  8 +
>  src/security/security_selinux.c | 70 
> +
>  src/security/security_stack.c   | 41 
>  7 files changed, 237 insertions(+)

Also need to add to the security_nop.c impl

> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index deb6980..f954aa5 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -39,6 +39,7 @@
>  #include "virstoragefile.h"
>  #include "virstring.h"
>  #include "virutil.h"
> +#include "virshm.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  
> @@ -922,6 +923,69 @@ 
> virSecurityDACRestoreSecurityTPMFileLabel(virSecurityManagerPtr mgr,
>  
>  
>  static int
> +virSecurityDACSetShmemLabel(virSecurityManagerPtr mgr,
> +virDomainDefPtr def,
> +virDomainShmemDefPtr shmem,
> +char *path)
> +{
> +virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> +virSecurityLabelDefPtr seclabel;
> +virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +char *tmppath;
> +uid_t user;
> +gid_t group;
> +
> +if (shmem->server.enabled)
> +tmppath = shmem->server.chr.data.nix.path;
> +else
> +tmppath = path;

Even when the server is enabled, QEMU still needs access to the path
doesn't it.

> +
> +if (!tmppath)
> +return 0;
> +
> +shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, 
> SECURITY_DAC_NAME);
> +
> +if (shmem_seclabel && !shmem_seclabel->relabel)
> +return 0;
> +
> +seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> +
> +if (shmem_seclabel && shmem_seclabel->label) {
> +if (virParseOwnershipIds(shmem_seclabel->label, &user, &group) < 0)
> +return -1;
> +} else {
> +if (virSecurityDACGetIds(seclabel, priv, &user, &group, NULL, NULL) 
> < 0)
> +return -1;
> +}
> +
> +return virSecurityDACSetOwnership(tmppath, user, group);
> +}
> +
> +
> +static int
> +virSecurityDACRestoreShmemLabel(virSecurityManagerPtr mgr,
> +   virDomainDefPtr def,
> +   virDomainShmemDefPtr shmem,
> +   char *path)
> +{
> +virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +
> +shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, 
> SECURITY_DAC_NAME);
> +
> +if (shmem_seclabel && !shmem_seclabel->relabel)
> +return 0;
> +
> +if (shmem->server.enabled)
> +return virSecurityDACRestoreChardevLabel(mgr, def, NULL, 
> &shmem->server.chr);

We need to restore path, even when server is enabled

> +
> +if (!path)
> +return 0;
> +
> +return virSecurityDACRestoreSecurityFileLabel(path);
> +}
> +
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 6e67a86..cbf89ee 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -46,6 +46,7 @@
>  #include "virconf.h"
>  #include "virtpm.h"
>  #include "virstring.h"
> +#include "virshm.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  
> @@ -1888,6 +1889,37 @@ 
> virSecuritySELinuxRestoreSecuritySmartcardCallback(virDomainDefPtr def,
>  }
>  
>  
> +static int
> +virSecuritySELinuxRestoreShmemLabel(virSecurityManagerPtr mgr,
> +virDomainDefPtr def,
> +virDomainShmemDefPtr shmem,
> +char *path)
> +{
> +char *tmppath = NULL;
> +virSecurityLabelDefPtr seclabel;
> +virSecurityDeviceLabelDefPtr shmem_seclabel = NULL;
> +
> +seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
> +if (!seclabel || !seclabel->relabel)
> +return 0;
> +
> +shmem_seclabel = virDomainShmemDefGetSecurityLabelDef(shmem, 
> SECURITY_SELINUX_NAME);
> +
> +if (shmem_seclabel && !shmem_seclabel->relabel)
> +return 0;
> +
> +if (shmem->server.enabled)
> +tmppath = shmem->server.chr.data.nix.path;
> +else
> +tmppath = path;

Same comment as earlier

> +
> +if (!tmppath)
> +return 0;
> +
> +return virSecuritySELinuxRestoreSecurityFileLabel(mgr, tmppath);
> +}
> +
> +
>  static const char *
>  virSecuritySELinuxGetBaseLabel(virSecurityManagerPtr mgr, int virtType)
>  {
> @@ -2284,6 +2316,41 @@ 
> virSecuritySELinuxSetSecuritySmartcardCallback(virDomainDefPtr def,
>  
>  
>  static int
>

Re: [libvirt] [PATCH 2/4] security: add security part for shmem device

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:47PM +0800, Luyao Huang wrote:
> A new api to help set/restore the shmem deivce dac/selinux label.
> 
> Signed-off-by: Luyao Huang 
> ---
>  src/libvirt_private.syms|  2 ++
>  src/security/security_dac.c | 67 +++
>  src/security/security_driver.h  | 11 +++
>  src/security/security_manager.c | 38 ++
>  src/security/security_manager.h |  8 +
>  src/security/security_selinux.c | 70 
> +
>  src/security/security_stack.c   | 41 
>  7 files changed, 237 insertions(+)
> 

> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index deb6980..f954aa5 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c

> @@ -1433,4 +1497,7 @@ virSecurityDriver virSecurityDriverDAC = {
>  .domainGetSecurityMountOptions  = virSecurityDACGetMountOptions,
>  
>  .getBaseLabel   = virSecurityDACGetBaseLabel,
> +
> +.domainSetSecurityShmemLabel= virSecurityDACSetShmemLabel,
> +.domainRestoreSecurityShmemLabel= virSecurityDACRestoreShmemLabel,

NB, you should also be modifying the virSecurityDACRestoreSecurityAllLabel
and virSecurityDACSetSecurityAllLabel methods to call this code during


> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 6e67a86..cbf89ee 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c

> @@ -2549,4 +2616,7 @@ virSecurityDriver virSecurityDriverSELinux = {
>  
>  .domainGetSecurityMountOptions  = 
> virSecuritySELinuxGetSecurityMountOptions,
>  .getBaseLabel   = virSecuritySELinuxGetBaseLabel,
> +
> +.domainSetSecurityShmemLabel= virSecuritySELinuxSetShmemLabel,
> +.domainRestoreSecurityShmemLabel= 
> virSecuritySELinuxRestoreShmemLabel,
>  };

Likewise virSecuritySELinuxRestoreSecurityAllLabel and
virSecuritySELinuxSetSecurityAllLabel

Doing this avoids the need to manually call these shmem specific
security methods during general guest startup/shutdown. They only
need to be called manually during hotplug/unplug.

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 3/4] util: introduce new helpers to manage shmem device

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:48PM +0800, Luyao Huang wrote:
> Signed-off-by: Luyao Huang 
> ---
>  configure.ac |  10 +
>  po/POTFILES.in   |   3 +-
>  src/Makefile.am  |   5 +-
>  src/libvirt_private.syms |  16 ++
>  src/util/virshm.c| 623 
> +++
>  src/util/virshm.h| 104 
>  6 files changed, 759 insertions(+), 2 deletions(-)
>  create mode 100644 src/util/virshm.c
>  create mode 100644 src/util/virshm.h

> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7338ab9..048a096 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -152,6 +152,7 @@ UTIL_SOURCES =
> \
>  noinst_LTLIBRARIES += libvirt_conf.la
> @@ -1284,6 +1285,7 @@ libvirt_driver_qemu_impl_la_LIBADD = $(CAPNG_LIBS) \
>  $(GNUTLS_LIBS) \
>   $(LIBNL_LIBS) \
>   $(LIBXML_LIBS) \
> +$(LIBRT_LIBS) \
>   $(NULL)
>  libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
>  
> @@ -2264,6 +2266,7 @@ libvirt_setuid_rpc_client_la_LDFLAGS =  \
>   $(AM_LDFLAGS)   \
>   $(LIBXML_LIBS)  \
>   $(SECDRIVER_LIBS)   \
> +$(LIBRT_LIBS)   \
>   $(NULL)
>  libvirt_setuid_rpc_client_la_CFLAGS =\
>   -DLIBVIRT_SETUID_RPC_CLIENT \

Indentation is messed up for these

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index af73177..977fd34 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2078,6 +2078,22 @@ sexpr_u64;
>  string2sexpr;
>  
>  
> +# util/virshm.h
> +virShmBuildPath;
> +virShmCreate;
> +virShmObjectFree;
> +virShmObjectNew;
> +virShmObjectListAdd;
> +virShmObjectListDel;
> +virShmObjectFindByName;
> +virShmObjectListGetDefault;
> +virShmObjectRemoveStateFile;
> +virShmObjectSaveState;
> +virShmOpen;
> +virShmRemoveUsedDomain;
> +virShmSetUsedDomain;
> +virShmUnlink;
> +

Did you run 'make check' because they should fail on the
sort ordering test.

> diff --git a/src/util/virshm.c b/src/util/virshm.c
> new file mode 100644
> index 000..7ab39be
> --- /dev/null
> +++ b/src/util/virshm.c
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +#ifdef HAVE_SHM_OPEN
> +# include 
> +#endif
> +
> +#include "virshm.h"
> +#include "virxml.h"
> +#include "virbuffer.h"
> +#include "virerror.h"
> +#include "virstring.h"
> +#include "virlog.h"
> +#include "virutil.h"
> +#include "viralloc.h"
> +#include "virfile.h"
> +#include "configmake.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +VIR_LOG_INIT("util.shm");
> +
> +#define SHMEM_STATE_DIR LOCALSTATEDIR "/run/libvirt/shmem"
> +
> +VIR_ENUM_IMPL(virShmObject, VIR_SHM_TYPE_LAST,
> +  "shm",
> +  "server");
> +
> +static virClassPtr virShmObjectListClass;
> +
> +static virShmObjectListPtr mainlist; /* global shm object list */
> +
> +static void virShmObjectListDispose(void *obj);

> +int
> +virShmSetUsedDomain(virShmObjectPtr shmobj,
> +const char *drvname,
> +const char *domname)
> +{
> +char *tmpdomain = NULL;
> +
> +if (shmobj->drvname) {
> +if (STRNEQ(drvname, shmobj->drvname)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("cannot use one shmem for different driver"));
> +goto error;
> +}
> +} else {
> +if (VIR_STRDUP(shmobj->drvname, drvname) < 0)
> +goto error;
> +}
> +
> +if (VIR_STRDUP(tmpdomain, domname) < 0)
> +goto error;
> +
> +if (VIR_APPEND_ELEMENT(shmobj->domains, shmobj->ndomains, tmpdomain) < 0)
> +goto error;
> +
> +return 0;
> +
> + error:
> +VIR_FREE(tmpdomain);

In this error codepath you have possibly already set
shmobj->drvname, so you need to be able to undo that.

> +return -1;
> +}

> +#ifdef HAVE_SHM_OPEN
> +int
> +virShmOpen(const char *name,
> +   unsigned long long size,
> +   mode_t mode)
> +{
> +int fd = -1;
> +struct stat sb;
> +
> +if ((fd = shm_open(name, O_RDWR, mode)) < 0) {
> +virReportSystemError(errno,
> + _("Unable to open shared memory"
> +   " objects '%s'"),
> + name);
> +return -1;
> +}
> +
> +if (fstat(fd, &sb) < 0) {
> +virReportSystemError(errno,
> + _("cannot stat file '%s'"), name);
> +goto error;
> +}
> +
> +if (sb.st_size < size) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("already exist shared memory object"
> + " size %ju is smaller than required size %llu"),
> + 

Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:
> Signed-off-by: Luyao Huang 
> ---
>  src/qemu/qemu_conf.h|   3 +
>  src/qemu/qemu_driver.c  |   4 ++
>  src/qemu/qemu_process.c | 158 
> 
>  3 files changed, 165 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1c0c734..84b3b5e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4321,6 +4321,154 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>  }
>  
>  
> +static int
> +qemuPrepareShmemDevice(virQEMUDriverPtr driver,
> +   virDomainObjPtr vm,
> +   virDomainShmemDefPtr shmem)
> +{
> +int ret = -1;
> +virShmObjectPtr tmp;
> +virShmObjectListPtr list = driver->shmlist;
> +bool othercreate = false;
> +char *path = NULL;
> +bool teardownlabel = false;
> +bool teardownshm = false;
> +int type, fd;
> +
> +virObjectLock(list);
> +
> +if ((tmp = virShmObjectFindByName(list, shmem->name))) {
> +if (shmem->size > tmp->size) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Shmem object %s is already exists and "
> + "size is smaller than require size"),
> +   tmp->name);
> +goto cleanup;
> +}
> +
> +if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm->def->name) < 0)
> +goto cleanup;
> +
> +if (virShmObjectSaveState(tmp, list->stateDir) < 0)
> +goto cleanup;
> +
> +virObjectUnlock(list);
> +return 0;
> +}
> +
> +if (!shmem->server.enabled) {
> +if ((fd = virShmCreate(shmem->name, shmem->size, false, 
> &othercreate, 0600)) < 0)
> +goto cleanup;
> +VIR_FORCE_CLOSE(fd);
> +
> +if ((ret = virShmBuildPath(shmem->name, &path)) == -1) {
> +ignore_value(virShmUnlink(shmem->name));
> +goto cleanup;
> +} else if (ret == -2 && !othercreate) {
> +ignore_value(virShmUnlink(shmem->name));
> +}
> +type = VIR_SHM_TYPE_SHM;
> +} else {
> +if (!virFileExists(shmem->server.chr.data.nix.path)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Shmem device server socket is not exist"));
> +goto cleanup;
> +} else {
> +othercreate = true;
> +}
> +type = VIR_SHM_TYPE_SERVER;
> +}
> +teardownshm = true;
> +
> +if (virSecurityManagerSetShmemLabel(driver->securityManager,
> +vm->def, shmem, path) < 0)
> +goto cleanup;

You shouldn't be setting labelling at this point. That should be done
by the later call to virSecurityManagerSetAllLabel

> +static int
> +qemuCleanUpShmemDevice(virQEMUDriverPtr driver,
> +   virDomainObjPtr vm,
> +   virDomainShmemDefPtr shmem)
> +{
> +virShmObjectPtr tmp;
> +virShmObjectListPtr list = driver->shmlist;
> +int ret = -1;
> +
> +virObjectLock(list);
> +
> +if (!(tmp = virShmObjectFindByName(list, shmem->name))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Cannot find share memory named '%s'"),
> +   shmem->name);
> +goto cleanup;
> +}
> +if ((shmem->server.enabled && tmp->type != VIR_SHM_TYPE_SERVER) ||
> +(!shmem->server.enabled && tmp->type != VIR_SHM_TYPE_SHM)) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Shmem object and shmem device type is not equal"));
> +goto cleanup;
> +}
> +
> +if (virShmRemoveUsedDomain(tmp, QEMU_DRIVER_NAME, vm->def->name) < 0)
> +goto cleanup;
> +
> +if (tmp->ndomains == 0) {
> +if (virSecurityManagerRestoreShmemLabel(driver->securityManager,
> +vm->def, shmem, tmp->path) < 
> 0)
> +VIR_WARN("Unable to restore shared memory device labelling");

Likewise this should be left to the main label restore code

> +
> +if (!shmem->server.enabled) {
> +if (!tmp->othercreate &&
> +virShmUnlink(tmp->name) < 0)
> +VIR_WARN("Unable to unlink shared memory object");
> +}
> +
> +if (virShmObjectRemoveStateFile(list, tmp->name) < 0)
> +goto cleanup;
> +virShmObjectListDel(list, tmp);
> +virShmObjectFree(tmp);
> +}
> +
> +ret = 0;
> + cleanup:
> +virObjectUnlock(list);
> +return ret;
> +}

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 :|

--
libv

Re: [libvirt] [PATCH 4/4] qemu: call the helpers in virshm.c to manage shmem device

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:49PM +0800, Luyao Huang wrote:
> Signed-off-by: Luyao Huang 
> ---
>  src/qemu/qemu_conf.h|   3 +
>  src/qemu/qemu_driver.c  |   4 ++
>  src/qemu/qemu_process.c | 158 
> 
>  3 files changed, 165 insertions(+)
> 

> +static int
> +qemuPrepareShmemDevice(virQEMUDriverPtr driver,
> +   virDomainObjPtr vm,
> +   virDomainShmemDefPtr shmem)
> +{
> +int ret = -1;
> +virShmObjectPtr tmp;
> +virShmObjectListPtr list = driver->shmlist;
> +bool othercreate = false;
> +char *path = NULL;
> +bool teardownlabel = false;
> +bool teardownshm = false;
> +int type, fd;
> +
> +virObjectLock(list);
> +
> +if ((tmp = virShmObjectFindByName(list, shmem->name))) {
> +if (shmem->size > tmp->size) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Shmem object %s is already exists and "
> + "size is smaller than require size"),
> +   tmp->name);
> +goto cleanup;
> +}
> +
> +if (virShmSetUsedDomain(tmp, QEMU_DRIVER_NAME, vm->def->name) < 0)
> +goto cleanup;
> +
> +if (virShmObjectSaveState(tmp, list->stateDir) < 0)
> +goto cleanup;
> +
> +virObjectUnlock(list);
> +return 0;
> +}
> +
> +if (!shmem->server.enabled) {
> +if ((fd = virShmCreate(shmem->name, shmem->size, false, 
> &othercreate, 0600)) < 0)
> +goto cleanup;
> +VIR_FORCE_CLOSE(fd);
> +
> +if ((ret = virShmBuildPath(shmem->name, &path)) == -1) {
> +ignore_value(virShmUnlink(shmem->name));
> +goto cleanup;
> +} else if (ret == -2 && !othercreate) {
> +ignore_value(virShmUnlink(shmem->name));

Why are you treating -1 differentl from -2 - in both cases we should
abort creation as that indicates the method either failed or is not
supported in this platform.

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 1/4] conf: introduce seclabels in shmem device element

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote:
> Introduce a new element in shmem device element, this
> could help users to change the shm label to a specified
> label.
> 
> Signed-off-by: Luyao Huang 
> ---
>  docs/formatdomain.html.in |  7 ++
>  docs/schemas/domaincommon.rng |  3 +++
>  src/conf/domain_conf.c| 55 
> ++-
>  src/conf/domain_conf.h|  5 
>  4 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d0c1741..e02c67c 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null
>vectors. The ioeventd attribute enables/disables (values
>"on"/"off", respectively) ioeventfd.
>  
> +seclabel
> +
> +  The  optional seclabel to override the way that labelling
> +  is done on the shm object path or shm server path.  If this
> +  element is not present, the security label is 
> inherited
> +  from the per-domain setting.
> +
>
>  
>  Memory devices
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 1120003..f58e8de 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3323,6 +3323,9 @@
>  
>
>  
> +
> +  
> +
>  
>
>  

So in the  XML we have an explicit element to indicate whether the
device is intended to be shared across multiple guests. 

I think we need to have the same flag added to the shm device too, so
that we sanity check whether a particular shm was intended to be shared
or whether it is a mistake when multiple guests use it. This will also
allow us to integrate with the virtlockd to acquire exclusive locks
against the shm device to actively prevent admin mistakes starting
2 guests with the same shm. It will also let us automatically choose
the right default SELinux label ie svirt_image_t:s0:c214,c3242 for
exclusive access vs svirt_image_t:s0 for shared access


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] [sandbox PATCH 02/11] Fix virt-sandbox-image

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 03:57:28PM +, Eren Yagdiran wrote:
> Authentication fix for Docker REST API.
> ---
>  virt-sandbox-image/virt-sandbox-image.py | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/virt-sandbox-image/virt-sandbox-image.py 
> b/virt-sandbox-image/virt-sandbox-image.py
> index 4da3dde..324e568 100644
> --- a/virt-sandbox-image/virt-sandbox-image.py
> +++ b/virt-sandbox-image/virt-sandbox-image.py
> @@ -1,8 +1,10 @@
>  #!/usr/bin/python -Es
>  #
>  # Authors: Daniel P. Berrange 
> +#  Eren Yagdiran 
>  #
>  # Copyright (C) 2013 Red Hat, Inc.
> +# Copyright (C) 2015 Universitat Politècnica de Catalunya.
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -168,7 +170,7 @@ def download_template(name, server, destdir):
>  # or more parents, in a linear stack. Here we are getting the list
>  # of layers for the image with the tag we used.
>  (data, res) = get_json(registryserver, "/v1/images/" + imagetagid + 
> "/ancestry",
> -   { "Cookie": cookie })
> +   { "Authorization": "Token " + token })
>  
>  if data[0] != imagetagid:
>  raise ValueError(["Expected first layer id '%s' to match image id 
> '%s'",
> @@ -190,9 +192,9 @@ def download_template(name, server, destdir):
>  if not os.path.exists(jsonfile) or not os.path.exists(datafile):
>  # The '/json' URL gives us some metadata about the layer
>  res = save_data(registryserver, "/v1/images/" + layerid + 
> "/json",
> -{ "Cookie": cookie }, jsonfile)
> +{ "Authorization": "Token " + token }, 
> jsonfile)
>  createdFiles.append(jsonfile)
> -layersize = int(res.info().getheader("x-docker-size"))
> +layersize = int(res.info().getheader("Content-Length"))
>  
>  datacsum = None
>  if layerid in checksums:
> @@ -201,7 +203,7 @@ def download_template(name, server, destdir):
>  # and the '/layer' URL is the actual payload, provided
>  # as a tar.gz archive
>  save_data(registryserver, "/v1/images/" + layerid + "/layer",
> -  { "Cookie": cookie }, datafile, datacsum, 
> layersize)
> +  { "Authorization": "Token " + token }, datafile, 
> datacsum, layersize)
>  createdFiles.append(datafile)
>  
>  # Strangely the 'json' data for a layer doesn't include

ACK


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] [sandbox PATCH 01/11] Add virt-sandbox-image

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 03:57:27PM +, Eren Yagdiran wrote:
> From: Daniel P Berrange 
> 
> virt-sandbox-image.py is a python script that lets you download Docker
> images easily. It is a proof of concept code and consumes Docker Rest API.
> ---
>  po/POTFILES.in   |   1 +
>  virt-sandbox-image/virt-sandbox-image.py | 397 
> +++
>  2 files changed, 398 insertions(+)
>  create mode 100644 virt-sandbox-image/virt-sandbox-image.py
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index afcb050..7204112 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -11,3 +11,4 @@ libvirt-sandbox/libvirt-sandbox-context-interactive.c
>  libvirt-sandbox/libvirt-sandbox-init-common.c
>  libvirt-sandbox/libvirt-sandbox-rpcpacket.c
>  libvirt-sandbox/libvirt-sandbox-util.c
> +virt-sandbox-image/virt-sandbox-image.py

This should really live in the 'bin/' directory and not have any
.py suffix


> +def get_url(server, path, headers):
> +url = "https://"; + server + path
> +debug("  Fetching %s..." % url)
> +

There is trailng whitespace here that can be chomped

> +def save_data(server, path, headers, dest, checksum=None, datalen=None):
> +try:
> +res = get_url(server, path, headers)
> +
> +csum = None
> +if checksum is not None:
> +csum = hashlib.sha256()
> +
> +pattern = [".", "o", "O", "o"]
> +patternIndex = 0
> +donelen = 0
> +
> +with open(dest, "w") as f:
> +while 1:
> +buf = res.read(1024*64)
> +if not buf:
> +break
> +if csum is not None:
> +csum.update(buf)
> +f.write(buf)
> +
> +if datalen is not None:
> +donelen = donelen + len(buf)
> +debug("\x1b[s%s (%5d Kb of %5d Kb)\x1b8" % (
> +pattern[patternIndex], (donelen/1024), (datalen/1024)
> +))
> +patternIndex = (patternIndex + 1) % 4
> +
> +debug("\x1b[K")
> +if csum is not None:
> +csumstr = "sha256:" + csum.hexdigest()
> +if csumstr != checksum:
> +debug("FAIL checksum '%s' does not match '%s'" % (csumstr, 
> checksum))
> +os.remove(dest)
> +raise IOError("Checksum '%s' for data does not match '%s'" % 
> (csumstr, checksum))
> +

And here and a few other places.


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] [sandbox PATCH 02/11] Fix virt-sandbox-image

[Daniel P. Berrange]

On Tue, Jul 28, 2015 at 02:43:44PM +0200, Cedric Bosdonnat wrote:
> Hi Eren,
> 
> As a fix of the PoC, this commit could introduce the man page for the
> new command.

I think it is fine to introduce the manpage as a separate
commit at the end of the series, not least because the rest
of the series significantly changes / extends the command
line syntax of this tool.

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] [sandbox PATCH 04/11] Image: Add download function

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 03:57:30PM +, Eren Yagdiran wrote:
> Refactor download function from virt-sandbox-image to use
> the newly introduced Source abstract class. The docker-specific
> download code is moved to a new DockerSource class.
> ---
>  virt-sandbox-image/sources/DockerSource.py | 193 +++
>  virt-sandbox-image/sources/Source.py   |   5 +
>  virt-sandbox-image/virt-sandbox-image.py   | 202 
> -
>  3 files changed, 225 insertions(+), 175 deletions(-)
>  create mode 100644 virt-sandbox-image/sources/DockerSource.py
> 
> diff --git a/virt-sandbox-image/sources/DockerSource.py 
> b/virt-sandbox-image/sources/DockerSource.py
> new file mode 100644
> index 000..5bcd613
> --- /dev/null
> +++ b/virt-sandbox-image/sources/DockerSource.py
> @@ -0,0 +1,193 @@
> +#!/usr/bin/python
> +
> +from Source import Source
> +import urllib2
> +import sys
> +import json
> +import traceback
> +import os
> +import subprocess
> +import shutil
> +
> +class DockerSource(Source):
> +default_index_server = "index.docker.io"
> +default_template_dir = "/var/lib/libvirt/templates"
> +default_image_path = "/var/lib/libvirt/templates"
> +default_disk_format = "qcow2"
> +
> +www_auth_username = None
> +www_auth_password = None
> +
> +def 
> __init__(self,server="index.docker.io",destdir="/var/lib/libvirt/templates"):
> +self.default_index_server = server
> +self.default_template_dir = destdir
> +
> +def download_template(self,**args):
> +name = args['name']
> +registry = args['registry'] if args['registry'] is not None else 
> self.default_index_server
> +username = args['username']
> +password = args['password']
> +templatedir = args['templatedir'] if args['templatedir'] is not None 
> else self.default_template_dir
> +self.__download_template(name,registry,username,password,templatedir)
> +
> +def __download_template(self,name, server,username,password,destdir):

Double underscores are used by python built-in methods, so you should
avoid them. Convention is to have a single leading _ for private
methods.


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] [sandbox PATCH 05/11] Image: Refactor create function

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 03:57:31PM +, Eren Yagdiran wrote:
> Move the docker-related code to the DockerSource and use
> the Source mechanism
> ---
>  virt-sandbox-image/sources/DockerSource.py | 95 
> ++
>  virt-sandbox-image/sources/Source.py   |  5 ++
>  virt-sandbox-image/virt-sandbox-image.py   | 72 +-
>  3 files changed, 115 insertions(+), 57 deletions(-)
> 
> @@ -215,6 +166,11 @@ def requires_source(parser):
>  default="docker",
>  help=_("name of the template"))
>  
> +def requires_driver(parser):
> +parser.add_argument("-d","--driver",
> +default="qemu:///session",
> +help=_("Type of the driver"))
> +

The convention we've adopted with libvirt applications is to
always use -c / --connect  to provide the URI. I'd suggest
the URI should default to None, so that we let libvirt choose
the best URI. Using qemu:///session default for example, breaks
when running as root.


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] [sandbox PATCH 03/11] Image: Add Hooking Mechanism

[Daniel P. Berrange]

On Thu, Jul 23, 2015 at 03:57:29PM +, Eren Yagdiran wrote:
> Any custom source provider can be added to virt-sandbox-image as a source
> ---
>  virt-sandbox-image/sources/Source.py |  8 
>  virt-sandbox-image/virt-sandbox-image.py | 30 +-
>  2 files changed, 37 insertions(+), 1 deletion(-)
>  create mode 100644 virt-sandbox-image/sources/Source.py
> 
> diff --git a/virt-sandbox-image/sources/Source.py 
> b/virt-sandbox-image/sources/Source.py
> new file mode 100644
> index 000..cfc75d3
> --- /dev/null
> +++ b/virt-sandbox-image/sources/Source.py
> @@ -0,0 +1,8 @@
> +#!/usr/bin/python
> +
> +from abc import ABCMeta, abstractmethod
> +
> +class Source():
> +__metaclass__ = ABCMeta
> +def __init__(self):
> +pass
> diff --git a/virt-sandbox-image/virt-sandbox-image.py 
> b/virt-sandbox-image/virt-sandbox-image.py
> index 324e568..99ed46e 100644
> --- a/virt-sandbox-image/virt-sandbox-image.py
> +++ b/virt-sandbox-image/virt-sandbox-image.py
> @@ -1,5 +1,5 @@
>  #!/usr/bin/python -Es
> -#
> +# -*- coding: utf-8 -*-
>  # Authors: Daniel P. Berrange 
>  #  Eren Yagdiran 
>  #
> @@ -38,6 +38,34 @@ default_template_dir = "/var/lib/libvirt/templates"
>  debug = True
>  verbose = True
>  
> +sys.dont_write_bytecode = True
> +
> +
> +##Hook mechanism starts##
> +import __builtin__
> +from sources.Source import Source
> +__builtin__.hookHolder = {}
> +def add_hook(driverName,clazz):
> +holder = __builtin__.hookHolder
> +if not issubclass(clazz,Source):
> +raise Exception("Loading %s failed. Make sure it is a subclass Of 
> %s" %(clazz,Source))
> +holder[driverName] = clazz
> +
> +def init_from_name(name):
> +holder = __builtin__.hookHolder
> +return holder.get(name,None)
> +
> +__builtin__.add_hook = add_hook
> +__builtin__.init_from_name = init_from_name
> +from sources import *
> +
> +def dynamic_source_loader(name):
> +obj = init_from_name(name)
> +if obj == None:
> +raise IOError
> +return obj()
> +##Hook mechanism ends

I think this can be a hell of alot simpler if we just define
a fixed convention for module and class naming.
eg something like this:

  import importlib

  def dynamic_source_loader(name)
  modname = "sources." + name
  mod = importlib.import_module(modname)
  classname = name[0].upper() + name[1:] + "Source"
  classimpl = mod.getattr(classname)
  return classimpl()



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 2/2] Avoid starting a PowerPC VM with floppy disk

[madhu pavan]

On 07/28/2015 05:32 PM, Ján Tomko wrote:

On Fri, Jul 24, 2015 at 03:30:49PM -0400, Kothapally Madhu Pavan wrote:

PowerPC pseries based VMs do not support a floppy disk controller.
This prohibits libvirt from creating qemu command with floppy device.

Signed-off-by: Kothapally Madhu Pavan 
---
  src/qemu/qemu_command.c |   47 +--

Extending the qemuxml2argvtest with a test case that fails on such
config would be nice.


  1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 42906a8..93f84e2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9486,6 +9486,12 @@ qemuBuildCommandLine(virConnectPtr conn,
  boot[i] = 'd';
  break;
  case VIR_DOMAIN_BOOT_FLOPPY:
+/* PowerPC pseries based VMs do not support floppy device */
+if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support 
floppy device"));
+goto error;
+}

There is no need to error out earlier for machines that do not use
deviceboot. This error can be dropped, we will hit the next one.


  boot[i] = 'a';
  break;
  case VIR_DOMAIN_BOOT_DISK:
@@ -9769,6 +9775,12 @@ qemuBuildCommandLine(virConnectPtr conn,
  bootCD = 0;
  break;
  case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
+/* PowerPC pseries based VMs do not support floppy device */
+if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support 
floppy device"));
+goto error;
+}

This is more appropriate place for the error message. Personally, I would move 
it
above the switch() and let the switch only deal with boot indexes.


  bootindex = bootFloppy;
  bootFloppy = 0;
  break;
@@ -9812,6 +9824,12 @@ qemuBuildCommandLine(virConnectPtr conn,
  
  if (withDeviceArg) {

  if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
+/* PowerPC pseries based VMs do not support floppy device 
*/
+if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support 
floppy device"));
+goto error;
+}

This is dead code, the condition will never be true here, we already
matched the error above.


  if (virAsprintf(&optstr, "drive%c=drive-%s",
  disk->info.addr.drive.unit ? 'B' : 'A',
  disk->info.alias) < 0)
@@ -9854,6 +9872,12 @@ qemuBuildCommandLine(virConnectPtr conn,
  /* Newer Q35 machine types require an explicit FDC controller */
  virBufferTrim(&fdc_opts, ",", -1);
  if ((fdc_opts_str = virBufferContentAndReset(&fdc_opts))) {
+/* PowerPC pseries based VMs do not support floppy device */
+if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support floppy 
device"));
+goto error;
+}

Same here.


  virCommandAddArg(cmd, "-device");
  virCommandAddArgFormat(cmd, "isa-fdc,%s", fdc_opts_str);
  VIR_FREE(fdc_opts_str);
@@ -9918,10 +9942,17 @@ qemuBuildCommandLine(virConnectPtr conn,
 _("cannot create virtual FAT disks in 
read-write mode"));
  goto error;
  }
-if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
+if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+/* PowerPC pseries based VMs do not support floppy device 
*/
+if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support 
floppy device"));
+goto error;
+}
  fmt = "fat:floppy:%s";
-else
+} else {
  fmt = "fat:%s";
+}
  

This code path can only be taken for QEMUs not having QEMU_CAPS_DRIVE,
i.e. older than at least 7 years. I do 

Re: [libvirt] [PATCH 0/2] Disable floppy disk for PowerPC VMs

[madhu pavan]

On 07/29/2015 06:25 PM, Andrea Bolognani wrote:

On Fri, 2015-07-24 at 15:29 -0400, Kothapally Madhu Pavan wrote:

This is an attempt to fix:
Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1180486

Libvirt currently assumes ISA_based floppy disks to be available
across all
architectures and machine types. However, PowerPC Book 3S compatible
('ie pseries)
virtual machines do not support Floppy disks.

This patch series prevents libvirt from launching ppc64[le] -based
'pseries'
VMs with floppy devices.

Hi,

sorry for not replying right away.

I started looking into that bug a while ago but I got sidetracked
shortly afterwards, so I don't have much to show for it. I'd like
to share my opinion on the matter anyway.

I believe you're basically following the right approach, eg. avoid
setting floppy-related capabilities and erroring out afterwards if
attempts are made to use devices that require such capabilites.

However, I think the implementation should be a little more
generic: ideally, the code would contain no references to the
ppc64 architecture and whether or not floppy disks are be allowed
would be determined by probing the QEMU binary.

Writing the code this way would allow us to handle automatically
other architectures where floppy disks do not make sense[1] and
situations where floppy support is not available[2].

If that turns out not to be possible or practical, at the very
least the check should be performed in one single spot, and not
replicated a dozen times thorough the library.So,
As, present method of capabilities parsing uses "qemu -h", and the list 
received in this case is arch agnostic. So, we cannot do arch-specific 
caps parsing unless qemu undergoes a rewrite. I will be providing a new 
patch set with as minimal changes as possible.


Thanks,
Madhu Pavan.

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

[libvirt] [PATCH v2 1/3] Caps: Disable floppy disk for PowerPC VM

[Kothapally Madhu Pavan]

PowerPC pseries based VMs do not support a floppy disk controller.
This prohibits libvirt from adding floppy disk for a PowerPC pseries VM.

Signed-off-by: Kothapally Madhu Pavan 
---
 src/qemu/qemu_capabilities.c |   15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d8cb32d..e304473 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3919,25 +3919,32 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps,
 
 static int
 virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps,
+const char *machine,
 virDomainCapsDeviceDiskPtr disk)
 {
 disk->device.supported = true;
 /* QEMU supports all of these */
 VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice,
  VIR_DOMAIN_DISK_DEVICE_DISK,
- VIR_DOMAIN_DISK_DEVICE_CDROM,
- VIR_DOMAIN_DISK_DEVICE_FLOPPY);
+ VIR_DOMAIN_DISK_DEVICE_CDROM);
+
+/* PowerPC pseries based VMs do not support floppy device */
+if (!(ARCH_IS_PPC64(qemuCaps->arch) && STRPREFIX(machine, "pseries")))
+VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, 
VIR_DOMAIN_DISK_DEVICE_FLOPPY);
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO))
 VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_LUN);
 
 VIR_DOMAIN_CAPS_ENUM_SET(disk->bus,
  VIR_DOMAIN_DISK_BUS_IDE,
- VIR_DOMAIN_DISK_BUS_FDC,
  VIR_DOMAIN_DISK_BUS_SCSI,
  VIR_DOMAIN_DISK_BUS_VIRTIO,
  /* VIR_DOMAIN_DISK_BUS_SD */);
 
+/* PowerPC pseries based VMs do not support floppy device */
+if (!(ARCH_IS_PPC64(qemuCaps->arch) && STRPREFIX(machine, "pseries")))
+VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_FDC);
+
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE))
 VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB);
 return 0;
@@ -4008,7 +4015,7 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
 
 if (virQEMUCapsFillDomainOSCaps(qemuCaps, os,
 loader, nloader) < 0 ||
-virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, disk) < 0 ||
+virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) 
< 0 ||
 virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0)
 return -1;
 return 0;

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

[libvirt] [PATCH v2 2/3] Avoid starting a PowerPC VM with floppy disk

[Kothapally Madhu Pavan]

PowerPC pseries based VMs do not support a floppy disk controller.
This prohibits libvirt from creating qemu command with floppy device.

Signed-off-by: Kothapally Madhu Pavan 
---
 src/qemu/qemu_command.c |8 
 1 file changed, 8 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 09f30c4..501c7df 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9767,6 +9767,14 @@ qemuBuildCommandLine(virConnectPtr conn,
 continue;
 }
 
+/* PowerPC pseries based VMs do not support floppy device */
+if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
+ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, 
"pseries")) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("PowerPC pseries machines do not support 
floppy device"));
+goto error;
+}
+
 switch (disk->device) {
 case VIR_DOMAIN_DISK_DEVICE_CDROM:
 bootindex = bootCD;

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

[libvirt] [PATCH v2 3/3] test: Introduce test case to disallow floppy disk on PowerPC VM

[Kothapally Madhu Pavan]

PowerPC pseries based VMs do not support a floppy disk controller.
This test case fails if libvirt allows a floppy disk on pseries VMs.

Signed-off-by: Kothapally Madhu Pavan 
---
 .../qemuxml2argv-disk-floppy-pseries.args  |6 +++
 .../qemuxml2argv-disk-floppy-pseries.xml   |   41 
 tests/qemuxml2argvtest.c   |1 
 3 files changed, 48 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args
new file mode 100644
index 000..6f68121
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args
@@ -0,0 +1,6 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 -S -M pseries -m 214 -smp 1 -nographic -monitor \
+unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -drive \
+file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0 -drive \
+file=/dev/fd0,if=floppy,unit=0 -drive file=/tmp/firmware.img,if=floppy,unit=1 \
+-net none -serial none -parallel none
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml
new file mode 100644
index 000..be0ede6
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml
@@ -0,0 +1,41 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-ppc64
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+
+
+
+
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f9b30d9..afd2ef3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -737,6 +737,7 @@ mymain(void)
 QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_VIRTIO_TX_ALG);
 DO_TEST("disk-cdrom-tray-no-device-cap", NONE);
 DO_TEST("disk-floppy", NONE);
+DO_TEST_FAILURE("disk-floppy-pseries", QEMU_CAPS_DRIVE);
 DO_TEST("disk-floppy-tray-no-device-cap", NONE);
 DO_TEST("disk-floppy-tray",
 QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE);

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

[libvirt] [PATCH v2 0/3] Disable floppy disk for PowerPC VMs

[Kothapally Madhu Pavan]

This is an attempt to fix:
Libvirt BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1180486

Libvirt currently assumes ISA_based floppy disks to be available across all
architectures and machine types. However, PowerPC Book 3S compatible ('ie 
pseries)
virtual machines do not support Floppy disks.

This patch series prevents libvirt from launching ppc64[le] -based 'pseries'
VMs with floppy devices.

This version of patch attempts to fulfill the coments of Andrea Bolognani
and Jan Tomko on previous version of patch.
---

Kothapally Madhu Pavan (3):
  Caps: Disable floppy disk for PowerPC VM
  Avoid starting a PowerPC VM with floppy disk
  test: Introduce test case to disallow floppy disk on pseries


 src/qemu/qemu_capabilities.c   |   15 +--
 src/qemu/qemu_command.c|8 
 .../qemuxml2argv-disk-floppy-pseries.args  |6 +++
 .../qemuxml2argv-disk-floppy-pseries.xml   |   41 
 tests/qemuxml2argvtest.c   |1 
 5 files changed, 67 insertions(+), 4 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-floppy-pseries.xml

--
Kothapally Madhu Pavan

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

Re: [libvirt] [PATCH] Inherit namespace feature

[Daniel P. Berrange]

On Wed, Jul 01, 2015 at 11:07:07PM +0530, ik.nitk wrote:
> This patch adds feature for lxc containers to inherit namespaces. This is 
> very similar to what
> lxc-tools or docker provides.  Look for "man lxc-start" and you will find 
> that you can pass command args as 
> [ --share-[net|ipc|uts] name|pid ]. Or check out docker networking option in 
> which you can give --net=container:NAME_or_ID as an option for sharing 
> namespace. 
> 
> >From this patch you can add extra libvirt option to share namespace in 
> >following way.
>  
>
>
>
>  
> 
> 
> ---
>  docs/drvlxc.html.in   |  18 +++
>  docs/schemas/domaincommon.rng |  42 ++
>  src/Makefile.am   |   4 +-
>  src/lxc/lxc_conf.c|   2 +-
>  src/lxc/lxc_conf.h|  15 +++
>  src/lxc/lxc_container.c   | 236 
> +-
>  src/lxc/lxc_domain.c  | 164 ++-
>  src/lxc/lxc_domain.h  |   1 +
>  tests/lxcxml2xmldata/lxc-sharenet.xml |  33 +
>  tests/lxcxml2xmltest.c|   1 +
>  10 files changed, 507 insertions(+), 9 deletions(-)
>  create mode 100644 tests/lxcxml2xmldata/lxc-sharenet.xml
> 
> diff --git a/docs/drvlxc.html.in b/docs/drvlxc.html.in
> index a094bd9..d14d4c7 100644
> --- a/docs/drvlxc.html.in
> +++ b/docs/drvlxc.html.in
> @@ -590,6 +590,24 @@ Note that allowing capabilities that are normally 
> dropped by default can serious
>  affect the security of the container and the host.
>  
>  
> +Inherit namespaces
> +
> +
> +Libvirt allows you to inherit the namespace from container/process just like 
> lxc tools
> +or docker provides to share the network namespace. The following can be used 
> to share
> +required namespaces. If we want to share only one then the other namespaces 
> can be ignored.
> +
> +
> + xmlns:lxc='http://libvirt.org/schemas/domain/lxc/1.0'>
> +...
> +
> +  
> +  
> +  
> +
> +
> +

Could you also document the attributes explicitly, the various 'type'
attribute values and what they expect for the corresponding 'value'
argument. In particular I'm unclear on what the value is when
using type='netns', so its a good idea to be explicit about this.

> diff --git a/src/Makefile.am b/src/Makefile.am
> index be63e26..ef96a5a 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1319,7 +1319,7 @@ libvirt_driver_lxc_impl_la_CFLAGS = \
>   -I$(srcdir)/access \
>   -I$(srcdir)/conf \
>   $(AM_CFLAGS)
> -libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS) $(FUSE_LIBS)
> +libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS) 
> $(LIBXML_LIBS) libvirt-lxc.la $(FUSE_LIBS)

What was the $LIBXML_LIBS addition needed for ?

I can see why you added libvirt-lxc.la but I will suggested
changes later to avoid that.


>  if WITH_BLKID
>  libvirt_driver_lxc_impl_la_CFLAGS += $(BLKID_CFLAGS)
>  libvirt_driver_lxc_impl_la_LIBADD += $(BLKID_LIBS)
> @@ -2709,6 +2709,8 @@ libvirt_lxc_LDADD = \
>   libvirt-net-rpc.la \
>   libvirt_security_manager.la \
>   libvirt_conf.la \
> + libvirt.la \
> + libvirt-lxc.la \

Again I want us to avoid this too

>   libvirt_util.la \
>   ../gnulib/lib/libgnu.la
>  if WITH_DTRACE_PROBES


> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 11e9514..d8362ab 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -27,6 +27,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -38,7 +39,6 @@
>  #include 
>  #include 
>  #include 
> -
>  /* Yes, we want linux private one, for _syscall2() macro */
>  #include 

Try to avoid adding/removing random whitespace in patches. If you
think something warrents a cleanup just send a separate patch

>  
> @@ -2321,6 +2321,181 @@ virArch lxcContainerGetAlt32bitArch(virArch arch)
>  return VIR_ARCH_NONE;
>  }
>  
> +struct ns_info {
> +const char *proc_name;
> +int clone_flag;

We usually use capitalization in struct / type names not
underscores. Also try to always use a prefix to make it
clearer that its libvirt code not some system header.
so eg  lxcNSInfo


> +}ns_info_local[VIR_DOMAIN_NAMESPACE_LAST] = {


> +[VIR_DOMAIN_NAMESPACE_SHARENET] = {"net", CLONE_NEWNET},
> +[VIR_DOMAIN_NAMESPACE_SHAREIPC] = {"ipc", CLONE_NEWIPC},
> +[VIR_DOMAIN_NAMESPACE_SHAREUTS] = {"uts", CLONE_NEWUTS}
> +};
> +
> +static int lxcOpen_ns(lxcDomainDefPtr lxcDef, int 
> ns_fd[VIR_DOMAIN_NAMESPACE_LAST])

We mostly use capitlization in method names rather than underscores
so eg lxcOpenNS

> +{
> +int i, n, rc = 0;
> +virDomainPtr dom = NULL;
> +virCo

[libvirt] [PATCH] qemu: Reject migration with memory-hotplug if destination doesn't support it

[Peter Krempa]

If destination libvirt doesn't support memory hotplug since all the
support was introduced by adding new elements the destination would
attempt to start qemu with an invalid configuration. The worse part is
that qemu might hang in such situation.

Fix this by sending a required migration feature called 'memory-hotplug'
to the destination. If the destination doesn't recognize it it will fail
the migration.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1248350
---
 src/qemu/qemu_migration.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f5866c4..824126f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -86,6 +86,7 @@ enum qemuMigrationCookieFlags {
 QEMU_MIGRATION_COOKIE_FLAG_NETWORK,
 QEMU_MIGRATION_COOKIE_FLAG_NBD,
 QEMU_MIGRATION_COOKIE_FLAG_STATS,
+QEMU_MIGRATION_COOKIE_FLAG_MEMORY_HOTPLUG,

 QEMU_MIGRATION_COOKIE_FLAG_LAST
 };
@@ -98,7 +99,8 @@ VIR_ENUM_IMPL(qemuMigrationCookieFlag,
   "persistent",
   "network",
   "nbd",
-  "statistics");
+  "statistics",
+  "memory-hotplug");

 enum qemuMigrationCookieFeatures {
 QEMU_MIGRATION_COOKIE_GRAPHICS  = (1 << 
QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
@@ -107,6 +109,7 @@ enum qemuMigrationCookieFeatures {
 QEMU_MIGRATION_COOKIE_NETWORK = (1 << QEMU_MIGRATION_COOKIE_FLAG_NETWORK),
 QEMU_MIGRATION_COOKIE_NBD = (1 << QEMU_MIGRATION_COOKIE_FLAG_NBD),
 QEMU_MIGRATION_COOKIE_STATS = (1 << QEMU_MIGRATION_COOKIE_FLAG_STATS),
+QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG = (1 << 
QEMU_MIGRATION_COOKIE_FLAG_MEMORY_HOTPLUG),
 };

 typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
@@ -1352,6 +1355,9 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig,
 qemuMigrationCookieAddStatistics(mig, dom) < 0)
 return -1;

+if (flags & QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG)
+mig->flagsMandatory |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG;
+
 if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig)))
 return -1;

@@ -2974,6 +2980,11 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
 }
 }

+if (vm->def->mem.max_memory ||
+(vm->newDef &&
+ vm->newDef->mem.max_memory))
+cookieFlags |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG;
+
 if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
 goto cleanup;

-- 
2.4.5

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

[libvirt] [PATCH] qemu: Properly check for incoming migration job

[Jiri Denemark]

In addition to checking the current asynchronous job
qemuMigrationJobIsActive reports an error if the current job does not
match the one we asked for. Let's just check the job directly since we
are not interested in the error in qemuProcessHandleMonitorEOF.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1c0c734..23baa82 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -310,7 +310,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon 
ATTRIBUTE_UNUSED,
 auditReason = "failed";
 }
 
-if (qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN))
+if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_IN)
 qemuMigrationErrorSave(driver, vm->def->name,
qemuMonitorLastError(priv->mon));
 
-- 
2.5.0

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

Re: [libvirt] libvirt 1.2.17 fails

[Jason Helfman]

On Mon, Jul 27, 2015 at 2:46 PM, Jason Helfman  wrote:

> Hello All,
>
> I am getting build failures on the latest build, and it is currently
> blocking my commit of the update. Looks like xhtml, is a new build
> requirement. I added it to the list of dependencies, however it is failing
> to build within our build cluster.
>
>
> http://meatwad.mouf.net/rubick/poudriere/data/93i386-jgh/2015-07-27_15h41m26s/logs/errors/libvirt-1.2.17.log
> (also attached for historical purposes)
>
> I am able to get this to build locally, but trying to distinguish what is
> different compared with a clean system.
>
> Here is the listing of files contained in the xhtml package:
>
> xhtml-1.0.20020801_4:
> /usr/local/share/xml/dtd/xhtml/catalog.xml
> /usr/local/share/xml/dtd/xhtml/xhtml-dcl.soc
> /usr/local/share/xml/dtd/xhtml/xhtml-lat1.ent
> /usr/local/share/xml/dtd/xhtml/xhtml-special.ent
> /usr/local/share/xml/dtd/xhtml/xhtml-symbol.ent
> /usr/local/share/xml/dtd/xhtml/xhtml.soc
> /usr/local/share/xml/dtd/xhtml/xhtml1-frameset.dtd
> /usr/local/share/xml/dtd/xhtml/xhtml1-strict.dtd
> /usr/local/share/xml/dtd/xhtml/xhtml1-transitional.dtd
> /usr/local/share/xml/dtd/xhtml/xhtml1.dcl
>
>
Any ideas on fixing this failure?

Thanks,
Jason

-- 
Jason Helfman  | FreeBSD Committer
j...@freebsd.org | http://people.freebsd.org/~jgh  | The Power to Serve
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] ambiguous ret of qemuDomainDetachVirtioDiskDevice

[zhang bo]

On 2015/7/30 17:41, zhang bo wrote:

> On 2015/7/28 16:33, Ján Tomko wrote:
> 
>> On Tue, Jul 28, 2015 at 04:25:13PM +0800, zhang bo wrote:
>>> static int
>>> qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver,
>>>  virDomainObjPtr vm,
>>>  virDomainDiskDefPtr detach)
>>> {
>>> ...
>>>
>>> rc = qemuDomainWaitForDeviceRemoval(vm);
>>> if (rc == 0 || rc == 1)
>>> ret = qemuDomainRemoveDiskDevice(driver, vm, detach);
>>> else
>>> ret = 0;  /*the return value of 2 is dismissed here, which refers 
>>> to ETIMEOUT.*/
>>> 
>>> }
>>>
>>> 
>>>
>>> If it timeouts when qemu tries to del the device, the return value would be 
>>> modified from 2 to 0 in 
>>> function qemuDomainDetachVirtioDiskDevice(), which means that, the users 
>>> would be misleaded that 
>>> the device has been deleted, however, the device maybe probably failed to 
>>> be detached after timeout and
>>> still in use. 
>>>
>>
>> This is intentional and documented:
>> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDetachDeviceFlags
>>
>> Unplugging a disk requires guest cooperation, so the best we can do is
>> ask qemu to unplug it and wait for a while.
>>
>>> That is to say, the function qemuDomainDetachVirtioDiskDevice()'s return 
>>> value is ambiguous when it's 0, 
>>> maybe successful, or timeout. Will it be better to pass ETIMEOUT to user? 
>>> or any other advises? for example,
>>> let users themselves dumpxml the guest to check whether the device has been 
>>> actually detached or not? 
>>
>> Either dump the XML, or wait for the VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED
>> event, as the API documentation suggests.
>>
>> Jan
> 
> 
> It seems to have fixed the problem by dumping the XML or wait for the 
> DEVICE_REMOVED event. However, it seems to 
> make nova or other apps to do more checking work, they need to dump XML or 
> wait the event even if the device has
> already been successfully removed. which is unnecessary. 
> 
> I think, it's better to return ETIMEOUT and let nova to dumpxml or wait the 
> event at this situation, rather than always
> doing that job.
> 
> It maybe a better design, what's your opinion?
> 

After thinking twice, it's an async job, thus returning 0 is acceptable, right?

-- 
Oscar
oscar.zhan...@huawei.com  

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

[libvirt] [python PATCH] examples: small fix for nodestats.py example

[Luyao Huang]

Add nodestats.py in MANIFEST.in and add a
small description for nodestats.py in README

Signed-off-by: Luyao Huang 
---
 MANIFEST.in | 1 +
 examples/README | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/MANIFEST.in b/MANIFEST.in
index b5ba783..0d66f9c 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -13,6 +13,7 @@ include examples/esxlist.py
 include examples/event-test.py
 include examples/guest-vcpus/guest-vcpu-daemon.py
 include examples/guest-vcpus/guest-vcpu.py
+include examples/nodestats.py
 include examples/topology.py
 include generator.py
 include libvirt-lxc-override-api.xml
diff --git a/examples/README b/examples/README
index 0cb4513..13211b6 100644
--- a/examples/README
+++ b/examples/README
@@ -14,6 +14,9 @@ dhcpleases.py - list dhcp leases for a given virtual network
 domipaddrs.py - list IP addresses for guest domains
 guest-vcpus - two helpers to make the guest agent event useful with agent based
   vCPU state modification
+nodestats.py - print total memory and free memory for each host NUMA node and
+   the memory strictly bound to certain host nodes for each running
+   domain.
 
 The XML files in this directory are examples of the XML format that libvirt
 expects, and will have to be adapted for your setup. They are only needed
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[lhuang]

Hi Marc-André

On 07/27/2015 11:42 PM, Marc-André Lureau wrote:

Hi

On Thu, Jul 23, 2015 at 12:13 PM, Luyao Huang  wrote:

Introduce a new element in shmem device element, this
could help users to change the shm label to a specified
label.

Signed-off-by: Luyao Huang 
---
  docs/formatdomain.html.in |  7 ++
  docs/schemas/domaincommon.rng |  3 +++
  src/conf/domain_conf.c| 55 ++-
  src/conf/domain_conf.h|  5 
  4 files changed, 59 insertions(+), 11 deletions(-)


It would be better with a small test, checking parsing and format.


Oh, right, i forgot that, thanks for pointing out that, i will add them 
in next version.



diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d0c1741..e02c67c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -6098,6 +6098,13 @@ qemu-kvm -net nic,model=? /dev/null
vectors. The ioeventd attribute enables/disables (values
"on"/"off", respectively) ioeventfd.
  
+seclabel
+
+  The  optional seclabel to override the way that labelling

The "element may contain an" optional ...


Okay


+  is done on the shm object path or shm server path.  If this
+  element is not present, the security label is 
inherited
+  from the per-domain setting.
+


  Memory devices
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 1120003..f58e8de 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3323,6 +3323,9 @@
  

  
+
+  
+
  

  
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 73ac537..cb3d72a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11261,6 +11261,8 @@ virDomainNVRAMDefParseXML(xmlNodePtr node,
  static virDomainShmemDefPtr
  virDomainShmemDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
+  virSecurityLabelDefPtr* vmSeclabels,
+  int nvmSeclabels,
unsigned int flags)
  {
  char *tmp = NULL;
@@ -11332,6 +11334,10 @@ virDomainShmemDefParseXML(xmlNodePtr node,
  if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
  goto cleanup;

+if (virSecurityDeviceLabelDefParseXML(&def->seclabels, &def->nseclabels,
+  vmSeclabels, nvmSeclabels,
+  ctxt, flags) < 0)
+goto cleanup;

  ret = def;
  def = NULL;
@@ -12457,7 +12463,11 @@ virDomainDeviceDefParse(const char *xmlStr,
  goto error;
  break;
  case VIR_DOMAIN_DEVICE_SHMEM:
-if (!(dev->data.shmem = virDomainShmemDefParseXML(node, ctxt, flags)))
+if (!(dev->data.shmem = virDomainShmemDefParseXML(node,
+  ctxt,
+  def->seclabels,
+  def->nseclabels,
+  flags)))
  goto error;
  break;
  case VIR_DOMAIN_DEVICE_TPM:
@@ -16136,7 +16146,8 @@ virDomainDefParseXML(xmlDocPtr xml,
  for (i = 0; i < n; i++) {
  virDomainShmemDefPtr shmem;
  ctxt->node = nodes[i];
-shmem = virDomainShmemDefParseXML(nodes[i], ctxt, flags);
+shmem = virDomainShmemDefParseXML(nodes[i], ctxt, def->seclabels,
+  def->nseclabels, flags);
  if (!shmem)
  goto error;

@@ -20308,6 +20319,8 @@ virDomainShmemDefFormat(virBufferPtr buf,
  virDomainShmemDefPtr def,
  unsigned int flags)
  {
+size_t n;
+
  virBufferEscapeString(buf, "name);

  if (!def->size &&
@@ -20341,6 +20354,9 @@ virDomainShmemDefFormat(virBufferPtr buf,
  virBufferAddLit(buf, "/>\n");
  }

+for (n = 0; n < def->nseclabels; n++)
+virSecurityDeviceLabelDefFormat(buf, def->seclabels[n], flags);
+
  if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
  return -1;

@@ -23851,11 +23867,25 @@ virDomainObjListExport(virDomainObjListPtr domlist,
  }


+static virSecurityDeviceLabelDefPtr
+virDomainGetDeviceSecurityLabelDef(virSecurityDeviceLabelDefPtr *seclabels,
+   size_t nseclabels,
+   const char *model)
+{
+size_t i;
+
+for (i = 0; i < nseclabels; i++) {
+if (STREQ_NULLABLE(seclabels[i]->model, model))
+return seclabels[i];
+}
+return NULL;
+}
+
+
  virSecurityLabelDefPtr
  virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model)
  {
  size_t i;
-virSecurityLabelDefPtr seclabel = NULL;

  if (def == NULL || model == NULL)
  ret

Re: [libvirt] [PATCH 1/4] conf: introduce seclabels in shmem device element

[lhuang]

On 07/30/2015 05:48 PM, Daniel P. Berrange wrote:

On Thu, Jul 23, 2015 at 06:13:46PM +0800, Luyao Huang wrote:

Introduce a new element in shmem device element, this
could help users to change the shm label to a specified
label.

Signed-off-by: Luyao Huang 
---
  docs/formatdomain.html.in |  7 ++
  docs/schemas/domaincommon.rng |  3 +++
  src/conf/domain_conf.c| 55 ++-
  src/conf/domain_conf.h|  5 
  4 files changed, 59 insertions(+), 11 deletions(-)

As already mentioned, this must include additions to the qemu tests
suite for XML to XML conversion.


I must forgot this during wrote this patch, thanks for pointing out 
that, i will add a tests for the new XML element in next version.


Thanks a lot for your review.


Regards,
Daniel


Luyao

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