Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Ian Jackson <ian.jackson@citrix.com>
To: Paul Durrant <pdurrant@amazon.com>
Cc: Anthony Perard <anthony.perard@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Wei Liu <wl@xen.org>
Subject: Re: [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids
Date: Thu, 16 Jan 2020 18:27:42 +0000
Message-ID: <24096.43806.179846.885653@mariner.uk.xensource.com> (raw)
In-Reply-To: <20200116093602.4203-4-pdurrant@amazon.com>

Thanks.  I think this is the algorithm as we discussed, thanks.
I have some comments about the implementation...

Paul Durrant writes ("[PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids"):
> A domid is considered retired if the domain it represents was destroyed
> less than a specified number of seconds ago. The number can be set using
> the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable does
> not exist then a default value of 60s is used.
...

I'm afraid I think your update protocol for this file is wrong.  In
general it is a bad idea to try to write over a file in-place.  Doing
so is full of gotchas.  (In this particular case for example I think
an interrupted attempt at cleaning the file can produce a corrupted
file containing nonsense.)

Can we please use the standard write-to-new-file-and-rename ?
Ie, to launder
    flock(open("domid-history.lock"))
    fopen("domid-history","r")
    fopen("domid-history.new","w")
    fgets/fputs
    fclose
    rename
    close

(And no uses of ftell, fopen(,"r+"), etc.)

Reading can be done without taking the lock, if you so fancy.

I think there are a lot of missing error checks in this patch, but
since I'm asking for a different approach I won't point them out
individually.

> +    fd = open(name, O_RDWR|O_CREAT, 0644);
> +    if (fd < 0) {
> +        LOGE(ERROR, "unexpected error while trying open %s, errno=%d",
> +             name, errno);
> +        goto fail;
> +    }
> +
> +    for (;;) {
> +        ret = flock(fd, LOCK_EX);

I looked for a utility function to do this but didn't find one.
I think this is complicated because it needs to be a `carefd' in libxl
terms because of concurrent forking by other threads in the
application.

I suggest generalising libxl__lock_domain_userdata, which has all the
necessary code (and which also would permit removing the file in the
future).

I feel responsible for this inconvenience.  If this is too tiresome
for you, I could do that part for you...

> +/* Write a domid retirement record */
> +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid)
> +{
...
> +    while (fgets(line, sizeof(line), f)) {
> +        unsigned long sec;
> +        unsigned int ignored;
> +
> +        roff = ftell(f);
> +
> +        if (sscanf(line, "%lu %u", &sec, &ignored) != 2)
> +            continue; /* Purge malformed lines */

I'm not sure why you bother with fgets into a buffer, when you could
just use fscanf rather than sscanf.  Your code doesn't need to take
much care about weird syntax which might occur (and indeed your code
here doesn't take such care).

> @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc,
>          if (!ctx->xch) goto badchild;
>  
>          if (!dis->soft_reset) {
> +            libxl__retire_domid(gc, domid);

I wonder if a better term than "retired" would be possible.  I
initially found this patch a bit confusing because I thought a retired
domid would be one which had *not* been used recently.  Maybe
"recent", "mark recent", etc. ?  Ultimately this is a bikeshed issue
which I will leave this up to you, though.


I don't much like the environment variable to configure this.  I don't
object to keeping it but can we have a comment saying this is not
intended for use in production ?  Personally I would rather it was
hardcoded, or failing that, written to some config file.


Finally, I think this patch needs an addition to xen-init-dom0 to
remove or empty the record file.  This is because while /run is
usually a tmpfs, this is not *necessarily* true.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16  9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
2020-01-16  9:35 ` [Xen-devel] [PATCH v3 1/6] libxl: add definition of INVALID_DOMID to the API Paul Durrant
2020-01-16  9:35 ` [Xen-devel] [PATCH v3 2/6] libxl_create: make 'soft reset' explicit Paul Durrant
2020-01-16  9:35 ` [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids Paul Durrant
2020-01-16 18:27   ` Ian Jackson [this message]
2020-01-17  9:26     ` Durrant, Paul
2020-01-17 11:31       ` Ian Jackson
2020-01-16  9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
2020-01-16  9:40   ` Jan Beulich
2020-01-16  9:46     ` Durrant, Paul
2020-01-16  9:59       ` Jan Beulich
2020-01-16 15:53   ` Jason Andryuk
2020-01-16 18:36   ` Ian Jackson
2020-01-17  9:37     ` Durrant, Paul
2020-01-17 11:35       ` Ian Jackson
2020-01-17 12:06         ` Durrant, Paul
2020-01-17 15:30           ` Ian Jackson
2020-01-20  8:18             ` Durrant, Paul
2020-01-16  9:36 ` [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy' Paul Durrant
2020-01-16 18:37   ` Ian Jackson
2020-01-16  9:36 ` [Xen-devel] [PATCH v3 6/6] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
2020-01-16 18:39   ` Ian Jackson
2020-01-16 18:43 ` [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Ian Jackson
2020-01-17  9:11   ` Durrant, Paul

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24096.43806.179846.885653@mariner.uk.xensource.com \
    --to=ian.jackson@citrix.com \
    --cc=anthony.perard@citrix.com \
    --cc=pdurrant@amazon.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git