Re: [PATCH] Add vms cli tool to the list of applications using libvirt

[Cedric Bosdonnat]

On Wed, 2022-11-16 at 13:18 +0100, Michal Prívozník wrote:
> On 11/16/22 09:24, Cédric Bosdonnat wrote:
> > Just adds a tool to the applications list. This tool helps managing
> > multiple VMs at once using the python binding.
> > 
> > Signed-off-by: Cédric Bosdonnat 
> > ---
> >  docs/apps.rst | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Reviewed-by: Michal Privoznik 
> 
> and it's good to see you again :-)

I'm still not hacking on libvirt itself... just making my life easier with it, 
but I'm happy to "see" you again too.

Cédric

Tuning tool

[Cedric Bosdonnat]

Hi all,

I have recently started a virt-tuner tool using the python3 binding.
The goal of this tool is to help users tune their domains using a few
templates.

https://github.com/SUSE/virt-tuner

As of today I only wrote a template for a giant VM taking almost all
the host resources and mapping the host CPU topology. Feel free to
submit pull requests to submit other templates that would be of
interest to you.

Kind regards,

--
Cédric

Re: [PATCH v2] network: Only check kernel added routes in virNetDevIPCheckIPv6Forwarding

[Cedric Bosdonnat]

On Sun, 2020-09-13 at 13:34 -0400, Laine Stump wrote:
> On 9/11/20 1:08 PM, Laine Stump wrote:
> > On 9/11/20 7:34 AM, Ian Wienand wrote:
> > > The original motivation for adding virNetDevIPCheckIPv6Forwarding
> > > (00d28a78b5d1f6eaf79f06ac59e31c568af9da37) was that networking routes
> > > would disappear when ipv6 forwarding was enabled for an interface.
> > > 
> > > This is a fairly undocumented side-effect of the "accept_ra" sysctl
> > > for an interface.  1 means the interface will accept_ra's if not
> > > forwarding, 2 means always accept_RAs; but it is not explained that
> > > enabling forwarding when accept_ra==1 will also clear any kernel RA
> > > assigned routes, very likely breaking your networking.
> > > 
> > > The check to warn about this currently uses netlink to go through all
> > > the routes and then look at the accept_ra status of the interfaces.
> > > 
> > > However, it has been noticed that this problem does not affect systems
> > > where IPv6 RA configuration is handled in userspace, e.g. via tools
> > > such as NetworkManager.  In this case, the error message from libvirt
> > > is spurious, and modifying the forwarding state will not affect the RA
> > > state or disable your networking.
> > > 
> > > If you refer to the function rt6_purge_dflt_routers() in the kernel,
> > > we can see that the routes being purged are only those with the
> > > kernel's RTF_ADDRCONF flag set; that is, routes added by the kernel's
> > > RA handling.  Why does it do this?  I think this is a Linux
> > > implementation decision; it has always been like that and there are
> > > some comments suggesting that it is because a router should be
> > > statically configured, rather than accepting external configurations.
> > > 
> > > The solution implemented here is to convert the existing check into a
> > > walk of /proc/net/ipv6_route and look for routes with this flag set.
> > > We then check the accept_ra status for the interface, and if enabling
> > > forwarding would break things raise an error.
> > > 
> > > This should hopefully avoid "interactive" users, who are likely to be
> > > using NetworkManager and the like, having false warnings when enabling
> > > IPv6, but retain the error check for users relying on kernel-based
> > > IPv6 interface auto-configuration.
> > > 
> > > Signed-off-by: Ian Wienand 
> > > ---
> > 
> > [...]
> > 
> > 
> > Reviewed-by: Laine Stump 
> > 
> > 
> > (pending successful completion of CI (see below) and verification that 
> > the error is triggered when appropriate. Once I've verified those two, 
> > I'll push it).
> 
> I've fixed the couple of VIR_DEBUG problems (the commented out line that 
> Cedric noticed, and the one that fails CI due to the extraneous arg). I 
> also checked on my private gitlab fork that it will pass CI when pushed.
> 
> 
> Additionally, I disabled NetworkManager on one of my systems and 
> re-enabled the old deprecation-bound network.service (which uses the 
> kernel for RA). When I tried to start an IPv6 network, I got this message:
> 
> error: Failed to start network ipv6
> error: internal error: Check the host setup: interface enp7s0 has kernel 
> autoconfigured IPv6 routes and enabling forwarding  without accept_ra 
> set to 2 will cause the kernel to flush them, breaking networking.
> 
> This is enough proof for me, so I'm pushing the patch!

Thanks Laine for your testing.

--
Cedric

Re: [PATCH v2] network: Only check kernel added routes in virNetDevIPCheckIPv6Forwarding

[Cedric Bosdonnat]

Hi Ian,

ACK. I've seen a commented VIR_DEBUG though that you surely want to
remove before pushing. I also really like the verbose commit message
explaining all the reasoning behind the change, thanks for the hard
work.

--
Cedric

On Fri, 2020-09-11 at 21:34 +1000, Ian Wienand wrote:
> The original motivation for adding virNetDevIPCheckIPv6Forwarding
> (00d28a78b5d1f6eaf79f06ac59e31c568af9da37) was that networking routes
> would disappear when ipv6 forwarding was enabled for an interface.
> 
> This is a fairly undocumented side-effect of the "accept_ra" sysctl
> for an interface.  1 means the interface will accept_ra's if not
> forwarding, 2 means always accept_RAs; but it is not explained that
> enabling forwarding when accept_ra==1 will also clear any kernel RA
> assigned routes, very likely breaking your networking.
> 
> The check to warn about this currently uses netlink to go through all
> the routes and then look at the accept_ra status of the interfaces.
> 
> However, it has been noticed that this problem does not affect systems
> where IPv6 RA configuration is handled in userspace, e.g. via tools
> such as NetworkManager.  In this case, the error message from libvirt
> is spurious, and modifying the forwarding state will not affect the RA
> state or disable your networking.
> 
> If you refer to the function rt6_purge_dflt_routers() in the kernel,
> we can see that the routes being purged are only those with the
> kernel's RTF_ADDRCONF flag set; that is, routes added by the kernel's
> RA handling.  Why does it do this?  I think this is a Linux
> implementation decision; it has always been like that and there are
> some comments suggesting that it is because a router should be
> statically configured, rather than accepting external configurations.
> 
> The solution implemented here is to convert the existing check into a
> walk of /proc/net/ipv6_route and look for routes with this flag set.
> We then check the accept_ra status for the interface, and if enabling
> forwarding would break things raise an error.
> 
> This should hopefully avoid "interactive" users, who are likely to be
> using NetworkManager and the like, having false warnings when enabling
> IPv6, but retain the error check for users relying on kernel-based
> IPv6 interface auto-configuration.
> 
> Signed-off-by: Ian Wienand 
> ---
>  src/util/virnetdevip.c | 323 -
>  1 file changed, 124 insertions(+), 199 deletions(-)
> 
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index 7bd5a75f85..e208089bd6 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -43,8 +43,11 @@
>  #ifdef __linux__
>  # include 
>  # include 
> +# include 
>  #endif
>  
> +#define PROC_NET_IPV6_ROUTE "/proc/net/ipv6_route"
> +
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  VIR_LOG_INIT("util.netdevip");
> @@ -370,203 +373,6 @@ virNetDevIPRouteAdd(const char *ifname,
>  }
>  
>  
> -static int
> -virNetDevIPGetAcceptRA(const char *ifname)
> -{
> -g_autofree char *path = NULL;
> -g_autofree char *buf = NULL;
> -char *suffix;
> -int accept_ra = -1;
> -
> -path = g_strdup_printf("/proc/sys/net/ipv6/conf/%s/accept_ra",
> -   ifname ? ifname : "all");
> -
> -if ((virFileReadAll(path, 512, ) < 0) ||
> -(virStrToLong_i(buf, , 10, _ra) < 0))
> -return -1;
> -
> -return accept_ra;
> -}
> -
> -struct virNetDevIPCheckIPv6ForwardingData {
> -bool hasRARoutes;
> -
> -/* Devices with conflicting accept_ra */
> -char **devices;
> -size_t ndevices;
> -};
> -
> -
> -static int
> -virNetDevIPCheckIPv6ForwardingAddIF(struct 
> virNetDevIPCheckIPv6ForwardingData *data,
> -char **ifname)
> -{
> -size_t i;
> -
> -/* add ifname to the array if it's not already there
> - * (ifname is char** so VIR_APPEND_ELEMENT() will move the
> - * original pointer out of the way and avoid having it freed)
> - */
> -for (i = 0; i < data->ndevices; i++) {
> -if (STREQ(data->devices[i], *ifname))
> -return 0;
> -}
> -return VIR_APPEND_ELEMENT(data->devices, data->ndevices, *ifname);
> -}
> -
> -
> -static int
> -virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
> -   void *opaque)
> -{
> -struct rtmsg *rtmsg = NLMSG_DATA(resp);
> -struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> -struct rtattr *rta_attr;
> -int accept_ra = -1;
> -int ifindex = -1;
> -g_autofree char *ifname = NULL;
> -
> -/* Ignore messages other than route ones */
> -if (resp->nlmsg_type != RTM_NEWROUTE)
> -return 0;
> -
> -/* No need to do anything else for non RA routes */
> -if (rtmsg->rtm_protocol != RTPROT_RA)
> -return 0;
> -
> -rta_attr = (struct rtattr *)nlmsg_find_attr(resp, sizeof(struct rtmsg), 
> RTA_OIF);
> -if (rta_attr) {
> -/* This is a single path 

Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding

[Cedric Bosdonnat]

Hi Ian,

On Thu, 2020-09-10 at 09:48 +1000, Ian Wienand wrote:
> On Wed, Sep 09, 2020 at 06:38:04AM +0000, Cedric Bosdonnat wrote:
> > The check didn't involve any NetworkManager at all, but a network with
> > RA route for the default route. Completely removing the check is rather
> > likely to introduce a regression on that side.
> 
> Thanks for putting up with my bumbling about here :)
> 
> So firstly; I think we can state the big picture original problem as
> "the kernel was seen to flush existing ipv6 routes when ipv6
> forwarding is enabled, unless accept_ra==2 is set, which
>  break peoples networking"?

That is right

> If that premise is correct, then I also think I'm correct in saying
> from ~[1] that the kernel will only flush routes with RTF_ADDRCONF
> set?
> 
>   if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) &&
>  (!idev || idev->cnf.accept_ra != 2) &&
>fib6_info_hold_safe(rt)) {
> rcu_read_unlock();
> ip6_del_rt(net, rt);
> goto restart;
>   }
> 
> If that premise is correct, then I also think that userspace tools do
> not set this flag on routes they setup when they handle RA's in
> userspace.  I couldn't see it set on any of my routes on my laptop,
> and enabling/disabling forwarding didn't seem to flush any routes.
> 
> If *that* premise is correct too, then as I understand the current
> code it queries netlink for all the routes, checks if they have
> RTPROT_RA set, and if accept_ra != 2 gives the error.
> 
> If **that** premise is correct, then I think that just checking
> RTPROT_RA is too strict -- per the prior steps the route will only be
> flushed by the kernel if it has RTF_ADDRCONF set on it, and for many
> people using userspace tools their routing is not affected by enabling
> forwarding at all.

After reproducing here, I see the RTF_ADDRCONF flag set on those routes
in /proc/net/ipv6_route. Looks like your theory fits with the use case.

> If ***that*** premise is correct -- what to do about it?  I don't
> think netlink exposes RTF_ADDRCONF?  It can be seen via the flags dump
> in /proc/net/ipv6_route however (maybe that's a field in netlink?).
> But there may be room for conversation on how much this warning helps
> v hinders in 2020; it's not like it fixes the problem for you.

You surely know more netlink than I do ;)

--
Cédric

Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding

[Cedric Bosdonnat]

On Wed, 2020-09-09 at 12:39 +1000, Ian Wienand wrote:
> On Tue, Sep 01, 2020 at 08:27:47AM +0000, Cedric Bosdonnat wrote:
> > So the hypervisor has at least one (Router Advertised) RA route.
> > After defining a network like the following, the RA route is removed if
> > accept_ra isn't set to 2.
> > 
> > 
> >   test5
> >   
> >   
> >   
> >>  family="ipv6"
> >  address="fc00:::000f::::0001"
> >  prefix="64"/>
> > 
> > 
> > The RA route was removed in networkEnableIPForwarding() when
> > setting /proc/sys/net/ipv6/conf/all/forwarding to 1.
> > 
> > Me not being a network expert (and even less on ipv6) doesn't help.
> > 
> > I hope this explanation will help you better see the use case I had.
> 
> So it seems to be the intention of the kernel that when you enable
> forwarding your routes are flushed; changing the sysctl gets into
> addrconf_fixup_forwarding() [1] which then calls
> rt6_purge_dflt_routers() when the forwarding status is changed.  That
> then purges default routes, unless accept_ra == 2; that was introduced
> with [3].
> 
> I guess the idea is that a router should not accept
> auto-configuration?
> 
> HOWEVER ...
> 
>if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) &&
> (!idev || idev->cnf.accept_ra != 2) &&
> fib6_info_hold_safe(rt)) {
> rcu_read_unlock();
> ip6_del_rt(net, rt);
> goto restart;
> }
> 
> I feel like this is checking the RTF_ADDRCONF flag before it flushes
> any routes.  Checking that flag ...
> 
>  #define RTF_ADDRCONF0x0004
> 
> which I do not have set at all, from :
> 
>  $ cat /proc/net/ipv6_route | awk  '{print $1 " " 
> and(strtonum("0x"$9),strtonum("0x4"))}'
> 
> Based on this, I'm concluding that the userspace tools do not set this
> flag on their routes, and so they are never flushed.  Empirically,
> fiddling forwarding on and off I don't see any routes flushed.
> 
> So, I do not think that enabling forwarding will remove routes on the
> most common "sitting in-front of the computer" cases where you're
> using NetworkManager/systemd userspace magic.
> 
> Given this, I'd propose we revert the check?

The check didn't involve any NetworkManager at all, but a network with
RA route for the default route. Completely removing the check is rather
likely to introduce a regression on that side.

--
Cedric

Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding

[Cedric Bosdonnat]

On Wed, 2020-08-12 at 19:21 -0400, Laine Stump wrote:
> Yay! A user who follows up their conversation on the libvirt-users list 
> with a patch! :-)
> 
> On 8/11/20 8:14 PM, Ian Wienand wrote:
> > The checks modified here were added with
> > 00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on
> > hosts.
> 
> I'm Cc'ing the author of that patch, Cédric Bosdonnat, to make sure that 
> whatever we end up doing doesn't upset his use case.

Ouch! that is old... even reading my bugzilla log I have troubles
explaining that thing properly. I'll try it though.

So the hypervisor has at least one (Router Advertised) RA route.
After defining a network like the following, the RA route is removed if
accept_ra isn't set to 2.


  test5
  
  
  
  


The RA route was removed in networkEnableIPForwarding() when
setting /proc/sys/net/ipv6/conf/all/forwarding to 1.

Me not being a network expert (and even less on ipv6) doesn't help.

I hope this explanation will help you better see the use case I had.

--
Cedric

> > However, tools such as systemd-networking and NetworkManager manage
> > RA's in userspace and thus IPv6 may be up and working on an interface
> > even with accept_ra == 0.
> > 
> > This modifies the check to only error if an interface's accept_ra is
> > already set to "1"; as noted inline this seems to when it is likely
> > that enabling forwarding may change the RA acceptance behaviour of the
> > interface.
> 
> Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, 
> not all the interfaces have accept_ra=0 by default. One of the bridge 
> devices (created by NetworkManager in response to an ifcfg file in 
> /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several 
> other interfaces that have accept_ra=1, but those interfaces are either 
> not online, or they don't have an IPv6 address.
> 
> 
> So this doesn't work for all cases. I think we need to get more 
> information on how to most easily, generically, and reliably determine 
> if the kernel accept_ra setting can be ignored. Possibly the 
> NetworkManager people can help us out here.
> 
> 
> (or alternately we could punt and just make a configuration setting, 
> although that is taking the easy road, and not a good precedent to set)
> 
> 
> > I have noticed this because I am using the IPv6 NAT features enabled
> > with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7.  I am using this on my
> > laptop which switches between wired and wireless connections; both of
> > which are configured in an unremarkable way by Fedora's NetworkManager
> > and get configured for IPv6 via SLAAC and whatever NetworkManager
> > magic it does.  With this I can define and start a libvirt network
> > with  and  > address='fc00:abcd:ef12:34::' prefix='64'> and it seems to "just work"
> > for guests.
> > 
> > Signed-off-by: Ian Wienand 
> > ---
> >   src/util/virnetdevip.c | 41 +++--
> >   1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > index 409f062c5c..de27cacfc9 100644
> > --- a/src/util/virnetdevip.c
> > +++ b/src/util/virnetdevip.c
> > @@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname)
> >   }
> >   
> >   struct virNetDevIPCheckIPv6ForwardingData {
> > -bool hasRARoutes;
> > +bool hasKernelRARoutes;
> >   
> >   /* Devices with conflicting accept_ra */
> >   char **devices;
> > @@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct 
> > nlmsghdr *resp,
> >   if (!ifname)
> >  return -1;
> >   
> > -accept_ra = virNetDevIPGetAcceptRA(ifname);
> > -
> >   VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d",
> > ifname, ifindex, accept_ra);
> >   
> > -if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, 
> > ) < 0)
> > +accept_ra = virNetDevIPGetAcceptRA(ifname);
> > +/* 0 = do no accept RA
> > + * 1 = accept if forwarding disabled
> > + * 2 = ovveride and accept RA when forwarding enabled
> > + *
> > + * When RA is managed by userspace (systemd-networkd or
> > + * NetworkManager) accept_ra is unset and we don't need to
> > + * worry about it.  If it is 1, enabling forwarding might
> > + * change the behaviour so the user needs to be warned.
> > + */
> > +if (accept_ra == 0)
> > +return 0;
> > +
> > +if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, 
> > ) < 0)
> >   return -1;
> >   
> > -data->hasRARoutes = true;
> > +data->hasKernelRARoutes = true;
> >   return 0;
> >   }
> >   
> > @@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct 
> > nlmsghdr *resp,
> >   VIR_DEBUG("Checking multipath route nexthop device %s (%d), 
> > accept_ra: %d",
> > ifname, nh->rtnh_ifindex, accept_ra);
> >   
> 

Re: [GSoC] Take migration in Salt virt module to the next level

[Cedric Bosdonnat]

stem parallel_connections=10
> 
> Live migration with enabled compression
> $ salt src virt.migrate guest qemu+tls://dst/system \
> compressed=True \
> comp_methods=mt \
> comp_mt_level=5 \
> comp_mt_threads=4 \
> comp_mt_dthreads=4
> 
> $ salt src virt.migrate guest qemu+tls://dst/system \
> compressed=True \
> comp_methods=xbzrle \
> comp_xbzrle_cache=1024
> 
> Migrate non-shared storage
> (Full disk copy)
> $ salt src virt.migrate guest qemu+tls://dst/system copy_storage=all
> 
> (Incremental copy)
> $ salt src virt.migrate guest qemu+tls://dst/system copy_storage=inc
> 
> Using post-copy migration
> $ salt src virt.migrate guest qemu+tls://dst/system postcopy=True
> $ salt src virt.migrate_start_postcopy guest
> 
> Using post-copy migration with bandwidth limit (MiB/s)
> $ salt src virt.migrate guest qemu+tls://dst/system \
> postcopy=True \
> postcopy_bandwidth=1
> $ salt src virt.migrate_start_postcopy guest
> 
> 
> Acknowledgments
> ===
> I would like to sincerely thank all those who provided me with their 
> time, assistance, and guidance during the course of this project, for 
> which I am enormously grateful.
> 
> I would like to thank Cedric Bosdonnat (cbosdo), Pedro Algarvio 
> (s0undt3ch), Wayne Werner (waynew), Charles McMarrow (cmcmarrow) and 
> Daniel Wozniak (dwoz) who offered a huge amount of support, expertise, 
> guidance and code reviews.
> 
> I would also like to thank Michal Privoznik (mprivozn), Martin 
> Kletzander (mkletzan) and the rest of the libvirt and SaltStack 
> communities who have been extremely supportive.
> 
> 
> Best wishes,
> Radostin
> 

Re: [GSoC] Introduction

[Cedric Bosdonnat]

Hey Radostin,

Welcome back to the libvirt community! I am looking forward to a cool
project with you this summer.

--
Cedric

On Thu, 2020-05-07 at 16:16 +0100, Radostin Stoyanov wrote:
> Hello everyone,
> 
> As a quick introduction, I’m Radostin Stoyanov, a Google Summer of Code 
> student from University of Cambridge.
> 
> In the next few months I’ll be working with my mentors Cédric Bosdonnat, 
> Pavel Hrdina and Tyler Johnson (Akmod) on the project "Take migration in 
> Salt virt module to the next level".
> 
> In short, the project aim is to refactor some of the existing Salt 
> migration functions to use the python-libvirt API, as well as to make 
> available more of the libvirt migration options.
> 
> You can find me in IRC as rst0irc.
> 
> - Radostin
> 
> 

Re: Google Summer of Code 2020 Student self introduction

[Cedric Bosdonnat]

Hi Lee,

Welcome to the libvirt community. I'm looking forward to a cool project
with you for this summer!

--
Cedric

On Thu, 2020-05-07 at 20:18 +0900, GUOQING LI wrote:
> Hi there
> 
> I am Lee, an undergraduate student from Thailand (currently based in Japan) 
> and I will be working as a Student Developer under Google Summer of Code 2020 
> program. 
> 
> The host organisation is Libivirt.  I will be contributing to virt module 
> under Saltstack project. I proposed to make CPU tuning, CPU model and 
> topology, Memory tuning, NUMA Node Tuning  available. Looking forward to this 
> learning journey.  
> 
> Any suggestions and comments will be appreciated   I would be nice if you 
> want to connect me on LinkedIn as well.  
> 
> Best regards,
> Lee

Re: [RFC] Adding docker driver to libvirt

[Cedric Bosdonnat]

On Sun, 2020-04-12 at 11:39 +0200, Martin Kletzander wrote:
> On Thu, Apr 09, 2020 at 03:30:11PM +0300, nshirokovskiy wrote:
> > Hi, all.
> > 
> > Does it make sense to add such a driver? I can't say I have a big picture
> > of docker functionality in mind but at least container lifecycle management
> > and container networking are common to both.
> > 
> 
> I think we had something in virt-tools that was able to pull an image from
> docker hub and run it with lxc.  Or was it part of sandbox?  I don't know.

This is the virt-bootstrap tool:

https://github.com/virt-manager/virt-bootstrap

--
Cedric

Re: Redfish API implementation for GSoC 2020.

[Cedric Bosdonnat]

Hi Rohan,

Thank you for your interest in the RedFish project idea. I'll will
answer you back the same thing I already said to the other students
that reached me.

In order to prepare a good application, throughly read the RedFish
specification and come up with a paper describing how you would map
this to the libvirt API. You can work on a few uses cases and describe
them.

The difficult part of the project is not to write the REST APIs, but
rather to come up with a meaningful binding between redfish and lbvirt.

--
Cedric

On Wed, 2020-03-04 at 18:30 +0530, ROHAN Gupta wrote:
> Hi everyone, I am Rohan Gupta a CSE undergraduate student at Shri Mata 
> Vaishno Devi University currently in the 4th semester. I would like to work 
> on the idea: Redfish API implementation for GSoC 2020. I find this 
> interesting as I have worked with REST apis in the past and have sound 
> knowledge about it. Here's the link to my GitHub profile: 
> https://github.com/Rohan-cod

Re: [libvirt] [PATCH] docs: rewrite hvsupport.html page generator in python

[Cedric Bosdonnat]

On Fri, 2019-08-30 at 13:25 +0100, Daniel P. Berrangé wrote:
> As part of an goal to eliminate Perl from libvirt build tools,
> rewrite the hvsupport.pl tool in Python.
> 
> This was a straight conversion, manually going line-by-line to
> change the syntax from Perl to Python. Thus the overall structure
> of the file and approach is the same.
> 
> The new impl generates byte-for-byte identical output to the
> old impl.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/Makefile.am  |   6 +-
>  docs/hvsupport.pl | 458 
> -
>  docs/hvsupport.py | 463
> ++
>  3 files changed, 466 insertions(+), 461 deletions(-)
>  delete mode 100755 docs/hvsupport.pl
>  create mode 100755 docs/hvsupport.py
> 
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 1cdb584b0b..f7aba5499d 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -194,7 +194,7 @@ EXTRA_DIST= \
>$(internals_html_in) $(internals_html) $(fonts) \
>$(kbase_html_in) $(kbase_html) \
>aclperms.htmlinc \
> -  hvsupport.pl \
> +  hvsupport.py \
>$(schema_DATA)
>  
>  acl_generated = aclperms.htmlinc
> @@ -230,11 +230,11 @@ web: $(dot_html) $(internals_html)
> $(kbase_html) \
>  
>  hvsupport.html: $(srcdir)/hvsupport.html.in
>  
> -$(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl $(api_DATA) \
> +$(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.py $(api_DATA) \
>   $(top_srcdir)/src/libvirt_public.syms \
>   $(top_srcdir)/src/libvirt_qemu.syms
> $(top_srcdir)/src/libvirt_lxc.syms \
>   $(top_srcdir)/src/driver.h
> - $(AM_V_GEN)$(PERL) $(srcdir)/hvsupport.pl $(top_srcdir)/src >
> $@ \
> + $(AM_V_GEN)$(PYTHON) $(srcdir)/hvsupport.py $(top_srcdir)/src >
> $@ \
>   || { rm $@ && exit 1; }
>  
>  news.html.in: \
> diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl
> deleted file mode 100755
> index 494b8a27ec..00
> --- a/docs/hvsupport.pl
> +++ /dev/null
> @@ -1,458 +0,0 @@
> -#!/usr/bin/env perl
> -
> -use strict;
> -use warnings;
> -
> -use File::Find;
> -
> -die "syntax: $0 SRCDIR\n" unless int(@ARGV) == 1;
> -
> -my $srcdir = shift @ARGV;
> -
> -my $symslibvirt = "$srcdir/libvirt_public.syms";
> -my $symsqemu = "$srcdir/libvirt_qemu.syms";
> -my $symslxc = "$srcdir/libvirt_lxc.syms";
> -my @drivertable = (
> -"$srcdir/driver-hypervisor.h",
> -"$srcdir/driver-interface.h",
> -"$srcdir/driver-network.h",
> -"$srcdir/driver-nodedev.h",
> -"$srcdir/driver-nwfilter.h",
> -"$srcdir/driver-secret.h",
> -"$srcdir/driver-state.h",
> -"$srcdir/driver-storage.h",
> -"$srcdir/driver-stream.h",
> -);
> -
> -my %groupheaders = (
> -"virHypervisorDriver" => "Hypervisor APIs",
> -"virNetworkDriver" => "Virtual Network APIs",
> -"virInterfaceDriver" => "Host Interface APIs",
> -"virNodeDeviceDriver" => "Host Device APIs",
> -"virStorageDriver" => "Storage Pool APIs",
> -"virSecretDriver" => "Secret APIs",
> -"virNWFilterDriver" => "Network Filter APIs",
> -);
> -
> -
> -my @srcs;
> -find({
> -wanted => sub {
> -if
> (m!$srcdir/.*/\w+_(driver|common|tmpl|monitor|hal|udev)\.c$!) {
> -push @srcs, $_ if $_ !~ /vbox_driver\.c/;
> -}
> -}, no_chdir => 1}, $srcdir);
> -
> -# Map API functions to the header and documentation files they're in
> -# so that we can generate proper hyperlinks to their documentation.
> -#
> -# The function names are grep'd from the XML output of apibuild.py.
> -sub getAPIFilenames {
> -my $filename = shift;
> -
> -my %files;
> -my $line;
> -
> -open FILE, "<", $filename or die "cannot read $filename: $!";
> -
> -while (defined($line = )) {
> -if ($line =~ /function name='([^']+)' file='([^']+)'/) {
> -$files{$1} = $2;
> -}
> -}
> -
> -close FILE;
> -
> -if (keys %files == 0) {
> -die "No functions found in $filename. Has the apibuild.py
> output changed?";
> -}
> -return \%files;
> -}
> -
> -sub parseSymsFile {
> -my $apisref = shift;
> -my $prefix = shift;
> -my $filename = shift;
> -my $xmlfilename = shift;
> -
> -my $line;
> -my $vers;
> -my $prevvers;
> -
> -my $filenames = getAPIFilenames($xmlfilename);
> -
> -open FILE, "<$filename"
> -or die "cannot read $filename: $!";
> -
> -while (defined($line = )) {
> -chomp $line;
> -next if $line =~ /^\s*#/;
> -next if $line =~ /^\s*$/;
> -next if $line =~ /^\s*(global|local):/;
> -if ($line =~ /^\s*${prefix}_(\d+\.\d+\.\d+)\s*{\s*$/) {
> -if (defined $vers) {
> -die "malformed syms file";
> -}
> -$vers = $1;
> -} elsif ($line =~ /\s*}\s*;\s*$/) {
> -if (defined $prevvers) {
> -die "malformed syms file";
> -}
> -$prevvers = $vers;
> -$vers = undef;
> -   

[libvirt] [PATCHv2 2/2] Generate status of the backend implementation in hvsupport.html

[Cedric Bosdonnat]

Since it helps a user to know which of the storage backends support what
operation, include an autogenerated matrix showing it in the docs.
---
 .gitignore   |  1 +
 docs/Makefile.am | 12 +--
 docs/apibuild.py |  2 ++
 docs/hvsupport.pl|  2 ++
 docs/storage.html.in | 13 
 docs/storagebackendstatus.py | 63 
 6 files changed, 90 insertions(+), 3 deletions(-)
 create mode 100644 docs/storagebackendstatus.py

diff --git a/.gitignore b/.gitignore
index 82495e8692..06875abebd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@
 /configure.lineno
 /conftest.*
 /docs/aclperms.htmlinc
+/docs/storagebackendstatus.htmlinc
 /docs/apibuild.py.stamp
 /docs/devhelp/libvirt.devhelp
 /docs/hvsupport.html.in
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 1cdb584b0b..3cf114d9e1 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -195,6 +195,8 @@ EXTRA_DIST= \
   $(kbase_html_in) $(kbase_html) \
   aclperms.htmlinc \
   hvsupport.pl \
+  storagebackendstatus.py \
+  storagebackendstatus.htmlinc \
   $(schema_DATA)
 
 acl_generated = aclperms.htmlinc
@@ -209,7 +211,8 @@ MAINTAINERCLEANFILES = \
   $(addprefix $(srcdir)/,$(devhelphtml)) \
   $(addprefix $(srcdir)/,$(internals_html)) \
   $(addprefix $(srcdir)/,$(kbase_html)) \
-  $(srcdir)/hvsupport.html.in $(srcdir)/aclperms.htmlinc
+  $(srcdir)/hvsupport.html.in $(srcdir)/aclperms.htmlinc \
+  $(srcdir)/storagebackendstatus.htmlinc
 
 timestamp="$(shell if test -n "$$SOURCE_DATE_EPOCH"; \
   then \
@@ -228,7 +231,10 @@ admin_api: $(srcdir)/libvirt-admin-api.xml 
$(srcdir)/libvirt-admin-refs.xml
 web: $(dot_html) $(internals_html) $(kbase_html) \
html/index.html devhelp/index.html
 
-hvsupport.html: $(srcdir)/hvsupport.html.in
+hvsupport.html: $(srcdir)/hvsupport.html.in 
$(srcdir)/storagebackendstatus.htmlinc
+
+$(srcdir)/storagebackendstatus.htmlinc: $(srcdir)/storagebackendstatus.py
+   $(PYTHON) $< $(top_srcdir)/src >$@
 
 $(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl $(api_DATA) \
$(top_srcdir)/src/libvirt_public.syms \
@@ -256,7 +262,7 @@ MAINTAINERCLEANFILES += \
convert -rotate 90 $< $@
 
 %.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
-   $(acl_generated)
+   $(acl_generated) storagebackendstatus.htmlinc
$(AM_V_GEN)name=`echo $@ | sed -e 's/.tmp//'`; \
  dir=`dirname $@` ; \
  if test "$$dir" = "."; \
diff --git a/docs/apibuild.py b/docs/apibuild.py
index dbdc1c95af..31944b8176 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -2010,6 +2010,8 @@ class docBuilder:
 self.includes = includes + list(lxc_included_files.keys())
 elif name == "libvirt-admin":
 self.includes = includes + list(admin_included_files.keys())
+else:
+self.includes = includes
 self.modules = {}
 self.headers = {}
 self.idx = index()
diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl
index 494b8a27ec..14e2336018 100755
--- a/docs/hvsupport.pl
+++ b/docs/hvsupport.pl
@@ -453,6 +453,8 @@ EOF
 }
 
 print 

[libvirt] [PATCHv2 1/2] Fix hvsupport toc

[Cedric Bosdonnat]

---
 docs/hvsupport.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl
index 4f4d86fbf1..494b8a27ec 100755
--- a/docs/hvsupport.pl
+++ b/docs/hvsupport.pl
@@ -364,7 +364,7 @@ when it was removed is also mentioned (highlighted in
 EOF
 
 foreach my $grp (sort { $a cmp $b } keys %groups) {
-print "", $groupheaders{$grp}, "\n";
+print "", $groupheaders{$grp}, "\n";
 print <
 
-- 
2.22.0


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

[libvirt] [PATCHv2 0/2] storage backend support matrix

[Cedric Bosdonnat]

Hello,

Here is an updated version of the previous Storage backend support matrix in 
the doc.
As discussed with Michal, the matrix is now part of hvsupport.html with a
paragraph and link from storage.html.

This has now been turned into a series to fix the links in TOC of the storage
page

Cédric Bosdonnat (2):
  Fix hvsupport toc
  Generate status of the backend implementation in hvsupport.html

 .gitignore   |  1 +
 docs/Makefile.am | 12 +--
 docs/apibuild.py |  2 ++
 docs/hvsupport.pl|  4 ++-
 docs/storage.html.in | 13 
 docs/storagebackendstatus.py | 63 
 6 files changed, 91 insertions(+), 4 deletions(-)
 create mode 100644 docs/storagebackendstatus.py

-- 
2.22.0


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

Re: [libvirt] [PATCH] Generate status of the backend implementation in storage.html

[Cedric Bosdonnat]

On Tue, 2019-08-27 at 16:38 +0200, Michal Privoznik wrote:
> On 8/27/19 11:17 AM, Cedric Bosdonnat wrote:
> > Since it helps a user to know which of the storage backends support
> > what
> > operation, include an autogenerated matrix showing it in the docs.
> > ---
> >   .gitignore   |  1 +
> >   docs/Makefile.am |  9 +++--
> >   docs/apibuild.py |  2 ++
> >   docs/storage.html.in | 14 
> >   docs/storagebackendstatus.py | 64
> > 
> >   5 files changed, 88 insertions(+), 2 deletions(-)
> >   create mode 100644 docs/storagebackendstatus.py
> > 
> > diff --git a/docs/storage.html.in b/docs/storage.html.in
> > index e0e4edec1e..2e4f66 100644
> > --- a/docs/storage.html.in
> > +++ b/docs/storage.html.in
> > @@ -826,5 +826,19 @@
> >   
> >   Valid volume format types
> >   The valid volume types are the same as for the directory
> > pool.
> > +
> > +Storage Pool Types implementation status
> > +
> > +
> > +  The storage backends have different level of support of the
> > various pool and volume actions.
> > +  Here is a matrix of the current support for each of them.
> > +
> > +
> > +
> > +  Note: some functions like Start and Stop
> > will not trigger an exception when
> > +  called on a backend that doesn't implement them.
> > +
> > +
> > +
> > 
> >   
> 
> I feel like this belongs to hvsupport.html better. Do you have any 
> objection to that?

On one hand hvsupport.html indeed lists all the status of each
hypervisor. But on the other hand we have a page dedicated to
storage... Both options don't seem stupid to me.

--
Cedric

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

[libvirt] [PATCH] Generate status of the backend implementation in storage.html

[Cedric Bosdonnat]

Since it helps a user to know which of the storage backends support what
operation, include an autogenerated matrix showing it in the docs.
---
 .gitignore   |  1 +
 docs/Makefile.am |  9 +++--
 docs/apibuild.py |  2 ++
 docs/storage.html.in | 14 
 docs/storagebackendstatus.py | 64 
 5 files changed, 88 insertions(+), 2 deletions(-)
 create mode 100644 docs/storagebackendstatus.py

diff --git a/.gitignore b/.gitignore
index 82495e8692..06875abebd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -58,6 +58,7 @@
 /configure.lineno
 /conftest.*
 /docs/aclperms.htmlinc
+/docs/storagebackendstatus.htmlinc
 /docs/apibuild.py.stamp
 /docs/devhelp/libvirt.devhelp
 /docs/hvsupport.html.in
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 1cdb584b0b..526821b1d3 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -195,6 +195,7 @@ EXTRA_DIST= \
   $(kbase_html_in) $(kbase_html) \
   aclperms.htmlinc \
   hvsupport.pl \
+  storagebackendstatus.py \
   $(schema_DATA)
 
 acl_generated = aclperms.htmlinc
@@ -209,7 +210,8 @@ MAINTAINERCLEANFILES = \
   $(addprefix $(srcdir)/,$(devhelphtml)) \
   $(addprefix $(srcdir)/,$(internals_html)) \
   $(addprefix $(srcdir)/,$(kbase_html)) \
-  $(srcdir)/hvsupport.html.in $(srcdir)/aclperms.htmlinc
+  $(srcdir)/hvsupport.html.in $(srcdir)/aclperms.htmlinc \
+  $(srcdir)/storagebackendstatus.htmlinc
 
 timestamp="$(shell if test -n "$$SOURCE_DATE_EPOCH"; \
   then \
@@ -230,6 +232,9 @@ web: $(dot_html) $(internals_html) $(kbase_html) \
 
 hvsupport.html: $(srcdir)/hvsupport.html.in
 
+storagebackendstatus.htmlinc: $(srcdir)/storagebackendstatus.py
+   $(PYTHON) $< >$@
+
 $(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl $(api_DATA) \
$(top_srcdir)/src/libvirt_public.syms \
$(top_srcdir)/src/libvirt_qemu.syms $(top_srcdir)/src/libvirt_lxc.syms \
@@ -256,7 +261,7 @@ MAINTAINERCLEANFILES += \
convert -rotate 90 $< $@
 
 %.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
-   $(acl_generated)
+   $(acl_generated) storagebackendstatus.htmlinc
$(AM_V_GEN)name=`echo $@ | sed -e 's/.tmp//'`; \
  dir=`dirname $@` ; \
  if test "$$dir" = "."; \
diff --git a/docs/apibuild.py b/docs/apibuild.py
index dbdc1c95af..31944b8176 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -2010,6 +2010,8 @@ class docBuilder:
 self.includes = includes + list(lxc_included_files.keys())
 elif name == "libvirt-admin":
 self.includes = includes + list(admin_included_files.keys())
+else:
+self.includes = includes
 self.modules = {}
 self.headers = {}
 self.idx = index()
diff --git a/docs/storage.html.in b/docs/storage.html.in
index e0e4edec1e..2e4f66 100644
--- a/docs/storage.html.in
+++ b/docs/storage.html.in
@@ -826,5 +826,19 @@
 
 Valid volume format types
 The valid volume types are the same as for the directory pool.
+
+Storage Pool Types implementation status
+
+
+  The storage backends have different level of support of the various pool 
and volume actions.
+  Here is a matrix of the current support for each of them.
+
+
+
+  Note: some functions like Start and Stop will not 
trigger an exception when
+  called on a backend that doesn't implement them.
+
+
+
   
 
diff --git a/docs/storagebackendstatus.py b/docs/storagebackendstatus.py
new file mode 100644
index 00..d105fea626
--- /dev/null
+++ b/docs/storagebackendstatus.py
@@ -0,0 +1,64 @@
+import os
+import os.path
+import re
+
+srcdir = os.path.abspath((os.environ.get("srcdir", os.path.join("..", "src"
+
+def get_allowed_functions():
+functions = []
+with open(os.path.join(srcdir, 'storage', 'storage_backend.h'), 'r') as 
handle:
+content = ''.join(handle.readlines())
+definition = re.search('struct _virStorageBackend {([^}]+)}', content)
+if definition is not None:
+functions = re.findall('virStorageBackend[^ ]+ ([^;]+)', 
definition.group(1))
+return functions
+
+class Backend:
+def __init__(self, name, code):
+self.name = name
+self.functions = [member[1:] for member in re.findall('.([^ ]+) = ', 
code) if member != '.type']
+
+def get_backends():
+backends = []
+for root, dirs, files in os.walk(os.path.join(srcdir, 'storage')):
+storage_impls = [os.path.join(root, f) for f in files if 
re.match('storage_backend_[^.]+.c', f)]
+for impl in storage_impls:
+handle = open(impl, 'r')
+content = ''.join(handle.readlines())
+handle.close()
+chunks = re.findall('virStorageBackend virStorageBackend([^ ]+) = 
{([^}]*)}', content)
+backends.extend([Backend(chunk[0], chunk[1]) for chunk in chunks])
+return backends
+
+def main():
+functions = get_allowed_functions()
+

[libvirt] ANNOUNCE: virt-bootstrap 1.1.0 released

[Cedric Bosdonnat]

I'm happy to announce the release of virt-bootstrap 1.1.0!

virt-bootstrap is a tool providing an easy way to setup the root file
system for libvirt-based containers.

The release can be downloaded from:

http://virt-manager.org/download/

This release includes:

- safe_untar: check for permissions to set attribs (Radostin Stoyanov)
- docker source, support blobs without .tar extension (Radostin Stoyanov)
- docker-source, preserve extended file attributes (Radostin Stoyanov)
- docker-source, get list of layers without `--raw` (Radostin Stoyanov)
- docker-source, void skopeo copy in cache (Radostin Stoyanov)
- Show error when guestfs-python or skopeo is not installed (Radostin Stoyanov)
- pylint cleanups (Radostin Stoyanov)

Thanks to everyone who has contributed to this release through testing,
bug reporting, submitting patches, and otherwise sending in feedback!

Thanks,

--
Cédric

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

Re: [libvirt] [PATCH] lxc: convert to typesafe virConf accessors in lxc_native.c

[Cedric Bosdonnat]

On Fri, 2018-05-04 at 18:46 +0530, Prafull wrote:
> From: Prafullkumar Tale 
> 
> Signed-off-by: Prafullkumar Tale 
> ---
>  src/lxc/lxc_native.c | 141 
> +--
>  1 file changed, 70 insertions(+), 71 deletions(-)
> 
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index 55ea774..35077e1 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -199,19 +199,15 @@ lxcSetRootfs(virDomainDefPtr def,
>   virConfPtr properties)
>  {
>  int type = VIR_DOMAIN_FS_TYPE_MOUNT;
> -virConfValuePtr value;
> +char *value = NULL;
>  
> -if (!(value = virConfGetValue(properties, "lxc.rootfs")) ||
> -!value->str) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("Missing lxc.rootfs configuration"));
> +if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
>  return -1;
> -}
>  
> -if (STRPREFIX(value->str, "/dev/"))
> +if (STRPREFIX(value, "/dev/"))
>  type = VIR_DOMAIN_FS_TYPE_BLOCK;
>  
> -if (lxcAddFSDef(def, type, value->str, "/", false, 0) < 0)
> +if (lxcAddFSDef(def, type, value, "/", false, 0) < 0)
>  return -1;
>  
>  return 0;
> @@ -684,17 +680,17 @@ lxcConvertNetworkSettings(virDomainDefPtr def, 
> virConfPtr properties)
>  static int
>  lxcCreateConsoles(virDomainDefPtr def, virConfPtr properties)
>  {
> -virConfValuePtr value;
> +char *value = NULL;
>  int nbttys = 0;
>  virDomainChrDefPtr console;
>  size_t i;
>  
> -if (!(value = virConfGetValue(properties, "lxc.tty")) || !value->str)
> +if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
>  return 0;
>  
> -if (virStrToLong_i(value->str, NULL, 10, ) < 0) {
> +if (virStrToLong_i(value, NULL, 10, ) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse int: 
> '%s'"),
> -   value->str);
> +   value);
>  return -1;
>  }
>  
> @@ -761,89 +757,91 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr 
> value, void *data)
>  static int
>  lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>  {
> -virConfValuePtr value;
> +char *value = NULL;
>  unsigned long long size = 0;
>  
> -if ((value = virConfGetValue(properties,
> -"lxc.cgroup.memory.limit_in_bytes")) &&
> -value->str && STRNEQ(value->str, "-1")) {
> -if (lxcConvertSize(value->str, ) < 0)
> -return -1;
> +if (virConfGetValueString(properties,
> +  "lxc.cgroup.memory.limit_in_bytes",
> +  ) > 0) {
> +if (lxcConvertSize(value, ) < 0)
> +goto error;
>  size = size / 1024;
>  virDomainDefSetMemoryTotal(def, size);
>  def->mem.hard_limit = virMemoryLimitTruncate(size);
>  }
>  
> -if ((value = virConfGetValue(properties,
> -"lxc.cgroup.memory.soft_limit_in_bytes")) &&
> -value->str && STRNEQ(value->str, "-1")) {
> -if (lxcConvertSize(value->str, ) < 0)
> -return -1;
> -
> +if (virConfGetValueString(properties,
> +  "lxc.cgroup.memory.soft_limit_in_bytes",
> +  ) > 0) {
> +if (lxcConvertSize(value, ) < 0)
> +goto error;
>  def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
>  }
>  
> -if ((value = virConfGetValue(properties,
> -"lxc.cgroup.memory.memsw.limit_in_bytes")) &&
> -value->str && STRNEQ(value->str, "-1")) {
> -if (lxcConvertSize(value->str, ) < 0)
> -return -1;
> -
> +if (virConfGetValueString(properties,
> +  "lxc.cgroup.memory.memsw.limit_in_bytes",
> +  ) > 0) {
> +if (lxcConvertSize(value, ) < 0)
> +goto error;
>  def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
>  }
>  return 0;
> +
> + error:
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("failed to parse integer: '%s'"), value);
> +return -1;
> +
>  }
>  
>  static int
>  lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
>  {
> -virConfValuePtr value;
> +char *value = NULL;
>  
> -if ((value = virConfGetValue(properties, "lxc.cgroup.cpu.shares")) &&
> -value->str) {
> -if (virStrToLong_ull(value->str, NULL, 10, >cputune.shares) < 0)
> +if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
> +  ) > 0) {
> +if (virStrToLong_ull(value, NULL, 10, >cputune.shares) < 0)
>  goto error;
>  def->cputune.sharesSpecified = true;
>  }
>  
> -if ((value = virConfGetValue(properties,
> - "lxc.cgroup.cpu.cfs_quota_us")) &&
> -

Re: [libvirt] [PATCH sandbox-image 9/9] docker: Add missing import base64

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/sources/docker.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libvirt_sandbox_image/sources/docker.py 
> b/libvirt_sandbox_image/sources/docker.py
> index 3ca1b10..8a231ee 100755
> --- a/libvirt_sandbox_image/sources/docker.py
> +++ b/libvirt_sandbox_image/sources/docker.py
> @@ -20,6 +20,7 @@
>  # Author: Eren Yagdiran 
>  #
>  
> +import base64
>  import sys
>  import json
>  import os

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 7/9] cli: Remove unused constants

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/cli.py | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/cli.py b/libvirt_sandbox_image/cli.py
> index 08c88a4..e96d422 100644
> --- a/libvirt_sandbox_image/cli.py
> +++ b/libvirt_sandbox_image/cli.py
> @@ -40,9 +40,6 @@ else:
>  default_template_dir = os.environ['HOME'] + 
> "/.local/share/libvirt/templates"
>  default_image_dir = os.environ['HOME'] + "/.local/share/libvirt/images"
>  
> -debug = False
> -verbose = False
> -
>  gettext.bindtextdomain("libvirt-sandbox", "/usr/share/locale")
>  gettext.textdomain("libvirt-sandbox")
>  try:

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 6/9] setup: Add shebang

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Signed-off-by: Radostin Stoyanov 
> ---
>  setup.py | 2 ++
>  1 file changed, 2 insertions(+)
>  mode change 100644 => 100755 setup.py
> 
> diff --git a/setup.py b/setup.py
> old mode 100644
> new mode 100755
> index 0b16ae7..aec7f03
> --- a/setup.py
> +++ b/setup.py
> @@ -1,3 +1,5 @@
> +#!/usr/bin/env python3
> +# -*- coding: utf-8 -*-
>  
>  import os
>  import re

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 8/9] cli: Remove redundant global statements

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> The global statement is needed for assignment of variables in global
> scope [1]. In this case, we only read the value of those variables,
> therefore we do not need to explicitly use the global statement.
> 
> [1] https://docs.python.org/3/reference/simple_stmts.html#the-global-statement
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/cli.py | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/cli.py b/libvirt_sandbox_image/cli.py
> index e96d422..95c5147 100644
> --- a/libvirt_sandbox_image/cli.py
> +++ b/libvirt_sandbox_image/cli.py
> @@ -167,13 +167,11 @@ def requires_connect(parser):
>  help=_("Connect string for libvirt"))
>  
>  def requires_template_dir(parser):
> -global default_template_dir
>  parser.add_argument("-t", "--template-dir",
>  default=default_template_dir,
>  help=_("Template directory for saving templates"))
>  
>  def requires_image_dir(parser):
> -global default_image_dir
>  parser.add_argument("-I", "--image-dir",
>  default=default_image_dir,
>  help=_("Image directory for saving images"))


ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 2/9] pylint: Move standard library imports on top

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Following PEP8 [1], imports should be grouped in the following order:
> 
> standard library imports
> related third party imports
> local application/library specific imports
> 
> With a blank line between each group of imports.
> 
> [1] https://www.python.org/dev/peps/pep-0008/#imports
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  scripts/virt-sandbox-image | 3 ++-
>  setup.py   | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/virt-sandbox-image b/scripts/virt-sandbox-image
> index 9be4f8c..9d0ff82 100755
> --- a/scripts/virt-sandbox-image
> +++ b/scripts/virt-sandbox-image
> @@ -1,8 +1,9 @@
>  #!/usr/bin/env python3
>  # -*- coding: utf-8 -*-
>  
> -from libvirt_sandbox_image import cli
>  import sys
>  
> +from libvirt_sandbox_image import cli
> +
>  if __name__ == '__main__':
> sys.exit(cli.main())
> diff --git a/setup.py b/setup.py
> index f29be2f..85f5d45 100644
> --- a/setup.py
> +++ b/setup.py
> @@ -1,11 +1,11 @@
>  
> -from setuptools import setup
> -from distutils.command.build import build
> -
>  import os
>  import re
>  import time
>  
> +from distutils.command.build import build
> +from setuptools import setup
> +
>  class my_build(build):
>  user_options = build.user_options
>  

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 1/9] pylint: Remove unused import statements

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/cli.py| 3 ---
>  libvirt_sandbox_image/sources/docker.py | 1 -
>  setup.py| 3 ---
>  3 files changed, 7 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/cli.py b/libvirt_sandbox_image/cli.py
> index 4bbeb5c..585df96 100644
> --- a/libvirt_sandbox_image/cli.py
> +++ b/libvirt_sandbox_image/cli.py
> @@ -23,12 +23,9 @@
>  
>  import argparse
>  import gettext
> -import hashlib
> -import json
>  import os
>  import os.path
>  import re
> -import shutil
>  import sys
>  import subprocess
>  import random
> diff --git a/libvirt_sandbox_image/sources/docker.py 
> b/libvirt_sandbox_image/sources/docker.py
> index be67e29..cf90112 100755
> --- a/libvirt_sandbox_image/sources/docker.py
> +++ b/libvirt_sandbox_image/sources/docker.py
> @@ -22,7 +22,6 @@
>  
>  import sys
>  import json
> -import traceback
>  import os
>  import subprocess
>  import shutil
> diff --git a/setup.py b/setup.py
> index c23c84d..f29be2f 100644
> --- a/setup.py
> +++ b/setup.py
> @@ -1,10 +1,7 @@
>  
>  from setuptools import setup
> -from setuptools import Command
>  from distutils.command.build import build
> -from distutils.util import get_platform
>  
> -import sys
>  import os
>  import re
>  import time

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 5/9] pylint: Fix white-space around keywords

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> This changes aim to resolve Pylint C0326.
> 
> http://pylint-messages.wikidot.com/messages:c0326
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/cli.py | 26 +-
>  libvirt_sandbox_image/sources/base.py|  4 ++--
>  libvirt_sandbox_image/sources/docker.py  |  4 ++--
>  libvirt_sandbox_image/sources/virtbuilder.py |  4 ++--
>  libvirt_sandbox_image/template.py|  4 ++--
>  setup.py |  4 ++--
>  6 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/cli.py b/libvirt_sandbox_image/cli.py
> index ec5fd6d..08c88a4 100644
> --- a/libvirt_sandbox_image/cli.py
> +++ b/libvirt_sandbox_image/cli.py
> @@ -48,7 +48,7 @@ gettext.textdomain("libvirt-sandbox")
>  try:
>  gettext.install("libvirt-sandbox",
>  localedir="/usr/share/locale",
> -codeset = 'utf-8')
> +codeset='utf-8')
>  except IOError:
>  import builtins
>  builtins.__dict__['_'] = str
> @@ -109,7 +109,7 @@ def run(args):
>  if args.connect is not None:
>  cmd.append("-c")
>  cmd.append(args.connect)
> -params = ['-m','host-image:/=%s,format=qcow2' % diskfile]
> +params = ['-m', 'host-image:/=%s,format=qcow2' % diskfile]
>  
>  networkArgs = args.network
>  if networkArgs is not None:
> @@ -144,40 +144,40 @@ def list_cached(args):
>  tmpls.extend(template.Template.get_all(source,
> "%s/%s" % 
> (args.template_dir, source)))
>  for tmpl in tmpls:
> -print (tmpl)
> +print(tmpl)
>  
>  def requires_template(parser):
>  parser.add_argument("template",
>  help=_("URI of the template"))
>  
>  def requires_name(parser):
> -parser.add_argument("-n","--name",
> +parser.add_argument("-n", "--name",
>  help=_("Name of the running sandbox"))
>  
>  def requires_debug(parser):
> -parser.add_argument("-d","--debug",
> +parser.add_argument("-d", "--debug",
>  default=False, action="store_true",
>  help=_("Run in debug mode"))
>  
>  def check_connect(connectstr):
> -supportedDrivers = ['lxc:///','qemu:///session','qemu:///system']
> +supportedDrivers = ['lxc:///', 'qemu:///session', 'qemu:///system']
>  if not connectstr in supportedDrivers:
>  raise ValueError("URI '%s' is not supported by virt-sandbox-image" % 
> connectstr)
>  return True
>  
>  def requires_connect(parser):
> -parser.add_argument("-c","--connect",
> +parser.add_argument("-c", "--connect",
>  help=_("Connect string for libvirt"))
>  
>  def requires_template_dir(parser):
>  global default_template_dir
> -parser.add_argument("-t","--template-dir",
> +parser.add_argument("-t", "--template-dir",
>  default=default_template_dir,
>  help=_("Template directory for saving templates"))
>  
>  def requires_image_dir(parser):
>  global default_image_dir
> -parser.add_argument("-I","--image-dir",
> +parser.add_argument("-I", "--image-dir",
>  default=default_image_dir,
>  help=_("Image directory for saving images"))
>  
> @@ -222,9 +222,9 @@ def gen_run_args(subparser):
>  parser.add_argument("args",
>  nargs=argparse.REMAINDER,
>  help=_("command arguments to run"))
> -parser.add_argument("-N","--network",
> +parser.add_argument("-N", "--network",
>  help=_("Network params for running template"))
> -parser.add_argument("-e","--env",action="append",
> +parser.add_argument("-e", "--env", action="append",
>  help=_("Environment params for running template"))
>  
>  parser.set_defaults(func=run)
> @@ -234,7 +234,7 @@ def gen_list_args(subparser):
>  _("List locally cached images"))
>  requires_template_dir(parser)
>  
> -parser.add_argument("-s","--source",
> +parser.add_argument("-s", "--source",
>  help=_("Name of the template source"))
>  
>  parser.set_defaults(func=list_cached)
> @@ -275,5 +275,5 @@ def main():
>  sys.stderr.flush()
>  sys.exit(1)
>  except Exception as e:
> -print (e)
> +print(e)
>  sys.exit(1)
> diff --git a/libvirt_sandbox_image/sources/base.py 
> b/libvirt_sandbox_image/sources/base.py
> index 0fc9243..c132489 100644
> --- a/libvirt_sandbox_image/sources/base.py
> +++ b/libvirt_sandbox_image/sources/base.py
> @@ -127,13 +127,13 @@ class Source():
>  
>  # Utility functions to share between the sources.
>  
> -def 

Re: [libvirt] [PATCH sandbox-image 4/9] py3: Use 'builtins' instead of '__builtin__'

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> In Python3, the __builtin__ module is renamed to builtins.
> 
> https://docs.python.org/3/library/builtins.html#module-builtins
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/cli.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/cli.py b/libvirt_sandbox_image/cli.py
> index 585df96..ec5fd6d 100644
> --- a/libvirt_sandbox_image/cli.py
> +++ b/libvirt_sandbox_image/cli.py
> @@ -50,8 +50,8 @@ try:
>  localedir="/usr/share/locale",
>  codeset = 'utf-8')
>  except IOError:
> -import __builtin__
> -__builtin__.__dict__['_'] = unicode
> +import builtins
> +builtins.__dict__['_'] = str
>  
>  
>  def debug(msg):

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image 3/9] pylint: Use consistent indentation of 4 spaces

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 21:57 +0100, Radostin Stoyanov wrote:
> Pylint warning W0311 - Bad indentation
> 
> http://pylint-messages.wikidot.com/messages:w0311
> 
> Signed-off-by: Radostin Stoyanov 
> ---
>  libvirt_sandbox_image/sources/docker.py | 4 ++--
>  scripts/virt-sandbox-image  | 2 +-
>  setup.py| 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libvirt_sandbox_image/sources/docker.py 
> b/libvirt_sandbox_image/sources/docker.py
> index cf90112..94eb6d1 100755
> --- a/libvirt_sandbox_image/sources/docker.py
> +++ b/libvirt_sandbox_image/sources/docker.py
> @@ -47,9 +47,9 @@ class DockerConfParser():
>  def getEnvs(self):
>  lst = self.json_data['config']['Env']
>  if lst is not None and isinstance(lst,list):
> -  return lst
> +return lst
>  else:
> -  return []
> +return []
>  
>  class DockerImage():
>  
> diff --git a/scripts/virt-sandbox-image b/scripts/virt-sandbox-image
> index 9d0ff82..c021850 100755
> --- a/scripts/virt-sandbox-image
> +++ b/scripts/virt-sandbox-image
> @@ -6,4 +6,4 @@ import sys
>  from libvirt_sandbox_image import cli
>  
>  if __name__ == '__main__':
> -   sys.exit(cli.main())
> +sys.exit(cli.main())
> diff --git a/setup.py b/setup.py
> index 85f5d45..dd22fd5 100644
> --- a/setup.py
> +++ b/setup.py
> @@ -97,7 +97,7 @@ setup(
>  ],
>  install_requires=[],
>  cmdclass = {
> -  'build': my_build,
> +'build': my_build,
>  },
>  classifiers = [
>  "License :: OSI Approved :: GNU Lesser General Public License v2 or 
> later (LGPLv2+)",

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox-image] Add manual pages to describe virt-sandbox-image

[Cedric Bosdonnat]

On Wed, 2018-04-18 at 17:12 +0100, Daniel P. Berrangé wrote:
> There's little point releasing a new tool if we don't provide any docs
> describing how it is used.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitignore |   1 +
>  MANIFEST.in|   2 +
>  libvirt-sandbox-image.spec.in  |   1 +
>  man/virt-sandbox-image-list.pod|  92 +
>  man/virt-sandbox-image-prepare.pod | 100 ++
>  man/virt-sandbox-image-purge.pod   |  96 +
>  man/virt-sandbox-image-run.pod | 165 
> +
>  man/virt-sandbox-image.pod | 136 ++
>  setup.py   |  33 +++-
>  9 files changed, 625 insertions(+), 1 deletion(-)
>  create mode 100644 man/virt-sandbox-image-list.pod
>  create mode 100644 man/virt-sandbox-image-prepare.pod
>  create mode 100644 man/virt-sandbox-image-purge.pod
>  create mode 100644 man/virt-sandbox-image-run.pod
>  create mode 100644 man/virt-sandbox-image.pod
> 
> diff --git a/.gitignore b/.gitignore
> index 052e94b..5c7f451 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -7,3 +7,4 @@ build/
>  AUTHORS
>  ChangeLog
>  *egg-info/
> +man/*.1
> diff --git a/MANIFEST.in b/MANIFEST.in
> index 5f690a3..794a949 100644
> --- a/MANIFEST.in
> +++ b/MANIFEST.in
> @@ -11,4 +11,6 @@ include libvirt_sandbox_image/source/*.py
>  include autobuild.sh
>  include libvirt-sandbox-image.spec
>  include libvirt-sandbox-image.spec.in
> +include man/*1
> +include man/*pod
>  global-exclude *.pyc
> diff --git a/libvirt-sandbox-image.spec.in b/libvirt-sandbox-image.spec.in
> index c122af0..69c18e3 100644
> --- a/libvirt-sandbox-image.spec.in
> +++ b/libvirt-sandbox-image.spec.in
> @@ -28,6 +28,7 @@ Currently docker and virt-builder images are supported.
>  %files
>  %doc README LICENSE
>  %{_bindir}/virt-sandbox-image
> +%{_mandir}/man1/virt-sandbox-image*1*
>  %{python3_sitelib}/libvirt_sandbox_image/
>  %{python3_sitelib}/libvirt_sandbox_image-%{version}-py3.*.egg-info
>  
> diff --git a/man/virt-sandbox-image-list.pod b/man/virt-sandbox-image-list.pod
> new file mode 100644
> index 000..69c8bd5
> --- /dev/null
> +++ b/man/virt-sandbox-image-list.pod
> @@ -0,0 +1,92 @@
> +=encoding utf8
> +
> +=head1 NAME
> +
> +virt-sandbox-image list - List cached image templates
> +
> +=head1 SYNOPSIS
> +
> +  virt-sandbox-image list [-h] [-c CONNECT] [-t TEMPLATE_DIR] template
> +
> +=head1 DESCRIPTION
> +
> +B is used to launch sandboxes from a pre-built container
> +image template. It is able to download image templates in a number of
> +formats from online image registries, including those built for Docker
> +and virt-builder. The images can be run with either QEMU (KVM) or LXC
> +virtualization drivers.
> +
> +The B command can be used to display the templates which are present
> +in the local cache. Existance of a cached template will allow the B
> +command to launch sandboxes immediately. Unwanted templates in the cache
> +can be deleted using the B command.
> +
> +=head1 OPTIONS
> +
> +=over 4
> +
> +=item B<-h>, B<--help>
> +
> +Display help message
> +
> +=item B<-c URI>, B<--connect URI>
> +
> +The connection URI for the hypervisor. The following URIs are currently
> +supported
> +
> + - lxc:///(if running as root)
> + - qemu:///system (if running as root)
> + - qemu:///session (if running as non-root)
> +
> +It is not permitted to use non-local URIs (ie URIs with a hostname)
> +at this time.
> +
> +=item B<-t TEMPLATE_DIR>
> +
> +Override the default template directory which is usually
> +C (root) or 
> C<$HOME/.local/share/libvirt/templates>.
> +
> +=item B
> +
> +The URI identifying the template to download and prepare.
> +
> +=back
> +
> +=head1 EXAMPLE
> +
> +Display a list of template images in the cache
> +
> + # virt-sandbox-image list -c qemu:///session
> +
> +
> +=head1 SEE ALSO
> +
> +C,
> +C, C,
> +C, C
> +
> +=head1 FILES
> +
> +Pristine downloaded templates will be stored in subdirectories of
> +C (root) or 
> C<$HOME/.local/share/libvirt/templates>
> +(non-root) by default.
> +
> +Overlay images created when running an instance of the template will
> +be stored in C (root) or 
> C<$HOME/.local/share/libvirt/images>
> +(non-root) by default.
> +
> +=head1 AUTHORS
> +
> +Daniel P. Berrangé , Eren Yagdiran 
> 
> +and Cédric Bosdonnat 
> +
> +=head1 COPYRIGHT
> +
> +Copyright (C) 2011-2018 Red Hat, Inc.
> +
> +=head1 LICENSE
> +
> +virt-sandbox is distributed under the terms of the GNU LGPL v2+.
> +This is free software; see the source for copying conditions.
> +There is NO warranty; not even for MERCHANTABILITY or FITNESS
> +FOR A PARTICULAR PURPOSE
> diff --git 

Re: [libvirt] [PATCH sandbox 1/1] Delete virt-sandbox-image tool now in separate repo

[Cedric Bosdonnat]

 datafile = layerdir + "/template.tar.gz"
> -
> -with open(jsonfile, "w") as fh:
> -fh.write(json.dumps(config))
> -
> -registry.save_data("/v2/%s/%s/blobs/%s" % (
> -   image.repo, image.name, layerChecksum),
> -   datafile, checksum=layerChecksum)
> -
> -
> -index = {
> -"repo": image.repo,
> -"name": image.name,
> -"tag": image.tag,
> -}
> -
> -indexfile = templatedir + "/" + layers[0]["id"] + "/index.json"
> -with open(indexfile, "w") as f:
> -f.write(json.dumps(index))
> -
> -
> -def create_template(self, template, templatedir, connect=None):
> -image = DockerImage.from_template(template)
> -
> -if not self._was_downloaded(image, templatedir):
> -self.download_template(image, template, templatedir)
> -
> -imagelist = self._get_image_list(image, templatedir)
> -imagelist.reverse()
> -
> -parentImage = None
> -for imagetagid in imagelist:
> -templateImage = templatedir + "/" + imagetagid + 
> "/template.qcow2"
> -cmd = ["qemu-img","create","-f","qcow2"]
> -if parentImage is not None:
> -cmd.append("-o")
> -cmd.append("backing_fmt=qcow2,backing_file=%s" % parentImage)
> -cmd.append(templateImage)
> -if parentImage is None:
> -cmd.append("10G")
> -subprocess.check_call(cmd)
> -
> -if parentImage is None:
> -self.format_disk(templateImage, "qcow2", connect)
> -
> -path = templatedir + "/" + imagetagid + "/template."
> -self.extract_tarball(path + "qcow2",
> - "qcow2",
> - path + "tar.gz",
> - connect)
> -parentImage = templateImage
> -
> -def _get_image_list(self, image, templatedir):
> -imageparent = {}
> -imagenames = {}
> -imagedirs = []
> -try:
> -imagedirs = os.listdir(templatedir)
> -except OSError:
> -pass
> -for imagetagid in imagedirs:
> -indexfile = templatedir + "/" + imagetagid + "/index.json"
> -if os.path.exists(indexfile):
> -with open(indexfile,"r") as f:
> -index = json.load(f)
> -thisimage = DockerImage(index.get("repo"),
> -index["name"],
> -index.get("tag"))
> -imagenames[str(thisimage)] = imagetagid
> -jsonfile = templatedir + "/" + imagetagid + "/template.json"
> -if os.path.exists(jsonfile):
> -with open(jsonfile,"r") as f:
> -    data = json.load(f)
> -parent = data.get("parent",None)
> -if parent:
> -imageparent[imagetagid] = parent
> -if str(image) not in imagenames:
> -raise ValueError(["Image %s does not exist locally" % image])
> -imagetagid = imagenames[str(image)]
> -imagelist = []
> -while imagetagid != None:
> -imagelist.append(imagetagid)
> -parent = imageparent.get(imagetagid,None)
> -imagetagid = parent
> -return imagelist
> -
> -def delete_template(self, template, templatedir):
> -image = DockerImage.from_template(template)
> -
> -imageusage = {}
> -imageparent = {}
> -imagenames = {}
> -imagedirs = []
> -try:
> -imagedirs = os.listdir(templatedir)
> -except OSError:
> -pass
> -for imagetagid in imagedirs:
> -indexfile = templatedir + "/" + imagetagid + "/index.json"
> -if os.path.exists(indexfile):
> -with open(indexfile,"r") as f:
> -index = json.load(f)
> -thisimage = DockerImage(index.get("repo"),
> -index["name"],
> -index.get("tag"))
> -  

Re: [libvirt] [RFC PATCH 0/4] LXC - Implement save/restore domain state

[Cedric Bosdonnat]

On Wed, 2018-04-11 at 12:34 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 11, 2018 at 12:29:11PM +0100, Radostin Stoyanov wrote:
> > This patch set contains rebased version of Katerina's work from GSoC 2016 
> > [1].
> > It allows integrates CRIU [2] with the libvirt-lxc to enable save/resore of 
> > containers.
> 
> I vaguely recall that when Katerina first did that work, we hit some
> limitations of CRIU at the time, that blocked us merging. Does anyone
> recall what that was, and if & when it was addressed in CRIU ?

What was missing in CRIU was the possibility to write the data into a stream
rather than in files. From what I recall this work has been merged upstream
in the mean time.

--
Cedric

> > 
> > [1] https://wiki.libvirt.org/page/Google_Summer_of_Code_2016/lxc_migration
> > [2] https://criu.org
> > 
> > Radostin Stoyanov (4):
> >   configure: Include support for CRIU
> >   lxc: Add save/restore helper functions
> >   lxc: Add restore mode for libvirt-lxc
> >   lxc: Add save/restore support
> > 
> >  configure.ac |   1 +
> >  m4/virt-criu.m4  |  27 +
> >  po/POTFILES.in   |   1 +
> >  src/lxc/Makefile.inc.am  |   4 +
> >  src/lxc/lxc_container.c  | 162 --
> >  src/lxc/lxc_container.h  |   3 +-
> >  src/lxc/lxc_controller.c | 104 ++-
> >  src/lxc/lxc_criu.c   | 253 
> > +++
> >  src/lxc/lxc_criu.h   |  36 +++
> >  src/lxc/lxc_driver.c | 238 +++-
> >  src/lxc/lxc_process.c|  23 -
> >  src/lxc/lxc_process.h|   1 +
> >  12 files changed, 836 insertions(+), 17 deletions(-)
> >  create mode 100644 m4/virt-criu.m4
> >  create mode 100644 src/lxc/lxc_criu.c
> >  create mode 100644 src/lxc/lxc_criu.h
> > 
> > -- 
> > 2.14.3
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> 
> Regards,
> Daniel

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

Re: [libvirt] [PATCH sandbox] Delete the virt-sandbox-service command

[Cedric Bosdonnat]

On Mon, 2018-03-26 at 15:35 +0100, Daniel P. Berrangé wrote:
> This command attempted to create sandboxed containers for running
> systemd services that exist on the host. This code has proved very
> fragile, however, since it needs heuristics to figure out which dirs
> need to be made private in the container vs shared with the host. Even
> a relatively simple "httpd.service" sandbox no longer works with
> current Fedora.
> 
> Users wanting to sandbox services are better served by using systemd's
> native container functionality, or using Docker container images. The
> virt-sandbox-image tool can even run Docker/virt-builder images directly.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  TODO|   24 -
>  bin/Makefile.am |   80 +-
>  bin/virt-sandbox-service| 1314 
> ---
>  bin/virt-sandbox-service-bash-completion.sh |  141 ---
>  bin/virt-sandbox-service-clone.pod  |  100 --
>  bin/virt-sandbox-service-connect.pod|   59 --
>  bin/virt-sandbox-service-create.pod |  264 --
>  bin/virt-sandbox-service-delete.pod |   65 --
>  bin/virt-sandbox-service-execute.pod|   71 --
>  bin/virt-sandbox-service-reload.pod |   63 --
>  bin/virt-sandbox-service-upgrade.pod|   74 --
>  bin/virt-sandbox-service-util.c |  305 ---
>  bin/virt-sandbox-service.logrotate  |9 -
>  bin/virt-sandbox-service.pod|   85 --
>  cfg.mk  |2 +-
>  libvirt-sandbox.spec.in |7 -
>  libvirt-sandbox/tests/containers_test.sh|   37 -
>  po/POTFILES.in  |1 -
>  18 files changed, 3 insertions(+), 2698 deletions(-)
>  delete mode 100644 TODO
>  delete mode 100755 bin/virt-sandbox-service
>  delete mode 100755 bin/virt-sandbox-service-bash-completion.sh
>  delete mode 100644 bin/virt-sandbox-service-clone.pod
>  delete mode 100644 bin/virt-sandbox-service-connect.pod
>  delete mode 100644 bin/virt-sandbox-service-create.pod
>  delete mode 100644 bin/virt-sandbox-service-delete.pod
>  delete mode 100644 bin/virt-sandbox-service-execute.pod
>  delete mode 100644 bin/virt-sandbox-service-reload.pod
>  delete mode 100644 bin/virt-sandbox-service-upgrade.pod
>  delete mode 100644 bin/virt-sandbox-service-util.c
>  delete mode 100644 bin/virt-sandbox-service.logrotate
>  delete mode 100644 bin/virt-sandbox-service.pod
>  delete mode 100755 libvirt-sandbox/tests/containers_test.sh
> 
> diff --git a/TODO b/TODO
> deleted file mode 100644
> index fc63361..000
> --- a/TODO
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -  libvirt-sandbox TODO list
> -  =
> -
> -systemd-tmpfiles --create needs to be run within the container, before any
> -apps are started, since it will populate /run (Completed)
> -
> -CGROUPFS: integration so libvirt does it rather then systemd within the 
> container
> -  We need kernel labeling support for cgroupfs so we can allow systemd 
> to write to its section of the
> cgroupfs.
> -
> -SYSLOG:  Currently syslog messages are going no where within the container.
> -If we run a syslog within the container will it get messages from the 
> outside?  Should we just use systemd-
> journal.  I think sysadmins will want to be able to look in /var/log/messages 
> within the container. (systemd-journal
> is now running within a container)
> -
> -EXECUTE:
> - virt-sandbox-service execute --command "BLAH" does not work.  We need 
> to have the ability to execute any
> random command within the container, and get stdin, stdout, stderror outside 
> the container. (Partially Completed)
> -Still needs kernel to implement missing container namespace files under 
> /proc/PID/ns, Also need a mechanism to get
> the PID of systemd from libvirt.
> -
> -HOSTNAME:
> - Currently if I execute hostname within the container it sees the name 
> of the host not the name based on the
> container name or the IP Address associated with dhclient. (Completed)
> -
> -virt-sandbox-service connect NAME hangs when you attempt to end the 
> connection.
> -^d should bring you back to the host terminal.
> -
> -Need a mechanism to allow admins to specify additional services to run within
> -the container.  For example you may want to run mysql and apache within the 
> same container. (Completed) You can do
> this using systemctl enabel BLAH
> diff --git a/bin/Makefile.am b/bin/Makefile.am
> index deedcf6..db0a1d1 100644
> --- a/bin/Makefile.am
> +++ b/bin/Makefile.am
> @@ -1,39 +1,12 @@
>  
>  bin_PROGRAMS = virt-sandbox
>  
> -libexec_PROGRAMS = virt-sandbox-service-util
> +bin_SCRIPTS = virt-sandbox-image
>  
> -bin_SCRIPTS = virt-sandbox-service \
> - virt-sandbox-image
> -
> -virtsandboxcompdir = $(datarootdir)/bash-completion/completions/
> -
> -crondailydir = $(sysconfdir)/cron.daily
> -crondaily_SCRIPTS = 

Re: [libvirt] [PATCH sandbox] Fix argparser incompatibilities with newer python 3

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 16:44 +, Daniel P. Berrangé wrote:
> Python 3 changes such that if no subparser command is listed, it just
> throws an error. To get back the old behavior we need to set the
> 'required'  attribute and a dest name.
> 
> Since we treat 'debug' as a global attribute we need to set that on the
> top level parser too, otherwise we get a missing attribute error with
> newish python 3 too.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt-sandbox/image/cli.py | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py
> index 490c5e0..d2035de 100644
> --- a/libvirt-sandbox/image/cli.py
> +++ b/libvirt-sandbox/image/cli.py
> @@ -202,7 +202,6 @@ Example supported URI formats:
>  def gen_purge_args(subparser):
>  parser = gen_command_parser(subparser, "purge",
>  _("Purge cached template"))
> -requires_debug(parser)
>  requires_template(parser)
>  requires_template_dir(parser)
>  parser.set_defaults(func=purge)
> @@ -210,7 +209,6 @@ def gen_purge_args(subparser):
>  def gen_prepare_args(subparser):
>  parser = gen_command_parser(subparser, "prepare",
>  _("Prepare local template"))
> -requires_debug(parser)
>  requires_template(parser)
>  requires_connect(parser)
>  requires_template_dir(parser)
> @@ -219,7 +217,6 @@ def gen_prepare_args(subparser):
>  def gen_run_args(subparser):
>  parser = gen_command_parser(subparser, "run",
>  _("Run an instance of a template"))
> -requires_debug(parser)
>  requires_name(parser)
>  requires_template(parser)
>  requires_connect(parser)
> @@ -238,7 +235,6 @@ def gen_run_args(subparser):
>  def gen_list_args(subparser):
>  parser = gen_command_parser(subparser, "list",
>  _("List locally cached images"))
> -requires_debug(parser)
>  requires_template_dir(parser)
>  
>  parser.add_argument("-s","--source",
> @@ -249,7 +245,11 @@ def gen_list_args(subparser):
>  def main():
>  parser = argparse.ArgumentParser(description="Sandbox Container Image 
> Tool")
>  
> +requires_debug(parser)
> +
>  subparser = parser.add_subparsers(help=_("commands"))
> +subparser.required = True
> +subparser.dest = "command"
>  gen_purge_args(subparser)
>  gen_prepare_args(subparser)
>  gen_run_args(subparser)

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox] rpm: fix references to python 2 packages / files

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 15:57 +, Daniel P. Berrangé wrote:
> Since we switched to python 3, we should have deps on the python 3 based
> packages, and look at the python 3 sitelib directory.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt-sandbox.spec.in | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libvirt-sandbox.spec.in b/libvirt-sandbox.spec.in
> index 17e9dd9..f5868c1 100644
> --- a/libvirt-sandbox.spec.in
> +++ b/libvirt-sandbox.spec.in
> @@ -36,12 +36,12 @@ BuildRequires: zlib-devel >= 1.2.0, zlib-static
>  BuildRequires: libtirpc-devel
>  BuildRequires: rpcgen
>  %endif
> -Requires: rpm-python
> +Requires: python3-rpm
>  # For virsh lxc-enter-namespace command
>  Requires: libvirt-client >= %{libvirt_version}
>  Requires: systemd >= 198
>  Requires: pygobject3-base
> -Requires: libselinux-python
> +Requires: libselinux-python3
>  Requires: %{name}-libs = %{version}-%{release}
>  Requires: %{_bindir}/virt-builder
>  
> @@ -108,7 +108,7 @@ rm -rf $RPM_BUILD_ROOT
>  %{_bindir}/virt-sandbox-service
>  %{_bindir}/virt-sandbox-image
>  %{_libexecdir}/virt-sandbox-service-util
> -%{python_sitelib}/libvirt_sandbox
> +%{python3_sitelib}/libvirt_sandbox
>  %{_mandir}/man1/virt-sandbox.1*
>  %{_mandir}/man1/virt-sandbox-service.1*
>  %{_mandir}/man1/virt-sandbox-service-*.1*

ACK
--
Cedric

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

Re: [libvirt] [PATCH sandbox] Explicitly check for python3 in configure.ac

[Cedric Bosdonnat]

On Fri, 2018-03-23 at 15:57 +, Daniel P. Berrangé wrote:
> A bare AM_PATH_PYTHON statement is satisfied by any version of python >=
> 2.0.0, but we converted to Python 3, so should be explicit about it.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index cb7a483..28305d2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -130,7 +130,7 @@ dnl Should be in m4/virt-gettext.m4 but intltoolize is too
>  dnl dumb to find it there
>  IT_PROG_INTLTOOL([0.35.0])
>  
> -AM_PATH_PYTHON
> +AM_PATH_PYTHON([3])
>  
>  AC_OUTPUT(Makefile
>libvirt-sandbox/Makefile

ACK
--
Cedric

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

Re: [libvirt] [PATCH] lxc: report error message raised by the failing function

[Cedric Bosdonnat]

Hello Prafull,

On Thu, 2018-03-22 at 20:04 +0530, Prafull wrote:
> The code that calls VIR_WARN after a function fails, doesn't
> report the error message raised by the failing function.
> Such error messages are now reported in lxc/lxc_driver.c
> 
> Signed-off-by: Prafullkumar T 

Sorry for asking a potentially dumb question, but I know nothing of the
Indian culture. Is 'Prafullkumar T' your firstname and lastname? Just make
sure your first and last names are used for signing off and committing.

For this, set the user.name in your git repo (or globally). You can also
change the author of an existing commit with:

git commit --amend --author 'fullname '

Other than that, your commit looks fine to me. Note that you haven't used
your full name as commit author. I'll wait for a v2 with updated names to push.

Regards,
--
Cedric

> ---
>  src/lxc/lxc_driver.c | 36 ++--
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 4f6b93b..4f600f3 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3915,8 +3915,8 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver,
>  major(sb.st_rdev),
>  minor(sb.st_rdev),
>  perms) < 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - src, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + src, vm->def->name, virGetLastErrorMessage());
>  goto cleanup;
>  }
>  
> @@ -4011,8 +4011,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn,
>  goto cleanup;
>  } else {
>  VIR_WARN("setting bandwidth on interfaces of "
> - "type '%s' is not implemented yet",
> - virDomainNetTypeToString(actualType));
> + "type '%s' is not implemented yet: %s",
> + virDomainNetTypeToString(actualType), 
> virGetLastErrorMessage());
>  }
>  }
>  
> @@ -4116,8 +4116,8 @@ 
> lxcDomainAttachDeviceHostdevSubsysUSBLive(virLXCDriverPtr driver,
>  if (virUSBDeviceFileIterate(usb,
>  virLXCTeardownHostUSBDeviceCgroup,
>  priv->cgroup) < 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - src, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + src, vm->def->name, virGetLastErrorMessage());
>  goto cleanup;
>  }
>  
> @@ -4190,8 +4190,8 @@ lxcDomainAttachDeviceHostdevStorageLive(virLXCDriverPtr 
> driver,
>  major(sb.st_rdev),
>  minor(sb.st_rdev),
>  VIR_CGROUP_DEVICE_RWM) < 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - def->source.caps.u.storage.block, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + def->source.caps.u.storage.block, vm->def->name, 
> virGetLastErrorMessage());
>  goto cleanup;
>  }
>  
> @@ -4262,8 +4262,8 @@ lxcDomainAttachDeviceHostdevMiscLive(virLXCDriverPtr 
> driver,
>  major(sb.st_rdev),
>  minor(sb.st_rdev),
>  VIR_CGROUP_DEVICE_RWM) < 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - def->source.caps.u.storage.block, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + def->source.caps.u.storage.block, vm->def->name, 
> virGetLastErrorMessage());
>  goto cleanup;
>  }
>  
> @@ -4434,8 +4434,8 @@ lxcDomainDetachDeviceDiskLive(virDomainObjPtr vm,
>  
>  if (virCgroupDenyDevicePath(priv->cgroup, src,
>  VIR_CGROUP_DEVICE_RWM, false) != 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - src, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + src, vm->def->name, virGetLastErrorMessage());
>  
>  virDomainDiskRemove(vm->def, idx);
>  virDomainDiskDefFree(def);
> @@ -4567,8 +4567,8 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr 
> driver,
>  if (virUSBDeviceFileIterate(usb,
>  virLXCTeardownHostUSBDeviceCgroup,
>  priv->cgroup) < 0)
> -VIR_WARN("cannot deny device %s for domain %s",
> - dst, vm->def->name);
> +VIR_WARN("cannot deny device %s for domain %s: %s",
> + dst, vm->def->name, virGetLastErrorMessage());
>  
>  virObjectLock(hostdev_mgr->activeUSBHostdevs);
>  

Re: [libvirt] [GSoC] Pointers for project idea

[Cedric Bosdonnat]

Hi Sukrit,

On Sat, 2018-03-17 at 19:42 +0530, Sukrit Bhatnagar wrote:
> Can anyone provide me with some pointers for the project "Conversion to and 
> from OCI formatted containers"?
> 
> I have referred to the relevant source files listed on the ideas page and 
> would like to read more about it.

Could you be more specific? The project idea already projects pointers to the 
OCI spec
and similar code in the lxc driver... what more would you need?

--
Cedric

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

Re: [libvirt] Emacs tip for easily adding Reviewed-by tags & friends

[Cedric Bosdonnat]

On Thu, 2018-03-15 at 12:38 +, Daniel P. Berrangé wrote:
> On Thu, Mar 15, 2018 at 01:34:31PM +0100, Erik Skultety wrote:
> > On Thu, Mar 15, 2018 at 11:36:28AM +, Daniel P. Berrangé wrote:
> > > It is nice that git has the short-hand for adding Signed-off-by, but
> > > adding other tags during reviews is kind of tedious and long winded.
> > > eg "ACK" is much shorter than typing "Reviewed-by: ...blah blah blah.."
> > > 
> > > Good editors have a way to setup macros though, and so I thought I'd
> > > share the emacs approach to making life easy again...
> > > 
> > > In my $HOME/.emacs.d/abbrev_defs file I have this:
> > > 
> > > (define-abbrev-table 'global-abbrev-table
> > >   '(
> > > ("8rev" "Reviewed-by: Daniel P. Berrangé " nil 1)
> > > ("8ack" "Acked-by: Daniel P. Berrangé " nil 1)
> > > ("8test" "Tested-by: Daniel P. Berrangé " nil 1)
> > > ("8sob" "Signed-off-by: Daniel P. Berrangé " nil 
> > > 1)
> > >))
> > > 
> > > Now, if I type the "8rev" [1] and then hit space-bar or enter, emacs 
> > > expands
> > > it to the full "Reviewed-by: blah blah blah..." line. This makes 
> > > adding
> > > the full tags just as quick & easy as it was to type a traditional "ACK".
> > > 
> > > Anyone have an equivalent tip for Vim ?
> > 
> > I'm using the following plugin for Vim which I tuned just a tiny bit so 
> > that I
> > could write even less :P.
> > 
> > https://github.com/vim-scripts/git_patch_tags.vim
> 
> What does it mean when it says  "" in that README. Is that a
> name referring to a magic key sequence, or literal text to type ?

the leader is a key used at the beginning of some commands. It's referred as
 in all vim docs since it can be changed by the user with mapleader 
command.
By default the leader is '\'

--
Cedric

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

Re: [libvirt] libvirt build failure with --disable-shared

[Cedric Bosdonnat]

Hello Rahul,

On Thu, 2018-03-15 at 00:24 -0400, Rahul Sharma wrote:
> Hi,
> 
> I am facing an error with libvirt build where it fails if I try to disable 
> shared libraries. Though I opened up the
> issue on gitlab, not sure if that was the right place to open one. Kindly do 
> let me know if this should be opened up
> at some other place.

No bug report on gitlab will ever be read, see yours in #1 ;) Read this page
for more infos on how to report a bug: https
://libvirt.org/bugs.html

> Build failure with libvirt-4.1.0 using the command:
> CFLAGS="-fPIC" ./configure --enable-static --disable-shared 
> --prefix=/home/ubuntu/libvirt-4.1.0/out
> 
> https://gitlab.com/libvirt/libvirt/issues/1


Building only static libs looks fishy to me... what is the reason behing that?

virDomainLifecycleTypeToString is declared by the VIR_ENUM_DECL macro
and implemented by VIR_ENUM_IMPL one. The VIR_ENUM_IMPL for that type
can be found in both tools/virsh-domain.c and src/conf/domain_conf.c,
hence the error.

I'm not sure virsh should be built with static libvirt libs at all.

--
Cedric

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

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

Re: [libvirt] [GSoC]: Help in setup

[Cedric Bosdonnat]

On Tue, 2018-03-13 at 16:45 +0530, Sukrit Bhatnagar wrote:
> Here is the output:
> 
> skrtbhtngr@ubuntu:~/libvirt$ ./autogen.sh --system
> 
> Updating submodules...
> Submodule 'gnulib' (git://git.sv.gnu.org/gnulib.git) registered for path 
> '.gnulib'
> Submodule 'keycodemapdb' (https://gitlab.com/keycodemap/keycodemapdb.git) 
> registered for path 'src/keycodemapdb'
> Cloning into '/home/skrtbhtngr/libvirt/.gnulib'...
> fatal: unable to connect to git.sv.gnu.org:
> git.sv.gnu.org[0: 208.118.235.201]: errno=Connection timed out
> 
> fatal: clone of 'git://git.sv.gnu.org/gnulib.git' into submodule path 
> '/home/skrtbhtngr/libvirt/.gnulib' failed
> Failed to clone '.gnulib'. Retry scheduled
> Cloning into '/home/skrtbhtngr/libvirt/src/keycodemapdb'...
> Cloning into '/home/skrtbhtngr/libvirt/.gnulib'...
> fatal: unable to connect to git.sv.gnu.org:
> git.sv.gnu.org[0: 208.118.235.201]: errno=Connection timed out
> 
> fatal: clone of 'git://git.sv.gnu.org/gnulib.git' into submodule path 
> '/home/skrtbhtngr/libvirt/.gnulib' failed
> Failed to clone '.gnulib' a second time, aborting
> error: Updating submodules failed
> 
> I have tried changing the gir URL 'git://git.sv.gnu.org/gnulib.git' to 
> 'git://git.savannah.gnu.org/gnulib.git', but
> even that doesn't work.

Could you try https://git.savannah.gnu.org/git/gnulib.git/ ? I have already 
seen students hit by
some firewall restrictions like that on their campus.

--
Cedric

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Cedric Bosdonnat]

On Mon, 2018-02-26 at 09:50 +0100, Erik Skultety wrote:
> Why can't we have both? Suits everyone, just like NHL jerseys - one with a
> light base and a second one with a darker-colored base :). I'm still to rework
> the design with the latest SVG set which Dan re-worked as part of the 
> webdesign
> work (I still like the original SVG more because the font is a bit bolder) but
> I had some issues with object alignment in inkscape because of a large 
> bounding
> box the 'Drop shadow' caused and I just didn't like micro pixel adjustments to
> the object position just to get it centered within the hexagon area, so I sent
> patches to mitigate the issue and I'll post a v2 of the design this week.

Sure, if we can afford having both let's go for both ;)

--
Cedric

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

Re: [libvirt] [RFC] libvirt hexagon sticker design

[Cedric Bosdonnat]

Hi Erik,

On Mon, 2018-02-19 at 09:44 +0100, Erik Skultety wrote:
> Hi folks,
> those who attended at least one conference for the past year have probably
> noticed the rising trend (more like "sticker hype") of FOSS projects giving 
> away
> these hexagon stickers, it's very inexpensive way of making some promo for
> their project and since we don't do many promos (AFAIK none to be precise) I
> guess as a project that's been going strong for 12 years already we should
> probably start somewhere, even baby steps count (as it turns out in this case 
> -
> - literally...). So, I've taken our libvirt-publican repo and came up with a
> few various color combinations for libvirt hexagon sticker design. Below you
> can find links to my personal google drive (these are hexagon meshes, I can,
> or anyone can for that matter, isolate individual designs and send them as
> separate patches on demand), since each of the SVGs is over 1.5MB and I'd 
> easily
> run into message size limits for the mailing list, had I gone with sending 
> these
> as patches against libvirt-publican.

Haven't seen that many hexagon stickers at the last Google mentor summit, but
generally speaking I agree that we need some sort of sticker (whatever shape it 
has).
I like the design of the link [2], may be more the dark fill and light (not 
white)
border, but may be we should gather those into a poll somehow.

--
Cedric

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

Re: [libvirt] [PATCH] libvirtd: Explicit dependency on systemd-machined

[Cedric Bosdonnat]

It's now pushed!
Thanks a lot Michal for your contribution, feel free to submit other ones in 
the future ;)

--
Cedric

On Wed, 2018-01-10 at 23:06 +0100, Michal Koutný wrote:
> The libvirtd daemon uses systemd-machined D-Bus API when manipulating
> domains. The systemd-machined is D-Bus activated on demand.
> 
> However, during system shutdown systemd-machined is stopped concurrently
> with libvirtd and virsh users also doing their final cleanup may
> transitively fail due to unavailability of systemd-machined. Example
> error message
> 
> > libvirtd[1390]: 2017-12-20 18:55:56.182+: 32700: error : 
> > virSystemdTerminateMachine:503 : Refusing activation,
> > D-Bus is shutting down.
> 
> To circumvent this we need to explicitly specify both ordering and
> requirement dependency (to avoid late D-Bus activation) on
> systemd-machined. See [1] for the dependency debate.
> 
> [1] 
> https://lists.freedesktop.org/archives/systemd-devel/2018-January/040095.html
> ---
> 
> The Wants= dependency is for the case when systemd-machined wasn't started
> neither D-Bus activated anytime before the shutdown transaction. AFAICS this 
> is
> very unlikely so for the sake of lazy activation, the Wants= hunk can be
> dropped.
> 
>  daemon/libvirtd.service.in | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in
> index c189f5e65..769702ea7 100644
> --- a/daemon/libvirtd.service.in
> +++ b/daemon/libvirtd.service.in
> @@ -7,6 +7,7 @@
>  Description=Virtualization daemon
>  Requires=virtlogd.socket
>  Requires=virtlockd.socket
> +Wants=systemd-machined.service
>  Before=libvirt-guests.service
>  After=network.target
>  After=dbus.service
> @@ -14,6 +15,7 @@ After=iscsid.service
>  After=apparmor.service
>  After=local-fs.target
>  After=remote-fs.target
> +After=systemd-machined.service
>  Documentation=man:libvirtd(8)
>  Documentation=https://libvirt.org
>  

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

Re: [libvirt] [PATCH] libvirtd: Explicit dependency on systemd-machined

[Cedric Bosdonnat]

The patch looks good to me, though I'm not a systemd expert.
If no one vetoes it I'll push your patch by the end of the day.

--
Cedric

On Mon, 2018-01-22 at 09:10 +0100, Michal Koutný wrote:
> Hello list.
> 
> On 01/10/2018 11:06 PM, Michal Koutný  wrote:
> > [...]
> 
> I see there's no response to the patch. Do you have any comments on this
> patch?
> 
> Thanks,
> Michal
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 06/12] apparmor, libvirt-qemu: Allow access to hugepage mounts

[Cedric Bosdonnat]

On Wed, 2018-01-03 at 17:55 +0100, Christian Ehrhardt wrote:
> [...]
> 
> > > To me, 1 feels most correct cause while the other two fix hugepages,
> > > there seem to be lurking bugs since we aren't implementing
> > > domainSetPathLabel.
> > > 
> > 
> > I work on #1 a while and I think we can do a lot good here.
> > Yet while I'm convinced at the changes this is currently a debugging 
> > nightmare.
> > I guess it wants to become a 2018 submission.
> 
> Note: I'll not user reply-to onto this thread to keep threading somewhat sane.
> Also the new submission, while inspired by this discussion, is a
> totally separate thing.
> 
> > So for now keep this hugepage patch out of consideration when looking
> > at applying all those with many +1's.
> 
> So as I expected the hugepage patch of this series will be covered by
> the new series that I submit.
> But I wanted to ask for all the others changes in the current series
> here to be considered - most have one or two acks already.
> Let me know if more is needed to commit them.

This is the only patch from the series that I haven't pushed or ignored...
What should be done with this one?

--
Cedric

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

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

Re: [libvirt] [PATCH] apparmor: fix virt-aa-helper profile

[Cedric Bosdonnat]

On Wed, 2018-01-03 at 11:54 +0100, intrigeri wrote:
> Cédric Bosdonnat:
> >  * to handle /var/run not being a symlink to /run
> 
> Does this still really exist in any distro that has chances to run
> a recent libvirt?

At least some people tweak their distro for that, since the openSUSE
AppArmor does it ;)

--
Cedric

> If yes, then:
> 
> > -  /run/libvirt/**/[sv]d[a-z] r
> > +  /{,var/}run/libvirt/**/[sv]d[a-z] r,
> 
> +1
> 
> And in any case, +1 the missing comma.
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images

[Cedric Bosdonnat]

On Thu, 2017-12-21 at 12:10 +0100, intrigeri wrote:
> 1. Doing the same for usr.sbin.libvirtd?

Is there any real good for the user to have local changes for the libvirtd 
profile?

> The apparmor_profiles_local_include.patch Debian patch does the same
> for usr.sbin.libvirtd:
> 
> diff --git a/examples/apparmor/usr.sbin.libvirtd 
> b/examples/apparmor/usr.sbin.libvirtd
> index fa4ebb3..5505bf6 100644
> --- a/examples/apparmor/usr.sbin.libvirtd
> +++ b/examples/apparmor/usr.sbin.libvirtd
> @@ -90,4 +90,7 @@
>  
> /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
>}
> +  
> +  # Site-specific additions and overrides. See local/README for details.
> +  #include 
>  }
> 
> Cédric and others, what about upstreaming this as well?

We don't have it yet in the openSUSE/SLES packages, so feel free to upstream it.
> 
> 2. Impact on packaging for distros that were already managing this
>local file themselves & differently
> 
> … i.e. I guess mostly Debian and derivatives, that use dh_apparmor.
> 
> If I got the build system changes right,
> local/usr.lib.libvirt.virt-aa-helper will be created at installation
> time so in practice it'll be shipped by distro packages.

Hum... In any case in a spec file (and I suppose for you too) that file can be
removed before creating the package. I'm not feeling good about including
files in the upstream profiles that don't exist.

> I *think* it's not a problem with dh_apparmor because the generated
> postinst scripts only manage that file if it does not exist yet (so it
> should be a no-op if dpkg has already installed it), and similarly the
> code for purging in postrm should work just fine if dpkg has already
> deleted it.

We mark all files in local with %config(noreplace), not sure what is best ;)

--
Cedric

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

Re: [libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

[Cedric Bosdonnat]

Hi John,

On Wed, 2017-12-20 at 16:52 -0500, John Ferlan wrote:
> So I moved the Unref(dmn) after virStateCleanup... That of course led to
> the virt-manager issue.  So I figured I'd give adding this patch in and
> that then fixed the virt-manager crash (obviously as we'd be Unref'ing
> all those servers we added).

Seems like I forgot to git-send my patch yesterday evening: here it is:

https://www.redhat.com/archives/libvir-list/2017-December/msg00739.html

That fixes the virt-manager crash at least.
--
Cedric

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

Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images

[Cedric Bosdonnat]

On Wed, 2017-12-20 at 10:17 +0100, intrigeri wrote:
> Hi,
> 
> Cedric Bosdonnat:
> > Has that one landed in abyssal depths of the mailing list?
> 
> Well, no, it's waiting for your comments about my feedback:
> https://www.redhat.com/archives/libvir-list/2017-December/msg00389.html
> 
> Thanks for pinging!
> 
> (Sorry I did not put you in explicit copy, I assumed you would
> monitor replies on the list.)

Oops, I overlooked that one.

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

Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images

[Cedric Bosdonnat]

On Tue, 2017-12-12 at 15:01 +0100, intrigeri wrote:
> Hi,
> 
> Cédric Bosdonnat:
> > This commit helps users allowing access to their images by adding their
> > own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper.
> > […]
> >  profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
> >#include 
> > +  #include 
> 
> The packaging helper we use in Debian adds exactly the same line at
> the *end* of the profile (and actually, at the end of almost every
> AppArmor profile included in Debian and derivatives); I don't know why
> it's added at the end and not at the beginning. I suspect Jamie will
> know better.
> 
> If there's no strong reason to add this line in the beginning of the
> profile, I suggest we add it at the end instead, so we avoid changing
> behaviour subtly once this gets merged upstream and we drop the
> Debian-specific line.
> 
> Other than this, ACK from me on the proposed profile modifications.
> 
> I am not well placed to comment on the build system changes though.

I'm perfectly fine in having that include at the end of the profile. I'll
push with that change.

--
Cedric

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

Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images

[Cedric Bosdonnat]

Hi there!

Has that one landed in abyssal depths of the mailing list?

--
Cedric

On Mon, 2017-12-11 at 16:23 +0100, Cédric Bosdonnat wrote:
> virt-aa-helper needs read access to the disk image to resolve symlinks
> and add the proper rules to the profile. Its profile whitelists a few
> common paths, but users can place their images anywhere.
> 
> This commit helps users allowing access to their images by adding their
> own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper.
> 
> This commit also adds rules to allow reading files named:
>   - *.raw as this is a rather common disk image extension
>   - /run/libvirt/**[vd]d[a-z] as these are used by virt-sandbox
> ---
>  examples/Makefile.am | 24 
> ++--
>  examples/apparmor/usr.lib.libvirt.virt-aa-helper |  4 
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/Makefile.am b/examples/Makefile.am
> index ef2f79db3..8a1d6919a 100644
> --- a/examples/Makefile.am
> +++ b/examples/Makefile.am
> @@ -67,6 +67,9 @@ admin_client_info_SOURCES = admin/client_info.c
>  admin_client_close_SOURCES = admin/client_close.c
>  admin_logging_SOURCES = admin/logging.c
>  
> +INSTALL_DATA_LOCAL =
> +UNINSTALL_LOCAL =
> +
>  if WITH_APPARMOR_PROFILES
>  apparmordir = $(sysconfdir)/apparmor.d/
>  apparmor_DATA = \
> @@ -85,20 +88,37 @@ templates_DATA = \
>   apparmor/TEMPLATE.qemu \
>   apparmor/TEMPLATE.lxc \
>   $(NULL)
> +
> +APPARMOR_LOCAL_DIR = "$(DESTDIR)$(apparmordir)/local"
> +install-apparmor-local:
> + $(MKDIR_P) "$(APPARMOR_LOCAL_DIR)"
> + echo "# Site-specific additions and overrides for \
> +  'usr.lib.libvirt.virt-aa-helper'" \
> +  >$(APPARMOR_LOCAL_DIR)/usr.lib.libvirt.virt-aa-helper
> +
> +INSTALL_DATA_LOCAL += install-apparmor-local
> +UNINSTALL_LOCAL += uninstall-apparmor-local
>  endif WITH_APPARMOR_PROFILES
>  
>  if WITH_NWFILTER
>  NWFILTER_DIR = "$(DESTDIR)$(sysconfdir)/libvirt/nwfilter"
>  
> -install-data-local:
> +install-nwfilter-local:
>   $(MKDIR_P) "$(NWFILTER_DIR)"
>   for f in $(FILTERS); do \
>   $(INSTALL_DATA) $$f "$(NWFILTER_DIR)"; \
>   done
>  
> -uninstall-local::
> +uninstall-nwfilter-local::
>   for f in $(FILTERS); do \
>   rm -f "$(NWFILTER_DIR)/`basename $$f`"; \
>   done
>   -test -z "$(shell ls $(NWFILTER_DIR))" || rmdir $(NWFILTER_DIR)
> +
> +INSTALL_DATA_LOCAL += install-nwfilter-local
> +UNINSTALL_LOCAL += uninstall-nwfilter-local
>  endif WITH_NWFILTER
> +
> +install-data-local: $(INSTALL_DATA_LOCAL)
> +
> +uninstall-local: $(UNINSTALL_LOCAL)
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index bd6181d00..f3069d369 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -3,6 +3,7 @@
>  
>  profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
>#include 
> +  #include 
>  
># needed for searching directories
>capability dac_override,
> @@ -50,8 +51,11 @@ profile virt-aa-helper 
> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>/var/lib/libvirt/images/ r,
>/var/lib/libvirt/images/** r,
>/{media,mnt,opt,srv}/** r,
> +  # For virt-sandbox
> +  /run/libvirt/**/[sv]d[a-z] r
>  
>/**.img r,
> +  /**.raw r,
>/**.qcow{,2} r,
>/**.qed r,
>/**.vmdk r,

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

Re: [libvirt] [sandbox 1/6] Pass debug and verbose values to init

[Cedric Bosdonnat]

On Tue, 2017-12-05 at 16:52 +, Daniel P. Berrange wrote:
> On Tue, Dec 05, 2017 at 10:53:17AM +0100, Cédric Bosdonnat wrote:
> > libvirt-sandbox-init-common is expecting -d and -v parameters to
> > set it in debug or verbose mode... but those will never be passed
> > by the launcher program.
> 
> Hmm, libvirt-sandbox-init-{qemu,lxc} both know how to pass -d
> to init-common.  If either of the virt specific init programs
> are in debug mode (as indicated by the word "debug" in kernel
> command line or equiv), then they pass -d to init-common.

Hum, then I should surely revise that commit to remove the debug-related
code. Instead I should change the init-{lxc,qemu} to actually pass
the debug flag to init-common.

> You're right that there's no way to pass -v though.
> 
> Also, it might be useful to run init-common in debug mode
> without the virt specific init being in debug mode, to avoid
> drowning in debug.

Hum, then keeping the debug-related flag I introduced could help,
but then I'll need to rename it to make it more explicit. Or maybe
I should change the debug parameter to take a level value:
 0: all (default)
 1: only the init-common

In which case the user-exposed options should be expanded to use these.

I'll push the other commits, but not this one.

--
Cedric

> > 
> > Writing the core.debug and core.verbose parameters in the sandbox
> > configuration makes those values actually usable from the init.
> > ---
> >  bin/virt-sandbox.c|  3 ++
> >  libvirt-sandbox/libvirt-sandbox-config.c  | 75 
> > +++
> >  libvirt-sandbox/libvirt-sandbox-config.h  |  6 +++
> >  libvirt-sandbox/libvirt-sandbox-init-common.c |  3 ++
> >  libvirt-sandbox/libvirt-sandbox.sym   |  4 ++
> >  5 files changed, 91 insertions(+)
> 
> Reviewed-by: Daniel P. Berrange 
> 
> if the commit message is fixed
> 
> 
> Regards,
> Daniel

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

Re: [libvirt] [sandbox 5/6] Convert to python3

[Cedric Bosdonnat]

On Tue, 2017-12-05 at 16:57 +, Daniel P. Berrange wrote:
> On Tue, Dec 05, 2017 at 10:53:21AM +0100, Cédric Bosdonnat wrote:
> > Python2 is going to die soon, convert to python3.
> 
> I'm unclear whether this change drops py2 support, or whether it makes it
> work with py2+3 in parallel.
> 
> The commit message suggests py3 only, but then this:
> 
> > @@ -418,6 +416,18 @@ class GenericContainer(Container):
> >  def is_template_unit(unit):
> >  return '@' in unit
> >  
> > +# Python 2 / 3 compability helpers
> > +def get_next(obj):
> > +if hasattr(obj, 'next'):
> > +return obj.next()
> > +else:
> > +return next(obj)
> > +
> > +def string(obj):
> > +if isinstance(obj, bytes):
> > +return str(obj, encoding='utf-8')
> > +return obj
> 
> 
> suggests py2+3 in parallel
> 
> I don't mind being py3 only.
> 
Oops, that one is a left over. I'll remove it and drop python2.
And at least the commit message will be in sync with the code

--
Cedric

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

Re: [libvirt] [PATCH] apparmor: add network netlink raw rule

[Cedric Bosdonnat]

On Thu, 2017-11-09 at 09:43 -0700, Jim Fehlig wrote:
> On 11/09/2017 09:24 AM, Cédric Bosdonnat wrote:
> > The rule 'network netlink raw' fixes these denials on libvirtd start:
> > 
> > apparmor="DENIED" operation="create" profile="/usr/sbin/libvirtd" pid=12969
> > comm="libvirtd" family="netlink" sock_type="raw" protocol=0
> > requested_mask="create" denied_mask="create"
> > ---
> >   examples/apparmor/usr.sbin.libvirtd | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/examples/apparmor/usr.sbin.libvirtd 
> > b/examples/apparmor/usr.sbin.libvirtd
> > index 819068ffc..8ac5233cc 100644
> > --- a/examples/apparmor/usr.sbin.libvirtd
> > +++ b/examples/apparmor/usr.sbin.libvirtd
> > @@ -36,6 +36,7 @@
> > network inet6 dgram,
> > network packet dgram,
> > network packet raw,
> > +  network netlink raw,
> 
> This is already included in intrigeri's patchset to fix other apparmor rules
> 
> https://www.redhat.com/archives/libvir-list/2017-November/msg00161.html

Oops, I was too quick, sorry for the noise.

--
Cedric

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

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[Cedric Bosdonnat]

On Tue, 2017-11-07 at 08:43 -0500, John Ferlan wrote:
> 
> On 11/07/2017 04:18 AM, Cedric Bosdonnat wrote:
> > On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> > > Resolve a storage driver crash as a result of a long running
> > > storageVolCreateXML when the virStorageVolPoolRefreshThread is
> > > run as a result of a storageVolUpload complete and ran the
> > > virStoragePoolObjClearVols without checking if the creation
> > > code was currently processing a buildVol after incrementing
> > > the driver->asyncjob count.
> > > 
> > > The refreshThread needs to wait until all creation threads
> > > are completed so as to not alter the volume list while the
> > > volume is being built.
> > > 
> > > Crash from valgrind is as follows (with a bit of editing):
> > > 
> > > ==21309== Invalid read of size 8
> > > ==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
> > > ==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
> > > ==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
> > > ==21309==by 0x153DE29E: storageVolCreateXML
> > > ==21309==by 0x562035B: virStorageVolCreateXML
> > > ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> > > ...
> > > ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 
> > > free'd
> > > ==21309==at 0x4C2F2BB: free
> > > ==21309==by 0x54CB9FA: virFree
> > > ==21309==by 0x55BC800: virStorageVolDefFree
> > > ==21309==by 0x55BF1D8: virStoragePoolObjClearVols
> > > ==21309==by 0x153D967E: virStorageVolPoolRefreshThread
> > > ...
> > > ==21309==  Block was alloc'd at
> > > ==21309==at 0x4C300A5: calloc
> > > ==21309==by 0x54CB483: virAlloc
> > > ==21309==by 0x55BDC1F: virStorageVolDefParseXML
> > > ==21309==by 0x55BDC1F: virStorageVolDefParseNode
> > > ==21309==by 0x55BE5A4: virStorageVolDefParse
> > > ==21309==by 0x153DDFF1: storageVolCreateXML
> > > ==21309==by 0x562035B: virStorageVolCreateXML
> > > ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> > > ...
> > > 
> > > Signed-off-by: John Ferlan <jfer...@redhat.com>
> > > ---
> > >  src/storage/storage_driver.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> > > index b0edf9f885..5e920fc14c 100644
> > > --- a/src/storage/storage_driver.c
> > > +++ b/src/storage/storage_driver.c
> > > @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
> > >  virStorageBackendPtr backend;
> > >  virObjectEventPtr event = NULL;
> > >  
> > > + retry:
> > >  storageDriverLock();
> > >  if (cbdata->vol_path) {
> > >  if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> > > @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
> > >  if (!(backend = virStorageBackendForType(def->type)))
> > >  goto cleanup;
> > >  
> > > +/* Some thread is creating a new volume in the pool, we need to 
> > > retry */
> > > +if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> > > +virStoragePoolObjUnlock(obj);
> > > +storageDriverUnlock();
> > > +usleep(100 * 1000);
> > > +goto retry;
> > > +}
> > > +
> > >  virStoragePoolObjClearVols(obj);
> > >  if (backend->refreshPool(NULL, obj) < 0)
> > >  VIR_DEBUG("Failed to refresh storage pool");
> > 
> > ACK, does the job here. I'm rather surprised to see you implementing it
> > with sleep, while you pointed me towards virCondWait yesterday. But using
> > sleep is a way less intrusive change.
> > 
> 
> Well you only asked about an alternative mechanism and condition
> variable was the first thing that popped into my mind; however, as I got
> to actually doing more thinking about it - asyncjobs is not blocking
> multiple creates from occurring; whereas, a condition variable would be
> waiting for one thing to complete.
> 
> My response to Jan also lists 2 other alternatives. This was just the
> "easiest" of the 3.  If there's other ideas, I'm open to suggestions.
> 
It looks fine to me, I was just surprised to see the sleep version ;)

--
Cedric

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

Re: [libvirt] [PATCH 2/2] scsi: Check for long running create in FCRefreshThread

[Cedric Bosdonnat]

On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> Similar to a recent patch in virStorageVolPoolRefreshThread to ensure
> that there were no pool AsyncJobs (e.g. nothing being created at the
> time in a long running buildVol job), modify virStoragePoolFCRefreshThread
> to check for async jobs before calling virStoragePoolObjClearVols and
> refreshing the volumes defined in the pool.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_scsi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/storage/storage_backend_scsi.c 
> b/src/storage/storage_backend_scsi.c
> index 02fd4b643c..63a9154102 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -159,6 +159,7 @@ virStoragePoolFCRefreshThread(void *opaque)
>  pool->def->allocation = pool->def->capacity = pool->def->available = 
> 0;
>  
>  if (virStoragePoolObjIsActive(pool) &&
> +virStoragePoolObjGetAsyncjobs(pool) == 0 &&
>  virSCSIHostGetNumber(fchost_name, ) == 0 &&
>  virStorageBackendSCSITriggerRescan(host) == 0) {
>  virStoragePoolObjClearVols(pool);

ACK
--
Cedric

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

Re: [libvirt] [PATCH 1/2] storage: Resolve storage driver crash

[Cedric Bosdonnat]

On Mon, 2017-11-06 at 15:53 -0500, John Ferlan wrote:
> Resolve a storage driver crash as a result of a long running
> storageVolCreateXML when the virStorageVolPoolRefreshThread is
> run as a result of a storageVolUpload complete and ran the
> virStoragePoolObjClearVols without checking if the creation
> code was currently processing a buildVol after incrementing
> the driver->asyncjob count.
> 
> The refreshThread needs to wait until all creation threads
> are completed so as to not alter the volume list while the
> volume is being built.
> 
> Crash from valgrind is as follows (with a bit of editing):
> 
> ==21309== Invalid read of size 8
> ==21309==at 0x153E47AF: storageBackendUpdateVolTargetInfo
> ==21309==by 0x153E4C30: virStorageBackendUpdateVolInfo
> ==21309==by 0x153E52DE: virStorageBackendVolRefreshLocal
> ==21309==by 0x153DE29E: storageVolCreateXML
> ==21309==by 0x562035B: virStorageVolCreateXML
> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> ==21309==  Address 0x2590a720 is 64 bytes inside a block of size 336 free'd
> ==21309==at 0x4C2F2BB: free
> ==21309==by 0x54CB9FA: virFree
> ==21309==by 0x55BC800: virStorageVolDefFree
> ==21309==by 0x55BF1D8: virStoragePoolObjClearVols
> ==21309==by 0x153D967E: virStorageVolPoolRefreshThread
> ...
> ==21309==  Block was alloc'd at
> ==21309==at 0x4C300A5: calloc
> ==21309==by 0x54CB483: virAlloc
> ==21309==by 0x55BDC1F: virStorageVolDefParseXML
> ==21309==by 0x55BDC1F: virStorageVolDefParseNode
> ==21309==by 0x55BE5A4: virStorageVolDefParse
> ==21309==by 0x153DDFF1: storageVolCreateXML
> ==21309==by 0x562035B: virStorageVolCreateXML
> ==21309==by 0x147366: remoteDispatchStorageVolCreateXML
> ...
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_driver.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index b0edf9f885..5e920fc14c 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2257,6 +2257,7 @@ virStorageVolPoolRefreshThread(void *opaque)
>  virStorageBackendPtr backend;
>  virObjectEventPtr event = NULL;
>  
> + retry:
>  storageDriverLock();
>  if (cbdata->vol_path) {
>  if (virStorageBackendPloopRestoreDesc(cbdata->vol_path) < 0)
> @@ -2270,6 +2271,14 @@ virStorageVolPoolRefreshThread(void *opaque)
>  if (!(backend = virStorageBackendForType(def->type)))
>  goto cleanup;
>  
> +/* Some thread is creating a new volume in the pool, we need to retry */
> +if (virStoragePoolObjGetAsyncjobs(obj) > 0) {
> +virStoragePoolObjUnlock(obj);
> +storageDriverUnlock();
> +usleep(100 * 1000);
> +goto retry;
> +}
> +
>  virStoragePoolObjClearVols(obj);
>  if (backend->refreshPool(NULL, obj) < 0)
>  VIR_DEBUG("Failed to refresh storage pool");

ACK, does the job here. I'm rather surprised to see you implementing it
with sleep, while you pointed me towards virCondWait yesterday. But using
sleep is a way less intrusive change.

--
Cedric

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

Re: [libvirt] storage lock issues

[Cedric Bosdonnat]

On Thu, 2017-11-02 at 07:03 -0400, John Ferlan wrote:
> So how to fix - well seems to me the storageDriverLock in VolCreateXML
> may be the way since the refresh thread takes the driver lock first,
> then the pool object second.  Perhaps even like the build code where it
> takes it temporarily while getting the pool object. I'd have to think a
> bit more about though. Still might be something to try since the Vol
> Refresh thread takes it while running...

This is surely a bad hack, but this is fixing the problem I'm seeing.
Wouldn't the VolCreateXML function taking the lock for a too long time
and thus lead to other troubles?

 %< 
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 18d630319..f5a1e7bc2 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1825,6 +1825,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
 if (backend->createVol(obj->conn, pool, voldef) < 0)
 goto cleanup;
 
+storageDriverLock();
 pool->volumes.objs[pool->volumes.count++] = voldef;
 volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name,
   voldef->key, NULL, NULL);
@@ -1858,9 +1859,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
 
 VIR_FREE(buildvoldef);
 
-storageDriverLock();
 virStoragePoolObjLock(pool);
-storageDriverUnlock();
 
 voldef->building = false;
 pool->asyncjobs--;
@@ -1897,6 +1896,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
 voldef = NULL;
 
  cleanup:
+storageDriverUnlock();
 virObjectUnref(volobj);
 virStorageVolDefFree(voldef);
 if (pool)
 %< 

--
Cedric

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

Re: [libvirt] storage lock issues

[Cedric Bosdonnat]

Hi John,

On Thu, 2017-11-02 at 07:03 -0400, John Ferlan wrote:
> 
> On 11/02/2017 06:20 AM, Cedric Bosdonnat wrote:
> > Hi all,
> > 
> > I'm investigating a crash in the storage driver, and it seems the solution
> > is not as obvious as I wanted it.
> > 
> > Basically what happens is that libvirtd crashes due to the pool refresh 
> > thread
> > clearing the list of pool volumes while someone is adding a new volume. Here
> > is the valgrind log I have for that:
> > 
> > http://paste.opensuse.org/58733866
> > 
> 
> What version of libvirt?  Your line numbers don't match up with current
> HEAD.

It's a 3.8.0.

> > I tried adding a call to virStoragePoolObjLock() in the
> > virStorageVolPoolRefreshThread(), since the storageVolCreateXML()
> > appears to have it, but I'm getting a dead lock. Here are the waiting
> > threads I got from gdb:
> 
> virStoragePoolObjFindByName returns a locked pool obj, so not sure
> where/why you added this...

Oh I overlooked that... then there is already a lock on it, the solution
is elsewhere

> > 
> > http://paste.opensuse.org/45961070
> > 
> > Any idea what would be a proper fix for that? My vision of the storage
> > drivers locks is too limited it seems ;)
> 
> From just a quick look...
> 
> The virStorageVolPoolRefreshThread will take storageDriverLock, get a
> locked pool object (virStoragePoolObjFindByName), clear out the pool,
> add back volumes that it knows about, unlock the pool object, and call
> storageDriverUnlock.
> 
> If you were adding a new pool... You'd take storageDriverLock, then
> eventually virStoragePoolObjAssignDef would be called and the pool's
> object lock taken and unlocked, and returns a locked pool object which
> gets later unlocked in cleanup, followd by a storageDriverUnlock.
> 
> If you're adding a new volume object, you'd get a locked pool object via
> virStoragePoolObjFromStoragePool, then if building, that lock gets
> dropped after increasing the async job count and setting the building
> flag, the volume is built, then the object lock retaken while
> temporarily holding the storageDriverLock, the async job count gets
> decremented and the building flag cleared, eventually we fall into
> cleanup with unlocks the pool again.

Thanks for the details: this is really useful to globally understand how
locks are working in the storage driver. Maybe worth turning it into a doc
somewhere?

> So how to fix - well seems to me the storageDriverLock in VolCreateXML
> may be the way since the refresh thread takes the driver lock first,
> then the pool object second.  Perhaps even like the build code where it
> takes it temporarily while getting the pool object. I'd have to think a
> bit more about though. Still might be something to try since the Vol
> Refresh thread takes it while running...

Ok, I'll look into that direction.

> John
> 
> Not related to this problem per se, but what may help even more is if I
> can get the storage driver usage of a common object model patches
> completely reviewed, but that's a different problem ;-)... I'll have to
> go look and see if I may have fixed this there. The newer model uses
> hash tables, RW locks, and reduces the storage driver hammer lock, but
> this one condition may not have been addressed.

I can have a go at it and see if I can reproduce the crash with it. Not sure
I'll be the one skilled enough for the full review though ;)

--
Cedric

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

[libvirt] storage lock issues

[Cedric Bosdonnat]

Hi all,

I'm investigating a crash in the storage driver, and it seems the solution
is not as obvious as I wanted it.

Basically what happens is that libvirtd crashes due to the pool refresh thread
clearing the list of pool volumes while someone is adding a new volume. Here
is the valgrind log I have for that:

http://paste.opensuse.org/58733866

I tried adding a call to virStoragePoolObjLock() in the
virStorageVolPoolRefreshThread(), since the storageVolCreateXML()
appears to have it, but I'm getting a dead lock. Here are the waiting
threads I got from gdb:

http://paste.opensuse.org/45961070

Any idea what would be a proper fix for that? My vision of the storage
drivers locks is too limited it seems ;)

--
Cedric

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

Re: [libvirt] [PATCH] virt-host-validate: require fuse for LXC if compiled in

[Cedric Bosdonnat]

>>> Guido Günther  10/12/17 6:12 PM >>>
Domains fail to start without fuse like

  error: internal error: guest failed to start: fuse: device not found, try 
'modprobe fuse' first
  Failure in libvirt_lxc startup: no error

so check for it too.

References: 
https://ci.debian.net/data/autopkgtest/unstable/amd64/libv/libvirt/20171012_105903/log.gz
---
 tools/virt-host-validate-lxc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tools/virt-host-validate-lxc.c b/tools/virt-host-validate-lxc.c
index 2b906cc88a..64d9279c30 100644
--- a/tools/virt-host-validate-lxc.c
+++ b/tools/virt-host-validate-lxc.c
@@ -93,5 +93,12 @@ int virHostValidateLXC(void)
 "BLK_CGROUP") < 0)
 ret = -1;
 
+#if WITH_FUSE
+if (virHostValidateDeviceExists("LXC", "/sys/fs/fuse/connections",
+VIR_HOST_VALIDATE_FAIL,
+_("Load the 'fuse' module to enable /proc/ 
overrides")) < 0)
+ret = -1;
+#endif
+
 return ret;
 }

ACK

--
Cedric

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

Re: [libvirt] [RFC PATCH 3/4] lxc: Fixed a typo

[Cedric Bosdonnat]

On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/lxc/lxc_container.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 1f220c602b0a..5791a9b1f958 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2167,7 +2167,7 @@ static int lxcContainerSetUserGroup(virCommandPtr cmd,
>   * This function is run in the process clone()'d in lxcStartContainer.
>   * Perform a number of container setup tasks:
>   * Setup container file system
> - * mount container /proca
> + * mount container /proc
>   * Then exec's the container init
>   *
>   * Returns 0 on success or -1 in case of error

ACK
--
Cedric

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

Re: [libvirt] [RFC PATCH 1/4] util: Add virCommandGetGID and virCommandGetUID

[Cedric Bosdonnat]

On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
> These functions are used by an upcoming commit.
> 
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/libvirt_private.syms |  2 ++
>  src/util/vircommand.c| 14 ++
>  src/util/vircommand.h|  4 
>  3 files changed, 20 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9243c5591042..26c5ddb40505 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1506,6 +1506,8 @@ virCommandDaemonize;
>  virCommandDoAsyncIO;
>  virCommandExec;
>  virCommandFree;
> +virCommandGetGID;
> +virCommandGetUID;
>  virCommandHandshakeNotify;
>  virCommandHandshakeWait;
>  virCommandNew;
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 60c1121dafea..fba73ca18eac 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -1073,6 +1073,20 @@ virCommandSetPidFile(virCommandPtr cmd, const char 
> *pidfile)
>  }
>  
>  
> +gid_t
> +virCommandGetGID(virCommandPtr cmd)
> +{
> +return cmd->gid;
> +}
> +
> +
> +uid_t
> +virCommandGetUID(virCommandPtr cmd)
> +{
> +return cmd->uid;
> +}
> +
> +
>  void
>  virCommandSetGID(virCommandPtr cmd, gid_t gid)
>  {
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index e7c2e513bae1..b401d7b238d7 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -68,6 +68,10 @@ int virCommandPassFDGetFDIndex(virCommandPtr cmd,
>  void virCommandSetPidFile(virCommandPtr cmd,
>    const char *pidfile) ATTRIBUTE_NONNULL(2);
>  
> +gid_t virCommandGetGID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1);
> +
> +uid_t virCommandGetUID(virCommandPtr cmd) ATTRIBUTE_NONNULL(1);
> +
>  void virCommandSetGID(virCommandPtr cmd, gid_t gid);
>  
>  void virCommandSetUID(virCommandPtr cmd, uid_t uid);

ACK.
I guess the commit using those is still to come, right?
--
Cedric

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

Re: [libvirt] [RFC PATCH 4/4] lxc: Fixed indentation

[Cedric Bosdonnat]

On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
> Signed-off-by: Marc Hartmayer 
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/lxc/lxc_container.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 5791a9b1f958..b7216d6ee863 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2254,8 +2254,8 @@ static int lxcContainerChild(void *data)
>  
>  if (!virFileExists(vmDef->os.init)) {
>  virReportSystemError(errno,
> -_("cannot find init path '%s' relative to container 
> root"),
> -vmDef->os.init);
> + _("cannot find init path '%s' relative to 
> container root"),
> + vmDef->os.init);
>  goto cleanup;
>  }
>  
> @@ -2275,7 +2275,7 @@ static int lxcContainerChild(void *data)
>  
>  if (lxcContainerSendContinue(argv->handshakefd) < 0) {
>  virReportSystemError(errno, "%s",
> -_("Failed to send continue signal to 
> controller"));
> + _("Failed to send continue signal to 
> controller"));
>  goto cleanup;
>  }
>  

ACK
--
Cedric

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

Re: [libvirt] [RFC PATCH 2/4] util: Fix deadlock across fork()

[Cedric Bosdonnat]

The commit looks good to me, but I'ld rather have Dan have a look at it.
I'm not expert enough to tell what's safe and what is not.

--
Cedric

On Mon, 2017-10-09 at 21:14 +0200, Marc Hartmayer wrote:
> This commit fixes the deadlock introduced by commit
> 0980764dee687e8da86dc410c351759867163389. The call getgrouplist() of
> the glibc library isn't safe to be called in between fork and
> exec (see commit 75c125641ac73473ba4b0542524d67a184769c8e).
> 
> Signed-off-by: Marc Hartmayer 
> Fixes: 0980764dee68 ("util: share code between virExec and virCommandExec")
> Reviewed-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  src/lxc/lxc_container.c | 12 +++-
>  src/util/vircommand.c   | 25 ++---
>  src/util/vircommand.h   |  2 +-
>  tests/commandtest.c | 15 ++-
>  4 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index ec6d6a86b0b6..1f220c602b0a 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -2182,6 +2182,8 @@ static int lxcContainerChild(void *data)
>  virDomainFSDefPtr root;
>  virCommandPtr cmd = NULL;
>  int hasReboot;
> +gid_t *groups = NULL;
> +int ngroups;
>  
>  if (NULL == vmDef) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2297,6 +2299,13 @@ static int lxcContainerChild(void *data)
>  goto cleanup;
>  }
>  
> +/* TODO is it safe to call it here or should this call be moved in
> + * front of the clone() as otherwise there might be a risk for a
> + * deadlock */
> +if ((ngroups = virGetGroupList(virCommandGetUID(cmd), 
> virCommandGetGID(cmd),
> +   )) < 0)
> +goto cleanup;
> +
>  ret = 0;
>   cleanup:
>  VIR_FREE(ttyPath);
> @@ -2307,7 +2316,7 @@ static int lxcContainerChild(void *data)
>  if (ret == 0) {
>  VIR_DEBUG("Executing init binary");
>  /* this function will only return if an error occurred */
> -ret = virCommandExec(cmd);
> +ret = virCommandExec(cmd, groups, ngroups);
>  }
>  
>  if (ret != 0) {
> @@ -2317,6 +2326,7 @@ static int lxcContainerChild(void *data)
>  virGetLastErrorMessage());
>  }
>  
> +VIR_FREE(groups);
>  virCommandFree(cmd);
>  return ret;
>  }
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index fba73ca18eac..41a61da49f82 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -465,15 +465,10 @@ virCommandHandshakeChild(virCommandPtr cmd)
>  }
>  
>  static int
> -virExecCommon(virCommandPtr cmd)
> +virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups)
>  {
> -gid_t *groups = NULL;
> -int ngroups;
>  int ret = -1;
>  
> -if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, )) < 0)
> -goto cleanup;
> -
>  if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
>  cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
>  VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> @@ -495,7 +490,6 @@ virExecCommon(virCommandPtr cmd)
>  ret = 0;
>  
>   cleanup:
> -VIR_FREE(groups);
>  return ret;
>  }
>  
> @@ -519,6 +513,8 @@ virExec(virCommandPtr cmd)
>  const char *binary = NULL;
>  int ret;
>  struct sigaction waxon, waxoff;
> +gid_t *groups = NULL;
> +int ngroups;
>  
>  if (cmd->args[0][0] != '/') {
>  if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) {
> @@ -589,6 +585,9 @@ virExec(virCommandPtr cmd)
>  childerr = null;
>  }
>  
> +if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, )) < 0)
> +goto cleanup;
> +
>  pid = virFork();
>  
>  if (pid < 0)
> @@ -756,7 +755,7 @@ virExec(virCommandPtr cmd)
>  }
>  # endif
>  
> -if (virExecCommon(cmd) < 0)
> +if (virExecCommon(cmd, groups, ngroups) < 0)
>  goto fork_error;
>  
>  if (virCommandHandshakeChild(cmd) < 0)
> @@ -799,6 +798,7 @@ virExec(virCommandPtr cmd)
> should never jump here on error */
>  
>  VIR_FREE(binarystr);
> +VIR_FREE(groups);
>  
>  /* NB we don't virReportError() on any failures here
> because the code which jumped here already raised
> @@ -2167,6 +2167,8 @@ virCommandProcessIO(virCommandPtr cmd)
>  /**
>   * virCommandExec:
>   * @cmd: command to run
> + * @groups: array of supplementary group IDs used for the command
> + * @ngroups: number of group IDs in @groups
>   *
>   * Exec the command, replacing the current process. Meant to be called
>   * in the hook after already forking / cloning, so does not attempt to
> @@ -2176,7 +2178,7 @@ virCommandProcessIO(virCommandPtr cmd)
>   * Will not return on success.
>   */
>  #ifndef WIN32
> -int virCommandExec(virCommandPtr cmd)
> +int virCommandExec(virCommandPtr cmd, gid_t *groups, int ngroups)
>  {
>  if (!cmd 

Re: [libvirt] [PATCH] lxcStateInitialize: Don't leak driver's caps

[Cedric Bosdonnat]

On Tue, 2017-08-29 at 18:16 +0200, Michal Privoznik wrote:
> Funny thing. So when initializing LXC driver's capabilities,
> firstly the virLXCDriverGetCapabilities() is called. This creates
> new capabilities, stores them under driver->caps, ref() them and
> return them. However, the return value is ignored. Secondly, the
> function is called yet again and since we have driver->caps set,
> they are ref()-ed again an returned. So in the end, driver's
> capabilities have refcount of three when in fact they should have
> refcount of one.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/lxc/lxc_driver.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 6eb88b0ba..784edad39 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1660,7 +1660,7 @@ static int lxcStateInitialize(bool privileged,
>  if (!(lxc_driver->hostdevMgr = virHostdevManagerGetDefault()))
>  goto cleanup;
>  
> -if ((virLXCDriverGetCapabilities(lxc_driver, true)) == NULL)
> +if (!(caps = virLXCDriverGetCapabilities(lxc_driver, true)))
>  goto cleanup;
>  
>  if (!(lxc_driver->xmlopt = lxcDomainXMLConfInit()))
> @@ -1669,9 +1669,6 @@ static int lxcStateInitialize(bool privileged,
>  if (!(lxc_driver->closeCallbacks = virCloseCallbacksNew()))
>  goto cleanup;
>  
> -if (!(caps = virLXCDriverGetCapabilities(lxc_driver, false)))
> -goto cleanup;
> -
>  if (virFileMakePath(cfg->stateDir) < 0) {
>  virReportSystemError(errno,
>   _("Failed to mkdir %s"),
> @@ -1700,6 +1697,7 @@ static int lxcStateInitialize(bool privileged,
>  goto cleanup;
>  
>  virNWFilterRegisterCallbackDriver();
> +virObjectUnref(caps);
>  return 0;
>  
>   cleanup:

ACK

--
Cedric

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

Re: [libvirt] [PATCH] LXC: set the right var to NULL

[Cedric Bosdonnat]

On Thu, 2017-07-20 at 10:02 +0800, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
>    For attaching hosdev, we should set dev->data.hostdev
>    rather than dev->data.disk
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  src/lxc/lxc_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 652e9cb..6a9c528 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -4358,7 +4358,7 @@ lxcDomainAttachDeviceLive(virConnectPtr conn,
>  case VIR_DOMAIN_DEVICE_HOSTDEV:
>  ret = lxcDomainAttachDeviceHostdevLive(driver, vm, dev);
>  if (!ret)
> -dev->data.disk = NULL;
> +dev->data.hostdev = NULL;
>  break;
>  
>  default:

ACK and pushed
--
Cedric

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

Re: [libvirt] [PATCHv2 1/2] lxc: add support for docker-json Memory and VCPU conversion

[Cedric Bosdonnat]

On Tue, 2017-06-27 at 16:07 -0400, John Ferlan wrote:
> 
> On 06/27/2017 01:02 PM, Venkat Datta N H wrote:
> > Docker Memory and VCPU configuration is converted to fit for LXC container 
> > XML configuration
> > ---
> >  po/POTFILES.in |   1 +
> >  src/Makefile.am|   1 +
> >  src/lxc/lxc_docker.c   | 116 
> >  src/lxc/lxc_docker.h   |  32 +
> >  src/lxc/lxc_driver.c   |  13 +-
> >  src/lxc/lxc_native.h   |   1 +
> >  tests/Makefile.am  |   8 +-
> >  .../lxcdocker2xmldata-simple.json  |  36 +
> >  .../lxcdocker2xmldata/lxcdocker2xmldata-simple.xml |  15 +++
> >  tests/lxcdocker2xmltest.c  | 149 
> > +
> >  10 files changed, 366 insertions(+), 6 deletions(-)
> >  create mode 100644 src/lxc/lxc_docker.c
> >  create mode 100644 src/lxc/lxc_docker.h
> >  create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.json
> >  create mode 100644 tests/lxcdocker2xmldata/lxcdocker2xmldata-simple.xml
> >  create mode 100644 tests/lxcdocker2xmltest.c
> > 
> 
> Should the drvlxc.html.in page be modified too?
> 
> http://libvirt.org/drvlxc.html
> 
> To include something that indicates docker config files being read and
> what fields are taken/used.. IOW how things are mapped.
> 
> > diff --git a/po/POTFILES.in b/po/POTFILES.in
> > index 275df1f..421b32e 100644
> > --- a/po/POTFILES.in
> > +++ b/po/POTFILES.in
> > @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c
> >  src/lxc/lxc_fuse.c
> >  src/lxc/lxc_hostdev.c
> >  src/lxc/lxc_native.c
> > +src/lxc/lxc_docker.c
> >  src/lxc/lxc_process.c
> >  src/network/bridge_driver.c
> >  src/network/bridge_driver_linux.c
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index eae32dc..1341e5a 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES =
> > \
> >     lxc/lxc_process.c lxc/lxc_process.h \
> >     lxc/lxc_fuse.c lxc/lxc_fuse.h   \
> >     lxc/lxc_native.c lxc/lxc_native.h   \
> > +   lxc/lxc_docker.c lxc/lxc_docker.h \
> >     lxc/lxc_driver.c lxc/lxc_driver.h
> >  
> >  LXC_CONTROLLER_SOURCES =   \
> > diff --git a/src/lxc/lxc_docker.c b/src/lxc/lxc_docker.c
> > new file mode 100644
> > index 000..dbb2a81
> > --- /dev/null
> > +++ b/src/lxc/lxc_docker.c
> > @@ -0,0 +1,116 @@
> > +/*
> > + * lxc_docker.c: LXC native docker configuration import
> > + *
> > + * Copyright (C) 2017 Venkat Datta N H
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * .
> > + *
> > + * Author: Venkat Datta N H 
> > + */
> > +
> > +#include 
> > +
> > +#include "util/viralloc.h"
> > +#include "util/virfile.h"
> > +#include "util/virstring.h"
> > +#include "util/virconf.h"
> > +#include "util/virjson.h"
> > +#include "util/virutil.h"
> > +#include "virerror.h"
> > +#include "virlog.h"
> > +#include "conf/domain_conf.h"
> > +#include "lxc_docker.h"
> > +#include "secret_conf.h"
> > +
> > +#define VIR_FROM_THIS VIR_FROM_LXC
> > +
> > +VIR_LOG_INIT("lxc.lxc_docker");
> > +
> > +static int virLXCDockerParseCpu(virDomainDefPtr dom,
> > +virDomainXMLOptionPtr xmlopt,
> > +virJSONValuePtr prop)
> 
> Stylistically, these should be:
> 
> static int
> virLXCDockerParseCpu(virDomainDefPtr dom,
>  virDomainXMLOptionPtr xmlopt,
>  virJSONValuePtr prop)
> 
> 
> note the multiple lines for function and how the arguments are aligned.
> 
> Similar point in every definition here.
> 
> And yes, I know/see that lxc_driver.c doesn't follow that model - this
> is the newer style we like to see in general.
> 
> > +{
> > +int vcpus;
> > +
> > +if (virJSONValueObjectGetNumberInt(prop, "CpuShares", )  != 0)
> 
> s/ !=/ 
> > +return -1;
> 
> Why CpuShares and not CpuCount?  What about CpusetCpus?

After some more digging around that, maxvcpus value isn't used in the lxc 
driver.
It needs to be set to get the 

Re: [libvirt] [PATCH v3 1/4] lxc: allow defining environment variables

[Cedric Bosdonnat]

On Mon, 2017-06-26 at 16:29 +0100, Daniel P. Berrange wrote:
> On Mon, Jun 26, 2017 at 11:40:57AM +0200, Cédric Bosdonnat wrote:
> > When running an application container, setting environment variables
> > could be important.
> > 
> > The newly introduced  tag in domain configuration will allow
> > setting environment variables to the init program.
> > ---
> >  docs/formatdomain.html.in|  5 +
> >  docs/schemas/domaincommon.rng| 10 ++
> >  src/conf/domain_conf.c   | 38 
> > 
> >  src/conf/domain_conf.h   |  8 
> >  src/lxc/lxc_container.c  |  5 +
> >  tests/lxcxml2xmldata/lxc-initenv.xml | 30 
> >  tests/lxcxml2xmltest.c   |  1 +
> >  7 files changed, 97 insertions(+)
> >  create mode 100644 tests/lxcxml2xmldata/lxc-initenv.xml
> 
> Reviewed-by: Daniel P. Berrange 

Thanks. I'll push it post freeze. Did you have some time to review the
other ones of the series?

Regards,
--
Cedric

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

Re: [libvirt] [PATCH 1/2] lxc: add support for docker-json Memory and VCPU conversion

[Cedric Bosdonnat]

Quick note: next time you send a patch series, add a cover letter to it.
You can get git to automatically ask you about one by running
 
git config add format.coverletter auto

Having reviewed them off-list first, ACK from me to those changes.
Daniel, do you want to double-review them?

--
Cedric

On Fri, 2017-06-16 at 19:59 +0530, Venkat Datta N H wrote:
> Docker Memory and VCPU configuration is converted to fit for LXC container 
> XML configuration
> ---
>  po/POTFILES.in |   1 +
>  src/Makefile.am|   1 +
>  src/lxc/lxc_driver.c   |  13 ++-
>  src/lxc/lxc_native.h   |   1 +
>  src/lxc/lxc_native_docker.c| 112 ++
>  src/lxc/lxc_native_docker.h|  30 +
>  tests/Makefile.am  |   8 +-
>  .../dockerjson2xmldata-simple.json |  36 ++
>  .../dockerjson2xmldata-simple.xml  |  15 +++
>  tests/dockerjson2xmltest.c | 127 
> +
>  10 files changed, 338 insertions(+), 6 deletions(-)
>  create mode 100644 src/lxc/lxc_native_docker.c
>  create mode 100644 src/lxc/lxc_native_docker.h
>  create mode 100644 tests/dockerjson2xmldata/dockerjson2xmldata-simple.json
>  create mode 100644 tests/dockerjson2xmldata/dockerjson2xmldata-simple.xml
>  create mode 100644 tests/dockerjson2xmltest.c
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 275df1f..098be9f 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -111,6 +111,7 @@ src/lxc/lxc_driver.c
>  src/lxc/lxc_fuse.c
>  src/lxc/lxc_hostdev.c
>  src/lxc/lxc_native.c
> +src/lxc/lxc_native_docker.c
>  src/lxc/lxc_process.c
>  src/network/bridge_driver.c
>  src/network/bridge_driver_linux.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index eae32dc..53d1bca 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -839,6 +839,7 @@ LXC_DRIVER_SOURCES =  
> \
>   lxc/lxc_process.c lxc/lxc_process.h \
>   lxc/lxc_fuse.c lxc/lxc_fuse.h   \
>   lxc/lxc_native.c lxc/lxc_native.h   \
> + lxc/lxc_native_docker.c lxc/lxc_native_docker.h \
>   lxc/lxc_driver.c lxc/lxc_driver.h
>  
>  LXC_CONTROLLER_SOURCES = \
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 22c8b58..0f5a7870 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -53,6 +53,7 @@
>  #include "lxc_driver.h"
>  #include "lxc_native.h"
>  #include "lxc_process.h"
> +#include "lxc_native_docker.h"
>  #include "viralloc.h"
>  #include "virnetdevbridge.h"
>  #include "virnetdevveth.h"
> @@ -1062,15 +1063,17 @@ static char 
> *lxcConnectDomainXMLFromNative(virConnectPtr conn,
>  if (virConnectDomainXMLFromNativeEnsureACL(conn) < 0)
>  goto cleanup;
>  
> -if (STRNEQ(nativeFormat, LXC_CONFIG_FORMAT)) {
> +if (STREQ(nativeFormat, DOCKER_CONFIG_FORMAT)) {
> +if (!(def = dockerParseJSONConfig(caps, driver->xmlopt, 
> nativeConfig)))
> +goto cleanup;
> +} else if (STREQ(nativeFormat, LXC_CONFIG_FORMAT)) {
> +if (!(def = lxcParseConfigString(nativeConfig, caps, 
> driver->xmlopt)))
> +goto cleanup;
> +} else {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("unsupported config type %s"), nativeFormat);
>  goto cleanup;
>  }
> -
> -if (!(def = lxcParseConfigString(nativeConfig, caps, driver->xmlopt)))
> -goto cleanup;
> -
>  xml = virDomainDefFormat(def, caps, 0);
>  
>   cleanup:
> diff --git a/src/lxc/lxc_native.h b/src/lxc/lxc_native.h
> index 15fa0d5..88263ae 100644
> --- a/src/lxc/lxc_native.h
> +++ b/src/lxc/lxc_native.h
> @@ -26,6 +26,7 @@
>  # include "domain_conf.h"
>  
>  # define LXC_CONFIG_FORMAT "lxc-tools"
> +# define DOCKER_CONFIG_FORMAT "docker"
>  
>  virDomainDefPtr lxcParseConfigString(const char *config,
>   virCapsPtr caps,
> diff --git a/src/lxc/lxc_native_docker.c b/src/lxc/lxc_native_docker.c
> new file mode 100644
> index 000..a278309
> --- /dev/null
> +++ b/src/lxc/lxc_native_docker.c
> @@ -0,0 +1,112 @@
> +/*
> + * lxc_native_docker.c: LXC native docker configuration import
> + *
> + * Copyright (C) 2017 Venkat Datta N H
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * 

Re: [libvirt] [PATCH] lxc: allow defining environment variables

[Cedric Bosdonnat]

On Mon, 2017-06-05 at 15:23 +0100, Daniel P. Berrange wrote:
> On Tue, May 30, 2017 at 09:32:43PM +0200, Cédric Bosdonnat wrote:
> > When running an application container, setting environment variables
> > could be important.
> > 
> > The newly introduced  tag in domain configuration will allow
> > setting environment variables to the init program.
> > ---
> >  docs/formatdomain.html.in|  5 +
> >  docs/schemas/domaincommon.rng| 10 ++
> >  src/conf/domain_conf.c   | 38 
> > 
> >  src/conf/domain_conf.h   |  8 
> >  src/lxc/lxc_container.c  |  5 +
> >  tests/lxcxml2xmldata/lxc-initenv.xml | 30 
> >  tests/lxcxml2xmltest.c   |  1 +
> >  7 files changed, 97 insertions(+)
> >  create mode 100644 tests/lxcxml2xmldata/lxc-initenv.xml
> 
> Generally looks fine, but do we need to wire this up for  native-to-xml
> conversion too ?

lxc config file doesn't have anything related to the command to run, it's env
variables and PWD. These will need to be added in docker's json native-to-xml 
though.

--
Cedric

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

Re: [libvirt] [PATCH 2/2] lxc: allow user to specify command working directory

[Cedric Bosdonnat]

On Mon, 2017-06-05 at 15:27 +0100, Daniel P. Berrange wrote:
> On Thu, Jun 01, 2017 at 02:26:17PM +0200, Cédric Bosdonnat wrote:
> > Some containers may want the application to run in a special directory.
> > Add  element in the domain configuration to handle this case
> > and use it in the lxc driver.
> > ---
> >  docs/formatdomain.html.in|  5 +
> >  docs/schemas/domaincommon.rng|  5 +
> >  src/conf/domain_conf.c   |  5 +
> >  src/conf/domain_conf.h   |  1 +
> >  src/lxc/lxc_container.c  |  4 +++-
> >  tests/lxcxml2xmldata/lxc-initdir.xml | 30 ++
> >  tests/lxcxml2xmltest.c   |  1 +
> >  7 files changed, 50 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/lxcxml2xmldata/lxc-initdir.xml
> 
> Any need for the native-to-xml conversion here too ?

Will be needed once we'll have docker json support in native-o-xml.
lxc config file doesn't have it AFAICT.

> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 03153b972..105f0b7a6 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1841,6 +1841,7 @@ struct _virDomainOSDef {
> >  char *init;
> >  char **initargv;
> >  virDomainOSEnvPtr *initenv;
> > +char *initdir;
> >  char *kernel;
> >  char *initrd;
> >  char *cmdline;
> > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> > index ffafc39d7..c122a588e 100644
> > --- a/src/lxc/lxc_container.c
> > +++ b/src/lxc/lxc_container.c
> > @@ -237,7 +237,7 @@ static virCommandPtr 
> > lxcContainerBuildInitCmd(virDomainDefPtr vmDef,
> >  virCommandAddEnvString(cmd, "PATH=/bin:/sbin");
> >  virCommandAddEnvString(cmd, "TERM=linux");
> >  virCommandAddEnvString(cmd, "container=lxc-libvirt");
> > -virCommandAddEnvString(cmd, "HOME=/");
> > +/*virCommandAddEnvString(cmd, "HOME=/"); */

Oops, looks like I forgot some cleanup

> >  virCommandAddEnvPair(cmd, "container_uuid", uuidstr);
> >  if (nttyPaths > 1)
> >  virCommandAddEnvPair(cmd, "container_ttys", 
> > virBufferCurrentContent());
> 
> This doesn't work obviously.

Hum... weird. I'll recheck my source tree here and resubmit properly

--
Cedric

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

Re: [libvirt] [libvirt-sandbox PATCH] docker: Don't ignore qemu-img errors

[Cedric Bosdonnat]

On Sat, 2017-05-27 at 18:30 +0200, Guido Günther wrote:
> ---
>  libvirt-sandbox/image/sources/docker.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libvirt-sandbox/image/sources/docker.py 
> b/libvirt-sandbox/image/sources/docker.py
> index 43e9c32..aa5675e 100755
> --- a/libvirt-sandbox/image/sources/docker.py
> +++ b/libvirt-sandbox/image/sources/docker.py
> @@ -662,7 +662,7 @@ class DockerSource(base.Source):
>  cmd.append("-o")
>  cmd.append("backing_fmt=qcow2,backing_file=%s" % diskfile)
>  cmd.append(tempfile)
> -subprocess.call(cmd)
> +subprocess.check_call(cmd)
>  return tempfile
>  
>  def get_command(self, template, templatedir, userargs):

ACK

--
Cedric

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

Re: [libvirt] [libvirt-sandbox PATCH] mkinitrd: Add missing fscrypto module

[Cedric Bosdonnat]

On Sat, 2017-05-27 at 13:04 +0200, Guido Günther wrote:
> ---
>  libvirt-sandbox/libvirt-sandbox-builder-machine.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libvirt-sandbox/libvirt-sandbox-builder-machine.c 
> b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> index bdec490..7204f71 100644
> --- a/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> +++ b/libvirt-sandbox/libvirt-sandbox-builder-machine.c
> @@ -186,6 +186,7 @@ static gchar 
> *gvir_sandbox_builder_machine_mkinitrd(GVirSandboxConfig *config,
>  
>  /* In case ext4 is built as a module, include it and its deps
>   * for the root mount */
> +gvir_sandbox_config_initrd_add_module(initrd, "fscrypto.ko");
>  gvir_sandbox_config_initrd_add_module(initrd, "mbcache.ko");
>  gvir_sandbox_config_initrd_add_module(initrd, "jbd2.ko");
>  gvir_sandbox_config_initrd_add_module(initrd, "crc16.ko");

ACK

--
Cedric

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

Re: [libvirt] [PATCH V2] libxl: add default controllers for USB devices

[Cedric Bosdonnat]

On Fri, 2017-05-05 at 15:48 -0600, Jim Fehlig wrote:
> Attempting to start a domain with USB hostdevs but no USB controllers
> fails with the rather cryptic error
> 
> libxl: error: libxl_qmp.c:287:qmp_handle_error_response: received an
> error message from QMP server: Bus 'xenusb-0.0' not found
> 
> This can be fixed by creating default USB controllers. When no USB
> controllers are defined, create the number of 8 port controllers
> necessary to accommodate the number of defined USB devices.
> 
> Note that USB controllers are already created as needed in the
> domainAttachDevice code path. E.g. a USB controller will be created,
> if necessary, when attaching a USB device with
> 'virsh attach-device dom usbdev.xml'.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> V1 here
> 
> https://www.redhat.com/archives/libvir-list/2017-April/msg00965.html
> 
> While further testing of V1 found a libvirtd segfault due to
> incorrectly using virDomainControllerInsertPreAlloced instead of
> virDomainControllerInsert.
> 
>  src/libxl/libxl_conf.c | 82 
> +++---
>  1 file changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 56bc09719..cdf6ec9f3 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1822,34 +1822,94 @@ libxlMakeUSBController(virDomainControllerDefPtr 
> controller,
>  }
>  
>  static int
> +libxlMakeDefaultUSBControllers(virDomainDefPtr def,
> +   libxl_domain_config *d_config)
> +{
> +virDomainControllerDefPtr l_controller = NULL;
> +libxl_device_usbctrl *x_controllers = NULL;
> +size_t nusbdevs = 0;
> +size_t ncontrollers;
> +size_t i;
> +
> +for (i = 0; i < def->nhostdevs; i++) {
> +if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> +def->hostdevs[i]->source.subsys.type == 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> +nusbdevs++;
> +}
> +
> +/* No controllers needed if there are no USB devs */
> +if (nusbdevs == 0)
> +return 0;
> +
> +/* Create USB controllers with 8 ports */
> +ncontrollers = VIR_DIV_UP(nusbdevs, 8);
> +if (VIR_ALLOC_N(x_controllers, ncontrollers) < 0)
> +return -1;
> +
> +for (i = 0; i < ncontrollers; i++) {
> +if (!(l_controller = 
> virDomainControllerDefNew(VIR_DOMAIN_CONTROLLER_TYPE_USB)))
> +goto error;
> +
> +l_controller->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QUSB2;
> +l_controller->idx = i;
> +l_controller->opts.usbopts.ports = 8;
> +
> +libxl_device_usbctrl_init(_controllers[i]);
> +
> +if (libxlMakeUSBController(l_controller, _controllers[i]) < 0)
> +goto error;
> +
> +if (virDomainControllerInsert(def, l_controller) < 0)
> +goto error;
> +
> +l_controller = NULL;
> +}
> +
> +d_config->usbctrls = x_controllers;
> +d_config->num_usbctrls = ncontrollers;
> +return 0;
> +
> + error:
> + virDomainControllerDefFree(l_controller);
> + for (i = 0; i < ncontrollers; i++)
> + libxl_device_usbctrl_dispose(_controllers[i]);
> + VIR_FREE(x_controllers);
> + return -1;
> +}
> +
> +static int
>  libxlMakeUSBControllerList(virDomainDefPtr def, libxl_domain_config 
> *d_config)
>  {
>  virDomainControllerDefPtr *l_controllers = def->controllers;
>  size_t ncontrollers = def->ncontrollers;
>  size_t nusbctrls = 0;
>  libxl_device_usbctrl *x_usbctrls;
> -size_t i;
> -
> -if (ncontrollers == 0)
> -return 0;
> -
> -if (VIR_ALLOC_N(x_usbctrls, ncontrollers) < 0)
> -return -1;
> +size_t i, j;
>  
>  for (i = 0; i < ncontrollers; i++) {
> +if (l_controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
> +nusbctrls++;
> +}
> +
> +if (nusbctrls == 0)
> +return libxlMakeDefaultUSBControllers(def, d_config);
> +
> +if (VIR_ALLOC_N(x_usbctrls, nusbctrls) < 0)
> +return -1;
> +
> +for (i = 0, j = 0; i < ncontrollers && j < nusbctrls; i++) {
>  if (l_controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_USB)
>  continue;
>  
> -libxl_device_usbctrl_init(_usbctrls[nusbctrls]);
> +libxl_device_usbctrl_init(_usbctrls[j]);
>  
>  if (libxlMakeUSBController(l_controllers[i],
> -   _usbctrls[nusbctrls]) < 0)
> +   _usbctrls[j]) < 0)
>  goto error;
>  
> -nusbctrls++;
> +j++;
>  }
>  
> -VIR_SHRINK_N(x_usbctrls, ncontrollers, ncontrollers - nusbctrls);
>  d_config->usbctrls = x_usbctrls;
>  d_config->num_usbctrls = nusbctrls;
>  

ACK

--
Cedric

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

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

[Cedric Bosdonnat]

Hi Christian,

On Fri, 2017-05-19 at 11:18 +0200, Christian Ehrhardt wrote:
> 
> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
> > But if we aim for a profile replace on blockcommit [1] the would't matter
> > since the whole profile would get replaced, wouldn't it?
> > 
> 
> Since this is based on [1][2] looping in Cédric here to share some old 
> explaiantions.
> See especially [1] for some reasoning for 'R' in general.
> 
> [1]: 
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
> [2]: 
> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e

Sadly the bug report isn't public since it has been reported again SLES. But 
here is the
description of the bug that motivated that fix:


-- %< --
Steps to reproduce:
  * run virt-sandbox /bin/sh as root

Expected result: Run a shell in a qemu domain, apparmor enforced
Actual result: Domain fails to start

After some more debugging it happens that the problem is caused by 


  
  
  


Since commit 
http://libvirt.org/git/?p=libvirt.git;a=commit;h=d0d4b8ad76d3e8a859ee90701a21a3f003a22c1f,
 virt-aa-helper
generates a "deny /** w" rule in such cases that takes precedence over the 
allow rules.

This has several effects:
  * It hides the DENIED/ALLOWED apparmor log entries
  * It prevents qemu to write to the log file, /dev/ptmx and other important 
files to run the domain.

To see the rules, add the audit flag to /etc/apparmor.d/libvirt/TEMPLATE.qemu 
file and rerun virt-sandbox.
-- %< --

Hi hope this will answer your questions

--
Cedric

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

Re: [libvirt] [PATCH v2 0/5] Prevent losing IPv6 routes due to forwarding

[Cedric Bosdonnat]

On Wed, 2017-05-10 at 13:30 +0800, Yalan Zhang wrote:
> I'm sorry that I missed the mail. 

没关系

> But currently I can not reproduce it. 
> For the error by net-create, it is executed when I set accept_ra to 1.

That sounds more normal. net-create and net-start are triggering the
same code in the end.

> I have just test on libvirt-3.2.0-4.el7.x86_64, the behavior changes, it 
> seems like there is no check for accept_ra
> before start a network with ipv6.
> 
> 1. define and start a network with ipv6 settings
> # virsh net-dumpxml default6
> 
>   default6
>   c502d02c-fbd0-49d9-91e4-0fcf0ef159d0
>   
>   
>   
>   
>     
>   
>     
>   
>   
>     
>   
>     
>   
> 
> 
> # cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra
> 1
> 
> # virsh net-start default6   => the network can start as well with 
> accept_ra=1
> Network default6 started
> 
> It seems that the "virNetDevIPGetAcceptRA()" in patch  "network: check 
> accept_ra before enabling ipv6 forwarding"
> with commit 00d28a78 is not executed when I start a network. Please help to 
> check, Thank you.

It won't complain at all if there is no RA route set on the host.
To reproduce, you need to setup a machine acting as an ipv6 router
with radvd on the guest network.

Do you actually have an RA route for the enp0s25 device? You can check
it by running `ip -6 r`. These routes are indicated with 'proto ra'

--
Cedric

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

Re: [libvirt] GSoC Project

[Cedric Bosdonnat]

Hi Venkat,

Congratulations for being selected! I look forward to count you as
a new regular contributor of the libvirt community.

--
Cedric

On Sat, 2017-05-06 at 09:01 +0530, Venkat Datta wrote:
> Hi Team,
> 
> I'm an undergraduate student studying Computer Science at PES University , 
> India . 
> I had applied for the GSoC program this year . I'm happy to hear that my 
> proposal has been accepted. 
> The project idea i will be working on " Conversion to and from OCI-formatted 
> containers " 
> [ 
> http://wiki.libvirt.org/page/Google_Summer_of_Code_Ideas#Conversion_to_and_from_OCI-formatted_containers
>  ].
> 
> I'm looking forward to learn more and keep contributing to the libvirt 
> community.
> 
> Thanks,
> Venkat Datta N H
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 4/5] qemuDomainAttachDeviceMknodRecursive: Don't try to create devices under preserved mount points

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> Just like in previous commit, this fixes the same issue for
> hotplug.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 112 
> ++---
>  1 file changed, 97 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5840c57..60f8f01 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8238,6 +8238,8 @@ static int
>  qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
>   virDomainObjPtr vm,
>   const char *file,
> + char * const *devMountsPath,
> + size_t ndevMountsPath,
>   unsigned int ttl)
>  {
>  struct qemuDomainAttachDeviceMknodData data;
> @@ -8315,20 +8317,36 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
> driver,
>  #endif
>  
>  if (STRPREFIX(file, DEVPREFIX)) {
> -if (qemuSecurityPreFork(driver->securityManager) < 0)
> -goto cleanup;
> +size_t i;
>  
> -if (virProcessRunInMountNamespace(vm->pid,
> -  qemuDomainAttachDeviceMknodHelper,
> -  ) < 0) {
> +for (i = 0; i < ndevMountsPath; i++) {
> +if (STREQ(devMountsPath[i], "/dev"))
> +continue;
> +if (STRPREFIX(file, devMountsPath[i]))
> +break;
> +}
> +
> +if (i == ndevMountsPath) {
> +if (qemuSecurityPreFork(driver->securityManager) < 0)
> +goto cleanup;
> +
> +if (virProcessRunInMountNamespace(vm->pid,
> +  
> qemuDomainAttachDeviceMknodHelper,
> +  ) < 0) {
> +qemuSecurityPostFork(driver->securityManager);
> +goto cleanup;
> +}
>  qemuSecurityPostFork(driver->securityManager);
> -goto cleanup;
> +} else {
> +VIR_DEBUG("Skipping dev %s because of %s mount point",
> +  file, devMountsPath[i]);
>  }
> -qemuSecurityPostFork(driver->securityManager);
>  }
>  
>  if (isLink &&
> -qemuDomainAttachDeviceMknodRecursive(driver, vm, target, ttl -1) < 0)
> +qemuDomainAttachDeviceMknodRecursive(driver, vm, target,
> + devMountsPath, ndevMountsPath,
> + ttl -1) < 0)
>  goto cleanup;
>  
>  ret = 0;
> @@ -8345,11 +8363,15 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
> driver,
>  static int
>  qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
>  virDomainObjPtr vm,
> -const char *file)
> +const char *file,
> +char * const *devMountsPath,
> +size_t ndevMountsPath)
>  {
>  long symloop_max = sysconf(_SC_SYMLOOP_MAX);
>  
> -return qemuDomainAttachDeviceMknodRecursive(driver, vm, file, 
> symloop_max);
> +return qemuDomainAttachDeviceMknodRecursive(driver, vm, file,
> +devMountsPath, 
> ndevMountsPath,
> +symloop_max);
>  }
>  
>  
> @@ -8389,6 +8411,9 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
>   virDomainObjPtr vm,
>   virStorageSourcePtr src)
>  {
> +virQEMUDriverConfigPtr cfg = NULL;
> +char **devMountsPath = NULL;
> +size_t ndevMountsPath = 0;
>  virStorageSourcePtr next;
>  struct stat sb;
>  int ret = -1;
> @@ -8396,6 +8421,12 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
>  if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>  return 0;
>  
> +cfg = virQEMUDriverGetConfig(driver);
> +if (qemuDomainGetPreservedMounts(cfg, vm,
> + , NULL,
> + ) < 0)
> +goto cleanup;
> +
>  for (next = src; next; next = next->backingStore) {
>  if (virStorageSourceIsEmpty(next) ||
>  !virStorageSourceIsLocalStorage(next)) {
> @@ -8414,12 +8445,15 @@ qemuDomainNamespaceSetupDisk(virQEMUDriverPtr driver,
>  
>  if (qemuDomainAttachDeviceMknod(driver,
>  vm,
> -next->path) < 0)
> +next->path,
> +devMountsPath, ndevMountsPath) < 0)
>  goto cleanup;
>  }
>  
>  ret = 0;
>   cleanup:
> +

Re: [libvirt] [PATCH 5/5] qemuDomainDetachDeviceUnlink: Don't unlink files we haven't created

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> Even though there are several checks before calling this function
> and for some scenarios we don't call it at all (e.g. on disk hot
> unplug), it may be possible to sneak in some weird files (e.g. if
> domain would have RNG with /dev/shm/some_file as its backend). No
> matter how improbable, we shouldn't unlink it as we would be
> unlinking a file from the host which we haven't created in the
> first place.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 86 
> --
>  1 file changed, 76 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 60f8f01..c393d5e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8395,14 +8395,32 @@ qemuDomainDetachDeviceUnlinkHelper(pid_t pid 
> ATTRIBUTE_UNUSED,
>  static int
>  qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>   virDomainObjPtr vm,
> - const char *file)
> + const char *file,
> + char * const *devMountsPath,
> + size_t ndevMountsPath)
>  {
> -if (virProcessRunInMountNamespace(vm->pid,
> -  qemuDomainDetachDeviceUnlinkHelper,
> -  (void *)file) < 0)
> -return -1;
> +int ret = -1;
> +size_t i;
>  
> -return 0;
> +if (STRPREFIX(file, DEVPREFIX)) {
> +for (i = 0; i < ndevMountsPath; i++) {
> +if (STREQ(devMountsPath[i], "/dev"))
> +continue;
> +if (STRPREFIX(file, devMountsPath[i]))
> +break;
> +}
> +
> +if (i == ndevMountsPath) {
> +if (virProcessRunInMountNamespace(vm->pid,
> +  
> qemuDomainDetachDeviceUnlinkHelper,
> +  (void *)file) < 0)
> +goto cleanup;
> +}
> +}
> +
> +ret = 0;
> + cleanup:
> +return ret;
>  }
>  
>  
> @@ -8521,6 +8539,9 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr 
> driver,
> virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev)
>  {
> +virQEMUDriverConfigPtr cfg = NULL;
> +char **devMountsPath = NULL;
> +size_t ndevMountsPath = 0;
>  int ret = -1;
>  char **path = NULL;
>  size_t i, npaths = 0;
> @@ -8532,8 +8553,15 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr 
> driver,
>   , , NULL) < 0)
>  goto cleanup;
>  
> +cfg = virQEMUDriverGetConfig(driver);
> +if (qemuDomainGetPreservedMounts(cfg, vm,
> + , NULL,
> + ) < 0)
> +goto cleanup;
> +
>  for (i = 0; i < npaths; i++) {
> -if (qemuDomainDetachDeviceUnlink(driver, vm, path[i]) < 0)
> +if (qemuDomainDetachDeviceUnlink(driver, vm, path[i],
> + devMountsPath, ndevMountsPath) < 0)
>  goto cleanup;
>  }
>  
> @@ -8542,6 +8570,8 @@ qemuDomainNamespaceTeardownHostdev(virQEMUDriverPtr 
> driver,
>  for (i = 0; i < npaths; i++)
>  VIR_FREE(path[i]);
>  VIR_FREE(path);
> +virStringListFreeCount(devMountsPath, ndevMountsPath);
> +virObjectUnref(cfg);
>  return ret;
>  }
>  
> @@ -8584,6 +8614,9 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr 
> driver,
>    virDomainObjPtr vm,
>    virDomainMemoryDefPtr mem)
>  {
> +virQEMUDriverConfigPtr cfg = NULL;
> +char **devMountsPath = NULL;
> +size_t ndevMountsPath = 0;
>  int ret = -1;
>  
>  if (mem->model != VIR_DOMAIN_MEMORY_MODEL_NVDIMM)
> @@ -8592,10 +8625,19 @@ qemuDomainNamespaceTeardownMemory(virQEMUDriverPtr 
> driver,
>  if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>  return 0;
>  
> -if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath) < 0)
> +cfg = virQEMUDriverGetConfig(driver);
> +if (qemuDomainGetPreservedMounts(cfg, vm,
> + , NULL,
> + ) < 0)
> +goto cleanup;
> +
> +if (qemuDomainDetachDeviceUnlink(driver, vm, mem->nvdimmPath,
> + devMountsPath, ndevMountsPath) < 0)
>  goto cleanup;
>  ret = 0;
>   cleanup:
> +virStringListFreeCount(devMountsPath, ndevMountsPath);
> +virObjectUnref(cfg);
>  return ret;
>  }
>  
> @@ -8643,6 +8685,9 @@ qemuDomainNamespaceTeardownChardev(virQEMUDriverPtr 
> driver,
> virDomainObjPtr vm,
> virDomainChrDefPtr chr)
>  {
> +

Re: [libvirt] [PATCH 3/5] qemuDomainCreateDeviceRecursive: Don't try to create devices under preserved mount points

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> While the code allows devices to already be there (by some
> miracle), we shouldn't try to create devices that don't belong to
> us. For instance, we shouldn't try to create /dev/shm/file
> because /dev/shm is a mount point that is preserved. Therefore if
> a file is created there from an outside (e.g. by mgmt application
> or some other daemon running on the system like vhostmd), it
> exists in the qemu namespace too as the mount point is the same.
> It's only /dev and /dev only that is different. The same

One 'only' should be dropped perhaps?

> reasoning applies to all other preserved mount points.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 39 ++-
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9e18f7e..5840c57 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7339,6 +7339,8 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
>  
>  struct qemuDomainCreateDeviceData {
>  const char *path; /* Path to temp new /dev location */
> +char * const *devMountsPath;
> +size_t ndevMountsPath;
>  };
>  
>  
> @@ -7392,17 +7394,34 @@ qemuDomainCreateDeviceRecursive(const char *device,
>   * For now, lets hope callers play nice.
>   */
>  if (STRPREFIX(device, DEVPREFIX)) {
> -if (virAsprintf(, "%s/%s",
> -data->path, device + strlen(DEVPREFIX)) < 0)
> -goto cleanup;
> +size_t i;
>  
> -if (virFileMakeParentPath(devicePath) < 0) {
> -virReportSystemError(errno,
> - _("Unable to create %s"),
> - devicePath);
> -goto cleanup;
> +for (i = 0; i < data->ndevMountsPath; i++) {
> +if (STREQ(data->devMountsPath[i], "/dev"))
> +continue;
> +if (STRPREFIX(device, data->devMountsPath[i]))
> +break;
> +}
> +
> +if (i == data->ndevMountsPath) {
> +/* Okay, @device is in /dev but not in any mount point under 
> /dev.
> + * Create it. */
> +if (virAsprintf(, "%s/%s",
> +data->path, device + strlen(DEVPREFIX)) < 0)
> +goto cleanup;
> +
> +if (virFileMakeParentPath(devicePath) < 0) {
> +virReportSystemError(errno,
> + _("Unable to create %s"),
> + devicePath);
> +goto cleanup;
> +}
> +VIR_DEBUG("Creating dev %s", device);
> +create = true;
> +} else {
> +VIR_DEBUG("Skipping dev %s because of %s mount point",
> +  device, data->devMountsPath[i]);
>  }
> -create = true;
>  }
>  
>  if (isLink) {
> @@ -7951,6 +7970,8 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  }
>  
>  data.path = devPath;
> +data.devMountsPath = devMountsPath;
> +data.ndevMountsPath = ndevMountsPath;
>  
>  if (virProcessSetupPrivateMountNS() < 0)
>  goto cleanup;

ACK

--
Cedric

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

Re: [libvirt] [PATCH 2/5] qemuDomainCreateDeviceRecursive: pass a structure instead of bare path

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> Currently, all we need to do in qemuDomainCreateDeviceRecursive() is to
> take given @device, get all kinds of info on it (major & minor numbers,
> owner, seclabels) and create its copy at a temporary location @path
> (usually /var/run/libvirt/qemu/$domName.dev), if @device live under
> /dev. This is, however, very loose condition, as it also means
> /dev/shm/* is created too. Therefor, we will need to pass more arguments
> into the function for better decision making (e.g. list of mount points
> under /dev). Instead of adding more arguments to all the functions (not
> easily reachable because some functions are callback with strictly
> defined type), lets just turn this one 'const char *' into a 'struct *'.
> New "arguments" can be then added at no cost.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 106 
> ++---
>  1 file changed, 57 insertions(+), 49 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index be02d54..9e18f7e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7337,9 +7337,14 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr 
> cfg,
>  }
>  
>  
> +struct qemuDomainCreateDeviceData {
> +const char *path; /* Path to temp new /dev location */
> +};
> +
> +
>  static int
>  qemuDomainCreateDeviceRecursive(const char *device,
> -const char *path,
> +const struct qemuDomainCreateDeviceData 
> *data,
>  bool allow_noent,
>  unsigned int ttl)
>  {
> @@ -7388,7 +7393,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
>   */
>  if (STRPREFIX(device, DEVPREFIX)) {
>  if (virAsprintf(, "%s/%s",
> -path, device + strlen(DEVPREFIX)) < 0)
> +data->path, device + strlen(DEVPREFIX)) < 0)
>  goto cleanup;
>  
>  if (virFileMakeParentPath(devicePath) < 0) {
> @@ -7449,7 +7454,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
>  tmp = NULL;
>  }
>  
> -if (qemuDomainCreateDeviceRecursive(target, path,
> +if (qemuDomainCreateDeviceRecursive(target, data,
>  allow_noent, ttl - 1) < 0)
>  goto cleanup;
>  } else {
> @@ -7533,12 +7538,12 @@ qemuDomainCreateDeviceRecursive(const char *device,
>  
>  static int
>  qemuDomainCreateDevice(const char *device,
> -   const char *path,
> +   const struct qemuDomainCreateDeviceData *data,
> bool allow_noent)
>  {
>  long symloop_max = sysconf(_SC_SYMLOOP_MAX);
>  
> -return qemuDomainCreateDeviceRecursive(device, path,
> +return qemuDomainCreateDeviceRecursive(device, data,
> allow_noent, symloop_max);
>  }
>  
> @@ -7546,7 +7551,7 @@ qemuDomainCreateDevice(const char *device,
>  static int
>  qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,
>    virDomainObjPtr vm ATTRIBUTE_UNUSED,
> -  const char *path)
> +  const struct qemuDomainCreateDeviceData *data)
>  {
>  const char *const *devices = (const char *const *) cfg->cgroupDeviceACL;
>  size_t i;
> @@ -7556,7 +7561,7 @@ qemuDomainPopulateDevices(virQEMUDriverConfigPtr cfg,
>  devices = defaultDeviceACL;
>  
>  for (i = 0; devices[i]; i++) {
> -if (qemuDomainCreateDevice(devices[i], path, true) < 0)
> +if (qemuDomainCreateDevice(devices[i], data, true) < 0)
>  goto cleanup;
>  }
>  
> @@ -7570,7 +7575,7 @@ static int
>  qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,
> virSecurityManagerPtr mgr,
> virDomainObjPtr vm,
> -   const char *path)
> +   const struct qemuDomainCreateDeviceData *data)
>  {
>  char *mount_options = NULL;
>  char *opts = NULL;
> @@ -7592,10 +7597,10 @@ qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,
>  "mode=755,size=65536%s", mount_options) < 0)
>  goto cleanup;
>  
> -if (virFileSetupDev(path, opts) < 0)
> +if (virFileSetupDev(data->path, opts) < 0)
>  goto cleanup;
>  
> -if (qemuDomainPopulateDevices(cfg, vm, path) < 0)
> +if (qemuDomainPopulateDevices(cfg, vm, data) < 0)
>  goto cleanup;
>  
>  ret = 0;
> @@ -7609,7 +7614,7 @@ qemuDomainSetupDev(virQEMUDriverConfigPtr cfg,
>  static int
>  qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
>  virDomainDiskDefPtr disk,
> -const char *devPath)
> +const struct qemuDomainCreateDeviceData *data)
>  {
>  virStorageSourcePtr next;
>  char *dst = NULL;
> @@ 

Re: [libvirt] [PATCH 1/5] qemuDomainBuildNamespace: Move /dev/* mountpoints later

[Cedric Bosdonnat]

On Fri, 2017-04-28 at 13:22 +0200, Michal Privoznik wrote:
> When setting up mount namespace for a qemu domain the following
> steps are executed:
> 
> 1) get list of mountpoints under /dev/
> 2) move them to /var/run/libvirt/qemu/$domName.ext
> 3) start constructing new device tree under /var/run/libvirt/qemu/$domName.dev
> 4) move the mountpoint of the new device tree to /dev
> 5) restore original mountpoints from step 2)
> 
> Not the problem with this approach is that if some device in step

You may have wanted to write "Note" rather than "Not".
Otherwise ACK.

--
Cedric

> 3) requires access to a mountpoint from step 2) it will fail as
> the mountpoint is not there anymore. For instance consider the
> following domain disk configuration:
> 
> 
>   
>   
>   
>    function='0x0'/>
> 
> 
> In this case operation fails as we are unable to create vhostmd0
> in the new device tree because after step 2) there is no /dev/shm
> anymore. Leave aside fact that we shouldn't try to create devices
> living in other mountpoints. That's a separate bug that will be
> addressed later.
> 
> Currently, the order described above is rearranged to:
> 
> 1) get list of mountpoints under /dev/
> 2) start constructing new device tree under /var/run/libvirt/qemu/$domName.dev
> 3) move them to /var/run/libvirt/qemu/$domName.ext
> 4) move the mountpoint of the new device tree to /dev
> 5) restore original mountpoints from step 3)
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 54 
> +-
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 00b0b4a..be02d54 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7950,33 +7950,6 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  if (qemuDomainSetupDev(cfg, mgr, vm, devPath) < 0)
>  goto cleanup;
>  
> -/* Save some mount points because we want to share them with the host */
> -for (i = 0; i < ndevMountsPath; i++) {
> -struct stat sb;
> -
> -if (devMountsSavePath[i] == devPath)
> -continue;
> -
> -if (stat(devMountsPath[i], ) < 0) {
> -virReportSystemError(errno,
> - _("Unable to stat: %s"),
> - devMountsPath[i]);
> -goto cleanup;
> -}
> -
> -/* At this point, devMountsPath is either a regular file or a 
> directory. */
> -if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 
> 0) ||
> -(S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], 
> sb.st_mode) < 0)) {
> -virReportSystemError(errno,
> - _("Failed to create %s"),
> - devMountsSavePath[i]);
> -goto cleanup;
> -}
> -
> -if (virFileMoveMount(devMountsPath[i], devMountsSavePath[i]) < 0)
> -goto cleanup;
> -}
> -
>  if (qemuDomainSetupAllDisks(cfg, vm, devPath) < 0)
>  goto cleanup;
>  
> @@ -8001,6 +7974,33 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  if (qemuDomainSetupAllRNGs(cfg, vm, devPath) < 0)
>  goto cleanup;
>  
> +/* Save some mount points because we want to share them with the host */
> +for (i = 0; i < ndevMountsPath; i++) {
> +struct stat sb;
> +
> +if (devMountsSavePath[i] == devPath)
> +continue;
> +
> +if (stat(devMountsPath[i], ) < 0) {
> +virReportSystemError(errno,
> + _("Unable to stat: %s"),
> + devMountsPath[i]);
> +goto cleanup;
> +}
> +
> +/* At this point, devMountsPath is either a regular file or a 
> directory. */
> +if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 
> 0) ||
> +(S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], 
> sb.st_mode) < 0)) {
> +virReportSystemError(errno,
> + _("Failed to create %s"),
> + devMountsSavePath[i]);
> +goto cleanup;
> +}
> +
> +if (virFileMoveMount(devMountsPath[i], devMountsSavePath[i]) < 0)
> +goto cleanup;
> +}
> +
>  if (virFileMoveMount(devPath, "/dev") < 0)
>  goto cleanup;
>  

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

Re: [libvirt] [PATCH v2 0/5] Prevent losing IPv6 routes due to forwarding

[Cedric Bosdonnat]

Yalan 你好

On Mon, 2017-04-17 at 17:30 +0800, Yalan Zhang wrote:
> I have tested it, it works well. But the interface name will repeat 2 times. 
> Please help to confirm this, and if below test for a single port host is 
> enough?
> 
> # cat /proc/sys/net/ipv6/conf/enp0s25/accept_ra
> 1
> 
> enable network default with ipv6 ip section
> 
> # virsh net-start default
> error: Failed to start network default
> error: internal error: Check the host setup: enabling IPv6 forwarding with RA 
> routes without accept_ra set to 2 is
> likely to cause routes loss. Interfaces to look at: enp0s25, enp0s25

Just to help me confirm my intuition: do you have several RA routes defined
for the same device?

> # echo 2 > /proc/sys/net/ipv6/conf/enp0s25/accept_ra
> 
> # virsh net-start default
> Network default started
> 
> try create:
> 
> # virsh net-create default.xml
> error: Failed to create network from default.xml
> error: internal error: Check the host setup: enabling IPv6 forwarding with RA 
> routes without accept_ra set to 2 is
> likely to cause routes loss. Interfaces to look at: enp0s25, enp0s25

This one sounds weird: if the accept_ra is set to 2 as you report you did,
you shouldn't get that error.

--
Cedric

> On Wed, Mar 15, 2017 at 10:45 PM, Cédric Bosdonnat  
> wrote:
> > Hi Laine, all,
> > 
> > Here is the v2 of my series. The changes are:
> > 
> >  * Add a commit to create a virNetDevGetName() function
> >  * Fix Laine's comments
> > 
> > Cédric Bosdonnat (5):
> >   util: extract the request sending code from virNetlinkCommand()
> >   util: add virNetlinkDumpCommand()
> >   bridge_driver.c: more uses of SYSCTL_PATH
> >   util: add virNetDevGetName() function
> >   network: check accept_ra before enabling ipv6 forwarding
> > 
> >  src/libvirt_private.syms    |   3 +
> >  src/network/bridge_driver.c |  25 ---
> >  src/util/virnetdev.c        |  19 ++
> >  src/util/virnetdev.h        |   2 +
> >  src/util/virnetdevip.c      | 158 
> > 
> >  src/util/virnetdevip.h      |   1 +
> >  src/util/virnetlink.c       | 145 ++--
> >  src/util/virnetlink.h       |   9 +++
> >  8 files changed, 319 insertions(+), 43 deletions(-)
> > 
> > --
> > 2.11.0
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> 

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

Re: [libvirt] [PATCH v2] virNetDevIPCheckIPv6ForwardingCallback fixes

[Cedric Bosdonnat]

Hi all,

Has that one been forgotten?

On Tue, 2017-03-28 at 16:00 +0200, Cédric Bosdonnat wrote:
> Add check for more than one RTA_OIF, even though this is rather
> unlikely.
> 
> Get rid of the buggy switch / break as this code won't need to
> handle more attributes.
> 
> Use VIR_WARNINGS_NO_CAST_ALIGN to fix impossible to fix
> util/virnetdevip.c:560:17: error: cast increases required alignment of target 
> type [-Werror=cast-align]
> ---
>  Diff to v1:
>    * Add error message
>    * Use VIR_WARNINGS_NO_CAST_ALIGN
>  src/util/virnetdevip.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> index c9ac6baf7..726fa6c3e 100644
> --- a/src/util/virnetdevip.c
> +++ b/src/util/virnetdevip.c
> @@ -556,15 +556,24 @@ virNetDevIPCheckIPv6ForwardingCallback(const struct 
> nlmsghdr *resp,
>  if (resp->nlmsg_type != RTM_NEWROUTE)
>  return ret;
>  
> -/* Extract a few attributes */
> +/* Extract a device ID attribute */
> +VIR_WARNINGS_NO_CAST_ALIGN
>  for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) {
> -switch (rta->rta_type) {
> -case RTA_OIF:
> +VIR_WARNINGS_RESET
> +if (rta->rta_type == RTA_OIF) {
>  oif = *(int *)RTA_DATA(rta);
>  
> +/* Should never happen: netlink message would be broken */
> +if (ifname) {
> +char *ifname2 = virNetDevGetName(oif);
> +VIR_WARN("Single route has unexpected 2nd interface "
> + "- '%s' and '%s'", ifname, ifname2);
> +VIR_FREE(ifname2);
> +break;
> +}
> +
>  if (!(ifname = virNetDevGetName(oif)))
>  goto error;
> -break;
>  }
>  }
>  

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

Re: [libvirt] [PATCH] network: only check for IPv6 RA routes when the network has an IPv6 address

[Cedric Bosdonnat]

On Thu, 2017-03-23 at 20:23 -0400, Laine Stump wrote:
> commit 00d28a78 added a check to see if there were any IPv6 routes
> added by RA (Router Advertisement) via an interface that had accept_ra
> set to something other than "2". The check was being done
> unconditionally, but it's only relevant if IPv6 forwarding is going to
> be turned on, and that will only happen if the network has an IPv6
> address.
> ---
>  src/network/bridge_driver.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 0336ece..10e8f0e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2391,10 +2391,9 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr 
> driver,
>  
>  /* If forward.type != NONE, turn on global IP forwarding */
>  if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) {
> -if (!virNetDevIPCheckIPv6Forwarding())
> +if (v6present && !virNetDevIPCheckIPv6Forwarding())
>  goto err3; /* Precise error message already provided */
>  
> -
>  if (networkEnableIPForwarding(v4present, v6present) < 0) {
>  virReportSystemError(errno, "%s",
>   _("failed to enable IP forwarding"));


Thanks for catching it! ACK

--
Cedric

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

Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

[Cedric Bosdonnat]

On Thu, 2017-03-23 at 12:09 -0400, John Ferlan wrote:
> 
> On 03/23/2017 04:44 AM, Cedric Bosdonnat wrote:
> > On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
> > > [...]
> > > 
> > > > +
> > > > +static int
> > > > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > > > +   void *opaque)
> > > > +{
> > > > +struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > > > +int accept_ra = -1;
> > > > +struct rtattr *rta;
> > > > +char *ifname = NULL;
> > > > +struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > > > +int ret = 0;
> > > > +int len = RTM_PAYLOAD(resp);
> > > > +int oif = -1;
> > > > +
> > > > +/* Ignore messages other than route ones */
> > > > +if (resp->nlmsg_type != RTM_NEWROUTE)
> > > > +return ret;
> > > > +
> > > > +/* Extract a few attributes */
> > > > +for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, 
> > > > len)) {
> > > > +switch (rta->rta_type) {
> > > > +case RTA_OIF:
> > > > +oif = *(int *)RTA_DATA(rta);
> > > > +
> > > > +if (!(ifname = virNetDevGetName(oif)))
> > > > +goto error;
> > > > +break;
> > > 
> > > Did you really mean to break from the for loop if ifname is set?  This
> > > breaks only from the switch/case.  Of course Coverity doesn't know much
> > > more than you'd be going back to the top of the for loop and could
> > > overwrite ifname again. It proclaims a resource leak...
> > 
> > In my dev version I was also getting the RTA_DST attribute to print some
> > debugging message that I removed in the submitted version. The break was
> > here between the switch cases.
> > 
> > In the current case I don't really care if we break out of the loop or not.
> > As there aren't that many attributes in an rtnetlink message to loop over,
> > breaking wouldn't gain a lot of cycles.
> > 
> > What I don't get though is what this break is actually doing? isn't it
> > for the switch case even though there is no other case after it?
> > 
> > --
> > Cedric
> 
> So should this be changed to:
> 
> if (rta->rta_type == RTA_OIF) {
> oif = *(int *)RTA_DATA(rta);
> 
> if (!(ifname = virNetDevGetName(oif)))
> goto error;
> break;
> }
> 
> leaving two questions in my mind
> 
>   1. Can there be more than one RTA_OIF
>   2. If we don't finish the loop (e.g. we break out), then does one of
> the subsequent checks fail?

IMHO getting more than one RTA_OIF per message would be a bug from the
kernel: a route is associated to one device. Think of these messages like
the lines printed by 'ip r'

> Is it more of a problem that we find *two* RTA_OIF's and thus should add a:
> 
> if (ifname) {
> VIR_DEBUG("some sort of message");
> goto error;
> }
> 
> prior to the virNetDevGetName call and then remove the break;?

I can remove the break without that for sure.

> John (who doesn't know the answer!)
> 
> BTW: The issue from patch4 is resolved by my 22 patch bomb from yesterday

Yep, seen that one: thanks a lot
--
Cedric

> > 
> > > John
> > > > +}
> > > > +}
> > > > +
> > > > +/* No need to do anything else for non RA routes */
> > > > +if (rtmsg->rtm_protocol != RTPROT_RA)
> > > > +goto cleanup;
> > > > +
> > > > +data->hasRARoutes = true;
> > > > +
> > > > +/* Check the accept_ra value for the interface */
> > > > +accept_ra = virNetDevIPGetAcceptRA(ifname);
> > > > +VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, 
> > > > accept_ra);
> > > > +
> > > > +if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, 
> > > > data->ndevices, ifname) < 0)
> > > > +goto error;
> > > > +
> > > > + cleanup:
> > > > +VIR_FREE(ifname);
> > > > +return ret;
> > > > +
> > > > + error:
> > > > +ret = -1;
> > > > +goto cleanup;
> > > > +}
> > > 
> > > 
> 
> 

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

Re: [libvirt] Live attaching a disk to a VM fails with apparmor enabled

[Cedric Bosdonnat]

Hello Frank,

I'm currently investigating some apparmor-related bug with namespaces. This one
is surely related. I'll look into it when I'm done with the one I'm working on.

--
Cedric

On Thu, 2017-03-23 at 12:07 +, Frank Schreuder wrote:
> Hello,
> 
> I'm running libvirt 3.1.0 on a Debian 8 server. I installed apparmor and 
> configured libvirt to use apparmor as
> security driver.
> After booting a VM, virsh dumpxml shows an apparmor seclabel.
> 
> As soon as I try to attach a second disk to the VM, apparmor blocks this.
> 
> virsh attach-device test-vps /tmp/virshXmlDefinition
> error: Failed to attach device from /tmp/virshXmlDefinition
> error: operation failed: Could not open '/mnt/images/disk2.raw': Permission 
> denied
> 
> Syslogs shows me the following:
> Mar 22 17:45:20 vps0 kernel: [1136647.318314] audit: type=1400 
> audit(1490201120.577:30): apparmor="DENIED"
> operation="open" profile="libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859" 
> name="/mnt/images/disk2.raw" pid=13453
> comm="kvm" requested_mask="r" denied_mask="r" fsuid=996 ouid=33
> Mar 22 17:45:20 vps0 kernel: [1136647.325155] audit: type=1400 
> audit(1490201120.577:31): apparmor="DENIED"
> operation="open" profile="libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859" 
> name="/mnt/images/disk2.raw" pid=13453
> comm="kvm" requested_mask="rw" denied_mask="rw" fsuid=996 ouid=33
> Mar 22 17:45:20 vps0 libvirtd[10282]: 2017-03-22 16:45:20.596+: 10283: 
> error : qemuMonitorTextAddDrive:1968 :
> operation failed: Could not open '/mnt/images/disk2.raw': Permission denied
> 
> In the VM specific apparmor file 
> /etc/apparmor.d/libvirt/libvirt-5747e4db-a3b7-fd69-ca89-7b0bf859.files I 
> see:
> "/mnt/images/disk1.raw" rw,
> 
> Which is my primary VM disk, I expected a virsh attach-device to append 
> /mnt/images/disk2.raw to this file and
> reload/refresh the apparmor profile?
> 
> I'm not able to attach a live disk to a running VM with apparmor. Am I 
> missing something? Or is this a bug/missing
> feature in libvirt?
> 
> Thanks,
> Frank
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

[Cedric Bosdonnat]

On Wed, 2017-03-22 at 07:16 -0400, John Ferlan wrote:
> [...]
> 
> > +
> > +static int
> > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > +   void *opaque)
> > +{
> > +struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > +int accept_ra = -1;
> > +struct rtattr *rta;
> > +char *ifname = NULL;
> > +struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > +int ret = 0;
> > +int len = RTM_PAYLOAD(resp);
> > +int oif = -1;
> > +
> > +/* Ignore messages other than route ones */
> > +if (resp->nlmsg_type != RTM_NEWROUTE)
> > +return ret;
> > +
> > +/* Extract a few attributes */
> > +for (rta = RTM_RTA(rtmsg); RTA_OK(rta, len); rta = RTA_NEXT(rta, len)) 
> > {
> > +switch (rta->rta_type) {
> > +case RTA_OIF:
> > +oif = *(int *)RTA_DATA(rta);
> > +
> > +if (!(ifname = virNetDevGetName(oif)))
> > +goto error;
> > +break;
> 
> Did you really mean to break from the for loop if ifname is set?  This
> breaks only from the switch/case.  Of course Coverity doesn't know much
> more than you'd be going back to the top of the for loop and could
> overwrite ifname again. It proclaims a resource leak...

In my dev version I was also getting the RTA_DST attribute to print some
debugging message that I removed in the submitted version. The break was
here between the switch cases.

In the current case I don't really care if we break out of the loop or not.
As there aren't that many attributes in an rtnetlink message to loop over,
breaking wouldn't gain a lot of cycles.

What I don't get though is what this break is actually doing? isn't it
for the switch case even though there is no other case after it?

--
Cedric

> John
> > +}
> > +}
> > +
> > +/* No need to do anything else for non RA routes */
> > +if (rtmsg->rtm_protocol != RTPROT_RA)
> > +goto cleanup;
> > +
> > +data->hasRARoutes = true;
> > +
> > +/* Check the accept_ra value for the interface */
> > +accept_ra = virNetDevIPGetAcceptRA(ifname);
> > +VIR_DEBUG("Checking route for device %s, accept_ra: %d", ifname, 
> > accept_ra);
> > +
> > +if (accept_ra != 2 && VIR_APPEND_ELEMENT(data->devices, 
> > data->ndevices, ifname) < 0)
> > +goto error;
> > +
> > + cleanup:
> > +VIR_FREE(ifname);
> > +return ret;
> > +
> > + error:
> > +ret = -1;
> > +goto cleanup;
> > +}
> 
> 

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

Re: [libvirt] [PATCH v2 5/5] network: check accept_ra before enabling ipv6 forwarding

[Cedric Bosdonnat]

On Thu, 2017-03-16 at 21:12 -0400, Laine Stump wrote:
> On 03/15/2017 10:45 AM, Cédric Bosdonnat wrote:
> > When enabling IPv6 on all interfaces, we may get the host Router
> > Advertisement routes discarded. To avoid this, the user needs to set
> > accept_ra to 2 for the interfaces with such routes.
> > 
> > See https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt
> > on this topic.
> > 
> > To avoid user mistakenly losing routes on their hosts, check
> > accept_ra values before enabling IPv6 forwarding. If a RA route is
> > detected, but neither the corresponding device nor global accept_ra
> > is set to 2, the network will fail to start.
> 
> Since I already asked the question and didn't hear a positive response,
> I'm guessing "no news is bad news", i.e. there isn't a reliable way to
> fix this problem automatically. Assuming that, reporting the problem and
> failing seems like the next best (least worse) thing...

The only automatic thing that we could do is remember the RA routes that
are set before enabling ipv6 forwarding and reset them after having lost them.

IMHO automatically changing the accept_ra parameter for the user could lead to
unknown (possibly buggy) cases. Before this patch, the network is started, but
the host may loose routes (the default one in my case). After, it keeps the
host routes, fails to start the network and tells the user to fix his host
network config.


--
Cedric

> 
> > ---
> >  src/libvirt_private.syms|   1 +
> >  src/network/bridge_driver.c |  16 +++--
> >  src/util/virnetdevip.c  | 158 
> > 
> >  src/util/virnetdevip.h  |   1 +
> >  4 files changed, 171 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 0fe88c3fa..ec6553520 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2056,6 +2056,7 @@ virNetDevBridgeSetVlanFiltering;
> >  virNetDevIPAddrAdd;
> >  virNetDevIPAddrDel;
> >  virNetDevIPAddrGet;
> > +virNetDevIPCheckIPv6Forwarding;
> >  virNetDevIPInfoAddToDev;
> >  virNetDevIPInfoClear;
> >  virNetDevIPRouteAdd;
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index 3f6561055..d02cd19f9 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -61,6 +61,7 @@
> >  #include "virlog.h"
> >  #include "virdnsmasq.h"
> >  #include "configmake.h"
> > +#include "virnetlink.h"
> >  #include "virnetdev.h"
> >  #include "virnetdevip.h"
> >  #include "virnetdevbridge.h"
> > @@ -2377,11 +2378,16 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr 
> > driver,
> >  }
> >  
> >  /* If forward.type != NONE, turn on global IP forwarding */
> > -if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE &&
> > -networkEnableIPForwarding(v4present, v6present) < 0) {
> > -virReportSystemError(errno, "%s",
> > - _("failed to enable IP forwarding"));
> > -goto err3;
> > +if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE) {
> > +if (!virNetDevIPCheckIPv6Forwarding())
> > +goto err3; /* Precise error message already provided */
> > +
> > +
> > +if (networkEnableIPForwarding(v4present, v6present) < 0) {
> > +virReportSystemError(errno, "%s",
> > + _("failed to enable IP forwarding"));
> > +goto err3;
> > +}
> >  }
> >  
> >  
> > diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
> > index 42fbba1eb..a4d382427 100644
> > --- a/src/util/virnetdevip.c
> > +++ b/src/util/virnetdevip.c
> > @@ -508,6 +508,158 @@ virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, 
> > size_t count)
> >  return ret;
> >  }
> >  
> > +static int
> > +virNetDevIPGetAcceptRA(const char *ifname)
> > +{
> > +char *path = NULL;
> > +char *buf = NULL;
> > +char *suffix;
> > +int accept_ra = -1;
> > +
> > +if (virAsprintf(, "/proc/sys/net/ipv6/conf/%s/accept_ra",
> > +ifname ? ifname : "all") < 0)
> > +goto cleanup;
> > +
> > +if ((virFileReadAll(path, 512, ) < 0) ||
> > +(virStrToLong_i(buf, , 10, _ra) < 0))
> > +goto cleanup;
> > +
> > + cleanup:
> > +VIR_FREE(path);
> > +VIR_FREE(buf);
> > +
> > +return accept_ra;
> > +}
> > +
> > +struct virNetDevIPCheckIPv6ForwardingData {
> > +bool hasRARoutes;
> > +
> > +/* Devices with conflicting accept_ra */
> > +char **devices;
> > +size_t ndevices;
> > +};
> > +
> > +static int
> > +virNetDevIPCheckIPv6ForwardingCallback(const struct nlmsghdr *resp,
> > +   void *opaque)
> > +{
> > +struct rtmsg *rtmsg = NLMSG_DATA(resp);
> > +int accept_ra = -1;
> > +struct rtattr *rta;
> > +char *ifname = NULL;
> > +struct virNetDevIPCheckIPv6ForwardingData *data = opaque;
> > +int ret = 0;
> > +int len = 

Re: [libvirt] libvirt and dmesg

[Cedric Bosdonnat]

Hello Pierre-Jacques,

First note that you posted your message on the developer's mailing list.
For such user questions, rather email: 
https://www.redhat.com/mailman/listinfo/libvirt-users

According to 
https://linuxcontainers.org/lxc/manpages//man5/lxc.container.conf.5.html
lxc.kmsg is only used to symlink /dev/kmsg to /dev/console. Thus setting
this to 0 only results in the lack of that symlink. libvirt doesn't setup
this link at all, may be it is coming from the lxc domain root file system.

On Wed, 2017-03-15 at 08:49 +0100, Michel Pierre-Jacques wrote:
> Hi all, il try to find the way to obtain the same use as in the
> lxc config file lxc.kmsg=0
> 
> how to do that, with the xml of a lxc domain, i make lot of try without
> success . 
> 
> is there another way to deny access to the dmesg for a libvirt lxc
> domain ? 

>From what I see, dmesg doesn't necessarily requires /dev/kmsg (at least
on opensuse). So blocking it may be more tricky. May be you should tell
us more how you setup your container.

--
Cedric

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

Re: [libvirt] [PATCH 0/4] Prevent loosing IPv6 routes due to forwarding

[Cedric Bosdonnat]

Hi guys,

Has that patch series fallen into the mailing list abysses?

--
Cedric

On Fri, 2017-03-03 at 16:00 +0100, Cédric Bosdonnat wrote:
> Hi all,
> 
> When enabling IPv6 forwarding on hosts getting Router Advertised routes,
> the host looses the RA routes. To prevent this, check if the host has
> such routes. In case there are some, check that the accept_ra is set
> to 2 before bringing the network up.
> 
> Cédric Bosdonnat (4):
>   util: extract the request sending code from virNetlinkCommand()
>   util: add virNetlinkDumpCommand()
>   bridge_driver.c: more uses of SYSCTL_PATH
>   network: check accept_ra before enabling ipv6 forwarding
> 
>  src/libvirt_private.syms|   1 +
>  src/network/bridge_driver.c | 187 
> +---
>  src/util/virnetlink.c   | 145 +-
>  src/util/virnetlink.h   |   9 +++
>  4 files changed, 298 insertions(+), 44 deletions(-)
> 

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

Re: [libvirt] Making containers creation easy

[Cedric Bosdonnat]

Hi Michal,

On Thu, 2017-03-09 at 08:54 +0100, Michal Privoznik wrote:
> On 03/08/2017 06:16 PM, Cedric Bosdonnat wrote:
> > Hi all,
> > 
> > I've worked on making the containers rootfs creation easy.
> > Here is a wrap-up of my work:
> > 
> > http://bosdonnat.fr/system-container-images.html
> > 
> > Any opinion on that? Anything to do to move it forward?
> 
> Awesome. I always felt like preparing rootfs for my containers was
> overwhelming. Now, with your tool I can use docker images at least. BTW:
> should we have the repo hosted on libvirt.org? Or at least link yours
> from our docs?

So far there is no openSUSE or SUSE registry holding the images, thus
having one on libvirt.org would be great. I don't know how other distros
are generating their docker images, that part of the process should be
investigated. Having a libvirt.org public repo would imply:

  * Having reviews of the published images
  * Make sure those are updated to include major fixes like the security ones

I'm OK to do that for the openSUSE part and to help reviewing the images,
but I'm not sure it's going to fly if I'm doing it alone.

As for the virt-bootstrap tool, it may deserve a better repository (I've set
a not-so-random license on it and I can change it at will). May be the name
could also be improved. As for the features:

  * I'm planning to add unpacking in qcow2 images to benefit from backing 
chains.
  * Is it possible at all to prepare a filesystem that works well with userns?
I haven't tried tar's --owner-map to see if it scales properly
  * Maybe I could add a virt-builder based source as well.

Could that be a GSoC project in the end?

--
Cedric

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

[libvirt] Making containers creation easy

[Cedric Bosdonnat]

Hi all,

I've worked on making the containers rootfs creation easy.
Here is a wrap-up of my work:

http://bosdonnat.fr/system-container-images.html

Any opinion on that? Anything to do to move it forward?

--
Cedric

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

Re: [libvirt] [PATCH v4] libxl: define a per-domain logger.

[Cedric Bosdonnat]

On Tue, 2017-01-10 at 11:42 -0700, Jim Fehlig wrote:
> Cédric Bosdonnat wrote:
> > libxl doesn't provide a way to write one log for each domain. Thus
> > we need to demux the messages. If our logger doesn't know to which
> > domain to attribute a message, then it will write it to the default
> > log file.
> > 
> > Starting with Xen 4.9 (commit f9858025 and following), libxl will
> > write the domain ID in an easy to grab manner. The logger introduced
> > by this commit will use it to demux the libxl log messages.
> > 
> > Thanks to the default log file, this logger will also work with older
> > versions of Xen.
> > ---
> >  * v4: delay the log file opening to the first message write.
> 
> Hmm, in hindsight I shouldn't have mentioned this in V3 since we'll always 
> have
> at least the json config written to the file correct? If so, there's really no
> reason to delay opening it. Sorry for the confusion.
> 
> I tend to prefer V3, but with the nits I pointed out there fixed :-).

Then I pushed the v3 with the changes.
--
Cedric

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

Re: [libvirt] [PATCH v4] libxl: define a per-domain logger.

[Cedric Bosdonnat]

On Tue, 2017-01-10 at 12:21 +, Joao Martins wrote:
> Hey Cedric,
> 
> On 01/10/2017 09:03 AM, Cédric Bosdonnat wrote:
> > libxl doesn't provide a way to write one log for each domain. Thus
> > we need to demux the messages. If our logger doesn't know to which
> > domain to attribute a message, then it will write it to the default
> > log file.
> > 
> > Starting with Xen 4.9 (commit f9858025 and following), libxl will
> > write the domain ID in an easy to grab manner. The logger introduced
> > by this commit will use it to demux the libxl log messages.
> > 
> > Thanks to the default log file, this logger will also work with older
> > versions of Xen.
> 
> This is great stuff!
> 
> BTW, while applying the patch in my local repo, I noticed some issues (and
> confirmed make syntax-check is failing with them as well). But I assume those
> could be fixed on commit.

Indeed, fixed it locally. Thanks for the heads up.
--
Cedric

> Joao
> 
> > ---
> >  * v4: delay the log file opening to the first message write.
> > 
> >  * v3: add JSON-formatted libxl_domain_config to the newly opened
> >    file when creating a guest.
> > 
> >  * v2: addressed Jim's comments
> > 
> >  src/Makefile.am  |   1 +
> >  src/libxl/libxl_conf.c   |  38 +--
> >  src/libxl/libxl_conf.h   |   4 +-
> >  src/libxl/libxl_domain.c |   7 ++
> >  src/libxl/libxl_driver.c |   2 +
> >  src/libxl/libxl_logger.c | 257 
> > +++
> >  src/libxl/libxl_logger.h |  40 
> >  7 files changed, 312 insertions(+), 37 deletions(-)
> >  create mode 100644 src/libxl/libxl_logger.c
> >  create mode 100644 src/libxl/libxl_logger.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 0dd2faaf6..58e3108a6 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =  
> > \
> >     libxl/libxl_capabilities.c libxl/libxl_capabilities.h   \
> >     libxl/libxl_domain.c libxl/libxl_domain.h   \
> >     libxl/libxl_driver.c libxl/libxl_driver.h   \
> > +   libxl/libxl_logger.c libxl/libxl_logger.h   \
> >     libxl/libxl_migration.c libxl/libxl_migration.h
> >  
> >  UML_DRIVER_SOURCES =   \
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index b569ddad8..ac83b51c7 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
> >  
> >  virObjectUnref(cfg->caps);
> >  libxl_ctx_free(cfg->ctx);
> > -xtl_logger_destroy(cfg->logger);
> > -if (cfg->logger_file)
> > -VIR_FORCE_FCLOSE(cfg->logger_file);
> > +libxlLoggerFree(cfg->logger);
> >  
> >  VIR_FREE(cfg->configDir);
> >  VIR_FREE(cfg->autostartDir);
> > @@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
> >  libxlDriverConfigNew(void)
> >  {
> >  libxlDriverConfigPtr cfg;
> > -char *log_file = NULL;
> > -xentoollog_level log_level = XTL_DEBUG;
> >  char ebuf[1024];
> >  unsigned int free_mem;
> >  
> > @@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
> >  if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
> >  goto error;
> >  
> > -if (virAsprintf(_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> > -goto error;
> > -
> >  if (virFileMakePath(cfg->logDir) < 0) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("failed to create log dir '%s': %s"),
> > @@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
> >  goto error;
> >  }
> >  
> > -if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
> > -VIR_ERROR(_("Failed to create log file '%s': %s"),
> > -  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> > -goto error;
> > -}
> > -VIR_FREE(log_file);
> > -
> > -switch (virLogGetDefaultPriority()) {
> > -case VIR_LOG_DEBUG:
> > -log_level = XTL_DEBUG;
> > -break;
> > -case VIR_LOG_INFO:
> > -log_level = XTL_INFO;
> > -break;
> > -case VIR_LOG_WARN:
> > -log_level = XTL_WARN;
> > -break;
> > -case VIR_LOG_ERROR:
> > -log_level = XTL_ERROR;
> > -break;
> > -}
> > -
> > -cfg->logger =
> > -(xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> > -  log_level, 
> > XTL_STDIOSTREAM_SHOW_DATE);
> > +cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
> >  if (!cfg->logger) {
> >  VIR_ERROR(_("cannot create logger for libxenlight, disabling 
> > driver"));
> >  goto error;
> >  }
> >  
> > -if (libxl_ctx_alloc(>ctx, LIBXL_VERSION, 0, cfg->logger)) {
> > +if (libxl_ctx_alloc(>ctx, LIBXL_VERSION, 0, (xentoollog_logger 
> > *)cfg->logger)) {
> >  VIR_ERROR(_("cannot 

Re: [libvirt] [PATCH] lxc: ensure libvirt_lxc and qemu-nbd move into systemd machine slice

[Cedric Bosdonnat]

On Thu, 2017-01-05 at 15:30 +, Daniel P. Berrange wrote:
> Currently when spawning containers with systemd, the container PID 1
> will get moved into the systemd machine slice. Libvirt then manually
> moves the libvirt_lxc and qemu-nbd processes into the cgroups associated
> with the slice, but skips the systemd controller cgroup. This means that
> from systemd's POV, libvirt_lxc and qemu-nbd are still part of the
> libvirtd.service unit.
> 
> On systemctl daemon-reload, it will notice that libvirt_lxc & qemu-nbd
> are in the libvirtd.service unit for the systemd controller, but in the
> machine cgroups for resources. Systemd will thus move them back into
> the libvirtd.service resource cgroups next time libvirtd is restarted.
> This causes libvirtd to kill off the container due to incorrect cgroup
> placement.
> 
> The solution is to ensure that when moving libvirt_lxc & qemu-nbd, we
> also move the systemd cgroup controller placement. Normally this is
> not something we ever want todo, but this is a special case as we are
> intentionally wanting to move them to a different systemd unit.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/libvirt_private.syms |  1 +
>  src/lxc/lxc_controller.c |  4 ++--
>  src/util/vircgroup.c | 52 
> +---
>  src/util/vircgroup.h |  1 +
>  4 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2d23e46..2dd5809 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1287,6 +1287,7 @@ virBufferVasprintf;
>  
>  
>  # util/vircgroup.h
> +virCgroupAddMachineTask;
>  virCgroupAddTask;
>  virCgroupAddTaskController;
>  virCgroupAllowAllDevices;
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 2170b0a..88ba8aa 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -869,12 +869,12 @@ static int 
> virLXCControllerSetupCgroupLimits(virLXCControllerPtr ctrl)
>  ctrl->nicindexes)))
>  goto cleanup;
>  
> -if (virCgroupAddTask(ctrl->cgroup, getpid()) < 0)
> +if (virCgroupAddMachineTask(ctrl->cgroup, getpid()) < 0)
>  goto cleanup;
>  
>  /* Add all qemu-nbd tasks to the cgroup */
>  for (i = 0; i < ctrl->nnbdpids; i++) {
> -if (virCgroupAddTask(ctrl->cgroup, ctrl->nbdpids[i]) < 0)
> +if (virCgroupAddMachineTask(ctrl->cgroup, ctrl->nbdpids[i]) < 0)
>  goto cleanup;
>  }
>  
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 80ce43c..ddf19e9 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1184,16 +1184,8 @@ virCgroupNew(pid_t pid,
>  }
>  
>  
> -/**
> - * virCgroupAddTask:
> - *
> - * @group: The cgroup to add a task to
> - * @pid: The pid of the task to add
> - *
> - * Returns: 0 on success, -1 on error
> - */
> -int
> -virCgroupAddTask(virCgroupPtr group, pid_t pid)
> +static int
> +virCgroupAddTaskInternal(virCgroupPtr group, pid_t pid, bool withSystemd)
>  {
>  int ret = -1;
>  size_t i;
> @@ -1203,8 +1195,10 @@ virCgroupAddTask(virCgroupPtr group, pid_t pid)
>  if (!group->controllers[i].mountPoint)
>  continue;
>  
> -/* We must never add tasks in systemd's hierarchy */
> -if (i == VIR_CGROUP_CONTROLLER_SYSTEMD)
> +/* We must never add tasks in systemd's hierarchy
> + * unless we're intentionally trying to move a
> + * task into a systemd machine scope */
> +if (i == VIR_CGROUP_CONTROLLER_SYSTEMD && !withSystemd)
>  continue;
>  
>  if (virCgroupAddTaskController(group, pid, i) < 0)
> @@ -1216,6 +1210,40 @@ virCgroupAddTask(virCgroupPtr group, pid_t pid)
>  return ret;
>  }
>  
> +/**
> + * virCgroupAddTask:
> + *
> + * @group: The cgroup to add a task to
> + * @pid: The pid of the task to add
> + *
> + * Will add the task to all controllers, except the
> + * systemd unit controller.
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int
> +virCgroupAddTask(virCgroupPtr group, pid_t pid)
> +{
> +return virCgroupAddTaskInternal(group, pid, false);
> +}
> +
> +/**
> + * virCgroupAddMachineTask:
> + *
> + * @group: The cgroup to add a task to
> + * @pid: The pid of the task to add
> + *
> + * Will add the task to all controllers, including the
> + * systemd unit controller.
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int
> +virCgroupAddMachineTask(virCgroupPtr group, pid_t pid)
> +{
> +return virCgroupAddTaskInternal(group, pid, true);
> +}
> +
>  
>  /**
>   * virCgroupAddTaskController:
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index 4b8f3ff..2de1bf2 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -131,6 +131,7 @@ int virCgroupPathOfController(virCgroupPtr group,
>    char **path);
>  
>  int virCgroupAddTask(virCgroupPtr 

Re: [libvirt] [PATCH v2] libxl: define a per-domain logger.

[Cedric Bosdonnat]

On Thu, 2017-01-05 at 15:45 -0700, Jim Fehlig wrote:
> Cédric Bosdonnat wrote:
> > libxl doesn't provide a way to write one log for each domain. Thus
> > we need to demux the messages. If our logger doesn't know to which
> > domain to attribute a message, then it will write it to the default
> > log file.
> > 
> > Starting with Xen 4.9 (commit f9858025 and following), libxl will
> > write the domain ID in an easy to grab manner. The logger introduced
> > by this commit will use it to demux the libxl log messages.
> 
> One thing I noticed when testing this on a Xen installation prior to that 
> commit
> is log files are created that will never be written to. E.g. after starting
> libvirtd and one domain
> 
> # ll /var/log/libvirt/libxl/
> -rw-r--r-- 1 root root 0 Jan  5 15:48 Domain-0.log
> -rw-r--r-- 1 root root 17512 Jan  5 15:49 libxl-driver.log
> -rw-r--r-- 1 root root 0 Jan  5 15:49 sles12sp2-hvm.log
> 
> IMO these empty, never-to-be-written-to log files might be a bit confusing to
> users. AFAIK there is no way to detect up front that the underlying Xen 
> supports
> this capability right?

No there is no way to know it since there is no public API change
in libxl. The only thing we can do is delaying the creation of the file
to the first message write.

--
Cedric

> Regards,
> Jim
> 
> > 
> > Thanks to the default log file, this logger will also work with older
> > versions of Xen.
> > ---
> > 
> >  * v2: addressed Jim's comments
> > 
> >  src/Makefile.am  |   1 +
> >  src/libxl/libxl_conf.c   |  38 +---
> >  src/libxl/libxl_conf.h   |   4 +-
> >  src/libxl/libxl_domain.c |   4 +
> >  src/libxl/libxl_driver.c |   2 +
> >  src/libxl/libxl_logger.c | 223 
> > +++
> >  src/libxl/libxl_logger.h |  39 +
> >  7 files changed, 274 insertions(+), 37 deletions(-)
> >  create mode 100644 src/libxl/libxl_logger.c
> >  create mode 100644 src/libxl/libxl_logger.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index b71378728..e34d52345 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =  
> > \
> >     libxl/libxl_capabilities.c libxl/libxl_capabilities.h   \
> >     libxl/libxl_domain.c libxl/libxl_domain.h   \
> >     libxl/libxl_driver.c libxl/libxl_driver.h   \
> > +   libxl/libxl_logger.c libxl/libxl_logger.h   \
> >     libxl/libxl_migration.c libxl/libxl_migration.h
> >  
> >  UML_DRIVER_SOURCES =   \
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index b569ddad8..ac83b51c7 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
> >  
> >  virObjectUnref(cfg->caps);
> >  libxl_ctx_free(cfg->ctx);
> > -xtl_logger_destroy(cfg->logger);
> > -if (cfg->logger_file)
> > -VIR_FORCE_FCLOSE(cfg->logger_file);
> > +libxlLoggerFree(cfg->logger);
> >  
> >  VIR_FREE(cfg->configDir);
> >  VIR_FREE(cfg->autostartDir);
> > @@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
> >  libxlDriverConfigNew(void)
> >  {
> >  libxlDriverConfigPtr cfg;
> > -char *log_file = NULL;
> > -xentoollog_level log_level = XTL_DEBUG;
> >  char ebuf[1024];
> >  unsigned int free_mem;
> >  
> > @@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
> >  if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
> >  goto error;
> >  
> > -if (virAsprintf(_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> > -goto error;
> > -
> >  if (virFileMakePath(cfg->logDir) < 0) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("failed to create log dir '%s': %s"),
> > @@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
> >  goto error;
> >  }
> >  
> > -if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
> > -VIR_ERROR(_("Failed to create log file '%s': %s"),
> > -  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> > -goto error;
> > -}
> > -VIR_FREE(log_file);
> > -
> > -switch (virLogGetDefaultPriority()) {
> > -case VIR_LOG_DEBUG:
> > -log_level = XTL_DEBUG;
> > -break;
> > -case VIR_LOG_INFO:
> > -log_level = XTL_INFO;
> > -break;
> > -case VIR_LOG_WARN:
> > -log_level = XTL_WARN;
> > -break;
> > -case VIR_LOG_ERROR:
> > -log_level = XTL_ERROR;
> > -break;
> > -}
> > -
> > -cfg->logger =
> > -(xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> > -  log_level, 
> > XTL_STDIOSTREAM_SHOW_DATE);
> > +cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
> >  if (!cfg->logger) {
> >  VIR_ERROR(_("cannot create logger 

Re: [libvirt] Loosing lxc guests when restarting libvirt

[Cedric Bosdonnat]

Hi Guido,

On Sun, 2016-12-25 at 00:21 +0100, Guido Günther wrote:
> On Sat, Dec 24, 2016 at 05:14:44PM +0100, Guido Günther wrote:
> > Hi Cedric,x
> > On Wed, Dec 21, 2016 at 02:36:39PM +0100, Cedric Bosdonnat wrote:
> > > Hey Christian,
> > > 
> > > On Tue, 2016-12-20 at 12:29 +0100, Christian Ehrhardt wrote:
> > > > Hi,
> > > > I found an issue in libvirt related to libvirt-lxc, but fail to find 
> > > > the root cause.
> > > > 
> > > > The TL;DR is: libvirt-lxc guests get killed on libvirt restart due to 
> > > > "internal error: No valid cgroup for
> > > > machine"
> > > > 
> > > > It was able to reproduce libvirt 1.3.1, 2.4 and 2.5 as packages in 
> > > > Ubuntu and Debian.
> > > > I wanted to ask for two things:
> > > > - wider coverage where this does reproduce
> > > 
> > > I couldn't reproduce here with openSUSE Tumbleweed and libvirt 2.5 
> > > packages.
> > 
> > I had a short look and it seems like this sequence is killing all running
> > libvirt-lxc guests reliably:
> > 
> >   # no lxc guest running yet
> >   export LIBVIRT_DEFAULT_URI=lxc:///
> >   DOMAIN=sl
> >   systemctl daemon-reload
> > 
> >   # start lxc guest
> >   virsh start ${DOMAIN}
> >   sleep 1  # give vm some time to start
> >   systemctl restart libvirtd
> 
> Using ftrae I can see that systemd moves the process into the wrong
> cgroup on start:
> 
> systemd-1 [000]    652.333068: cgroup_attach_task: dst_root=3 
> dst_id=80 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> systemd-1 [000]    652.333117: cgroup_attach_task: dst_root=3 
> dst_id=80 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> systemd-1 [000]    652.333160: cgroup_attach_task: dst_root=6 
> dst_id=80 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> systemd-1 [000]    652.333203: cgroup_attach_task: dst_root=4 
> dst_id=107 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> systemd-1 [000]    652.333245: cgroup_attach_task: dst_root=8 
> dst_id=80 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> systemd-1 [000]    652.333286: cgroup_attach_task: dst_root=7 
> dst_id=84 dst_level=2
> dst_path=/system.slice/libvirtd.service pid=4073 comm=libvirt_lxc
> 
> I've attached the script to reproduce this and would be happy about
> ideas of the root cause.

Thanks for your work: it really helped me reproduce it here too. After quite a 
lot
of fiddling with the thing I discovered that it happens to me only if 
daemon-reload is done
between the container start and the libvirtd restart.

I've also seen that the following doesn't lead to the problem:

virsh start sl
systemctl daemon-reload
systemctl stop libvirtd
libvirtd (manual start)

But after that, if I kill libvirtd and start it using systemctl start libvirtd, 
then the
container disappears too.

I tried debugging this, but didn't come to anything interesting thus far. I'll 
try again later
with a less confused brain ;)

--
Cedric

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