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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > +#
> > +# 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@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
