[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/3] Add support for limiting guest coredump



On 09/20/2012 02:54 PM, Michal Privoznik wrote:
> On 20.09.2012 10:58, Martin Kletzander wrote:
>> Sometimes when guest machine crashes, coredump can get huge due to the
>> guest memory. This can be limited using madvise(2) system call and is
>> being used in QEMU hypervisor. This patch adds an option for configuring
>> that in the domain XML and related documentation.
>> ---
>>  docs/formatdomain.html.in     | 12 +++++++++---
>>  docs/schemas/domaincommon.rng |  8 ++++++++
>>  src/conf/domain_conf.c        | 25 ++++++++++++++++++++++++-
>>  src/conf/domain_conf.h        | 10 ++++++++++
>>  src/libvirt_private.syms      |  2 ++
>>  5 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index d021837..210ebe0 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -515,9 +515,15 @@
>>          However, the value will be rounded up to the nearest kibibyte
>>          by libvirt, and may be further rounded to the granularity
>>          supported by the hypervisor.  Some hypervisors also enforce a
>> -        minimum, such as
>> -        4000KiB. <span class='since'><code>unit</code> since
>> -        0.9.11</span></dd>
>> +        minimum, such as 4000KiB.
>> +
>> +        In the case of crash, optional attribute <code>dump-core</code>
>> +        can be used to control whether the guest memory should be
>> +        included in the generated coredump or not (values "on", "off").
>> +
>> +        <span class='since'><code>unit</code> since 0.9.11</span>,
>> +        <span class='since'><code>dump-core</code> since 0.10.2
>> +        (QEMU only)</span></dd>
>>        <dt><code>currentMemory</code></dt>
>>        <dd>The actual allocation of memory for the guest. This value can
>>          be less than the maximum allocation, to allow for ballooning
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index ed25f58..bf6afbb 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -470,6 +470,14 @@
>>      <interleave>
>>        <element name="memory">
>>          <ref name='scaledInteger'/>
>> +        <optional>
>> +          <attribute name="dump-core">
> 
> s/-c/C/  as Dan pointed out
> 

Done and double checked with tests.

>> +            <choice>
>> +              <value>on</value>
>> +              <value>off</value>
>> +            </choice>
>> +          </attribute>
>> +        </optional>
>>        </element>
>>        <optional>
>>          <element name="currentMemory">
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 4714312..c0cff7b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -381,6 +381,11 @@ VIR_ENUM_IMPL(virDomainSoundModel, VIR_DOMAIN_SOUND_MODEL_LAST,
>>                "ac97",
>>                "ich6")
>>
>> +VIR_ENUM_IMPL(virDomainMemDump, VIR_DOMAIN_MEM_DUMP_LAST,
>> +              "default",
>> +              "on",
>> +              "off")
>> +
>>  VIR_ENUM_IMPL(virDomainMemballoonModel, VIR_DOMAIN_MEMBALLOON_MODEL_LAST,
>>                "virtio",
>>                "xen",
>> @@ -8525,6 +8530,19 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>                               &def->mem.cur_balloon, false) < 0)
>>          goto error;
>>
>> +    /* and info about it */
>> +    tmp = virXPathString("string(./memory[1]/@dump-core)", ctxt);
>> +    if (tmp) {
>> +        def->mem.dump_core = virDomainMemDumpTypeFromString(tmp);
>> +        VIR_FREE(tmp);
>> +
>> +        if (def->mem.dump_core < 0) {
> 
> s/</<=/  because we don't want to let users type in 'default'
> 

Fixed.

>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>> +                           _("Invalid value for <memory> 'dump-core' attribute"));
> 
> we tend to write the value passed by user:
> 
> virReportError(VIR_ERR_XML_ERROR,
>                _("Bad value '%s'"),
>                tmp);
> 
> but that requires you won't free 'tmp' just a few lines above.
> 

I did that except the function fitted on one line.

>> +            goto error;
>> +        }
>> +    }
>> +
>>      if (def->mem.cur_balloon > def->mem.max_balloon) {
>>          /* Older libvirt could get into this situation due to
>>           * rounding; if the discrepancy is less than 1MiB, we silently
>> @@ -13267,8 +13285,13 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>          xmlIndentTreeOutput = oldIndentTreeOutput;
>>      }
>>
>> -    virBufferAsprintf(buf, "  <memory unit='KiB'>%llu</memory>\n",
>> +    virBufferAddLit(buf, "  <memory");
>> +    if (def->mem.dump_core)
>> +        virBufferAsprintf(buf, " dump-core='%s'",
>> +                          virDomainMemDumpTypeToString(def->mem.dump_core));
>> +    virBufferAsprintf(buf, " unit='KiB'>%llu</memory>\n",
>>                        def->mem.max_balloon);
>> +
>>      virBufferAsprintf(buf, "  <currentMemory unit='KiB'>%llu</currentMemory>\n",
>>                        def->mem.cur_balloon);
>>
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index d719d57..fae699f 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -1320,6 +1320,14 @@ struct _virDomainRedirFilterDef {
>>      virDomainRedirFilterUsbDevDefPtr *usbdevs;
>>  };
>>
>> +enum virDomainMemDump {
>> +    VIR_DOMAIN_MEM_DUMP_DEFAULT = 0,
>> +    VIR_DOMAIN_MEM_DUMP_ON,
>> +    VIR_DOMAIN_MEM_DUMP_OFF,
>> +
>> +    VIR_DOMAIN_MEM_DUMP_LAST,
>> +};
>> +
>>  enum {
>>      VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO,
>>      VIR_DOMAIN_MEMBALLOON_MODEL_XEN,
>> @@ -1641,6 +1649,7 @@ struct _virDomainDef {
>>          unsigned long long max_balloon; /* in kibibytes */
>>          unsigned long long cur_balloon; /* in kibibytes */
>>          bool hugepage_backed;
>> +        int dump_core; /* enum virDomainMemDump */
>>          unsigned long long hard_limit; /* in kibibytes */
>>          unsigned long long soft_limit; /* in kibibytes */
>>          unsigned long long min_guarantee; /* in kibibytes */
>> @@ -2177,6 +2186,7 @@ VIR_ENUM_DECL(virDomainChrTcpProtocol)
>>  VIR_ENUM_DECL(virDomainChrSpicevmc)
>>  VIR_ENUM_DECL(virDomainSoundCodec)
>>  VIR_ENUM_DECL(virDomainSoundModel)
>> +VIR_ENUM_DECL(virDomainMemDump)
>>  VIR_ENUM_DECL(virDomainMemballoonModel)
>>  VIR_ENUM_DECL(virDomainSmbiosMode)
>>  VIR_ENUM_DECL(virDomainWatchdogModel)
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 0b53895..0b6068d 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -426,6 +426,8 @@ virDomainLiveConfigHelperMethod;
>>  virDomainLoadAllConfigs;
>>  virDomainMemballoonModelTypeFromString;
>>  virDomainMemballoonModelTypeToString;
>> +virDomainMemDumpTypeFromString;
>> +virDomainMemDumpTypeToString;
>>  virDomainNetDefFree;
>>  virDomainNetFind;
>>  virDomainNetGetActualBandwidth;
>>
> 
> ACK if you fix those nits.
> 
> Michal
> 

Thanks for the review. Nits fixed, syntax-check and check ran OK, it's
pushed now.

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]