On 4/3/23 19:08, skran wrote:
> I have implemented your corrections successfully. Thank you.
This is not a commit message that describes what the patch does. If you
look at git log you'll see plenty of good examples. The idea is that
anybody reading through git log would understand what the commit does
without needing to read the actual diff.
> Signed-off-by: K Shiva <shiva_kr@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 42 +++-------------------------
> src/conf/domain_validate.c | 56 ++++++++++++++++++++++++++++++++++++++
> src/conf/domain_validate.h | 7 +++++
> 3 files changed, 67 insertions(+), 38 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 70e4d52ee6..1e0ac737bb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5836,20 +5836,9 @@ virDomainStorageNetworkParseHost(xmlNodePtr hostnode,
>
> host->socket = virXMLPropString(hostnode, "socket");
>
> - if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> - host->socket == NULL) {
> - virReportError(VIR_ERR_XML_ERROR, "%s",
> - _("missing socket for unix transport"));
> + // Socket Validation
We don't use this style of comments.
https://libvirt.org/coding-style.html#code-formatting-especially-for-new-code
> + if (virDomainStorageNetHostSocketValidate(host, transport) < 0)
This doesn't really do what was suggested in the previous review though:
https://listman.redhat.com/archives/libvir-list/2023-April/239183.html
https://listman.redhat.com/archives/libvir-list/2023-April/239184.html
While it's okay to move the code into domain_validate.c file, it's also
necessary to stop calling the function from here and start calling it
from validation phase. Maybe my reasoning wasn't clear the first time.
One of the advantages of moving validation checks out from parsing phase
into validation phase is that we often introduce new validation checks.
Now, if there's an XML saved on a disk and these validation checks would
run in parsing phase we wouldn't even parse the XML document. BUT, if
they live in validation phase, then the document can be parsed and
later, when an user tries to start the domain with 'broken' XML, they
are given an error message, and can fix their XML, e.g. like this:
virsh edit myDomain
And it's true that especially for old code we still mix validation and
parsing (just like you can see in the code you're changing). The reason
is - nobody rolled up their sleeves and changed it yet. We're gradually
changing the code as we maintain it though. But there's another reason:
sometimes the cut isn't that clear. For instance, if an attribute must
be a power of two, should the check happen in parse phase or validation
phase? I'd vote for the latter, but sometimes we take a shortcut and
place it into the former.
> goto cleanup;
> - }
> -
> - if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> - host->socket != NULL) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("transport '%1$s' does not support socket attribute"),
> - transport);
> - goto cleanup;
> - }
>
> if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) {
> if (!(host->name = virXMLPropString(hostnode, "name"))) {
> @@ -11004,7 +10993,6 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> unsigned int flags)
> {
> int ret = -1;
> - const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
> g_autofree char *address = virXMLPropString(node, "address");
> g_autofree char *network = virXMLPropString(node, "network");
> g_autofree char *socketPath = virXMLPropString(node, "socket");
> @@ -11021,30 +11009,8 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDef *def,
> VIR_XML_PROP_REQUIRED, &def->type) < 0)
> goto error;
>
> - switch (def->type) {
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> - if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> - graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'socket' is not available for graphics type '%1$s'"),
> - graphicsType);
> - goto error;
> - }
> - break;
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> - if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> - graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("listen type 'none' is not available for graphics type '%1$s'"),
> - graphicsType);
> - goto error;
> - }
> - break;
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> - case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> - break;
> - }
> + if (virDomainGraphicsListenDefValidate(def, graphics) == -1)
> + goto error;
>
> if (def->type == VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS) {
> if (address && addressCompat && STRNEQ(address, addressCompat)) {
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 9265fef4f9..10f8242c84 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -2669,6 +2669,39 @@ virDomainGraphicsDefValidate(const virDomainDef *def,
> return 0;
> }
>
> +int
> +virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
> + const virDomainGraphicsDef *graphics)
> +{
> + const char *graphicsType = virDomainGraphicsTypeToString(graphics->type);
> +
> + switch (def->type) {
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
> + if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> + graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("listen type 'socket' is not available for graphics type '%1$s'"),
> + graphicsType);
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE:
> + if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> + graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("listen type 'none' is not available for graphics type '%1$s'"),
> + graphicsType);
> + return -1;
> + }
> + break;
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS:
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK:
> + case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_LAST:
> + break;
> + }
> + return 0;
> +}
> +
> static int
> virDomainIOMMUDefValidate(const virDomainIOMMUDef *iommu)
> {
> @@ -2898,3 +2931,26 @@ virDomainDeviceDefValidate(const virDomainDeviceDef *dev,
>
> return 0;
> }
> +
> +int
> +virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
> + const char* transport)
> +{
> + if (host->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> + host->socket == NULL) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing socket for unix transport"));
> + return -1;
> + }
> +
> + if (host->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX &&
> + host->socket != NULL) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("transport '%1$s' does not support socket attribute"),
> + transport);
> + return -1;
> + }
> +
> +
> + return 0;
> +}
> diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
> index fc441cef5b..baeae4b2a3 100644
> --- a/src/conf/domain_validate.h
> +++ b/src/conf/domain_validate.h
> @@ -47,3 +47,10 @@ int virDomainDiskDefSourceLUNValidate(const virStorageSource *src);
>
> int virDomainDefOSValidate(const virDomainDef *def,
> virDomainXMLOption *xmlopt);
> +
> +int
> +virDomainGraphicsListenDefValidate(const virDomainGraphicsListenDef *def,
> + const virDomainGraphicsDef *graphics);
Please keep style laid out by earlier code, i.e.
int virDomain...
> +
> +int virDomainStorageNetHostSocketValidate(const virStorageNetHostDef *host,
> + const char *transport);
Side note: In the end none of these functions will need to be declared
in a header file and will be static ones. But, for every function that's
in a header file, we need a corresponding record in
src/libvirt_private.syms. This file is then passed to linker so that
these functions (or symbols as they are called on linker level) can be
called from other modules (e.g. QEMU driver). But as I've said, you will
not need to declare any of these functions in header file, they can be
static as they will be called from domain_validate.c only.
Looking forward to v2.
Michal
