[virt-tools-list] [PATCH 1/3] virtinst: Add vsock device type

[Slavomir Kaslev]

On Mon, Dec 10, 2018 at 11:46:14AM +0800, Pavel Hrdina wrote:
> On Fri, Dec 07, 2018 at 05:02:14PM +0200, Slavomir Kaslev wrote:
> > Signed-off-by: Slavomir Kaslev <kaslevs at vmware.com>
> > ---
> >  virtinst/devices/__init__.py |  1 +
> >  virtinst/devices/vsock.py    | 41 ++++++++++++++++++++++++++++++++++++
> >  virtinst/guest.py            |  3 ++-
> >  3 files changed, 44 insertions(+), 1 deletion(-)
> >  create mode 100644 virtinst/devices/vsock.py
> 
> This patch fails './setup.py test'.

That's interesting. This is the output of './setup.py test' on my machine:

$ ./setup.py test
running test
...........................................................................................................................ssss..........................................................................................................s.s..s...s.s....ss.ssss....s........................................................................................................................................................................................s
----------------------------------------------------------------------
Ran 446 tests in 7.120s

OK (skipped=17)

Am I running it with wrong flags?

> Every new XML element should have
> it's command line option for virt-install and also we require some basic
> test to be available.  So you need to extend this patch to introduce
> command line option(s).

OK, I'll add it to virt-install cli in v2. Can you point me to an existing test
I can use as reference?

> 
> > diff --git a/virtinst/devices/__init__.py b/virtinst/devices/__init__.py
> > index 6da0766d..6120f5d0 100644
> > --- a/virtinst/devices/__init__.py
> > +++ b/virtinst/devices/__init__.py
> > @@ -22,6 +22,7 @@ from .redirdev import DeviceRedirdev
> >  from .rng import DeviceRng
> >  from .tpm import DeviceTpm
> >  from .video import DeviceVideo
> > +from .vsock import DeviceVsock
> >  from .watchdog import DeviceWatchdog
> >  
> >  
> > diff --git a/virtinst/devices/vsock.py b/virtinst/devices/vsock.py
> > new file mode 100644
> > index 00000000..beb00e50
> > --- /dev/null
> > +++ b/virtinst/devices/vsock.py
> > @@ -0,0 +1,41 @@
> > +# Copyright (C) 2018 VMware, Inc.
> > +#
> > +# Copyright 2018
> > +# Slavomir Kaslev <kaslevs at vmware.com>
> > +#
> > +# This work is licensed under the GNU GPLv2 or later.
> > +# See the COPYING file in the top-level directory.
> > +
> > +from .device import Device
> > +from ..xmlbuilder import XMLProperty
> > +
> > +
> > +class DeviceVsock(Device):
> > +    XML_NAME = "vsock"
> > +
> > +    model = XMLProperty("./@model")
> > +    auto_cid = XMLProperty("./cid/@auto", is_yesno=True)
> > +    cid = XMLProperty("./cid/@address", is_int=True)
> > +
> > +
> > +    ##############
> > +    # Validation #
> > +    ##############
> > +
> > +    def validate(self):
> > +        if not self.auto_cid and (self.cid is None or self.cid < 3):
> > +            raise ValueError(_("guest CID {0} must be >= 3").format(self.cid))
> 
> It would be better to have some const variable, for example
> CID_MIN_GUEST_ADDRESS or something like that instead of "magic number".

Makes sense. I added `VMADDR_CID_HOST = 2` as constant to make it clearer.

> 
> > +
> > +
> > +    ##################
> > +    # Default config #
> > +    ##################
> > +
> > +    def set_defaults(self, guest):
> > +        if not self.model:
> > +            self.model = "virtio"
> > +
> > +        if self.auto_cid is None and self.cid is None:
> > +            self.auto_cid = True
> > +        if self.cid is None and not self.auto_cid:
> > +            self.cid = 3
> 
> I think that it would be better not to have any default CID.  We can
> default to set cid auto to True if nothing is provided and if cid auto
> is set to False we should require cid address.

I don't quite follow. If auto_cid is not True and no CID is provided, should
set_defaults raise an exception (OTOH no other Device.set_defaults raises) or
just ignore it here and let libvirt catch it and complain?

Thank you,

-- Slavi

> 
> Otherwise looks good.
> 
> Pavel
> 
> > diff --git a/virtinst/guest.py b/virtinst/guest.py
> > index eeb40cb6..9acff3b9 100644
> > --- a/virtinst/guest.py
> > +++ b/virtinst/guest.py
> > @@ -29,7 +29,7 @@ class _DomainDevices(XMLBuilder):
> >              'smartcard', 'serial', 'parallel', 'console', 'channel',
> >              'input', 'tpm', 'graphics', 'sound', 'video', 'hostdev',
> >              'redirdev', 'watchdog', 'memballoon', 'rng', 'panic',
> > -            'memory']
> > +            'memory', 'vsock']
> >  
> >  
> >      disk = XMLChildProperty(DeviceDisk)
> > @@ -53,6 +53,7 @@ class _DomainDevices(XMLBuilder):
> >      rng = XMLChildProperty(DeviceRng)
> >      panic = XMLChildProperty(DevicePanic)
> >      memory = XMLChildProperty(DeviceMemory)
> > +    vsock = XMLChildProperty(DeviceVsock)
> >  
> >      def get_all(self):
> >          retlist = []
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list



> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list