Signed-off-by: Liu Yiding <liuyd.fnst@fujitsu.com>
---
 src/conf/domain_validate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 33b6f47159..668210cd35 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -2194,8 +2194,9 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
 
     case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
         if (mem->requestedsize > mem->size) {
-            virReportError(VIR_ERR_XML_DETAIL, "%s",
-                           _("requested size must be smaller than or equal to @size"));
+            virReportError(VIR_ERR_XML_DETAIL,
+                           _("requested size must be smaller than or equal to %lluKiB"),
+                           mem->size);
             return -1;
         }
 
-- 
2.34.1

On 6/27/22 10:49, Liu Yiding wrote:
> Signed-off-by: Liu Yiding <liuyd.fnst@fujitsu.com>

Hey, couple of points:

1) the commit subject is a bit verbose/has wrong prefix. You can use git
log --oneline to view what prefix we usually use,

2) the commit message is a bit sparse. We like to document what's the
scenario a commit fixes, or what's the purpose of the commit in general.
You can use git log to view examples of commit messages.

> ---
>  src/conf/domain_validate.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 33b6f47159..668210cd35 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -2194,8 +2194,9 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
>  
>      case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>          if (mem->requestedsize > mem->size) {
> -            virReportError(VIR_ERR_XML_DETAIL, "%s",
> -                           _("requested size must be smaller than or equal to @size"));
> +            virReportError(VIR_ERR_XML_DETAIL,
> +                           _("requested size must be smaller than or equal to %lluKiB"),
> +                           mem->size);

With current code I get the following message:

libvirt: Domain Config error : requested size must be smaller than or
equal to @size

and with your patch I get:

libvirt: Domain Config error : requested size must be smaller than or
equal to 2097152KiB

Both of these suffer the same problem: it's not very clear what the
problem is. While the former gives one a clue about the problematic
attribute, the other gives clue about the limit, but neither gives the
full information. Therefore, I think we should join them, like this:

libvirt: Domain Config error : requested size must be smaller than or
equal to @size (2097152KiB)

What do you think?

Michal

Hi, Michal

On 6/27/22 20:59, Michal Prívozník wrote:
> On 6/27/22 10:49, Liu Yiding wrote:
>> Signed-off-by: Liu Yiding <liuyd.fnst@fujitsu.com>
> 
> Hey, couple of points:
> 
> 1) the commit subject is a bit verbose/has wrong prefix. You can use git
> log --oneline to view what prefix we usually use,
> 

> 2) the commit message is a bit sparse. We like to document what's the
> scenario a commit fixes, or what's the purpose of the commit in general.
> You can use git log to view examples of commit messages.

Got it.

> 
>> ---
>>   src/conf/domain_validate.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>> index 33b6f47159..668210cd35 100644
>> --- a/src/conf/domain_validate.c
>> +++ b/src/conf/domain_validate.c
>> @@ -2194,8 +2194,9 @@ virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
>>   
>>       case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
>>           if (mem->requestedsize > mem->size) {
>> -            virReportError(VIR_ERR_XML_DETAIL, "%s",
>> -                           _("requested size must be smaller than or equal to @size"));
>> +            virReportError(VIR_ERR_XML_DETAIL,
>> +                           _("requested size must be smaller than or equal to %lluKiB"),
>> +                           mem->size);
> 
> With current code I get the following message:
> 
> libvirt: Domain Config error : requested size must be smaller than or
> equal to @size
> 
> and with your patch I get:
> 
> libvirt: Domain Config error : requested size must be smaller than or
> equal to 2097152KiB
> 
> Both of these suffer the same problem: it's not very clear what the
> problem is. While the former gives one a clue about the problematic
> attribute, the other gives clue about the limit, but neither gives the
> full information. Therefore, I think we should join them, like this:
> 
> libvirt: Domain Config error : requested size must be smaller than or
> equal to @size (2097152KiB)
> 
> What do you think?

Agree. I made this commit because I think an explicit value would be 
more user friendly. So join the 2 ways make sense to me.

Thanks.

> 
> Michal
>